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

Remove common error handler from ExceptionMiddleware #1841

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

julienschuermans
Copy link
Contributor

This PR removes the catch-all error handler for unhandled exceptions from the ExceptionMiddleware.

Since the introduction of @nielsbox' ServerErrorMiddleware, the duplication of this feature has become kind of confusing / unnecessary.

@RobbeSneyders
Copy link
Member

Thanks @julienschuermans.

Agree with the goal of the PR, but seems like we need some more work to fix this since the tests are failing.

As far as I understand the issue, we are wrapping the error_response in the ServerErrorMiddleware with our connexion_wrapper from the ExceptionMiddleware which returns a coroutine, while the ServerErrorMiddleware expects a normal function here. We could solve this by just implementing the relevant part (translating between Connexion and Starlette) synchronously in the error_response method.

Another difference is that the ServerErrorMiddleware still raises the error after returning the error response. So we'll need to update our tests to accommodate this and catch the error. We also need to check what the effect of this is when running the application with different servers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants