Skip to content

Conversation

thrau
Copy link
Member

@thrau thrau commented Mar 2, 2023

This PR adds logic to our fallback exception handler to render werkzeug HTTPException instances.

Previously, calling internal resources (or custom routes that would not be detected as s3 routes) with wrong methods would output:

{
  "error": "Unexpected exception",
  "message": "405 Method Not Allowed: The method is not allowed for the requested URL.",
  "type": "MethodNotAllowed"
}

because it interprets the MethodNotAllowed exception as unexpected exception. with this new logic that generalize capturing of werkzeug HTTP exceptions, you can actually raise HTTP exceptions from within handlers, and get proper exception messages:

{
  "error": "Method Not Allowed",
  "message": "The method is not allowed for the requested URL."
}

Unfortunately, because the service name parser is still executed before the router handler, it doesn't work for most requests, since those are captured by the ServiceExceptionSerializer which will serialize exceptions into s3 exceptions although the calls are not being made to s3. I added a bunch of tests & xfailed tests to document the expected behavior once that is remedied.

@thrau thrau requested a review from alexrashed March 2, 2023 23:22
@coveralls
Copy link

Coverage Status

Coverage: 85.117% (-0.007%) from 85.123% when pulling 17a30a1 on http-exception-handler into 4ac24cb on master.

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

The implementation is looking good, and it will become quite useful once we can move the service name parser down the handler chain! 🚀
I only have one comment concerning a unit test, but nothing blocking a merge.

@thrau thrau merged commit e858045 into master Mar 17, 2023
@thrau thrau deleted the http-exception-handler branch March 17, 2023 21:16
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