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
Better handling of multipart parsing when request is missing body. #1997
Conversation
2a20f08
to
e403966
Compare
@MSP-Greg @dentarg @nateberkopec can I please get your feedback on this change as you would be expected to add begin
status, headers, body = @app.call(env)
rescue Rack::BadRequest => error
status = 400
headers = {'content-type' => 'text/plain'}
body = [error.to_s]
end It also means you should not provide |
LGTM
If that's a possibility, might |
I think Personally, I don't think servers should directly be rescuing the exception. We should probably ship a middleware that does the handling (maybe
I don't think we should do that. Anything that could provide the status, headers, and body should just return the expected rack response array instead of raising an exception and relying on something else to do the translation. |
@jeremyevans I don't disagree with your points. I agree servers should be able to deal with Rack as an interface without needing rack the gem. But I don't know what a better alternative is. Rack as an interface is different from all the other stuff like
Sure, and they can just blow up with a 500 error if that's their preference because there is no way to handle this case generally without having some class of exceptions that are "The request did not meet the requirements" vs "Something unexpected happened".
How, in your opinion, should we turn this error into a 400 Bad Request error? The only other option is some kind of middleware, but is that really good enough? As it stands, it feels like some parts of |
Directly after the sentence you quoted, I wrote
Code that uses |
Sure, I understand your position, but I don't consider it good enough from a developer UX POV. Because it won't work by default, it's another overhead users need to deal with, and it's version specific (although everything here is version specific, at least servers can hide the complexity and application users don't need to deal with it). |
From RFC 9110 15.5 "Client Error 4xx": "the server SHOULD send a representation containing an explanation of the error situation, and whether it is a temporary or permanent condition. These status codes are applicable to any request method. User agents SHOULD display any included representation to the user." Maybe it's best left to the app, rather than Rack? |
"No one has to |
Well, I'm also fine with this just being a 500 error. But that won't solve the original problem. |
I think it is okay if Rack just raises some error that web frameworks can rescue and return whatever status code they want for it. It is of course nice if there's a middleware included with Rack that can handle them automatically if you are fine with the responses that middleware makes. I'll comment on the suggestion in #1996 where |
I think that other language implementations would often ignore this. |
8caf16a
to
5878656
Compare
The problem with this, is that Rack is the one actually generating the errors, because |
@jeremyevans what do you think of the proposed change? The middleware The only issue is now: class EmptyContentError < ::EOFError
include BadRequest
# "Middleware" as a constant is defined here?
end Is it acceptable or should we reorganise the modules, e.g. module Rack::BadRequest... etc
class Rack::ErrorHandler # (or ExceptionHandler?)
def call(env)
@app.call(env)
rescue Rack::BadRequest
[400 ...
rescue
[500 ...
end
end etc |
1edc104
to
46047b5
Compare
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.
I think this PR should be split. There should be a separate PRs for the spec change from must to may, for BadRequest, and for ErrorHandler.
555e95a
to
65782e6
Compare
9b56e6b
to
d90c916
Compare
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.
Please split this into 3 separate pull requests, one for rack.input, one for BadRequest, and one for ErrorHandler.
end | ||
|
||
Error = BoundaryTooLongError | ||
deprecate_constant :Error |
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.
Deprecation at this point is too aggressive. Otherwise, how do you write code that rescues this error and works in both Rack 3.0 and 3.1 (and ideally, Rack 2)?
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.
This error AFAIK doesn't exist in Rack 2.x?
It was introduced here: d866dc4
Deprecations only show up with warnings enabled. We can keep it for one more minor version if you think that makes sense. How would you like to deprecate this?
I think going forward, people should probably rescue Rack::BadRequest
rather than specific multipart/query_parser errors.
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.
I think we should keep it until Rack 4.
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.
My question was when should we deprecate it? I believe it can be removed in Rack 4, are you suggesting we should deprecate it it in Rack 4?
Rack::MockRequest.env_for("/", *args) | ||
def env(options = {}) | ||
unless options.key?(:input) | ||
options[:input] = String.new |
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.
This seems like a smell. If we are changing rack.input
to not be required, we shouldn't have the tests default to using it with an empty string.
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.
Lot's of tests fail without it because they assume that it's an empty string. We can rewrite those tests. However if we change that, it will make backporting (semi-unrelated changes) harder if the tests overlap since they will have diverged. This is about changing as few tests as possible - we can always rewrite the tests later once backporting is no longer expected.
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.
That should give you a rough indication of how much code this change will break. If we aren't willing to modify rack's own tests to fully implement the change, it's hard to argue the benefits of the change are worth the cost.
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.
If we aren't willing to modify rack's own tests to fully implement the change
Well, we did modify rack's own tests, and it was just in one place since the test fixture uses this and these tests assume an empty string. That shows minimal changes were required in this specific case.
81108f4
to
c3812e3
Compare
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.
I would still like this split into 3 pull requests, as previously indicated.
Also, please do not mark conversations as resolved. The reviewer should decide when a conversation has been resolved, not the person proposing the change.
Rack::MockRequest.env_for("/", *args) | ||
def env(options = {}) | ||
unless options.key?(:input) | ||
options[:input] = String.new |
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.
That should give you a rough indication of how much code this change will break. If we aren't willing to modify rack's own tests to fully implement the change, it's hard to argue the benefits of the change are worth the cost.
end | ||
|
||
Error = BoundaryTooLongError | ||
deprecate_constant :Error |
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.
I think we should keep it until Rack 4.
c3812e3
to
f11e933
Compare
Rails is already doing this basically, but in a worse way. |
- Introduce `module Rack::BadRequest` for unified exception handling. - Add `BadRequest` to query parser too. - Make `env['rack.input']` optional. # Conflicts: # lib/rack/request.rb
f11e933
to
25de02a
Compare
- Introduce `module Rack::BadRequest` for unified exception handling. - Add `BadRequest` to query parser too. - Make `env['rack.input']` optional.
ecabf18
to
0c10e7a
Compare
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.
I like the idea of having a BadRequest exception that people can rescue, but I agree with @jeremyevans that this PR should be split. I am having a hard time reasoning about the changes with such a big diff.
Okay, I will split it. |
module Rack::BadRequest
for unified exception handling.env['rack.input']
optional.rack.input
feels like a hangover from the CGI days when$stdin
was always present and open. But in modern servers, we know that there is no entity body in most cases where it's relevant. However, we still need to allocate something which fails on the first call toread
(EOFError
,IOError
or something else). There is actually no way to detect that there is no entity body. So, I'd like to fix that, as it makes handling the multipart error more straight forward.If you think there are any other Rack error classes that should include
BadRequest
, please feel free to suggest it.Fixes #1994. Fixes #1995. Fixes #1996.