diff --git a/diracx-core/src/diracx/core/exceptions.py b/diracx-core/src/diracx/core/exceptions.py index a4d927393..889752c50 100644 --- a/diracx-core/src/diracx/core/exceptions.py +++ b/diracx-core/src/diracx/core/exceptions.py @@ -3,7 +3,6 @@ __all__ = [ "AuthorizationError", "DiracError", - "DiracHttpResponseError", "IAMClientError", "IAMServerError", "InvalidCredentialsError", @@ -16,19 +15,8 @@ "TokenNotFoundError", ] -from http import HTTPStatus - - -class DiracHttpResponseError(RuntimeError): - def __init__(self, status_code: int, data): - self.status_code = status_code - self.data = data - class DiracError(RuntimeError): - http_status_code = HTTPStatus.BAD_REQUEST # 400 - http_headers: dict[str, str] | None = None - def __init__(self, detail: str = "Unknown"): self.detail = detail diff --git a/diracx-routers/src/diracx/routers/auth/device_flow.py b/diracx-routers/src/diracx/routers/auth/device_flow.py index bfcf3bb8f..aeb70ccf3 100644 --- a/diracx-routers/src/diracx/routers/auth/device_flow.py +++ b/diracx-routers/src/diracx/routers/auth/device_flow.py @@ -78,7 +78,7 @@ async def initiate_device_flow( except ValueError as e: raise HTTPException( status_code=HTTPStatus.BAD_REQUEST, - detail=e.args[0], + detail=str(e), ) from e return device_flow_response @@ -116,19 +116,19 @@ async def do_device_flow( logger.warning("Invalid or expired user_code: %s", e) raise HTTPException( status_code=HTTPStatus.BAD_REQUEST, - detail=e.args[0], + detail=str(e), ) from e except ValueError as e: logger.warning("Invalid scope during device flow: %s", e) raise HTTPException( status_code=HTTPStatus.BAD_REQUEST, - detail=e.args[0], + detail=str(e), ) from e except IAMServerError as e: logger.warning("IAM server error during device flow: %s", e) raise HTTPException( status_code=HTTPStatus.BAD_GATEWAY, - detail=e.args[0], + detail=str(e), ) from e return RedirectResponse(authorization_flow_url) @@ -163,13 +163,13 @@ async def finish_device_flow( logger.warning("IAM server error during device flow completion: %s", e) raise HTTPException( status_code=HTTPStatus.BAD_GATEWAY, - detail=e.args[0], + detail=str(e), ) from e except IAMClientError as e: logger.warning("IAM client error during device flow completion: %s", e) raise HTTPException( status_code=HTTPStatus.UNAUTHORIZED, - detail=e.args[0], + detail=str(e), ) from e return RedirectResponse(f"{request_url}/finished") diff --git a/diracx-routers/src/diracx/routers/auth/token.py b/diracx-routers/src/diracx/routers/auth/token.py index 3b3f7942d..4dab15499 100644 --- a/diracx-routers/src/diracx/routers/auth/token.py +++ b/diracx-routers/src/diracx/routers/auth/token.py @@ -7,10 +7,10 @@ from typing import Annotated, Literal from fastapi import Depends, Form, Header, HTTPException +from fastapi.responses import JSONResponse from joserfc.errors import JoseError from diracx.core.exceptions import ( - DiracHttpResponseError, InvalidCredentialsError, PendingAuthorizationError, ) @@ -138,11 +138,11 @@ async def get_oidc_token( code_verifier=code_verifier, refresh_token=refresh_token, ) - except PendingAuthorizationError as e: - raise DiracHttpResponseError( + except PendingAuthorizationError: + return JSONResponse( status_code=HTTPStatus.BAD_REQUEST, - data={"error": "authorization_pending"}, - ) from e + content={"error": "authorization_pending"}, + ) except ValueError as e: raise HTTPException( status_code=HTTPStatus.BAD_REQUEST, diff --git a/diracx-routers/src/diracx/routers/factory.py b/diracx-routers/src/diracx/routers/factory.py index bbd58e227..98477a9e0 100644 --- a/diracx-routers/src/diracx/routers/factory.py +++ b/diracx-routers/src/diracx/routers/factory.py @@ -26,7 +26,7 @@ from uvicorn.logging import AccessFormatter, DefaultFormatter from diracx.core.config import ConfigSource -from diracx.core.exceptions import DiracError, DiracHttpResponseError, NotReadyError +from diracx.core.exceptions import DiracError, NotReadyError from diracx.core.extensions import DiracEntryPoint, select_from_extension from diracx.core.settings import FactorySettings, ServiceSettingsBase from diracx.core.sources import AsyncCacheableSource @@ -327,9 +327,6 @@ def create_app_inner( # with a subclass of Exception (https://mypy.readthedocs.io/en/latest/generics.html#variance-of-generic-types) handler_signature = Callable[[Request, Exception], Response | Awaitable[Response]] app.add_exception_handler(DiracError, cast(handler_signature, dirac_error_handler)) - app.add_exception_handler( - DiracHttpResponseError, cast(handler_signature, http_response_handler) - ) app.add_exception_handler( DBUnavailableError, cast(handler_signature, route_unavailable_error_hander) ) @@ -427,17 +424,15 @@ def create_app() -> DiracFastAPI: def dirac_error_handler(request: Request, exc: DiracError) -> Response: + status_code = getattr(exc, "http_status_code", HTTPStatus.BAD_REQUEST) + headers = getattr(exc, "http_headers", None) return JSONResponse( - status_code=exc.http_status_code, + status_code=status_code, content={"detail": exc.detail}, - headers=exc.http_headers, + headers=headers, ) -def http_response_handler(request: Request, exc: DiracHttpResponseError) -> Response: - return JSONResponse(status_code=exc.status_code, content=exc.data) - - def route_unavailable_error_hander(request: Request, exc: DBUnavailableError): logger.warning( "503 Service Unavailable: %s (path=%s)", @@ -505,7 +500,7 @@ async def is_db_unavailable(db: BaseSQLDB | BaseOSDB) -> str: _db_alive_cache[db] = "" except DBUnavailableError as e: - _db_alive_cache[db] = e.args[0] + _db_alive_cache[db] = str(e) return _db_alive_cache[db] diff --git a/docs/dev/reference/coding-conventions.md b/docs/dev/reference/coding-conventions.md index 0d8b93613..7def05a2f 100644 --- a/docs/dev/reference/coding-conventions.md +++ b/docs/dev/reference/coding-conventions.md @@ -135,3 +135,10 @@ To make sure that each part of DiracX is doing only what it is supposed to do, y - `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. - `diracx-logic` should embed Dirac specificities. It should encapsulate the logic of the services and should call `diracx-db` to interact with databases. - `diracx-db` should contain atomic methods (complex logic is expected to be located in `diracx-db`). + +### Error handling boundaries + +- `diracx-db` and `diracx-logic` should raise domain exceptions (typically `DiracError` subclasses), and should not depend on `fastapi`. +- `diracx-routers` is the HTTP boundary and should translate known backend/domain exceptions into `HTTPException` with the desired status code, message, and headers. +- `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. +- 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.