Sanitize error responses (#44)

CHANGES.md

@@ -4,6 +4,10 @@
 0.9.3 (unreleased)
 ------------------
 
+- Sanitize error responses.
+
+- Remove 501 error response on NotImplementedError.
+
 - Solved aiohttp 'Unclosed client session' warning.
 
 

clean_python/base/domain/exceptions.py

@@ -1,13 +1,13 @@
 # (c) Nelen & Schuurmans
 
 from typing import Any
-from typing import Dict
 from typing import List
 from typing import Optional
 from typing import Union
 
 from pydantic import create_model
 from pydantic import ValidationError
+from pydantic_core import ErrorDetails
 
 from .types import Id
 
@@ -62,10 +62,17 @@ class BadRequest(Exception):
         self._internal_error = err_or_msg
         super().__init__(err_or_msg)
 
-    def errors(self) -> List[Dict[str, Any]]:
+    def errors(self) -> List[ErrorDetails]:
         if isinstance(self._internal_error, ValidationError):
-            return [dict() for x in self._internal_error.errors()]
-        return [{"error": self}]
+            return self._internal_error.errors()
+        return [
+            ErrorDetails(
+                type="value_error",
+                msg=self._internal_error,
+                loc=[],  # type: ignore
+                input=None,
+            )
+        ]
 
     def __str__(self) -> str:
         error = self._internal_error

clean_python/fastapi/error_responses.py

@@ -1,6 +1,7 @@
 # (c) Nelen & Schuurmans
 
 from typing import List
+from typing import Optional
 from typing import Union
 
 from fastapi.encoders import jsonable_encoder
@@ -21,7 +22,6 @@ __all__ = [
     "not_found_handler",
     "conflict_handler",
     "validation_error_handler",
-    "not_implemented_handler",
     "permission_denied_handler",
     "unauthorized_handler",
 ]
@@ -34,51 +34,58 @@ class ValidationErrorEntry(ValueObject):
 
 
 class ValidationErrorResponse(ValueObject):
+    message: str
     detail: List[ValidationErrorEntry]
 
 
 class DefaultErrorResponse(ValueObject):
     message: str
+    detail: Optional[str]
 
 
-async def not_found_handler(request: Request, exc: DoesNotExist):
+async def not_found_handler(request: Request, exc: DoesNotExist) -> JSONResponse:
     return JSONResponse(
         status_code=status.HTTP_404_NOT_FOUND,
-        content={"message": f"Could not find {exc.name} with id={exc.id}"},
+        content={
+            "message": f"Could not find {exc.name}{' with id=' + str(exc.id) if exc.id else ''}"
+        },
     )
 
 
-async def conflict_handler(request: Request, exc: Conflict):
+async def conflict_handler(request: Request, exc: Conflict) -> JSONResponse:
     return JSONResponse(
         status_code=status.HTTP_409_CONFLICT,
-        content={"message": str(exc)},
+        content={
+            "message": "Conflict",
+            "detail": jsonable_encoder(exc.args[0] if exc.args else None),
+        },
     )
 
 
-async def validation_error_handler(request: Request, exc: BadRequest):
+async def validation_error_handler(request: Request, exc: BadRequest) -> JSONResponse:
     return JSONResponse(
         status_code=status.HTTP_400_BAD_REQUEST,
-        content=jsonable_encoder({"detail": exc.errors()}),
-    )
-
-
-async def not_implemented_handler(request: Request, exc: NotImplementedError):
-    return JSONResponse(
-        status_code=status.HTTP_501_NOT_IMPLEMENTED,
-        content={"message": str(exc)},
+        content=ValidationErrorResponse(
+            message="Validation error", detail=exc.errors()  # type: ignore
+        ).model_dump(mode="json"),
     )
 
 
-async def unauthorized_handler(request: Request, exc: Unauthorized):
+async def unauthorized_handler(request: Request, exc: Unauthorized) -> JSONResponse:
     return JSONResponse(
         status_code=status.HTTP_401_UNAUTHORIZED,
-        content={"message": "Unauthorized"},
+        content={"message": "Unauthorized", "detail": None},
         headers={"WWW-Authenticate": "Bearer"},
     )
 
 
-async def permission_denied_handler(request: Request, exc: PermissionDenied):
+async def permission_denied_handler(
+    request: Request, exc: PermissionDenied
+) -> JSONResponse:
     return JSONResponse(
         status_code=status.HTTP_403_FORBIDDEN,
-        content={"message": "Permission denied", "detail": str(exc)},
+        content={
+            "message": "Permission denied",
+            "detail": jsonable_encoder(exc.args[0] if exc.args else None),
+        },
     )

clean_python/fastapi/service.py

@@ -27,7 +27,6 @@ from clean_python.oauth2 import TokenVerifierSettings
 from .error_responses import conflict_handler
 from .error_responses import DefaultErrorResponse
 from .error_responses import not_found_handler
-from .error_responses import not_implemented_handler
 from .error_responses import permission_denied_handler
 from .error_responses import unauthorized_handler
 from .error_responses import validation_error_handler
@@ -138,7 +137,6 @@ class Service:
         app.add_exception_handler(Conflict, conflict_handler)
         app.add_exception_handler(RequestValidationError, validation_error_handler)
         app.add_exception_handler(BadRequest, validation_error_handler)
-        app.add_exception_handler(NotImplementedError, not_implemented_handler)
         app.add_exception_handler(PermissionDenied, permission_denied_handler)
         app.add_exception_handler(Unauthorized, unauthorized_handler)
         return app

tests/fastapi/test_error_responses.py

@@ -0,0 +1,115 @@
+import json
+from http import HTTPStatus
+
+from pydantic import BaseModel
+from pydantic import ValidationError
+
+from clean_python import BadRequest
+from clean_python import Conflict
+from clean_python import DoesNotExist
+from clean_python import PermissionDenied
+from clean_python import Unauthorized
+from clean_python.fastapi.error_responses import conflict_handler
+from clean_python.fastapi.error_responses import not_found_handler
+from clean_python.fastapi.error_responses import permission_denied_handler
+from clean_python.fastapi.error_responses import unauthorized_handler
+from clean_python.fastapi.error_responses import validation_error_handler
+
+
+async def test_does_not_exist():
+    actual = await not_found_handler(None, DoesNotExist("record", id=15))
+
+    assert actual.status_code == HTTPStatus.NOT_FOUND
+    assert json.loads(actual.body) == {"message": "Could not find record with id=15"}
+
+
+async def test_does_not_exist_no_id():
+    actual = await not_found_handler(None, DoesNotExist("tafeltje"))
+
+    assert actual.status_code == HTTPStatus.NOT_FOUND
+    assert json.loads(actual.body) == {"message": "Could not find tafeltje"}
+
+
+async def test_conflict():
+    actual = await conflict_handler(None, Conflict("foo"))
+
+    assert actual.status_code == HTTPStatus.CONFLICT
+    assert json.loads(actual.body) == {"message": "Conflict", "detail": "foo"}
+
+
+async def test_conflict_no_msg():
+    actual = await conflict_handler(None, Conflict())
+
+    assert actual.status_code == HTTPStatus.CONFLICT
+    assert json.loads(actual.body) == {"message": "Conflict", "detail": None}
+
+
+async def test_unauthorized():
+    actual = await unauthorized_handler(None, Unauthorized())
+
+    assert actual.status_code == HTTPStatus.UNAUTHORIZED
+    assert json.loads(actual.body) == {"message": "Unauthorized", "detail": None}
+    assert actual.headers["WWW-Authenticate"] == "Bearer"
+
+
+async def test_unauthorized_with_msg():
+    # message should be ignored
+    actual = await unauthorized_handler(None, Unauthorized("foo"))
+
+    assert actual.status_code == HTTPStatus.UNAUTHORIZED
+    assert json.loads(actual.body) == {"message": "Unauthorized", "detail": None}
+    assert actual.headers["WWW-Authenticate"] == "Bearer"
+
+
+async def test_permission_denied():
+    actual = await permission_denied_handler(None, PermissionDenied())
+
+    assert actual.status_code == HTTPStatus.FORBIDDEN
+    assert json.loads(actual.body) == {"message": "Permission denied", "detail": None}
+
+
+async def test_permission_denied_with_msg():
+    actual = await permission_denied_handler(None, PermissionDenied("foo"))
+
+    assert actual.status_code == HTTPStatus.FORBIDDEN
+    assert json.loads(actual.body) == {"message": "Permission denied", "detail": "foo"}
+
+
+class Book(BaseModel):
+    title: str
+
+
+async def test_validation_error():
+    try:
+        Book(name="foo")
+    except ValidationError as e:
+        actual = await validation_error_handler(None, e)
+
+    assert actual.status_code == HTTPStatus.BAD_REQUEST
+    assert json.loads(actual.body) == {
+        "message": "Validation error",
+        "detail": [{"loc": ["title"], "msg": "Field required", "type": "missing"}],
+    }
+
+
+async def test_bad_request_from_validation_error():
+    try:
+        Book(name="foo")
+    except ValidationError as e:
+        actual = await validation_error_handler(None, BadRequest(e))
+
+    assert actual.status_code == HTTPStatus.BAD_REQUEST
+    assert json.loads(actual.body) == {
+        "message": "Validation error",
+        "detail": [{"loc": ["title"], "msg": "Field required", "type": "missing"}],
+    }
+
+
+async def test_bad_request_from_msg():
+    actual = await validation_error_handler(None, BadRequest("foo"))
+
+    assert actual.status_code == HTTPStatus.BAD_REQUEST
+    assert json.loads(actual.body) == {
+        "message": "Validation error",
+        "detail": [{"loc": [], "msg": "foo", "type": "value_error"}],
+    }