-
Notifications
You must be signed in to change notification settings - Fork 5
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
Make API errors uniform #480
Conversation
ef61664
to
23f3120
Compare
|
||
|
||
async def handle_internal_error(request: Request, exception: Exception): | ||
# Don't expose this unexpected internal error as that could expose a security vulnerability. |
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.
We could also change that behavior and expose our internal errors. Considering our code is open source, that might not be a real threat.
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.
Hmm interesting, maybe we start with not exposing them, and see if it happens often enough?
with a JSON body `{"detail": "..."}`
23f3120
to
c8b3705
Compare
return JSONResponse( | ||
status_code=HTTP_404_NOT_FOUND # for errors in paths, e.g., /dataset_splits/potato | ||
if "path" in (error["loc"][0] for error in exception.errors()) | ||
else HTTP_400_BAD_REQUEST, # for other errors like in query params, e.g., pipeline_index=-1 | ||
content={"detail": str(exception)}, | ||
) |
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.
I am not sure the { "detail": "..." }
JSON object wrapping the error message is useful. I could instead return a plain text error message.
return JSONResponse( | |
status_code=HTTP_404_NOT_FOUND # for errors in paths, e.g., /dataset_splits/potato | |
if "path" in (error["loc"][0] for error in exception.errors()) | |
else HTTP_400_BAD_REQUEST, # for other errors like in query params, e.g., pipeline_index=-1 | |
content={"detail": str(exception)}, | |
) | |
return PlainTextResponse( | |
status_code=HTTP_404_NOT_FOUND # for errors in paths, e.g., /dataset_splits/potato | |
if "path" in (error["loc"][0] for error in exception.errors()) | |
else HTTP_400_BAD_REQUEST, # for other errors like in query params, e.g., pipeline_index=-1 | |
content=str(exception), | |
) |
I would simply add an exception handler for HTTPException
that does the same thing. I could then remove the 500 exception handler, as it was already returning plain text.
However, FastAPI's http_exception_handler()
for HTTPException
is a little bit more sophisticated as it supports setting response headers, so I thought it was cool to keep it as is, but it's also not the end of the world; just a couple of lines to copy-paste.
Let me know if you have an opinion on this.
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.
I'll let @christyler3030 make the official review, it's a bit over my head :)
by calling `str(e)` instead of `e.json()`.
Description:
Don't worry, this is less than 50 lines of code if we ignore the generated types.
We used to have 3 different schemas of errors (as follows).
HTTPException
s we raise in our code usually have adetail
string that is used to produce a JSON response. This stays the same. For example:ValidationError
s FastAPI raises have a harder-to-read JSON response. For exampleThis PR transforms it to
so we can use the
detail
in the front end:This PR transforms it to
Checklist:
You should check all boxes before the PR is ready. If a box does not apply, check it to acknowledge it.
ran
pre-commit run --all-files
at the end.our users.
README
files and our wiki for any big design decisions, if relevant.