express
express copied to clipboard
expressjs
Throw on invalid status codes
Closes #3143
Will throw a RangeError if status code:
- is less than 100
- is greater than 999
This aligns with Node.js' behavior of throwing if given something outside that range
Will throw a TypeError if status code is:
- Not an integer (string representation of integer included)
This is a choice we are making to limit the acceptable input.
Use res.status internally when setting status codes
the PR also ensures we use res.status internally when setting status codes, to allow us to use the validation logic internally.
Test changes
I cleaned up the tests to test acceptable range, and invalid codes, and removed the iojs logic as its not supported in v5.
TODO:
- [x] Update the PR description to be specific to the actual changes in this PR, possibly reopen the PR since direction has changed
- Notably, this PR currently throws on strings, redefines the valid range of codes to between 1xx and 9xx, throws on non-integer floats (e.g.
500.5, but allows500.00bc it is the same to JS), throws a RangeError if we get a status code outside 1xx to 9xx range
- Notably, this PR currently throws on strings, redefines the valid range of codes to between 1xx and 9xx, throws on non-integer floats (e.g.
- [x] Ensure the tests are accurate to these changes, and clean up the tests in here
- [ ] Update the v5 docs to reflect said changes (separate PR to expressjs.com)
related: https://github.com/expressjs/discussions/issues/233
I realized after opening this that Node.js does not throw on inputs like 500.5, this PR however does. From the other PR, I think we decided to throw on these cases, but I wanted to make clear that from my limited testing Node.js is not throwing on floats.
I wanted to make clear that from my limited testing Node.js is not throwing on floats.
@jonchurch - my assertion is that 500.5 is definitely invalid, so throwing is the right thing to do.
Yea, I don't have any issues with this throwing on a float; we want to throw on whatever Node.js throws on as the minimum bar. If we can also help the users by also throwing on definitely nonsensical inputs (like status codes with fractions) that makes sense of course :) !
Fixed the tests. Now the CI fails on Node.js 0.10 because there is no Number.isInteger function available there.
Assuming we're dropping that version, then this should be good to go pending any requested changes.
Fixed the tests. Now the CI fails on Node.js 0.10 because there is no Number.isInteger function available there.
Assuming we're dropping that version, then this should be good to go pending any requested changes.
A few of us thumbs upped that, but also wanted to call out that yes it is being dropped, so let 0.10 fail on this PR; 0.10 will be removed from the branch's CI prior to this change, so that version failing will turn to passing as things are merged.
as per the TC discussion, this is ready to merge, who is going to do that? @dougwilson , I see your red-X on this - is that still valid, or you are going to remove it and land?
Thinking more about this and something bothers me. I took the approach the previous PR did, since it had been reviewed, but now I'm questioning the use of res.status internally to set statuses.
If someone monkey-patches res.status it will alter the internal behavior of setting status codes on responses. That's not different though for other functions used in response, like res.type for example.
My question has two parts:
- Is it a breaking change to be relying on
res.status()to set status codes internally? - Do we want to distinguish between private vs public methods? (there are only two things marked private in
responseaccording to jsdoc comments)
See an example of the change in the diff: https://github.com/expressjs/express/pull/4212/files#diff-d86a59ede7d999db4b7bc43cb25a1c11L137-R142
Is it a breaking change to be relying on res.status() to set status codes internally?
Yes, as stated in https://github.com/expressjs/express/pull/4223#issuecomment-602102852 , but this PR is already a breaking change, right? So I'm not sure if it's super relevant. The change itself makes sense to make, just like we call res.type internally and not directly get the content-type response header. Even getting headers we internally use req.get and not req.headers.
Do we want to distinguish between private vs public methods? (there are only two things marked private in response according to jsdoc comments)
I'm not really following on what this part of the question really is. The main reason the internals use Express' own API is especially useful for AOP type of design patterns, if the user so chooses to do them. The Node.js HTTP APIs do the same patterns as well, AFAIK.
I wasn't clear. Re: breaking, I meant that someone's v4 res.status monkey patch might affect code in unexpected ways under v5, because it is used in more places than before.
You've answered my second question I believe. We aren't interested in making some methods private and off limits to users.
Thanks, I wanted to bring up this point (re: effects of monkey-patching res.status with these changes) just so someone else could check it.
Realizing that we use a lot of these helper methods internally would indicate that this change is not out of step with what is standard.
Just read that linked comment and saw you did directly address the concern already 👍
Yep! I did, though, not directly address the monkey patching messing something up; that is indeed the case, but I don't think any more so than other aspects of Node.js and Javascript. I think that it is going to be possible for users to override something and cause a breakage, but I'm not sure that the effort in order to prevent such a thing is really worth it. From support experience, I think it is extremely rare for such an issue to really show up, haha.
I think this PR should target branch https://github.com/expressjs/express/tree/5.0 and not master. And if it's ok, it should be merged aswell in v5 branch.
