Skip to content

Conversation

nielsbox
Copy link
Collaborator

@nielsbox nielsbox commented Nov 28, 2023

Fixes #1820.
Correct error handling in response to CORS.

Changes proposed in this pull request:

  • Add a MiddlewarePosition before Exception handling so CORS is always returned
  • Add ServerError Middleware to handle unhandled errors between the ServerError- and ExceptionMiddleware
  • Update corresponding docs

@nielsbox nielsbox force-pushed the bugfix/cors-headers-on-exception branch from 55c82b1 to a0f2647 Compare November 28, 2023 14:49
@nielsbox nielsbox force-pushed the bugfix/cors-headers-on-exception branch 2 times, most recently from ad1ee80 to 4628bcb Compare November 28, 2023 16:54
@nielsbox nielsbox force-pushed the bugfix/cors-headers-on-exception branch from 4628bcb to 1a43872 Compare November 28, 2023 16:55
@coveralls
Copy link

coveralls commented Nov 28, 2023

Coverage Status

coverage: 94.266% (-0.02%) from 94.287%
when pulling 918d58c on bugfix/cors-headers-on-exception
into bbd085b on main.

@RobbeSneyders
Copy link
Member

Thanks @nielsbox!

This now returns different errors than the ExceptionMiddleware though. We probably want to align it:

@staticmethod
def common_error_handler(
_request: StarletteRequest, exc: Exception
) -> ConnexionResponse:
"""Default handler for any unhandled Exception"""
logger.error("%r", exc, exc_info=exc)
return InternalServerError().to_problem()

@nielsbox nielsbox requested a review from Ruwann November 30, 2023 14:16
Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks @nielsbox! Really clean.

I had some minor comments on the docs and the typing, so I quickly added commits handling those so I can merge and release.

@RobbeSneyders RobbeSneyders merged commit 0082d7a into main Nov 30, 2023
@RobbeSneyders RobbeSneyders deleted the bugfix/cors-headers-on-exception branch November 30, 2023 23:04
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.

CORS headers not set on exception when using MiddlewarePosition.BEFORE_ROUTING
4 participants