Unify Error Handling - #124
Conversation
0a62d71 to
416a612
Compare
|
|
||
| def __init__( | ||
| self, | ||
| message: str, |
There was a problem hiding this comment.
could we add '*' after 'message' so that the remaining parameters are keyword-only? This makes call sites clearer and prevents accidentally passing status_code, errors, or headers in the wrong order. We should check and update any existing positional usages as part of this change
| ) -> None: | ||
| super().__init__(message) | ||
| self.message = message | ||
| if status_code is not None: |
There was a problem hiding this comment.
non-blocking: we could make this explicit:
self.status_code = type(self).status_code if status_code is None else status_code
current code also works-it falls back to the subclass’s class attribute when no instance value is assigned
| async def request_validation_handler(_: Request, exc: RequestValidationError) -> JSONResponse: | ||
| # jsonable_encoder is required here: pydantic puts raw exception objects in `ctx`. | ||
| return to_error_response( | ||
| UnprocessableEntityError(message="Request validation failed", errors={"fields": exc.errors()}), |
There was a problem hiding this comment.
could we avoid passing exc.errors() through unchanged? it may contain PII in input/msg and non-serialisable exceptions in ctx. perhaps we could expose only safe loc and type values
| def _(exc: SQLAIntegrityError | PsycopgIntegrityError, entity: str) -> AppError: | ||
| return BadRequestError( | ||
| message=f"{entity.capitalize()} entity already exists.", | ||
| errors={"detail": str(exc.args[0])}, |
There was a problem hiding this comment.
could we avoid returning the raw database exception? it may expose constraint names and submitted values. a fixed error code would be safer
| }, | ||
| ) | ||
| async def get_user_activity(request: Request, user_id: UUID, session: AsyncDbSession): | ||
| if not await user_service.is_user_active(session, user_id): |
There was a problem hiding this comment.
could we distinguish a missing user from an inactive one? is_user_active returns False for both, so an unknown ID currently receives 403 instead of 404.
| """ | ||
|
|
||
| message: str | ||
| status_code: int |
There was a problem hiding this comment.
non-blocking: could we omit status_code from AppErrorResponse? AppError still needs it to set the HTTP status, but serialising the same value in the body is redundant and could drift in other response paths.
| status_code = 403 | ||
|
|
||
| def __init__(self, user_id: UUID) -> None: | ||
| super().__init__(message=f"User {user_id} is inactive") |
There was a problem hiding this comment.
could we avoid including user_id in the exception message? exception messages may be captured by logs or Sentry, so this unnecessarily propagates a user identifier. a fixed message with a stable user_inactive code would be safer
2 participants
Summary
Every API error now returns one JSON shape.
AppError is now the single currency: domain modules subclass it, handle_exception converts infrastructure exceptions into it and one handler serializes all of them to AppErrorResponse.
Changes
Verified