Raise Rack::Multipart::EmptyContentError instead of EOFError for parsing empty body

[jeremyevans]

This makes it simpler to catch this specific error.

Fixes #1603

[ioquatix]

This looks okay to me, but does it fix the root cause of the issue, or does it just kick it down the road (into a slightly more elaborate exception class)?

[jeremyevans]

It just changes the exception class so that a user can rescue this error specifically instead of EOFError in general.

[ioquatix]

Right, but what I mean is:

• We still raise an exception.
• Use code still has to deal with exception (where previously it was not exception).

So, even thought we make capturing exception more easily, fundamentally the interface is different than before.

I agree we should probably raise exception here. But the core problem was, should this be 5xx or 4xx.

If EOFError propagates back to web server, it cannot know how to handle it, it seems 5xx because naturally it's exceptional situation. Maybe we need to introduce Rack::BadRequest or something, so that server can identify 4xx from general exception.

However, if this is a violation of the protocol, maybe it should happen in the server before it gets to rack.

[jeremyevans]

Right, but what I mean is:

• We still raise an exception.
• Use code still has to deal with exception (where previously it was not exception).

So, even thought we make capturing exception more easily, fundamentally the interface is different than before.

Before the fix that caused this issue (#1572) we would use empty params for an empty multipart boundary (#761). That's a silent failure instead of an error. It is different, but I would think worse.

In this patch, we just make it easier to catch the error. This patch itself should not have backwards compatibility issues, because the new exception subclasses from the previous exception raised.

I agree we should probably raise exception here. But the core problem was, should this be 5xx or 4xx.

If EOFError propagates back to web server, it cannot know how to handle it, it seems 5xx because naturally it's exceptional situation. Maybe we need to introduce Rack::BadRequest or something, so that server can identify 4xx from general exception.

That would require SPEC changes, and is against one of Rack's principles, which is that neither the server nor the client need to require the rack library to be compliant with the rack SPEC.

However, if this is a violation of the protocol, maybe it should happen in the server before it gets to rack.

I don't think the server wants to check request bodies for correctness, as it is not responsible for parsing multipart bodies.

I think if we want to make this easier on users, we would need to introduce a middleware that calls the app and rescues the error and returns 4xx response to the server.

[ioquatix]

That seems reasonable.

Server should check request body because some request doesn't have request body or requires request body, then it should be 400 bad request immediately. So there is a precedent for it.

[gingerlime]

Would this be included in a release? as far as I can tell, the latest release is 2.2.3 (16th June 2020), and this was merged in Sep 2020...

[jeremyevans]

This will be included the next major release, which I think will be 3.0.0. It's scheduled for near the end of the year, as I currently understand it.

[JacobEvelyn]

@jeremyevans is there any chance of a v2.2.4 release to allow folks to more easily use this and other recent improvements?

As an outsider it's hard for me to tell how much progress is being made toward v3.0.0 but I don't see a lot of activity from the past few months and am wondering if v3.0.0 might perhaps take longer than originally planned (which is of course fine and understandable!).

[jeremyevans]

I don't handle rack releases, but from what I know, this isn't getting backported to 2.2, so you'll probably have to wait until rack 3.0. I've heard that the 3.0 release is planned for near the end of the year, but I haven't heard anything beyond that.