Skip to content

Commit 953f442

Browse files
mmascherCopilotaldbr
authored
refactor(routers): remove DiracHttpResponseError and simplify OAuth pending response handling (#988)
* feat(routers): move DiracHttpResponseError to routers layer * 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 * Forgot to inline class name in coding-conventions.md Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Update .gitignore Remove local ignored file Co-authored-by: aldbr <aldbr@outlook.com> * refactor(routers): return OAuth pending response directly in token route * return JSONResponse directly for authorization_pending * refactor(routers): replace DiracHttpResponseError with direct JSONResponse in token flow * docs(dev-reference): use plain text path for factory.py reference --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: aldbr <aldbr@outlook.com>
1 parent dfda77a commit 953f442

5 files changed

Lines changed: 24 additions & 34 deletions

File tree

diracx-core/src/diracx/core/exceptions.py

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
__all__ = [
44
"AuthorizationError",
55
"DiracError",
6-
"DiracHttpResponseError",
76
"IAMClientError",
87
"IAMServerError",
98
"InvalidCredentialsError",
@@ -16,19 +15,8 @@
1615
"TokenNotFoundError",
1716
]
1817

19-
from http import HTTPStatus
20-
21-
22-
class DiracHttpResponseError(RuntimeError):
23-
def __init__(self, status_code: int, data):
24-
self.status_code = status_code
25-
self.data = data
26-
2718

2819
class DiracError(RuntimeError):
29-
http_status_code = HTTPStatus.BAD_REQUEST # 400
30-
http_headers: dict[str, str] | None = None
31-
3220
def __init__(self, detail: str = "Unknown"):
3321
self.detail = detail
3422

diracx-routers/src/diracx/routers/auth/device_flow.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ async def initiate_device_flow(
7878
except ValueError as e:
7979
raise HTTPException(
8080
status_code=HTTPStatus.BAD_REQUEST,
81-
detail=e.args[0],
81+
detail=str(e),
8282
) from e
8383

8484
return device_flow_response
@@ -116,19 +116,19 @@ async def do_device_flow(
116116
logger.warning("Invalid or expired user_code: %s", e)
117117
raise HTTPException(
118118
status_code=HTTPStatus.BAD_REQUEST,
119-
detail=e.args[0],
119+
detail=str(e),
120120
) from e
121121
except ValueError as e:
122122
logger.warning("Invalid scope during device flow: %s", e)
123123
raise HTTPException(
124124
status_code=HTTPStatus.BAD_REQUEST,
125-
detail=e.args[0],
125+
detail=str(e),
126126
) from e
127127
except IAMServerError as e:
128128
logger.warning("IAM server error during device flow: %s", e)
129129
raise HTTPException(
130130
status_code=HTTPStatus.BAD_GATEWAY,
131-
detail=e.args[0],
131+
detail=str(e),
132132
) from e
133133
return RedirectResponse(authorization_flow_url)
134134

@@ -163,13 +163,13 @@ async def finish_device_flow(
163163
logger.warning("IAM server error during device flow completion: %s", e)
164164
raise HTTPException(
165165
status_code=HTTPStatus.BAD_GATEWAY,
166-
detail=e.args[0],
166+
detail=str(e),
167167
) from e
168168
except IAMClientError as e:
169169
logger.warning("IAM client error during device flow completion: %s", e)
170170
raise HTTPException(
171171
status_code=HTTPStatus.UNAUTHORIZED,
172-
detail=e.args[0],
172+
detail=str(e),
173173
) from e
174174

175175
return RedirectResponse(f"{request_url}/finished")

diracx-routers/src/diracx/routers/auth/token.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@
77
from typing import Annotated, Literal
88

99
from fastapi import Depends, Form, Header, HTTPException
10+
from fastapi.responses import JSONResponse
1011
from joserfc.errors import JoseError
1112

1213
from diracx.core.exceptions import (
13-
DiracHttpResponseError,
1414
InvalidCredentialsError,
1515
PendingAuthorizationError,
1616
)
@@ -138,11 +138,11 @@ async def get_oidc_token(
138138
code_verifier=code_verifier,
139139
refresh_token=refresh_token,
140140
)
141-
except PendingAuthorizationError as e:
142-
raise DiracHttpResponseError(
141+
except PendingAuthorizationError:
142+
return JSONResponse(
143143
status_code=HTTPStatus.BAD_REQUEST,
144-
data={"error": "authorization_pending"},
145-
) from e
144+
content={"error": "authorization_pending"},
145+
)
146146
except ValueError as e:
147147
raise HTTPException(
148148
status_code=HTTPStatus.BAD_REQUEST,

diracx-routers/src/diracx/routers/factory.py

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
from uvicorn.logging import AccessFormatter, DefaultFormatter
2727

2828
from diracx.core.config import ConfigSource
29-
from diracx.core.exceptions import DiracError, DiracHttpResponseError, NotReadyError
29+
from diracx.core.exceptions import DiracError, NotReadyError
3030
from diracx.core.extensions import DiracEntryPoint, select_from_extension
3131
from diracx.core.settings import FactorySettings, ServiceSettingsBase
3232
from diracx.core.sources import AsyncCacheableSource
@@ -327,9 +327,6 @@ def create_app_inner(
327327
# with a subclass of Exception (https://mypy.readthedocs.io/en/latest/generics.html#variance-of-generic-types)
328328
handler_signature = Callable[[Request, Exception], Response | Awaitable[Response]]
329329
app.add_exception_handler(DiracError, cast(handler_signature, dirac_error_handler))
330-
app.add_exception_handler(
331-
DiracHttpResponseError, cast(handler_signature, http_response_handler)
332-
)
333330
app.add_exception_handler(
334331
DBUnavailableError, cast(handler_signature, route_unavailable_error_hander)
335332
)
@@ -427,17 +424,15 @@ def create_app() -> DiracFastAPI:
427424

428425

429426
def dirac_error_handler(request: Request, exc: DiracError) -> Response:
427+
status_code = getattr(exc, "http_status_code", HTTPStatus.BAD_REQUEST)
428+
headers = getattr(exc, "http_headers", None)
430429
return JSONResponse(
431-
status_code=exc.http_status_code,
430+
status_code=status_code,
432431
content={"detail": exc.detail},
433-
headers=exc.http_headers,
432+
headers=headers,
434433
)
435434

436435

437-
def http_response_handler(request: Request, exc: DiracHttpResponseError) -> Response:
438-
return JSONResponse(status_code=exc.status_code, content=exc.data)
439-
440-
441436
def route_unavailable_error_hander(request: Request, exc: DBUnavailableError):
442437
logger.warning(
443438
"503 Service Unavailable: %s (path=%s)",
@@ -505,7 +500,7 @@ async def is_db_unavailable(db: BaseSQLDB | BaseOSDB) -> str:
505500
_db_alive_cache[db] = ""
506501

507502
except DBUnavailableError as e:
508-
_db_alive_cache[db] = e.args[0]
503+
_db_alive_cache[db] = str(e)
509504

510505
return _db_alive_cache[db]
511506

docs/dev/reference/coding-conventions.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,3 +135,10 @@ To make sure that each part of DiracX is doing only what it is supposed to do, y
135135
- `diracx-routers` should deal with user interactions through HTTPs. It is expected to deal with permissions and should call `diracx-logic`. Results returned should be translated into HTTP responses.
136136
- `diracx-logic` should embed Dirac specificities. It should encapsulate the logic of the services and should call `diracx-db` to interact with databases.
137137
- `diracx-db` should contain atomic methods (complex logic is expected to be located in `diracx-db`).
138+
139+
### Error handling boundaries
140+
141+
- `diracx-db` and `diracx-logic` should raise domain exceptions (typically `DiracError` subclasses), and should not depend on `fastapi`.
142+
- `diracx-routers` is the HTTP boundary and should translate known backend/domain exceptions into `HTTPException` with the desired status code, message, and headers.
143+
- `DiracError` handling registered at app level (see diracx-routers/src/diracx/routers/factory.py, especially `app.add_exception_handler(DiracError, cast(handler_signature, dirac_error_handler))`) is a fallback safety net for uncaught domain exceptions, not a replacement for explicit mapping when the router needs specific HTTP behavior.
144+
- Protocol-specific responses can be handled directly in routers when `HTTPException` shape is not suitable (for example OAuth2-style payloads in tokens.py), e.g. by returning a `JSONResponse` directly.

0 commit comments

Comments
 (0)