Hi all,
The proposal SOAR-0011: Improved error handling for Swift OpenAPI Generator is now up and In Review:
The review period will run until October 1st, 2024 - please post your feedback either as a reply to this thread, or on the pull request.
Thanks!
Error reporting with the OpenAPI runtime just came up in the Hummingbird discord the other day. This proposal should help a lot, but I question why we need the three protocols. It means any error conversion middleware has to check all three protocol conformances. It seems simpler to just provide a single protocol with extensions for default values
public protocol HTTPResponseConvertible {
var httpStatus: HTTPResponse.Status { get }
var httpHeaderFields: HTTPTypes.HTTPFields { get }
var httpBody: OpenAPIRuntime.HTTPBody? { get }
}
extension HTTPResponseConvertible {
var httpHeaderFields: HTTPTypes.HTTPFields { [:] }
var httpBody: OpenAPIRuntime.HTTPBody? { nil }
}
I’m with Adam here. It’s more complicated and less discoverable to have separate protocols
I'm +1 on this proposal and think this would be a great quality of life improvement for adopters.
My initial position was that there should be a hierarchy of protocols but I'm also no longer convinced that's the correct approach.
I'm not questioning this because of the complexity in the middleware implementation, because this isn't a burden for our adopters, or even our transport providers, because this proposal includes providing a centralised, transport-agnostic, middleware implementation in OpenAPIRuntime library.
My initial thinking was to do with the usage for the adopter: I was thinking it made sense to force users to provide a continuum of overrides from less-specific to more-specific, with the following precedence:
I initially felt that users shouldn't be able to override later parts without the former.
However, my position has changed on this. The principle reason for this change is related to my principle concern with adoption of this middleware: that it puts the burden of producing OpenAPI-document–compliant responses onto the user. Concretely, users of this middleware will be able to transform a thrown Swift error into a HTTP response with a status code which is entirely outside of the documented behaviour of the API, as defined in the OpenAPI document.
If we relaxed this requirement, and provided one protocol with default identity implementations for each response component, we provide a way for folks to use it more safely. For example, they could allow the status to remain as an internal error (500), but smuggle in useful diagnostic into the header or body (potentially only in debug environments, of course).
I'm overall +1 on the proposal, but I'd also like to discuss the possible alternative spelling using a single protocol suggested by @adam-fowler.
Having that single protocol where the httpStatus is always required, but httpHeaderFields and httpBody are defaulted to empty/nil still provides the "incremental" requirement (such that you cannot override headers/body without explicitly providing an HTTP status). However it puts the header fields and body on the same "level", meaning it is possible to provide a body without headers - do we consider that valid? I imagine that if you provide a body, you also want to provide at least the content-type header, so the question is whether we're okay losing that guarantee compared to the 3-protocol approach. Personally, I think both spellings are defensible, just wanted to get more discussion on this specific point.
@beaumont said:
If we relaxed this requirement, and provided one protocol with default identity implementations for each response component, we provide a way for folks to use it more safely. For example, they could allow the status to remain as an internal error (500), but smuggle in useful diagnostic into the header or body (potentially only in debug environments, of course).
Si, could you elaborate with an example here? I'm not sure I fully understand what that'd look like.
I initially was pushing for the triple protocol myself but I'm not sure I really think it buys us anything unless we could help adopters only return documented responses, which we can't.
The crux of the issue is that we generated throwing protocol requirements for each of the documented API operations, which means adopter handlers can just throw whatever they choose.
As an aside: if we wanted to redesign this with even more type safety we could have generated non-throwing requirements, which would force users to return a value of type Operations.<OperationName>.Output—i.e. a documented response, or explicitly reach for .undocumented(...).
It's possible we could also leverage the power of typed-throws to allow handlers to only throw documented errors, but that would have a whole bunch of design questions that would need answering.
In any event, both of these would be API breaking at this point so we need to accept where we are: adopter handlers can throw.
One could adopt a principled view here: if the handlers have thrown an error, it's likely to be an internal error, otherwise they could have returned a documented response. But, as this proposal highlights, folks are using errors for a convenience—where they will have to take responsibility for the response.
At that point, I no longer see value in us trying to police the response.
Furthermore, I think our existing approach of using the HTTP 500 response code when the handler has thrown is entirely defensible.
So I'm starting to warm to the idea that adopters might want to use this as a debugging mechanism, rather than a layer to try and rescue the error and reform a documented response.
In that scenario, I could see myself wanting to use my own personal header in debug builds to capture the underlying error, but happy to take whatever OpenAPIRuntime thought was appropriate for the response code and body. Others might prefer to use a free-form JSON in the body, but leave the response code and headers to whatever OpenAPIRuntime would have returned for an uncaught error.
So, in summary, I've cooled off on policing an incremental specificity to what folks provide and I'm sympathetic to providing an API that's easy to discover.
I think we have a few options for the protocol(s):
If we see this proposal as an escape-hatch, where we're allowing adopters to do custom logic and take full responsibility for whether they follow the OpenAPI document or not, then I think (4) might be clearest, possibly (2).
Thanks for the details, @beaumont, I largely agree.
I think (2), which is basically @adam-fowler's proposal (with the difference that the HTTP status code is not defaulted, only the headers and body are), is also the best path forward now.
I am beginning to think that the alternative proposal of doing error mapping in OpenAPI handler might be more intuitive.
Imagine a use case (which I recently ran into) - There is a transport middleware (for eg: an Auth middleware) and an OpenAPI Error middleware. On the request chain, transport middleware is executed first.
Request -> Transport Middleware(s) -> OpenAPI middleware(s) -> OpenAPI handler -> ....
If there is an error in the transport middleware, OpenAPI error middleware is going to be skipped.
If error mapping is done in OpenAPI handler, it might be a more intuitive experience without the user having to think about the order of middlewares.
The OpenAPI error middleware isn't skipped, it catches the error first and turns it into a regular response, which is then passed through the Transport middleware unchanged. I think that's desirable.
But you're right that if the Transport middleware throws an error, then the user needs to rely on a Transport error middleware (both Vapor and Hummingbird have one, but in theory other transports could handle this differently).
Moving the error mapping closer to the handler (but not necessarily into it) would indeed make it clearer. It would also mean, however, that the error mapping middleware won't help you map errors thrown from other middlewares. I'm not sure how important that is however, so I'll let others chime in here.
I agree that it'd be nice to hear some affirmative desire for that requirement. Right now, this pitch is motivated around translating the errors thrown by the handler, and not by the rest of the responder chain.
Thinking aloud here... its position in the responder chain might compose undesirably with other middleware. For example, imagine there was an observability middleware that was keeping track of the HTTP response status code counts. We'd want to make sure that this was running last (on the response). A bit contrived of an example because this is unlikely to throw, but, if it did, and the error-handling middleware transformed that error then it'd be bad.
My lean here is that this is why we should just let the user define the order of the responder chain as they do today, with the middlewares: parameter when creating the client or server.
But I'd love to hear more opinions here.
@adam-fowler, @Joannis_Orlandos: do you have any thoughts on this?
From my perspective, it makes most sense to leverage the transport here. My first thoughts are adding an extension to HTTPError:
extension HTTPError {
init(mappedFrom error: some HTTPResponseConvertible) {
....
}
}
Errors would then be transformed by the transport, so that it ties in with how Hummingbird presents this - and so that Hummingbird(-based) middleware function as expected.
Oh, interesting.
Just so I understand, this would mean that:
protocol HTTPResponseConvertible.HTTPResponseConvertible.Until now I think we've been assuming that (3) would be the job of OpenAPIRuntime.
That's exactly what I've been thinking.
Edit: Both Vapor and Hummingbird already have protocols for the same purpose, with a concrete type as a default implementation. I think bridging to the framework's expected format/behaviour makes most sense here.
I'm extending the review period until Monday, Oct 7, 2024 to allow the ongoing discussion to converge and for the finalized proposal to be reviewed by the community.
To pull on this thread - if the protocol is defined by swift-openapi-runtime, but the transport is responsible for checking the errors and transforming them into a response, why do we need the protocol in the first place? As you mention, both Vapor and Hummingbird have their own protocols that adopters can already use today.
However, we have heard from some users that they'd prefer to handle their error -> HTTP response mapping within the OpenAPI, transport-agnostic code (thus this proposal). One example is the generated handlers (which are fully transport-agnostic) calling into utility code, which in turn is also transport-agnostic. Having to conform errors in the utility code using a transport-specific error mapping protocol might be a layering violation in some places. So I think having a transport-agnostic protocol here does make sense, and it allows adopters to bootstrap their preferred transport in one place, rather than propagating that knowledge into their utility code.
Personally, I think either the runtime library provides both the protocol and the mapping (be it a middleware or built into the generated code), or neither. I lean towards the former, but I also want to give this opportunity to anyone who prefers the latter to speak up and make their case.
I think that's a fair take as well. The only reason I'd prefer the transport to be "in on it" is because some middleware in Hummingbird or Vapor would want to know more about the error - as defined by the protocol.
Ultimately, I think it would be best to define these protocols elsewhere - for example in the swift-middleware package.
I'm a proponent of writing transport agnostic handler logic. It's for more than just maintaining choice of backend when standing up the HTTP server, but opens the door to the same implementation being portable to different deployment environments entirely, e.g. using a Hummingbird transport in one setting and the AWS Lambda transport in another.
If the protocol is provided in the OpenAPIRuntime library, then nothing stops a transport package being in on it, but I'm not sure about the development experience here. For example, if the adopters conformed some of their errors to the protocol and chose not to use the transport-agnostic middleware provided by OpenAPIRuntime, then, in theory, the transport could do something with the untransformed error. But I do wonder if that would be a little surprising to the adopter.
Similarly to converging on single bag-of-bytes type, we didn't want to couple this ecosystem-wide decision with the development of Swift OpenAPI. I expect that once a unified Swift Middleware package is ready, we'd need to revisit how Swift OpenAPI composes with it.
My take is that it's unexpected that the transport is not aware. Though there are many other folks that should chime in on that so we don't have a sample size of 2. But I think either way is fine if documented as such.
Right, it's not something that's ready now anyways.
That's fair, but since this proposal is purely additive, users can continue doing what they're doing today - either rolling their custom solution, or relying on the transport-specific middleware.
After thinking about this more, I tend to agree.
To summarize, I think so far there seems to be consensus on:
What is still being discussed:
The above was to help converge this discussion (from my role as review manager of this proposal).
My personal lean is the original proposal and using a single protocol for all 3 requirements.
@gayathrisairam, please update us which way you're leaning now, and anyone else, please chime in here before the end of the review period on Oct 7. Thanks!
Is the mapping done by the runtime, or is that a new optional feature of transports?
If the former, is the mapping done in the generated code around the handler, or is it a middleware that the user inserts at whichever position in their middleware stack that they want?