-
-
Notifications
You must be signed in to change notification settings - Fork 902
Ensure raw error for API exceptions by marking NiceGUI page requests in request scope #5510
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
Ensure raw error for API exceptions by marking NiceGUI page requests in request scope #5510
Conversation
falkoschindler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @evnchn!
Looks good to me. 😊
falkoschindler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I approved too early. I found an issue while the CI was running:
We are logging twice for raw API requests.
-
Should we move
log.exception(exception)below the early exit? Otherwise we log twice for exceptions in raw API endpoints. -
Alternatively we could return a response rather than raising:
return PlainTextResponse('Internal Server Error', status_code=500)
This way the
_exception_handler_500always logs and - depending on the scope - returns a plain text or page response, without raising and letting Starlette handle the problem intransparently.
What do you think?
evnchn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix things. We do not mock the Internal Server Error because one may customize the FastAPI error handling.
|
And if I am not mistaken, the native FastAPI behavior is to not log. Not nice to the developer, but again, it's not called NiceAPI. |
Motivation
Fix #5508, where NiceGUI returns the HTML for the "sad face error page" under this FastAPI GET request:
Problems:
client_idis dynamic)Optimally for
@app, it should act as much like FastAPI as possible. This PR does exactly that.Implementation
return client.build_response(request, 500)if foundraise exceptionand FastAPI proceeds to "Internal Server Error" exactly like if a plain FastAPI app would.Results
Progress