Add exception handling to the rest api #3708
Conversation
| [tool.pytest.ini_options] | ||
| addopts = "--cov scheduler/ --cov-config=pyproject.toml --cov-report xml --cov-branch --cov-report=term-missing:skip-covered" |
There was a problem hiding this comment.
Added such that coverage from pytest doesn't include unnecessary folders/files to count towards coverage
There was a problem hiding this comment.
Looks good to me, but I was hoping that this #3695 (comment) would be implemented. But perhaps there has been a misunderstanding with the term exception_handler? What I meant was using FastAPI's way of dealing with custom exceptions like this. This is a common and less verbose solution. It works pretty well and we've also used this in other places
Co-authored-by: Jan Klopper <janklopper+underdark@gmail.com>
mula/scheduler/server/errors.py
Outdated
| def exception_handler(request: fastapi.Request, exc: Exception): | ||
| return JSONResponse( | ||
| status_code=fastapi.status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| content={"detail": f"An internal error occurred: {exc}"}, | ||
| ) |
There was a problem hiding this comment.
If I'm not mistaken this isn't needed because it's handled by the default exception handler
There was a problem hiding this comment.
This is indeed the nicer approach. The only addition here could be importing from FastAPI import status and use status.HTTP_... instead. This would make some lines a bit shorter, but more importantly consistent with the way you're already doing in the scheduler.server.handlers package.
Checklist for QA:
What works:Doesn't seem to break anything. Not really sure if all the error messages are coming through. Double checking this with @jpbruinsslot. In either case this can be merged and if the error messages are not coming through properly we can fix that in additional PRs. What doesn't work:n/a Bug or feature?:n/a |
|
* main: Fix reports with organization tags (#3790) Update README.rst - Fix guidelines URLs (#3789) Exclude Report from ooi list (#3768) Update `croniter` (#3767) Fixes for dropdowns (#3732) Fix scheduled Aggregate Report naming (#3748) Make systemctl call for kat-rocky-worker conditional (#3782) Fix vulnerability chapters in Aggregate table of content (#3780) Fix auth token middleware with wrong format header (#3755) Add rocky REST API for report recipes (#3746) Add exception handling to the rest api (#3708) Refactor Multi Report to comply to the new report flow (#3705) Fix report names for scheduled reports (#3726) Fix Multi Report recursion error (#3714) Bump waitress from 3.0.0 to 3.0.1 in /octopoes (#3760) Docs/add muted findings (#3699) Edit report recipe (#3690) Add start date to report schedule (#3701) Add REST API to list report and download pdf report (#3689) Fixes in Report Overview (#3707)
Changes
exception_handlerdecorator for the rest API in the scheduler. Mainly cleans up repeated code in the scheduler api.Issue link
Closes #2994
QA notes
Since I restructured and deleted some code from the rest API, the main gist is that everything should still work with services that interact with the scheduler. Everything should return the correct response and status codes with normal usage.
Perhaps checking if error messages in rocky are relayed correctly when interacting with the scheduler.
Code Checklist
.envchanges files if required and changed the.env-distaccordingly.Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.