From 2db635ae199cfa35efe4dad0d6269caa15d4ce89 Mon Sep 17 00:00:00 2001 From: Lukas Date: Thu, 30 Apr 2026 19:44:43 +0200 Subject: [PATCH] Fix exception handler overlap issue - add DomainError catch-all handler **Problem:** Broad exception handlers created fragility where adding a new DomainError subclass without explicit registration would silently fall through to the generic exception handler, losing the specific error_code and metadata. **Solution:** 1. Import DomainError in main.py for explicit handler registration 2. Fix type hints in exception handlers from 'Exception' to specific types - NotFoundError handler now typed as 'NotFoundError' - BadRequestError handler now typed as 'BadRequestError' - ConflictError handler now typed as 'ConflictError' - DomainError handler now typed as 'DomainError' - ServiceUnavailableError handler now typed as 'ServiceUnavailableError' 3. Add DomainError as an explicit catch-all handler in the registration chain - Positioned after specific handlers, before HTTPException - Any unregistered DomainError subclass now gets correct error_code + metadata 4. Document the exception handler hierarchy with detailed comments 5. Update Backend-Development.md with handler hierarchy documentation 6. Update Architekture.md section 2.2 with exception handler details 7. Fix test expectations in test_main.py to verify ErrorResponse format **Impact:** Any new DomainError subclass now automatically gets correct HTTP 500 status, error_code, and metadata - even if developer forgets explicit handler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Architekture.md | 16 +++++++++- Docs/Backend-Development.md | 25 ++++++++++++++++ Docs/Tasks.md | 59 ------------------------------------- backend/app/main.py | 44 +++++++++++++++++++-------- backend/tests/test_main.py | 16 ++++++++-- 5 files changed, 84 insertions(+), 76 deletions(-) diff --git a/Docs/Architekture.md b/Docs/Architekture.md index c9a6543..b6fe5dd 100644 --- a/Docs/Architekture.md +++ b/Docs/Architekture.md @@ -555,9 +555,23 @@ The FastAPI app factory. Responsibilities: - Creates the `FastAPI` instance with metadata (title, version, docs URL) - Registers the **lifespan** context manager (startup: open DB, create aiohttp session, start scheduler; shutdown: close all) - Mounts all routers -- Registers global exception handlers that map domain exceptions to HTTP status codes +- Registers global exception handlers that map domain exceptions to HTTP status codes with a hierarchical fallback chain - Applies the setup-redirect middleware (returns `423 Locked` for all API requests when no configuration exists, except for `/api/setup` and `/api/health`) +**Exception Handler Hierarchy:** + +Exception handlers are registered in order of specificity to ensure each exception type is caught by the most appropriate handler: + +1. **Specific network errors** (Fail2BanConnectionError, Fail2BanProtocolError) → HTTP 502 Bad Gateway +2. **Specific auth/rate errors** (AuthenticationError, RateLimitError) → HTTP 401 Unauthorized / 429 Too Many Requests +3. **Category handlers** (NotFoundError, BadRequestError, ConflictError, OperationError, ServiceUnavailableError) → HTTP 404/400/409/500/503 +4. **DomainError catch-all** → HTTP 500 (catches any unregistered DomainError subclass, ensuring proper error_code and metadata are returned) +5. **HTTPException** → HTTP status from exception (FastAPI built-in validation and routing errors) +6. **ValueError** → HTTP 400 Bad Request (Pydantic validation errors) +7. **Exception catch-all** → HTTP 500 Internal Server Error (absolute fallback for unexpected errors) + +The DomainError catch-all handler (step 4) is critical: it ensures that any new DomainError subclass automatically gets the correct HTTP status (500), error_code, and metadata through its inherited `error_code` attribute and `get_error_metadata()` method, even if the developer forgot to create an explicit handler for it. This prevents silent failures where an unhandled exception would return a generic "internal_error" code instead of the specific error code defined by the exception class. + ### 2.3 Dependency Wiring and Service Composition BanGUI uses a **lightweight dependency injection (DI) pattern** based on FastAPI's `Depends()` framework. There is no heavy container library — the composition root is implicit and managed through simple provider functions in `app/dependencies.py`. diff --git a/Docs/Backend-Development.md b/Docs/Backend-Development.md index 03de69d..dfa7444 100644 --- a/Docs/Backend-Development.md +++ b/Docs/Backend-Development.md @@ -672,10 +672,35 @@ class MySpecificError(BadRequestError): 4. **Implement `get_error_metadata()`** — Return only data safe for client consumption. Never leak internal state, file paths, or system details. 5. **Raise from service code** — Never from repositories or utils. Exceptions represent business logic violations, not infrastructure errors. +### Exception Handler Hierarchy + +All domain exceptions are automatically converted to `ErrorResponse` via handlers registered in `backend/app/main.py`. The handler registration order is critical: + +```python +# Handlers are registered from most specific to least specific: +1. Network errors (Fail2BanConnectionError, etc.) → HTTP 502 +2. Auth/rate errors (AuthenticationError, RateLimitError) → HTTP 401/429 +3. Category handlers (NotFoundError, BadRequestError, ConflictError, etc.) → HTTP 404/400/409/500/503 +4. DomainError catch-all → HTTP 500 # ← Catches unregistered DomainError subclasses +5. HTTPException (FastAPI built-ins) → HTTP varies +6. ValueError (Pydantic validation) → HTTP 400 +7. Exception catch-all → HTTP 500 # ← Absolute last resort +``` + +**Important:** The `DomainError` catch-all handler (step 4) is the safety net. If you add a new `DomainError` subclass without placing it in a category (e.g., `class MyError(DomainError)` instead of `class MyError(BadRequestError)`), it will still get the correct `error_code` and `metadata` via this handler instead of silently falling through to the generic exception handler. + +**Critical caveat:** Every new `DomainError` subclass **must**: +- Define an `error_code` class attribute (e.g., `error_code: str = "my_error"`) +- Override `get_error_metadata()` if it needs to return context data +- Inherit from the appropriate category (NotFoundError, BadRequestError, ConflictError, OperationError, or ServiceUnavailableError) + +If you forget to implement `error_code` or `get_error_metadata()`, the fallback to parent class implementations will produce misleading error codes and empty metadata — check your tests! + **What NOT to do:** - ❌ Don't raise `HTTPException` from service code (bypass the ErrorResponse format). - ❌ Don't put sensitive information in `metadata` (database paths, SQL, internal IDs). - ❌ Don't derive error codes from class names using regex (fragile and non-self-documenting). +- ❌ Don't create a `DomainError` subclass without a category (always inherit from one of the seven categories) ### Frontend Error Parsing diff --git a/Docs/Tasks.md b/Docs/Tasks.md index f0e5b38..0f3fbe7 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,62 +1,3 @@ -## [Backend] Pydantic validators execute at import time - -**Status:** ✅ **COMPLETE** — Codebase is fully compliant - -**Verification:** -- ✓ Audited all 14 model files in `backend/app/models/` -- ✓ No model files import from `app.config`, `app.services`, `app.utils`, or `app.routers` -- ✓ No validators, field defaults, or computed fields call runtime-dependent functions -- ✓ `BanGuiBaseModel` is pure with no side-effects -- ✓ Path validation correctly placed in `app.utils.path_utils` and called from routers/services only - -**Current Architecture (Correct):** - -The codebase follows the proper pattern: -1. **Models** (`app/models/`) contain only pure data classes and stateless validators (if any) -2. **Validation requiring app state** (settings, file I/O, database) happens in **routers or services** at request time -3. **Path validation helper** (`app.utils.path_utils.validate_log_path()`) is called from routers/services, never imported in models - -Example (correct pattern in place): -```python -# ✅ Correct: Validation in router with access to settings -from app.utils.path_utils import validate_log_path - -@router.post("/jails/{name}/logpath") -async def add_log_path(name: str, body: AddLogPathRequest) -> None: - validate_log_path(body.log_path) # Called at request time - await config_service.add_log_path(name, body) -``` - -**Documentation Updates (Completed):** -- ✓ `Docs/Architekture.md` § 2.1: Updated Models section with explicit constraints on I/O and side effects -- ✓ `Docs/Backend-Development.md`: Enhanced validator documentation with import-time execution explanation - -**Important Note on Task Description:** - -The original task suggested using `@model_validator` with imports inside the method. This approach is **incorrect** and would not solve the problem: -```python -# ❌ WRONG — Still executes at import time: -@model_validator(mode="after") -def validate_range(self): - from app.config import get_settings # ← Decorator still runs at class definition time - ... -``` - -The **correct solution** (already in place) is to perform validation in the **router or service layer** where settings and services are available at request/execution time. - -**Regression Prevention:** - -To prevent future violations, run this check before committing: -```bash -# Verify no app-layer imports in model files -find backend/app/models -name "*.py" -exec grep -l "from app\.\(config\|services\|utils\|routers\)" {} \; -# Should return: 0 files (empty) -``` - -Consider adding this as a pre-commit hook or CI check for long-term prevention. - ---- - ## [Backend] Exception handler overlap — broad handlers catching everything **Where found** diff --git a/backend/app/main.py b/backend/app/main.py index 4b178ff..aedd23c 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -33,6 +33,7 @@ from app.exceptions import ( AuthenticationError, BadRequestError, ConflictError, + DomainError, Fail2BanConnectionError, Fail2BanProtocolError, NotFoundError, @@ -328,7 +329,7 @@ async def _fail2ban_protocol_handler( async def _not_found_handler( request: Request, - exc: Exception, + exc: NotFoundError, ) -> JSONResponse: """Return a ``404 Not Found`` response for missing domain entities. @@ -359,7 +360,7 @@ async def _not_found_handler( async def _bad_request_handler( request: Request, - exc: Exception, + exc: BadRequestError, ) -> JSONResponse: """Return a ``400 Bad Request`` response for validation and domain contract errors. @@ -390,7 +391,7 @@ async def _bad_request_handler( async def _conflict_handler( request: Request, - exc: Exception, + exc: ConflictError, ) -> JSONResponse: """Return a ``409 Conflict`` response for domain state conflicts.""" log.warning( @@ -413,7 +414,7 @@ async def _conflict_handler( async def _domain_error_handler( request: Request, - exc: Exception, + exc: DomainError, ) -> JSONResponse: """Return a ``500 Internal Server Error`` response for domain write failures.""" log.error( @@ -468,7 +469,7 @@ async def _value_error_handler( async def _service_unavailable_handler( request: Request, - exc: Exception, + exc: ServiceUnavailableError, ) -> JSONResponse: """Return a ``503 Service Unavailable`` response for infrastructure errors. @@ -778,18 +779,35 @@ def create_app(settings: Settings | None = None) -> FastAPI: # --- Exception handlers --- - # Ordered from most specific to least specific. FastAPI evaluates handlers - # in the order they were registered, so fail2ban network errors get a 502 - # rather than falling through to the generic 500 handler. + # + # Exception handlers are registered from most specific to least specific. FastAPI evaluates + # them in registration order, allowing specific handlers to match before fallback handlers. + # + # The hierarchy (in order) is: + # 1. Network-specific errors (Fail2BanConnectionError, Fail2BanProtocolError) → HTTP 502 + # 2. Auth/rate-limit errors (AuthenticationError, RateLimitError) → HTTP 401/429 + # 3. Category handlers (NotFoundError, BadRequestError, ConflictError) → HTTP 404/400/409 + # 4. OperationError handler → HTTP 500 + # 5. ServiceUnavailableError handler → HTTP 503 + # 6. Generic DomainError handler (catch-all for any unregistered DomainError subclass) → HTTP 500 + # 7. HTTPException (FastAPI built-ins, validation errors) → HTTP varies + # 8. ValueError (Pydantic validation) → HTTP 400 + # 9. Exception (absolute catch-all for unexpected errors) → HTTP 500 + # + # This ensures that any new DomainError subclass that inherits from a registered category + # is automatically handled with the correct error_code and metadata. If a developer adds + # a DomainError subclass without putting it in a category, it falls through to the + # generic DomainError handler rather than the unhandled_exception_handler. app.add_exception_handler(Fail2BanConnectionError, _fail2ban_connection_handler) # type: ignore[arg-type] app.add_exception_handler(Fail2BanProtocolError, _fail2ban_protocol_handler) # type: ignore[arg-type] app.add_exception_handler(AuthenticationError, _authentication_error_handler) # type: ignore[arg-type] app.add_exception_handler(RateLimitError, _rate_limit_error_handler) # type: ignore[arg-type] - app.add_exception_handler(NotFoundError, _not_found_handler) - app.add_exception_handler(BadRequestError, _bad_request_handler) - app.add_exception_handler(ConflictError, _conflict_handler) - app.add_exception_handler(OperationError, _domain_error_handler) - app.add_exception_handler(ServiceUnavailableError, _service_unavailable_handler) + app.add_exception_handler(NotFoundError, _not_found_handler) # type: ignore[arg-type] + app.add_exception_handler(BadRequestError, _bad_request_handler) # type: ignore[arg-type] + app.add_exception_handler(ConflictError, _conflict_handler) # type: ignore[arg-type] + app.add_exception_handler(OperationError, _domain_error_handler) # type: ignore[arg-type] + app.add_exception_handler(ServiceUnavailableError, _service_unavailable_handler) # type: ignore[arg-type] + app.add_exception_handler(DomainError, _domain_error_handler) # type: ignore[arg-type] app.add_exception_handler(HTTPException, _http_exception_handler) # type: ignore[arg-type] app.add_exception_handler(ValueError, _value_error_handler) # type: ignore[arg-type] app.add_exception_handler(Exception, _unhandled_exception_handler) diff --git a/backend/tests/test_main.py b/backend/tests/test_main.py index 728ae32..4105689 100644 --- a/backend/tests/test_main.py +++ b/backend/tests/test_main.py @@ -112,15 +112,25 @@ async def test_create_app_global_domain_exception_handlers() -> None: async with AsyncClient(transport=transport, base_url="http://test") as client: response = await client.get("/not-found") assert response.status_code == 404 - assert response.json() == {"detail": "Jail not found: 'ssh'"} + data = response.json() + assert data["code"] == "jail_not_found" + assert data["detail"] == "Jail not found: 'ssh'" + assert data["metadata"] == {"jail_name": "ssh"} + assert "correlation_id" in data response = await client.get("/bad-request") assert response.status_code == 400 - assert response.json() == {"detail": "invalid payload"} + data = response.json() + assert data["code"] == "config_validation_failed" + assert data["detail"] == "invalid payload" + assert "correlation_id" in data response = await client.get("/server-error") assert response.status_code == 500 - assert response.json() == {"detail": "write failed"} + data = response.json() + assert data["code"] == "config_write_failed" + assert data["detail"] == "write failed" + assert "correlation_id" in data def test_create_app_disables_cors_by_default() -> None: