Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Better handling of multipart parsing when request is missing body. #1997
Changes from all commits
25de02a
0c10e7a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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?
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.
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.