Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error handling middleware #126

Merged
merged 19 commits into from
Nov 20, 2024

Conversation

gayathrisairam
Copy link
Contributor

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.

Copy link
Collaborator

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up @gayathrisairam!

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

Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gayathrisairam gayathrisairam force-pushed the error_handling_middleware branch from d235fb3 to 60cb9b9 Compare November 19, 2024 00:25
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@czechboy0
Copy link
Contributor

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

Gayathri Sairamkrishnan and others added 19 commits November 20, 2024 12:33
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
…re.swift

Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
@gayathrisairam gayathrisairam force-pushed the error_handling_middleware branch from 0eeeb48 to 4e7c638 Compare November 20, 2024 12:35
@czechboy0
Copy link
Contributor

Looks great, thank you @gayathrisairam! 👏

@czechboy0 czechboy0 merged commit 3d5d957 into apple:main Nov 20, 2024
25 checks passed
@czechboy0 czechboy0 added the 🆕 semver/minor Adds new public API. label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants