feat(routers): move DiracHttpResponseError to routers layer#988
feat(routers): move DiracHttpResponseError to routers layer#988mmascher wants to merge 7 commits into
Conversation
mmascher
commented
Jul 10, 2026
mmascher
commented
- move DiracHttpResponseError from core exceptions to diracx-routers
- update imports in router factory and auth token router
- keep app-level custom exception handling behavior unchanged
- document error-boundary/fallback intent in coding conventions
- normalize remaining e.args[0] usage to str(e) for consistency
* move DiracHttpResponseError from core exceptions to diracx-routers * update imports in router factory and auth token router * keep app-level custom exception handling behavior unchanged * document error-boundary/fallback intent in coding conventions * normalize remaining e.args[0] usage to str(e) for consistency
There was a problem hiding this comment.
Pull request overview
This PR moves DiracHttpResponseError out of diracx-core and into the diracx-routers layer to better reflect the HTTP boundary, while keeping the existing app-level exception-handling behavior intact. It also standardizes exception-to-string conversion in a few places and documents the intended error-handling boundaries in the developer conventions.
Changes:
- Relocate
DiracHttpResponseErrorintodiracx-routersand update router/factory imports accordingly. - Normalize some
e.args[0]usage tostr(e)for safer, consistent error-string extraction. - Document error-handling boundaries and the intended “fallback vs explicit mapping” behavior in coding conventions.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/dev/reference/coding-conventions.md | Documents the intended exception boundary and fallback handler behavior, including when to use DiracHttpResponseError. |
| diracx-routers/src/diracx/routers/factory.py | Updates imports and keeps the existing exception handler wiring; switches cached DB-unavailable reason extraction to str(e). |
| diracx-routers/src/diracx/routers/exceptions.py | Introduces DiracHttpResponseError in the routers layer. |
| diracx-routers/src/diracx/routers/auth/token.py | Updates the import location for DiracHttpResponseError used by the token endpoint’s OAuth-style error payload. |
| diracx-routers/src/diracx/routers/auth/device_flow.py | Replaces e.args[0] with str(e) when translating exceptions to HTTP error details. |
| diracx-core/src/diracx/core/exceptions.py | Removes DiracHttpResponseError from core exceptions and clarifies DiracError’s role as a fallback HTTP representation. |
| .gitignore | Ignores a TIPS file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
aldbr
left a comment
aldbr
left a comment
There was a problem hiding this comment.
Thanks a lot for your PR!
The documentation looks reasonable. I just a few comments.
Note: the changes you made, or my suggestions, are breaking changes, because extensions could, in theory, already rely on these DiracHttpResponseError and DiracError.
In practice, only LHCb is running an extension in production as of now so it should be ok. We would just need to double check on our side + ask other VOs running diracx extensions in certification if they are already relying on the exceptions.
|
@aldbr I didn't realize moving Leaving it in Let's make sure my very first contribution is safe and doesn't break extensions or upset people. 😄 |
Remove local ignored file Co-authored-by: aldbr <aldbr@outlook.com>
* return JSONResponse directly for authorization_pending
…ponse in token flow
|
@aldbr I think I am done here, I added commits as a reference and then solved the various threads. |
