-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix for Reason-Phrase being overwritten on proxy response. #1051
Conversation
Calling res.writeHead has the side effect of defaulting the Reason-Phrase to default ones. I'm using Reason-Phrase codes to sub-route api responses and they were all being reset. This change only sets the statusMessage (Reason-Phrase) if it exists on the proxyRes and is successfully passing it through in my tests.
res.writeHead(proxyRes.statusCode); | ||
res.statusCode = proxyRes.statusCode; | ||
if(proxyRes.statusMessage) | ||
res.statusMessage = proxyRes.statusMessage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 space tabs
Sorry for the slow response this dropped off my radar for a bit. |
@jcrugzz - Any time frame on when / if this will get merged? Not trying to rush, I just got some prod dependencies going out this week that are reliant on the status reason being passed through so I will have to fork if it does not get accepted soon. |
@cchamberlain sorry it took so long. Thanks! |
This breaks plugins such as harmon that patch the (edit: although it looks like harmon doesn't properly account for that overload, so it would need to be patched in order for this method to work) |
@briman0094 - I considered that solution as well but being unaware of harmon's dependency, went with this one since it seemed lighter weight (setting the fields directly has less of this to worry about) - I can modify / open a new PR? Should we implement the |
@cchamberlain I don't think we need to implement |
@briman0094 - Awesome, guess my work here is done! |
Calling res.writeHead has the side effect of setting the Reason-Phrase back to node defaults regardless of what is in the response status line. I'm using Reason-Phrase codes to sub-route api responses and they were all being reset. This change only sets the statusMessage (Reason-Phrase) if it exists on the proxyRes and is successfully passing it through in my tests.
from docs:
So if just calling writeHead and not passing the optional statusMessage second argument, I believe it is getting reset to undefined and then defaulting it. I'm not sure if there is a good reason for using writeHead in general here since it seems like setting statusCode / statusMessage properties directly accomplishes the same with less side effects, could definitely be overlooking something.
What I see before change:
What I see after change: