Handle EOFError raised by Rack #2161
Conversation
bschmeck
force-pushed
the
handle-eof-from-rack
branch
2 times, most recently
from
2676741
to
a723b9f
Compare
In v2.2.0, Rack started raising an EOFError when given an empty body with a multipart upload - rack/rack#1572 Previously, Rack had swallowed this error.
bschmeck
force-pushed
the
handle-eof-from-rack
branch
from
a723b9f
to
f405ecb
Compare
lib/grape/request.rb
Outdated
| @@ -15,6 +15,8 @@ def initialize(env, **options) | |||
|
|
|||
| def params | |||
| @params ||= build_params | |||
| rescue EOFError | |||
| raise Grape::Exceptions::InvalidMessageBody, 'multipart/form-data' | |||
There was a problem hiding this comment.
I am concerned that this is hardcoding multipart/form-data, input doesn't have to be this way, right?
There was a problem hiding this comment.
I don't know when Rack would raise this error other than when given a multipart/form-data, but I'm not a Rack expert. I've pushed a commit to use the object's content_type method instead.
This error class seemed the most appropriated, but it requires a content type. I'm happy to create a different error class if that would be preferable.
There was a problem hiding this comment.
I'm ok with reusing this error, it makes sense to me.
There was a problem hiding this comment.
On a second thought, should we create a EmptyMessageBody error?
We most likely are here due to a multipart/form-data Content-Type, but there's no guarantee of that.
spec/grape/endpoint_spec.rb
Outdated
|
|
||
| it 'returns a 400 if given an invalid multipart body' do | ||
| # Rack swallowed this error until v2.2.0 | ||
| major, minor, _patch = Rack.release.split('.').map(&:to_i) |
There was a problem hiding this comment.
The "correct" way to do this is to use Gem::Version.new(Rack.release) >= ..., see https://code.dblock.org/2020/07/16/comparing-version-numbers-in-ruby.html if it's helpful.
Let's also externalize the if as a spec condition so we see it properly skipped.
There was a problem hiding this comment.
Thanks! I figured there had to be a better way.
lib/grape/request.rb
Outdated
| @@ -15,6 +15,8 @@ def initialize(env, **options) | |||
|
|
|||
| def params | |||
| @params ||= build_params | |||
| rescue EOFError | |||
| raise Grape::Exceptions::InvalidMessageBody, 'multipart/form-data' | |||
There was a problem hiding this comment.
On a second thought, should we create a EmptyMessageBody error?
There was a problem hiding this comment.
Let's also add a gemfile and run it as part of the matrix explicitly with this version of Rack, ala https://github.com/ruby-grape/grape/blob/master/gemfiles/rack2.gemfile
Also move the conditional to a spec condition, to properly skip the spec on versions of Rack that don't raise an error.
|
I've cleaned up how the test is skipped for older versions of Rack, added a custom I think that addresses all of your comments, but let me know if I missed something. |
|
Thanks for hanging in here, great job. Merged. |
|
Related, we had #2130, maybe you care to resolve it one way or another? I do think a note on all Rack-related issues could be useful somewhere. |
In v2.2.0, Rack started raising an EOFError when given an empty body with a multipart upload: rack/rack#1572. Previously, Rack swallowed this error and execution continued normally. This PR translates that error to an
InvalidMessageBodyexception, letting Grape return a 400 class error, instead of a 500 class error.Note that the error raised by Rack will change to
Rack::Multipart::EmptyContentErrorin v3 - rack/rack#1688