Conversation
simonjbeaumont
left a comment
There was a problem hiding this comment.
I'm in two minds on this one.
On the one hand, since the ErrorHandlingMiddleware is currently opt-in, this will also be opt-in.
On the other hand, I'm a little concerned we're making it too easy to diverge from the documented API for server implementors, who are then on the hook for ideally throwing only documented responses.
It's a pretty shallow type, that doesn't unlock anything users cannot already do, but does bring up some questions. For example, are there any affordances to prevent misuse of this type, e.g. throw HTTPResponseError(httpStatus: .ok)?
|
This feature being opt-in (and not trivially, like using a bool, but by adding a middleware) makes me believe that only people who really need it will use it, and will understand the caveats. I wrote this type 5 minutes into adopting the new error handling middleware in a toy project, and I suspect others might write similar ones as well. Similarly to easy-to-write middlewares, I think it's defensible not to have this type, and just ensure the API is clear enough that adopters can serve themselves. But it's also a shame if when adopters use the runtime library, they always go to the last project they used it and copy over utilities like this, that could be useful to everyone. I think both taking and declining this is reasonable. But we might want to consider it in context of what we'll do with middlewares, which I suspect will need to clear a similar bar: adopters can write them, we have examples, but they're likely identical between many adopters, so it's unclear why we don't provide them out of the box. |
Motivation
When throwing
HTTPResponseConvertible-conforming types from middlewares and utility functions, I often find myself just creating a single type for the whole project that represents an "HTTP response error", matching the protocol exactly - just as a concrete struct that conforms to error.This might be more generally useful, and having one in the runtime library avoids others needing to do the same.
Domain-specific errors can continue to conform to HTTPResponseConvertible, this is just for the cases where a domain-specific error isn't adding much beyond the status code (for example, throwing a 401 out of an authentication server middleware.)
Modifications
Added
HTTPResponseError, which provides a convenience type mirroringHTTPResponseConvertible.Result
Adopters can use this type, rather than write their own similar/identical one.
Test Plan
Just a struct with stored properties, no other logic.