diff --git a/src/aare/daq/server_exception_handler.py b/src/aare/daq/server_exception_handler.py index e064a013..86342735 100644 --- a/src/aare/daq/server_exception_handler.py +++ b/src/aare/daq/server_exception_handler.py @@ -1,319 +1,202 @@ # aare/common/server_error_handler.py +"""Server-side exception handlers for the DAQ FastAPI app. + +Per the exception-handling redesign (EXCEPTION_REDESIGN_PLAN.md §2), error +handling is collapsed into a small number of handlers that route on the new +``AareException`` hierarchy: + +- ``AutomationError``: 503 if ``critical``, else 422 +- ``AareUserError``: 400 +- ``AareAuthError``: 401 (auth) or 403 (rights) +- ``HTTPException``: pass-through status, with the new body shape +- ``Exception``: 500, marked critical (fail-safe for unknown errors) + +All handlers produce the unified response body defined in §3: + +.. code-block:: json + + { + "critical": true, + "code": "TELL_COMMUNICATION_ERROR", + "exception_class": "TellCommunicationError", + "message": "Tell did not respond within 5s", + "context": {"endpoint": "/state", "operation": "GET"} + } +""" from __future__ import annotations +import json +from typing import Any + from fastapi import HTTPException from fastapi import status as api_status from starlette.requests import Request from starlette.responses import JSONResponse from aare.common.logger_config import setup_logger +from aare.common.error_codes import AareErrorCode, code_for_exception_class from aare.common.exception_handler import ( - MountingFailed, - WarningTellException, - CriticalTellException, - LoopCenteringFailed, - TransformationInvalidException, - StateTransitionFailed, - MaintenanceStateException, - BeamlineBusyException, + AareException, + AutomationError, + AareUserError, + AareAuthError, AuthenticationException, - SampleException, - UserRightsException, - SmargonCommunicationError, - TellCommunicationError, - BECCommunicationError, - JFJochCommunicationError, - AerotechCommunicationError, - DataCollectionException, - RasterScanException, - UnmountingFailed, - AXCFailed, - AareDBCommunicationError, - MagnetPositionSensorErorr, - ManualMountException, - SmartMagnetFaultException, - TellMountFailedException, - TellCommandWhileBusyException, - TellConnectionException, ) logger = setup_logger("aareDAQ") -def _error_payload(*, code: str, message: str, extra: dict | None = None) -> dict: - payload = {"code": code, "message": message} - if extra: - payload["extra"] = extra - return payload +# Attributes that live on exceptions for routing/transport, NOT for the +# context payload. +_NON_CONTEXT_ATTRS = frozenset({ + "args", + "message", + "critical", + "code", + "headers", + "status_code", + "exception", # BECCommunicationError stores a Python exception here -- not JSON-safe +}) + + +def _extract_context(exc: Exception) -> dict[str, Any]: + """Pull JSON-serializable structured fields off an exception for the + response ``context`` field. + + Allow-list approach via ``_NON_CONTEXT_ATTRS`` exclusions: walks + ``vars(exc)`` and includes anything that's not private, not in the + excluded set, and JSON-serializable. Per plan §3 the schema is + per-exception; clients tolerate unknown keys.""" + ctx: dict[str, Any] = {} + for k, v in vars(exc).items(): + if k.startswith("_"): + continue + if k in _NON_CONTEXT_ATTRS: + continue + if v is None: + continue + try: + json.dumps(v) + except (TypeError, ValueError): + continue + ctx[k] = v + return ctx + + +def _error_body(exc: Exception, *, code_override: str | None = None, + critical_override: bool | None = None) -> dict[str, Any]: + """Build the unified error response body for any exception. + + ``code_override`` is used by the auth handler to surface the finer-grained + ``AuthErrorCode`` carried on the instance, instead of the class-derived + code. ``critical_override`` is used by the bare-Exception fallback to + force critical=True regardless of the exception's own flag.""" + if code_override is not None: + code = code_override + else: + code = code_for_exception_class(type(exc).__name__) + + if critical_override is not None: + critical = critical_override + else: + # Default for non-AareException is critical=True (fail-safe). + critical = bool(getattr(exc, "critical", True)) + + return { + "critical": critical, + "code": code, + "exception_class": type(exc).__name__, + "message": str(exc) or type(exc).__name__, + "context": _extract_context(exc), + } def register_exception_handlers(app) -> None: """ - Register server-wide exception handlers on the given FastAPI app. - Call once right after `app = FastAPI()`. + Register the four routing-root handlers (plus HTTPException + Exception + fallback) on the given FastAPI app. Call once right after + ``app = FastAPI()``. """ - @app.exception_handler(HTTPException) - async def http_exception_handler(request: Request, exc: HTTPException) -> JSONResponse: - # Keep explicit HTTP errors, but normalize response shape - detail = exc.detail - if isinstance(detail, dict) and "code" in detail and "message" in detail: - body = detail - else: - body = _error_payload(code="HTTP_ERROR", message=str(detail)) - return JSONResponse(status_code=exc.status_code, content=body, headers=exc.headers) - - @app.exception_handler(MountingFailed) - async def mounting_failed_handler(request: Request, exc: MountingFailed) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_409_CONFLICT, - content=_error_payload(code="MOUNTING_FAILED", message=str(exc) or "Failed to mount sample"), + @app.exception_handler(AutomationError) + async def _handle_automation_error(request: Request, exc: AutomationError) -> JSONResponse: + status = ( + api_status.HTTP_503_SERVICE_UNAVAILABLE + if exc.critical + else api_status.HTTP_422_UNPROCESSABLE_ENTITY ) + return JSONResponse(status_code=status, content=_error_body(exc)) - @app.exception_handler(UnmountingFailed) - async def unmounting_failed_handler(request: Request, exc: UnmountingFailed) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_500_INTERNAL_SERVER_ERROR, - content=_error_payload(code="UNMOUNTING_FAILED", message=str(exc) or "Failed to unmount sample"), - ) - - @app.exception_handler(WarningTellException) - async def warning_tell_handler(request: Request, exc: WarningTellException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_410_GONE, - content=_error_payload(code="TELL_WARNING", message=str(exc)), - ) - - @app.exception_handler(CriticalTellException) - async def critical_tell_handler(request: Request, exc: CriticalTellException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_417_EXPECTATION_FAILED, - content=_error_payload(code="TELL_CRITICAL", message=str(exc)), - ) - - @app.exception_handler(LoopCenteringFailed) - async def loop_centering_failed_handler(request: Request, exc: LoopCenteringFailed) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_404_NOT_FOUND, - content=_error_payload(code="LOOP_CENTERING_FAILED", message=str(exc)), - ) - - @app.exception_handler(AXCFailed) - async def axc_failed_handler(request: Request, exc: AXCFailed) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_500_INTERNAL_SERVER_ERROR, - content=_error_payload(code="AXC_FAILED", message=str(exc) or "Auto X-ray centering failed"), - ) - - @app.exception_handler(TransformationInvalidException) - async def transformation_invalid_handler(request: Request, exc: TransformationInvalidException) -> JSONResponse: + @app.exception_handler(AareUserError) + async def _handle_user_error(request: Request, exc: AareUserError) -> JSONResponse: return JSONResponse( status_code=api_status.HTTP_400_BAD_REQUEST, - content=_error_payload(code="TRANSFORMATION_INVALID", message=str(exc)), + content=_error_body(exc), ) - @app.exception_handler(StateTransitionFailed) - async def state_transition_failed_handler(request: Request, exc: StateTransitionFailed) -> JSONResponse: + @app.exception_handler(AareAuthError) + async def _handle_auth_error(request: Request, exc: AareAuthError) -> JSONResponse: + # Auth flow needs the finer-grained AuthErrorCode (e.g. INVALID_TOKEN + # vs SESSION_ALREADY_ACTIVE) so the GUI can route between re-auth / + # claim-session / login dialogs. Fall back to the class-derived code + # when the instance carries no `code` attribute. + instance_code = getattr(exc, "code", None) + code_override = str(instance_code) if instance_code is not None else None + status = getattr(exc, "status_code", None) + if status is None: + status = ( + api_status.HTTP_401_UNAUTHORIZED + if isinstance(exc, AuthenticationException) + else api_status.HTTP_403_FORBIDDEN + ) return JSONResponse( - status_code=api_status.HTTP_500_INTERNAL_SERVER_ERROR, - content=_error_payload(code="STATE_TRANSITION_FAILED", message=str(exc)), - ) - - @app.exception_handler(MaintenanceStateException) - async def maintenance_state_handler(request: Request, exc: MaintenanceStateException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_409_CONFLICT, - content=_error_payload(code="MAINTENANCE_STATE", message=str(exc)), - ) - - @app.exception_handler(BeamlineBusyException) - async def beamline_busy_handler(request: Request, exc: BeamlineBusyException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_423_LOCKED, - content=_error_payload(code="BEAMLINE_BUSY", message=str(exc) or "Beamline is busy"), - ) - - @app.exception_handler(DataCollectionException) - async def data_collection_failed_handler(request: Request, exc: DataCollectionException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_500_INTERNAL_SERVER_ERROR, - content=_error_payload(code="DATA_COLLECTION_FAILED", message=str(exc) or "Data collection failed"), - ) - - @app.exception_handler(RasterScanException) - async def raster_scan_failed_handler(request: Request, exc: RasterScanException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_500_INTERNAL_SERVER_ERROR, - content=_error_payload(code="RASTER_SCAN_FAILED", message=str(exc) or "Raster scan failed"), - ) - - @app.exception_handler(AuthenticationException) - async def authentication_exception_handler(request: Request, exc: AuthenticationException) -> JSONResponse: - return JSONResponse( - status_code=getattr(exc, "status_code", api_status.HTTP_401_UNAUTHORIZED), - content=_error_payload( - code=str(getattr(exc, "code", "AUTHENTICATION_ERROR")), - message=str(exc) or "Invalid authentication", - ), + status_code=status, + content=_error_body(exc, code_override=code_override), headers=getattr(exc, "headers", None), ) - @app.exception_handler(UserRightsException) - async def user_rights_exception_handler(request: Request, exc: UserRightsException) -> JSONResponse: - return JSONResponse( - status_code=getattr(exc, "status_code", api_status.HTTP_403_FORBIDDEN), - content=_error_payload( - code=str(getattr(exc, "code", "FORBIDDEN")), - message=str(exc) or "Forbidden", - ), - headers=getattr(exc, "headers", None), - ) - - @app.exception_handler(SampleException) - async def sample_exception_handler(request: Request, exc: SampleException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_404_NOT_FOUND, - content=_error_payload(code="SAMPLE_NOT_FOUND", message=str(exc) or "Sample not found"), - ) - - @app.exception_handler(ManualMountException) - async def manual_mount_handler(request: Request, exc: ManualMountException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_409_CONFLICT, - content=_error_payload(code="MANUAL_MOUNT_REQUIRED", message=str(exc) or "Manual mount intervention required"), - ) - - @app.exception_handler(SmartMagnetFaultException) - async def smart_magnet_fault_handler(request: Request, exc: SmartMagnetFaultException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_503_SERVICE_UNAVAILABLE, - content=_error_payload(code="SMART_MAGNET_FAULT", message=str(exc) or "Smart magnet fault"), - ) - - @app.exception_handler(TellMountFailedException) - async def tell_mount_failed_handler(request: Request, exc: TellMountFailedException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_409_CONFLICT, - content=_error_payload(code="TELL_MOUNT_FAILED", message=str(exc) or "TELL mount failed"), - ) - - @app.exception_handler(TellCommandWhileBusyException) - async def tell_busy_handler(request: Request, exc: TellCommandWhileBusyException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_409_CONFLICT, - content=_error_payload(code="TELL_BUSY", message=str(exc) or "TELL is busy"), - ) - - @app.exception_handler(TellConnectionException) - async def tell_connection_error_handler(request: Request, exc: TellConnectionException) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_503_SERVICE_UNAVAILABLE, - content=_error_payload(code="TELL_CONNECTION_ERROR", message=str(exc) or "TELL connection error"), - ) - - @app.exception_handler(SmargonCommunicationError) - async def smargon_comm_handler(request: Request, exc: SmargonCommunicationError) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_503_SERVICE_UNAVAILABLE, - content=_error_payload( - code="SMARGON_UNAVAILABLE", - message=str(exc) or "Smargon is unavailable", - extra={ - "operation": getattr(exc, "operation", None), - "endpoint": getattr(exc, "endpoint", None), + @app.exception_handler(HTTPException) + async def _handle_http_exception(request: Request, exc: HTTPException) -> JSONResponse: + """HTTPException isn't in the AareException hierarchy but FastAPI raises + it internally (e.g. validation, route-not-found) and routes raise it + manually. Adapt to the new body shape so clients see one schema.""" + detail = exc.detail + # Preserve legacy callers that passed a {code, message} dict + if isinstance(detail, dict) and "code" in detail and "message" in detail: + legacy_code = str(detail.get("code")) + legacy_message = str(detail.get("message")) + return JSONResponse( + status_code=exc.status_code, + content={ + "critical": exc.status_code >= 500, + "code": legacy_code, + "exception_class": "HTTPException", + "message": legacy_message, + "context": {k: v for k, v in detail.items() if k not in ("code", "message")}, }, - ), - ) - - @app.exception_handler(AareDBCommunicationError) - async def aaredb_comm_handler(request: Request, exc: AareDBCommunicationError) -> JSONResponse: + headers=exc.headers, + ) return JSONResponse( - status_code=api_status.HTTP_503_SERVICE_UNAVAILABLE, - content=_error_payload( - code="AAREDB_UNAVAILABLE", - message=str(exc) or "AareDB is unavailable", - extra={ - "operation": getattr(exc, "operation", None), - "endpoint": getattr(exc, "endpoint", None), - "base_url": getattr(exc, "base_url", None), - }, - ), - ) - - @app.exception_handler(TellCommunicationError) - async def tell_comm_handler(request: Request, exc: TellCommunicationError) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_503_SERVICE_UNAVAILABLE, - content=_error_payload( - code="TELL_UNAVAILABLE", - message=str(exc) or "TELL is unavailable", - extra={ - "operation": getattr(exc, "operation", None), - "endpoint": getattr(exc, "endpoint", None), - }, - ), - ) - - @app.exception_handler(BECCommunicationError) - async def bec_comm_handler(request: Request, exc: BECCommunicationError) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_503_SERVICE_UNAVAILABLE, - content=_error_payload( - code="BEC_UNAVAILABLE", - message=str(exc) or "BEC is unavailable", - extra={ - "operation": getattr(exc, "operation", None), - "endpoint": getattr(exc, "endpoint", None), - "base_url": getattr(exc, "base_url", None), - }, - ), - ) - - - @app.exception_handler(JFJochCommunicationError) - async def jfjoch_comm_handler(request: Request, exc: JFJochCommunicationError) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_503_SERVICE_UNAVAILABLE, - content=_error_payload( - code="JFJOCH_UNAVAILABLE", - message=str(exc) or "JFJoch detector is unavailable", - extra={ - "operation": getattr(exc, "operation", None), - "endpoint": getattr(exc, "endpoint", None), - "base_url": getattr(exc, "base_url", None), - }, - ), - ) - - @app.exception_handler(AerotechCommunicationError) - async def aerotech_comm_handler(request: Request, exc: AerotechCommunicationError) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_503_SERVICE_UNAVAILABLE, - content=_error_payload( - code="AEROTECH_UNAVAILABLE", - message=str(exc) or "Aerotech is unavailable", - extra={ - "operation": getattr(exc, "operation", None), - "endpoint": getattr(exc, "endpoint", None), - "base_url": getattr(exc, "base_url", None), - }, - ), - ) - - @app.exception_handler(MagnetPositionSensorErorr) - async def magnet_position_sensor_handler(request: Request, exc: MagnetPositionSensorErorr) -> JSONResponse: - return JSONResponse( - status_code=api_status.HTTP_500_INTERNAL_SERVER_ERROR, - content=_error_payload( - code="MAGNET_POSITION_SENSOR_ERROR", - message=str(exc) or "Magnet position sensor error", - ), + status_code=exc.status_code, + content={ + "critical": exc.status_code >= 500, + "code": "HTTP_ERROR", + "exception_class": "HTTPException", + "message": str(detail) or "HTTP error", + "context": {}, + }, + headers=exc.headers, ) @app.exception_handler(Exception) - async def unhandled_exception_handler(request: Request, exc: Exception) -> JSONResponse: - logger.exception(f"Unhandled server exception: {exc}") + async def _handle_unknown(request: Request, exc: Exception) -> JSONResponse: + """Fail-safe: any unrecognized exception is treated as critical.""" + logger.exception("Unhandled exception in DAQ", exc_info=exc) return JSONResponse( status_code=api_status.HTTP_500_INTERNAL_SERVER_ERROR, - content=_error_payload(code="INTERNAL_SERVER_ERROR", message=str(exc) or "Internal server error"), + content=_error_body( + exc, + code_override=str(AareErrorCode.INTERNAL_ERROR), + critical_override=True, + ), ) diff --git a/tests/unit/common/test_error_codes.py b/tests/unit/common/test_error_codes.py index abd80d4a..d1e40e0d 100644 --- a/tests/unit/common/test_error_codes.py +++ b/tests/unit/common/test_error_codes.py @@ -1,4 +1,12 @@ -from aare.common.error_codes import AuthErrorCode, DAQErrorCode, error_code_help, export_error_codes +from aare.common.error_codes import ( + AuthErrorCode, + DAQErrorCode, + AareErrorCode, + code_for_exception_class, + error_code_help, + export_error_codes, + export_error_codes_grouped, +) def test_error_code_help_returns_string(): help_text = error_code_help(AuthErrorCode.AUTHENTICATION_FAILED) @@ -12,3 +20,65 @@ def test_error_code_help_unknown_code(): def test_export_error_codes_contains_known_codes(): exported = export_error_codes() assert AuthErrorCode.AUTHENTICATION_FAILED.name in exported + + +def test_code_for_exception_class_basic(): + assert code_for_exception_class("TellCommunicationError") == "TELL_COMMUNICATION_ERROR" + assert code_for_exception_class("MountingFailed") == "MOUNTING_FAILED" + assert code_for_exception_class("LoopCenteringFailed") == "LOOP_CENTERING_FAILED" + + +def test_code_for_exception_class_acronyms(): + assert code_for_exception_class("AXCFailed") == "AXC_FAILED" + assert code_for_exception_class("AareDBCommunicationError") == "AARE_DB_COMMUNICATION_ERROR" + assert code_for_exception_class("JFJochCommunicationError") == "JF_JOCH_COMMUNICATION_ERROR" + + +def test_code_for_each_concrete_exception_is_in_aare_error_code_enum(): + """Every code we'd produce from the rebuilt hierarchy must exist in the + AareErrorCode enum -- otherwise clients have no symbol to branch on.""" + from aare.common.exception_handler import ( + TellCommunicationError, TellConnectionException, CriticalTellException, + WarningTellException, TellCommandWhileBusyException, + TellMountFailedException, MountingFailed, UnmountingFailed, + SmargonCommunicationError, AerotechCommunicationError, + JFJochCommunicationError, BECCommunicationError, + AareDBCommunicationError, StateTransitionFailed, + MaintenanceStateException, BeamlineBusyException, + BeamlineBusyTimeoutException, DataCollectionException, + RasterScanException, LoopCenteringFailed, AXCFailed, + AutoRasterSampleSkipped, TransformationInvalidException, + MagnetPositionSensorErorr, SmartMagnetFaultException, + ManualMountException, SampleException, AuthenticationException, + UserRightsException, + ) + valid = {c.value for c in AareErrorCode} + classes = [ + TellCommunicationError, TellConnectionException, CriticalTellException, + WarningTellException, TellCommandWhileBusyException, + TellMountFailedException, MountingFailed, UnmountingFailed, + SmargonCommunicationError, AerotechCommunicationError, + JFJochCommunicationError, BECCommunicationError, + AareDBCommunicationError, StateTransitionFailed, + MaintenanceStateException, BeamlineBusyException, + BeamlineBusyTimeoutException, DataCollectionException, + RasterScanException, LoopCenteringFailed, AXCFailed, + AutoRasterSampleSkipped, TransformationInvalidException, + MagnetPositionSensorErorr, SmartMagnetFaultException, + ManualMountException, SampleException, AuthenticationException, + UserRightsException, + ] + missing = [] + for cls in classes: + code = code_for_exception_class(cls.__name__) + if code not in valid: + missing.append((cls.__name__, code)) + assert not missing, f"Codes missing from AareErrorCode: {missing}" + + +def test_export_error_codes_grouped_includes_aare_error_code(): + exported = export_error_codes_grouped() + assert "AareErrorCode" in exported + assert "DAQErrorCode" in exported + assert "AuthErrorCode" in exported + assert "TELL_COMMUNICATION_ERROR" in exported["AareErrorCode"] diff --git a/tests/unit/daq/test_server_exception_handler.py b/tests/unit/daq/test_server_exception_handler.py index 665bd92b..dd40c283 100644 --- a/tests/unit/daq/test_server_exception_handler.py +++ b/tests/unit/daq/test_server_exception_handler.py @@ -1,99 +1,307 @@ +"""Tests for the collapsed server exception handlers (Phase 2 of the +exception-handling redesign). + +Per plan §2, server-side handling is now four handlers + HTTPException + +bare-Exception fallback. Each emits the unified response body from §3: + + {"critical", "code", "exception_class", "message", "context"} +""" + +from __future__ import annotations + +import json + import pytest from fastapi import FastAPI, HTTPException from starlette.requests import Request from starlette.responses import JSONResponse from unittest.mock import MagicMock -import asyncio from aare.daq.server_exception_handler import register_exception_handlers +from aare.common.error_codes import AareErrorCode, AuthErrorCode from aare.common.exception_handler import ( - MountingFailed, WarningTellException, CriticalTellException, - LoopCenteringFailed, TransformationInvalidException, BeamlineBusyException, - AuthenticationException, SampleException, UserRightsException, - SmargonCommunicationError, TellCommunicationError, JFJochCommunicationError, - AerotechCommunicationError, DataCollectionException, RasterScanException, - UnmountingFailed, AXCFailed, AareDBCommunicationError, - MagnetPositionSensorErorr, ManualMountException, SmartMagnetFaultException, - TellMountFailedException, TellCommandWhileBusyException, TellConnectionException + AareException, + AutomationError, + AareUserError, + AareAuthError, + MountingFailed, + UnmountingFailed, + LoopCenteringFailed, + TellCommunicationError, + CriticalTellException, + WarningTellException, + SmargonCommunicationError, + AareDBCommunicationError, + AuthenticationException, + UserRightsException, + ManualMountException, + SampleException, ) + @pytest.fixture def app(): app = FastAPI() register_exception_handlers(app) return app + @pytest.fixture def mock_request(): return MagicMock(spec=Request) + +# Helper: pull JSON body out of a Starlette JSONResponse +def _body(response: JSONResponse) -> dict: + return json.loads(response.body) + + +# --------------------------------------------------------------------------- +# AutomationError handler -- 503 if critical, 422 otherwise +# --------------------------------------------------------------------------- + @pytest.mark.asyncio -async def test_http_exception_handler(app, mock_request): +async def test_automation_error_not_critical_returns_422(app, mock_request): + # MountingFailed has class default critical=False in Phase 1 + exc = MountingFailed("mount failed") + handler = app.exception_handlers[AutomationError] + response = await handler(mock_request, exc) + assert response.status_code == 422 + body = _body(response) + assert body["critical"] is False + assert body["code"] == "MOUNTING_FAILED" + assert body["exception_class"] == "MountingFailed" + assert body["message"] == "mount failed" + assert body["context"] == {} + + +@pytest.mark.asyncio +async def test_automation_error_critical_via_instance_flag_returns_503(app, mock_request): + # Instance-level override: critical=True + exc = MountingFailed("threshold breached", critical=True) + handler = app.exception_handlers[AutomationError] + response = await handler(mock_request, exc) + assert response.status_code == 503 + body = _body(response) + assert body["critical"] is True + + +@pytest.mark.asyncio +async def test_automation_error_context_contains_endpoint_and_operation(app, mock_request): + exc = TellCommunicationError("timeout", endpoint="/state", operation="GET", + base_url="http://tell:8000") + handler = app.exception_handlers[AutomationError] + response = await handler(mock_request, exc) + body = _body(response) + assert body["code"] == "TELL_COMMUNICATION_ERROR" + assert body["context"]["endpoint"] == "/state" + assert body["context"]["operation"] == "GET" + assert body["context"]["base_url"] == "http://tell:8000" + + +@pytest.mark.asyncio +async def test_automation_error_excludes_critical_and_headers_from_context(app, mock_request): + # critical kwarg should not appear in context (it has its own field) + exc = MountingFailed("x", critical=True) + handler = app.exception_handlers[AutomationError] + response = await handler(mock_request, exc) + body = _body(response) + assert "critical" not in body["context"] + assert "headers" not in body["context"] + + +# --------------------------------------------------------------------------- +# AareUserError handler -- always 400 +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_user_error_returns_400(app, mock_request): + exc = ManualMountException("user must intervene") + handler = app.exception_handlers[AareUserError] + response = await handler(mock_request, exc) + assert response.status_code == 400 + body = _body(response) + assert body["critical"] is False + assert body["code"] == "MANUAL_MOUNT_EXCEPTION" + + +@pytest.mark.asyncio +async def test_sample_exception_returns_400_as_user_error(app, mock_request): + # SampleException parented under AareUserError (see Phase 1 note in plan) + exc = SampleException("Sample SAR0035 not in dewar") + handler = app.exception_handlers[AareUserError] + response = await handler(mock_request, exc) + assert response.status_code == 400 + body = _body(response) + assert body["code"] == "SAMPLE_EXCEPTION" + + +# --------------------------------------------------------------------------- +# AareAuthError handler -- 401 / 403, code from instance.code +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_authentication_exception_returns_401_with_auth_code(app, mock_request): + exc = AuthenticationException("Bad token", code=AuthErrorCode.INVALID_TOKEN) + handler = app.exception_handlers[AareAuthError] + response = await handler(mock_request, exc) + assert response.status_code == 401 + body = _body(response) + assert body["critical"] is False + # auth handler surfaces the instance code, not the class-derived one + assert body["code"] == "INVALID_TOKEN" + assert body["exception_class"] == "AuthenticationException" + + +@pytest.mark.asyncio +async def test_user_rights_exception_returns_403(app, mock_request): + exc = UserRightsException("Not a staff member", code=AuthErrorCode.NOT_STAFF) + handler = app.exception_handlers[AareAuthError] + response = await handler(mock_request, exc) + assert response.status_code == 403 + body = _body(response) + assert body["code"] == "NOT_STAFF" + + +@pytest.mark.asyncio +async def test_authentication_exception_preserves_explicit_status(app, mock_request): + # AuthenticationException can be constructed with a custom status_code + exc = AuthenticationException("Forbidden auth path", status_code=403, + code=AuthErrorCode.FORBIDDEN) + handler = app.exception_handlers[AareAuthError] + response = await handler(mock_request, exc) + assert response.status_code == 403 + + +# --------------------------------------------------------------------------- +# HTTPException handler -- pass-through status, new shape +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_http_exception_handler_string_detail(app, mock_request): exc = HTTPException(status_code=418, detail="I'm a teapot") handler = app.exception_handlers[HTTPException] response = await handler(mock_request, exc) assert response.status_code == 418 - assert response.body == b'{"code":"HTTP_ERROR","message":"I\'m a teapot"}' + body = _body(response) + assert body["code"] == "HTTP_ERROR" + assert body["message"] == "I'm a teapot" + assert body["critical"] is False # 418 < 500 + assert body["exception_class"] == "HTTPException" + @pytest.mark.asyncio -async def test_mounting_failed_handler(app, mock_request): - exc = MountingFailed("Mount failed") - handler = app.exception_handlers[MountingFailed] - response = await handler(mock_request, exc) - assert response.status_code == 409 - assert b"MOUNTING_FAILED" in response.body - -@pytest.mark.asyncio -async def test_unmounting_failed_handler(app, mock_request): - exc = UnmountingFailed("Unmount failed") - handler = app.exception_handlers[UnmountingFailed] - response = await handler(mock_request, exc) - assert response.status_code == 500 - assert b"UNMOUNTING_FAILED" in response.body - -@pytest.mark.asyncio -async def test_smargon_comm_handler(app, mock_request): - exc = SmargonCommunicationError("Smargon dead", operation="MOVE", endpoint="/move") - handler = app.exception_handlers[SmargonCommunicationError] - response = await handler(mock_request, exc) - assert response.status_code == 503 - assert b"SMARGON_UNAVAILABLE" in response.body - assert b"MOVE" in response.body - -@pytest.mark.asyncio -async def test_unhandled_exception_handler(app, mock_request): - exc = ValueError("Something went wrong") - handler = app.exception_handlers[Exception] - response = await handler(mock_request, exc) - assert response.status_code == 500 - assert b"INTERNAL_SERVER_ERROR" in response.body - -@pytest.mark.asyncio -async def test_all_handlers_and_payloads(app, mock_request): - # Test all registered handlers to ensure they return JSONResponse and cover the code - for exc_class, handler in app.exception_handlers.items(): - if exc_class in (HTTPException, Exception, Request): - continue - - # Try to instantiate the exception - try: - # Some exceptions might need specific args, but most in aare.common.exception_handler - # have defaults or take a message - if exc_class in (SmargonCommunicationError, AareDBCommunicationError, TellCommunicationError, - JFJochCommunicationError, AerotechCommunicationError): - exc = exc_class("error", operation="OP", endpoint="/EP") - else: - exc = exc_class("error") - - response = await handler(mock_request, exc) - assert isinstance(response, JSONResponse) - assert response.status_code != 200 - except Exception as e: - print(f"Skipping {exc_class} due to {e}") - - # Special case for HTTPException with dict detail - exc = HTTPException(status_code=400, detail={"code": "CUSTOM", "message": "Msg"}) +async def test_http_exception_handler_dict_detail(app, mock_request): + exc = HTTPException(status_code=400, detail={"code": "CUSTOM", "message": "Msg", "field": "foo"}) handler = app.exception_handlers[HTTPException] response = await handler(mock_request, exc) assert response.status_code == 400 - assert b"CUSTOM" in response.body + body = _body(response) + assert body["code"] == "CUSTOM" + assert body["message"] == "Msg" + assert body["context"] == {"field": "foo"} + + +@pytest.mark.asyncio +async def test_http_exception_5xx_is_critical(app, mock_request): + exc = HTTPException(status_code=503, detail="downstream gone") + handler = app.exception_handlers[HTTPException] + response = await handler(mock_request, exc) + body = _body(response) + assert body["critical"] is True + + +# --------------------------------------------------------------------------- +# Bare Exception fallback -- 500, critical=True +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_unhandled_exception_is_critical_500(app, mock_request): + exc = ValueError("oops") + handler = app.exception_handlers[Exception] + response = await handler(mock_request, exc) + assert response.status_code == 500 + body = _body(response) + assert body["critical"] is True + assert body["code"] == "INTERNAL_ERROR" + assert body["exception_class"] == "ValueError" + assert body["message"] == "oops" + + +@pytest.mark.asyncio +async def test_unhandled_exception_empty_message_falls_back_to_class_name(app, mock_request): + exc = ValueError() + handler = app.exception_handlers[Exception] + response = await handler(mock_request, exc) + body = _body(response) + assert body["message"] == "ValueError" + + +# --------------------------------------------------------------------------- +# Handler count -- the design contract is 4 + HTTPException + Exception +# --------------------------------------------------------------------------- + +def test_only_four_root_handlers_plus_fallbacks(app): + """Plan §2: collapse the ~25 per-class handlers down to 4 roots, plus the + HTTPException pass-through and the bare-Exception fallback. + + FastAPI pre-registers handlers for RequestValidationError / + WebSocketRequestValidationError -- those are framework-level and not + counted toward our handler budget. We assert (a) our expected handlers + are present and (b) no per-class AareException handlers remain. + """ + from aare.common.exception_handler import ( + MountingFailed, UnmountingFailed, TellCommunicationError, + LoopCenteringFailed, CriticalTellException, WarningTellException, + SmargonCommunicationError, AareDBCommunicationError, + ) + + registered = set(app.exception_handlers.keys()) + expected = {AutomationError, AareUserError, AareAuthError, + HTTPException, Exception} + assert expected.issubset(registered), ( + f"Missing required handlers: {expected - registered}" + ) + + # No leftover per-class handlers from the old design + forbidden = {MountingFailed, UnmountingFailed, TellCommunicationError, + LoopCenteringFailed, CriticalTellException, + WarningTellException, SmargonCommunicationError, + AareDBCommunicationError} + leftover = forbidden & registered + assert not leftover, f"Per-class handlers must be removed: {leftover}" + + +# --------------------------------------------------------------------------- +# Response body shape is uniform across handlers +# --------------------------------------------------------------------------- + +REQUIRED_BODY_KEYS = {"critical", "code", "exception_class", "message", "context"} + + +@pytest.mark.asyncio +async def test_response_body_shape_uniform(app, mock_request): + cases: list[tuple[type, Exception]] = [ + (AutomationError, MountingFailed("x")), + (AutomationError, LoopCenteringFailed("y")), + (AutomationError, SmargonCommunicationError("z", operation="GET", endpoint="/e")), + (AutomationError, AareDBCommunicationError("db", critical=True)), + (AareUserError, ManualMountException("m")), + (AareUserError, SampleException("s")), + (AareAuthError, AuthenticationException("a")), + (AareAuthError, UserRightsException("u")), + (Exception, RuntimeError("r")), + (HTTPException, HTTPException(status_code=404, detail="not found")), + ] + for handler_key, exc in cases: + handler = app.exception_handlers[handler_key] + response = await handler(mock_request, exc) + body = _body(response) + assert REQUIRED_BODY_KEYS == set(body.keys()), ( + f"Body keys mismatch for {type(exc).__name__}: got {set(body.keys())}" + ) + assert isinstance(body["critical"], bool) + assert isinstance(body["code"], str) and body["code"] + assert isinstance(body["exception_class"], str) and body["exception_class"] + assert isinstance(body["message"], str) and body["message"] + assert isinstance(body["context"], dict)