Introduce Rack::BadRequest exception.

[ioquatix]

Something like:

class Rack::BadRequest < StandardError
  def initialize(message, status = 400)
    super(message)
    @status = status
  end
  attr :status
end

This class would be used to indicate failures within Rack, and servers should be able to catch it and correctly map it to a failed response.

There are places in the multipart parsing code and Rack::Request where various conditions can be the result of a bad client HTTP request. It feels like it would be useful to have an exception that indicates that. The current situation right now, is we may raise an IOError or some other exception which becomes a 500 Internal Server Error, which while correct in a way (the server basically blew up), doesn't really reflect the nature of the "request being invalid" - e.g. a bad request, preconditions failed, etc.

See #1994 (comment) for an example use case.

[jeremyevans]

We already have Rack::Multipart::EmptyContentError for the case you specify. I don't think we should consider adding an exception class without a pull request showing every place you want to use it.

[ioquatix]

I've given an example of where I want to use it initially. I don't feel strongly about introducing it in other areas, but I suppose we could (I'm sure there are other areas which are a bad request). Do you expect servers to rescue Rack::Multipart::EmptyContentError and turn it into [400, {}, ["Bad Request"]]? What about other places where this can occur? Every time we introduce a specific exception, servers would need to be updated. I don't think that's sustainable.

[dentarg]

I guess one concern could be someone wanting to control the response (status code, body) when Rack::Multipart::EmptyContentError happens.

We have gotten similar feedback in Puma, that users want control over the "lowlevel error" response (puma/puma#2341).

[ioquatix]

Someone can always add a middleware to intercept the error and handle it.

It really depends on whether we want the multipart issue to default to a 500 error (unhandled exception) or have some standard way of the server knowing it was actually a bad request.

[dentarg]

Makes sense to me letting the server know that it was a bad request.

[jeremyevans]

Currently, EmptyContentError subclasses EOFError (for backwards compatibility). I don't see a way to switch to a generic BadRequest without breaking backwards compatibility. I don't see a reason to add a BadRequest class if the only usage would be replacing the existing EmptyContentError. If you can determine additional places where you think a BadRequest class would be a useful abstraction, I can reconsider.

A generic 400 error by the server is probably not what most people want, though I can understand it being an improvement on the existing 500 error. For browser-based apps, you'd probably want a nicer error page (created by the framework in use). For JSON-based APIs, you would probably want the body to be in JSON format.

One backwards compatible possibility is a Rack::BadRequest module, that is included in Rack::Multipart::{EmptyContentError,Error,MultipartPartLimitError} (and maybe other exception classes). That way servers/middleware/frameworks/applications can rescue Rack::BadRequest to unify the handling of the current various exception classes without breaking existing code.

[ioquatix]

I agree, the web framework should catch and deal with errors.

I agree, using a module can be a preferable approach with few backwards compatibility issues.

I actually don't really mind what we do, as long as it improves the current situation.

[ioquatix]

Okay, in summary, in Rack 3.1, there will exist Rack::BadRequest and it will be possible to catch this and return 400 errors.

We could consider backporting this to 3.0 but I don't have a strong opinion about it.

[jeremyevans]

I'm against backporting this. We should only backport bug fixes, not features, and only if there is not significant compatibility breakage (or the bug fix is a security fix).

[ioquatix]

I'm fine with that.