typestack / routing-controllers

Create structured, declarative and beautifully organized class-based controllers with heavy decorators usage in Express / Koa using TypeScript and Routing Controllers Framework.
MIT License
4.41k stars 394 forks source link

Get and Head requests #114

Open sarath-sasikumar opened 7 years ago

sarath-sasikumar commented 7 years ago

When the Get and Head requests are provided with the same paths, both the functions corresponding to the path are getting executed.

The test code that I tried was:

@Controller("/test")
export default class TestController {

    @Head()
    head(@Res() resp:any){
        console.log("head");
        resp.send();
    }        
    @Get()
    get(@Res() resp:any){
        console.log("get");
        resp.send("Get");
    }
}

If I give a GET request to the above code, both head and get are getting logged to the console. Even if I give a head request, the Get request function is also getting called and the error: Can't set headers after they are sent is being encountered

pleerock commented 7 years ago

I think you'll have same issue with express:

const e = express();
        e.head("/test", function(req, res, next) {
            console.log("head");
            next();
        });
        e.get("/test", function(req, res, next) {
            console.log("get");
            res.send("");
            next();
        });
        e.listen(3000);

I just run this code and when I do head request to /test it executes both console.log

sarath-sasikumar commented 7 years ago
var express = require('express')
var app = express()
app.head('/test', function (req, res,next) {
    console.log("Head");
    res.end();
})

app.get('/test', function (req, res,next) {
    console.log("Get");
    res.end();
})

app.listen(3000, function () {
    console.log('Example app listening on port 3000!')
});

This is the express code I tried, And in this, a head request does not log "Get" in the output.

pleerock commented 7 years ago

but you didn't call next method. You have to call next method or your middleware that goes after your method won't work.

MichalLytek commented 7 years ago

But why call next() if we matched method and path? It's a handler, not an middleware ;)

NoNameProvided commented 7 years ago

Was it a CORS request? Because then this is the expected behaviour. Browsers will send a HEAD request and will send the GET only if the HEAD request returned with 2xx status code.

Check the Network tab in browser inspector to see it for yourself..

sarath-sasikumar commented 7 years ago

@NoNameProvided It wasn't a CORS request @pleerock why is it necessary to call the next(). I understand that there can be a middleware that might be required to be called after route execution which would not be called if next() isn't called

My understanding was that next() is only called when you have not sent a response to the client, so you need to continue processing.

pleerock commented 7 years ago

no, not response-send related things. If we remove next call middleware executed after controller actions wont work. For example you want simple logging. If you to log result of execution (route, it took x seconds to execute this route) you need to do it after action execution in the middleware. Without calling next it wont work.

sarath-sasikumar commented 7 years ago

So is it not possible to replicate this behavior of express

var express = require('express')
var app = express()
app.head('/test', function (req, res,next) {
    console.log("Head");
    res.end();
})

app.get('/test', function (req, res,next) {
    console.log("Get");
    res.end();
})

app.listen(3000, function () {
    console.log('Example app listening on port 3000!')
});

when this code without the next() is used

MichalLytek commented 7 years ago

you need to do it after action execution in the middleware. Without calling next it wont work.

But you have actions and middlewares stored in separate arrays. So you can just call the first after middleware after handling action result instead of calling next and rely on express middlewares stack.

pleerock commented 7 years ago

Not sure if it is correct way of doing things

MichalLytek commented 7 years ago

It's really bad that the next() is implicitly called after handling action. This is actually a bad way of doing things because Express doesn't work like this.

So to make the feature middleware executed after controller actions like logging works, we should distinguish action handlers method + path and middlewares that do app.use. Then if user want to chain action like HEAD and GET, he should explicitly call next, and if not, routing-controllers should call first global after middleware manually, not by express next chain.

pleerock commented 7 years ago

Okay, if you think so, then lets remove this functionality. Can you please do that?

MichalLytek commented 7 years ago

Sure, I will take a look at this when I will have some free time.

I had similar issue - I got GET /api/resource/:id and GET /api/resource/someShortcutMethod like endpoints and despite the fact that someShortcutMethod has been registered earlier, the second path has been triggered too which resulted in headers already sent error.

mcwebb commented 6 years ago

I'm facing the same issue as mentioned by @19majkel94 in the comment above.

I'm currently working round it by checking that the value of the :id parameter in the greedy method !== 'someShortcutMethod'. However it feels like a bit of a hack.

Are there any best practices for handling this usecase?

github-actions[bot] commented 4 years ago

Stale issue message

attilaorosz commented 2 years ago

I think the clear workaround for this is to return the response object itself.

@Controller("/test")
export default class TestController {

    @Head()
    head(@Res() resp:any){
        console.log("head");
        resp.send();
        return resp;
    }

    @Get()
    get(@Res() resp:any){
        console.log("get");
        resp.send("Get");
        return resp;
    }
}

With this I don't see the fall through logs or errors.