Add error handling middleware

[gayathrisairam]

Motivation

Implementation of Improved error handling proposal

Modifications

Added HTTPResponseConvertible protocol
Added ErrorHandlingMiddleware that converts errors confirming to HTTPResponseConvertible to a HTTP response.

Result

The new middleware is an opt-in middleware. So there won't be any change to existing clients.
Clients who wish to have OpenAPI error handling can include the new error middleware in their application.

Test Plan

Added E2E tests to test the new middleware.

[simonjbeaumont]

Thanks for picking this up @gayathrisairam!

I've left some initial comments / questions, and a note on how you can run the linters locally.

[simonjbeaumont]

I'll go and double check the proposal myself but I thought we were going to make these all optional? Right now, we have all three flavours:

• httpStatus is non-optional, with no default implementation;
• httpHeaderFields is non-optional, but has a default implementation; and
• httpBody is optional, with a default implementation.

IMO, if we're providing a default implementation in a protocol extension, we should not use optional for the requirement.

I'll go double-check the proposal again, but, regardless of API spelling, is this the semantics we wanted here?

Specifically, if someone adds the ErrorHandlingMiddleware, don't we want it transform errors that do not conform into 500 and, in debug mode, possibly include something in the header or body?

[czechboy0]

IMO, if we're providing a default implementation in a protocol extension, we should not use optional for the requirement.

I'm not sure I agree, I think it's good the way it is - header fields is a container that can be empty, so it's non-optional and defaults to empty in the protocol extension. The body, however, is a stateful async sequence, and the middleware API allows returning nil, so I think it should be optional and default to nil in the protocol extension.

Specifically, if someone adds the ErrorHandlingMiddleware, don't we want it transform errors that do not conform into 500 and, in debug mode, possibly include something in the header or body?

Yes I also caught this one, I believe here we agreed to always return a response, and use 500 for unconformed errors.

[simonjbeaumont]

IMO, if we're providing a default implementation in a protocol extension, we should not use optional for the requirement.

I'm not sure I agree, I think it's good the way it is - header fields is a container that can be empty, so it's non-optional and defaults to empty in the protocol extension. The body, however, is a stateful async sequence, and the middleware API allows returning nil, so I think it should be optional and default to nil in the protocol extension.

Right I think the way it's currently structured and documented is conflating two things. The optionality of whether a user needs to provide an override, and the optionality of the underlying field. We should have these match the target response type in terms of optionality, which, as you point out, they do already. Concretely, the HTTPResponse has non-optional status and headers (although headers may be empty), and an optional body.

I think what's confusing then is the use of "(Optional)" in the documentation comments for the protocol requirements. How about we remove that term from the documentation and provide defaults for everything in the protocol extension to make it clear, including httpBody, which will default to nil and make it clear what the default is in the protocol level documentation?

[gayathrisairam]

You are right. I will convert undocumented errors to a 500 response.
What do we want to do with header and body fields? Should we preserve the additional information in ServerError.underlyingError in the body as an application/json?

[czechboy0]

Should we preserve the additional information in ServerError.underlyingError in the body as an application/json?

I'd personally hold off on doing that, as it has the risk of leaking sensitive information in the error without the user explicitly opting into that behavior. I'd leave that up to the user in their HTTPResponseConvertible to send back as much information as they want, and for unconformed errors to only send 500, without any other headers/body.

Users can always add a logging middleware to at least see the error in logs, and this more conservative default behavior could further motivate users to conform their errors to the protocol and make an explicit decision about the response.

[simonjbeaumont]

There's precedent for this being done in debug and not in release. And I'm personally in favour of having this behaviour. Not having it is quite an annoying developer experience IMO.

I'd also propose making it configurable and defaulting to off. But it should be a backwards compatible change so happy to defer it altogether for now.

[czechboy0]

I'd prefer to leave it out of this PR, however I would be supportive of a subsequent PR adding it as an explicit option. I really dislike the behavior being controlled by debug vs release.

[simonjbeaumont]

OK, @gayathrisairam feel free to break this out into a new GitHub issue where we can flesh out how this would look.

[czechboy0]

Looks great overall, added a few suggestions for more polish, mainly in the doc comments.

[czechboy0]

Thanks for the fixes, added a few more minor suggestions as part of the final pass

[czechboy0]

@gayathrisairam please rerun the formatter and unit tests, those are failing