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

Default Pyotr and Starlette way of exception handling loses important information from InvalidSecurity and OpenAPIError #17

Open
insi-eb opened this issue Oct 27, 2021 · 2 comments
Labels
enhancement New feature or request
Projects

Comments

@insi-eb
Copy link

insi-eb commented Oct 27, 2021

Is your feature request related to a problem? Please describe.
When anything in a request is not exactly as expected in the OpenAPI spec, I only get "Bad request" without any further information, as the default configuration of Pyotr and Starlette just prints the status code (and translation) of the HTTPException raised in the wrapper. This makes debugging any issues much harder than necessary:

e.g. on the server log I only get
INFO: 127.0.0.1:37666 - "POST /bla HTTP/1.1" HTTPStatus.BAD_REQUEST Bad Request

and on the client only:
Bad Request

Describe the solution you'd like
The easiest way to fix this would be to change the wrapper catch blocks as follows:

            except InvalidSecurity as ex:
                raise HTTPException(HTTPStatus.FORBIDDEN, "Invalid security: " + str(ex)) from ex
            except OpenAPIError as ex:
                raise HTTPException(HTTPStatus.BAD_REQUEST, "Bad request: " + str(ex)) from ex

This will then still only show the "Bad Request" status in the server log, but the client will get the complete information from e.g. the OpenAPIError:
Bad request: Content for the following mimetype not found: application/x-www-form-urlencoded. Valid mimetypes: ['application/json']

Describe alternatives you've considered
Extend the documentation to recommend installing an ExceptionHandler that provides also the info from the other exceptions that have been chained to the HTTPException. I haven't tried this yet, so I don't know whether that works or how much effort that really is.

Additional context
n/a

@berislavlopac berislavlopac added this to Awaiting triage in Server via automation Oct 29, 2021
@berislavlopac berislavlopac added the enhancement New feature or request label Oct 29, 2021
@berislavlopac
Copy link
Owner

The problem with passing the detailed information in the response is that it can inadvertently reveal too much information, especially in the case of InvalidSecurity and similar issues.

A quick solution is to allow passing the additional information only if debug is on, which might be good enough for now. In a longer term, it might be beneficial to implement a mechanism for passing structured error messages, with a customisable structure.

As for logging, Pyotr has been completely relying on Starlette's logging mechanism; it might be a good idea to improve on it.

@insi-eb
Copy link
Author

insi-eb commented Nov 4, 2021

yes, checking whether debug is on would be a good compromise between too little and too much information. Anyone using this not in production (i.e. me) can enable debug to their hearts content.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Server
  
Awaiting triage
Development

No branches or pull requests

2 participants