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.

Sign up for GitHub

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

Jump to bottom

Handle Rack errors when too many files are uploaded #2256

Merged
merged 12 commits into from
Apr 26, 2022
Merged

Handle Rack errors when too many files are uploaded #2256

merged 12 commits into from
Apr 26, 2022

Conversation

bschmeck
Copy link
Contributor

Rack enforces a limit on the number of files that can be uploaded in a single request. Currently, Grape does not handle the error that Rack raises when such a request is received. This PR catches that error and returns with a Payload Too Large (413) status code.

dblock
dblock requested changes Apr 25, 2022
View reviewed changes
Copy link
Member

@dblock dblock left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this does change API behavior, increment the version to 1.7.0 as part of this PR?

Also do check those edge errors please. We can ignore them, but I want to make sure it's not related to this change.

Thanks!

CHANGELOG.md Outdated
@@ -18,6 +18,7 @@
* [#2227](https://github.com/ruby-grape/grape/pull/2222): Rename `MissingGroupType` and `UnsupportedGroupType` exceptions - [@ericproulx](https://github.com/ericproulx).
* [#2244](https://github.com/ruby-grape/grape/pull/2244): Fix a breaking change in `Grape::Validations` provided in 1.6.1 - [@dm1try](https://github.com/dm1try).
* [#2250](https://github.com/ruby-grape/grape/pull/2250): Add deprecation warning for `UnsupportedGroupTypeError` and `MissingGroupTypeError` - [@ericproulx](https://github.com/ericproulx).
* [#2256](https://github.com/ruby-grape/grape/pull/2256): Handle MultipartPartLimitError from Rack when too many files are uploaded - [@bschmeck](https://github.com/bschmeck).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say "Raise Grape::Exceptions::TooManyMultipartFiles on MultipartPartLimitError, when too many files are uploaded"?

@bschmeck
Copy link
Contributor Author

Thanks. I've updated the wording in the changelog. Please let me know if I need to move anything around as a result of the minor version bump.

I see the same failures against rails_edge and rack_edge when running the specs on the master branch, they do not appear to be related to this change. The ruby-head failures are all in the mustermann gem and the jruby-head build fails due to a problem inside of the rubocop-ast gem. Neither of those failures would seem to be related to this change.

@dblock
Copy link
Member

dblock commented Apr 25, 2022

@bschmeck Yes, please increment the version in version.rb and anywhere else we say "1.6.3", including CHANGELOG

dblock
dblock reviewed Apr 25, 2022
View reviewed changes
CHANGELOG.md Outdated
@@ -18,6 +18,7 @@
* [#2227](https://github.com/ruby-grape/grape/pull/2222): Rename `MissingGroupType` and `UnsupportedGroupType` exceptions - [@ericproulx](https://github.com/ericproulx).
* [#2244](https://github.com/ruby-grape/grape/pull/2244): Fix a breaking change in `Grape::Validations` provided in 1.6.1 - [@dm1try](https://github.com/dm1try).
* [#2250](https://github.com/ruby-grape/grape/pull/2250): Add deprecation warning for `UnsupportedGroupTypeError` and `MissingGroupTypeError` - [@ericproulx](https://github.com/ericproulx).
* [#2256](https://github.com/ruby-grape/grape/pull/2256): Raise Grape::Exceptions::MultipartPartLimitError from Rack when too many files are uploaded - [@bschmeck](https://github.com/bschmeck).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back-tick quote Grape::Exceptions::MultipartPartLimitError

@dblock
Copy link
Member

dblock commented Apr 25, 2022

Should/can the message include the system limit number?

@bschmeck
Backticks around classname in CHANGELOG
190db46
@bschmeck
Change next version of Grape from 1.6.3 to 1.7.0
2d8da41 
The introduction of the TooManyMultipartFiles changes API behavior, so bump the
minor version.
@bschmeck
Include the system's configured multipart file limit in the error mes…
46cbf07 
…sage

The number of allowed multipart files is a configurable value in Rack, pull that
limit and include it in the generated error message.
dblock
dblock reviewed Apr 25, 2022
View reviewed changes
Copy link
Member

@dblock dblock left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promise last one: in UPGRADING.md add a paragraph that says something along the lines of

Handling Multipart Limit Errors

... if you were handling Rack::Multipart::MultipartPartLimitError, Grape will now throw Grape::Exceptions::TooManyMultipartFiles ...

@bschmeck
Copy link
Contributor Author

Done. Thanks for your help. Let me know if there's anything else!

@dblock dblock merged commit 75276be into ruby-grape:master Apr 26, 2022
@dblock
Copy link
Member

dblock commented Apr 26, 2022

Merged, thanks for hanging in here with me @bschmeck !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bschmeck @dblock