I'm leaning towards it being a middleware, I think @beaumont had a similar lean.
If that's workable for you, I think it would be good to update the proposal now to be reflective of where we got in this discussion. Just add a Versions section (check out the proposal 12 for inspiration who did the same yesterday).
Thanks!
@Honza_Dvorsky - PR updated based on discussion from forums.
Great, thank you @gayathrisairam.
Last thing I'd like to bring back up is the name of the middleware: right now the proposed name is ErrorMiddleware, which is okay, but it's also pretty generic and might not convey what the middleware does. Is it a client or a server middleware/does it throw errors or catch errors/etc.
Some potential other names:
HTTPResponseConvertibleErrorMiddleware (most verbose, but unambiguous)ResponseConvertibleErrorMiddleware (drop the HTTP)ErrorToHTTPResponseConvertingMiddleware (that's what it does)ErrorToHTTPResponseMiddleware (can we imply "conversion" with "to"?)Do other folks have a preference?
IMO these have swung too far the other way.
If we were to provide middlewares for observability I think we'd be happy calling them e.g. MetricsMiddleware or TracingMiddleware, and I don't think we'd go as far as RecordingHTTPResponseAsMetricMiddleware. I realise this is a bit of an artificial example and exaggerated for effect, but with that in mind, how much lifting do we really need the name to do here?
I do accept that ErrorMiddleware is very vague—heck, it could even be injecting errors for testing, or something. Would ErrorHandlingMiddleware suffice?
Yes, or ErrorConvertingMiddleware (an uglier name).
One question we haven't discussed is the implementation of the middleware, which might affect the name.
When it catches an error that doesn't conform to HTTPResponseConvertible, what does it do?
Option 1: It rethrows it, letting the transport handle it.
Option 2: It returns some default response code, such as 500.
If Option 1, it provides the promised functionality but still lets the transport handle default errors. So it wouldn't handle all errors.
If Option 2, it truly handles all errors and never itself throws an error upstream. In that case, the ErrorHandlingMiddleware would be accurate, we'd just have to choose good defaults for the non-conforming errors.
I personally prefer option 2, because if an adopter has opted-in to this middleware it would be nice for them to be able to reason about it in one place and have this all be orthogonal to any transport-specific logic.
Open to hear other arguments, though.
I agree, thoughts, @gayathrisairam @adam-fowler @Joannis_Orlandos? I also wonder if the Vapor folks have had a chance to skim this proposal, @0xTim?
I prefer option 2 for the same reason that @beaumont has provided.
After some further thought I concur with option 2. So everything is handled by the openapi generator
Great, thanks everyone.
@gayathrisairam, please update the proposal to reflect the new middleware name ErrorHandlingMiddleware, and add a sentence about the expected behavior (that it handles all errors, and returns 500 for errors that don't conform to the protocol).
After that, we can finalize the review. Thanks!
Thanks for the updates, @gayathrisairam, this proposal is now Ready for Implementation.
The next steps are to open a PR with the implementation to GitHub - apple/swift-openapi-runtime: API package for code generated by Swift OpenAPI Generator., and also to enhance the documentation/examples in GitHub - apple/swift-openapi-generator: Generate Swift client and server code from an OpenAPI document., to reach adopters how to use this new API.
Sorry for the delay! Happy with the direction this is headed in and all makes sense