-
Notifications
You must be signed in to change notification settings - Fork 29
fix(web): add exception handling #269
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks @amit-nagish, this issue does look like an oversight. I probably assumed that the response body didn't matter much if the status was not 200, but what you have here looks much better. I might have more to say once I get a chance to properly review it. |
|
Hi @goodmami ! did you get a chance to take a closer look? I have a bunch of updates to the web server in my fork / locally that I believe improve the experience, and would love to contribute them one by one |
I'm currently traveling, which is why I'm being slow to respond. I hope to have some more time to review in the coming days. Thanks for your patience. |
goodmami
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.
Mostly looks good. I just have some style suggestions.
Also instead of making calls with dicts like this:
some_function({
"key": val,
}, keyword=xyz)Let's try to be more compliant with code formatting tools like Ruff/Black:
some_function(
{
"key": val,
},
keyword=xyz,
)| # Exception handlers | ||
|
|
||
| async def http_exception_handler(request: Request, exc: Exception): | ||
| status_code = exc.status_code if hasattr(exc, 'status_code') else 500 |
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 is recreating getattr behavior:
| status_code = exc.status_code if hasattr(exc, 'status_code') else 500 | |
| status_code = getattr(exc, 'status_code', 500) |
| return JSONResponse({ | ||
| "error": { | ||
| "status": status_code, | ||
| "message": exc.detail if hasattr(exc, 'detail') else str(exc), |
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 could also be a getattr call. There's a case for leaving it as an inline conditional if str(exc) is something you don't want to call unless it's needed, but I don't think that's a big concern here.
| app = Starlette(debug=True, routes=routes, exception_handlers={ | ||
| HTTPException: http_exception_handler, | ||
| wn.Error: http_exception_handler, | ||
| Exception: http_exception_handler, | ||
| }) |
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.
Maybe add this above at the end of the "Exception handlers" section of the code:
exception_handlers = {
HTTPException: http_exception_handler,
wn.Error: http_exception_handler,
Exception: http_exception_handler,
}Then simplify this line as follows:
| app = Starlette(debug=True, routes=routes, exception_handlers={ | |
| HTTPException: http_exception_handler, | |
| wn.Error: http_exception_handler, | |
| Exception: http_exception_handler, | |
| }) | |
| app = Starlette(debug=True, routes=routes, exception_handlers=exception_handlers) |
when making an incorrect request, i want to see a json response, not an HTML.
For example, I ran http://127.0.0.1:8000/lexicons/omw-en:1.4/words/bass (which is wrong, should have done
?form=bass) and it gave me an HTML error rather than JSON