Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get and Head requests #114

Open
sarath-sasikumar opened this issue Apr 17, 2017 · 16 comments
Open

Get and Head requests #114

sarath-sasikumar opened this issue Apr 17, 2017 · 16 comments
Labels
status: awaiting answer Awaiting answer from the author of issue or PR.

Comments

@sarath-sasikumar
Copy link

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
Copy link
Contributor

pleerock commented Apr 19, 2017

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@NoNameProvided
Copy link
Member

NoNameProvided commented May 7, 2017

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
Copy link
Author

sarath-sasikumar commented May 9, 2017

@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
Copy link
Contributor

pleerock commented May 9, 2017

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

Not sure if it is correct way of doing things

@MichalLytek
Copy link
Contributor

MichalLytek commented May 10, 2017

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
Copy link
Contributor

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

@MichalLytek
Copy link
Contributor

MichalLytek commented May 22, 2017

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
Copy link

mcwebb commented Apr 18, 2018

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
Copy link

Stale issue message

@github-actions github-actions bot added the status: awaiting answer Awaiting answer from the author of issue or PR. label Feb 18, 2020
@attilaorosz
Copy link
Member

attilaorosz commented Feb 27, 2022

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 attilaorosz removed type: fix Issues describing a broken feature. status: duplicate Issue is being tracked already in another issue. labels Feb 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting answer Awaiting answer from the author of issue or PR.
Development

No branches or pull requests

6 participants