routing-controllers icon indicating copy to clipboard operation
routing-controllers copied to clipboard

Get and Head requests

Open sarath-sasikumar opened this issue 8 years ago • 16 comments

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

sarath-sasikumar avatar Apr 17 '17 11:04 sarath-sasikumar

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

pleerock avatar Apr 19 '17 19:04 pleerock

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.

sarath-sasikumar avatar Apr 24 '17 13:04 sarath-sasikumar

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

pleerock avatar Apr 24 '17 19:04 pleerock

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

MichalLytek avatar May 07 '17 20:05 MichalLytek

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..

NoNameProvided avatar May 07 '17 20:05 NoNameProvided

@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.

sarath-sasikumar avatar May 09 '17 04:05 sarath-sasikumar

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.

pleerock avatar May 09 '17 06:05 pleerock

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

sarath-sasikumar avatar May 09 '17 07:05 sarath-sasikumar

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.

MichalLytek avatar May 09 '17 10:05 MichalLytek

Not sure if it is correct way of doing things

pleerock avatar May 10 '17 05:05 pleerock

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.

MichalLytek avatar May 10 '17 09:05 MichalLytek

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

pleerock avatar May 22 '17 08:05 pleerock

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.

MichalLytek avatar May 22 '17 18:05 MichalLytek

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?

mcwebb avatar Apr 18 '18 13:04 mcwebb

Stale issue message

github-actions[bot] avatar Feb 18 '20 00:02 github-actions[bot]

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.

attilaorosz avatar Feb 27 '22 13:02 attilaorosz