Skip to content

Commit

Permalink
Зафиксировать иерархию классов ошибок (#140)
Browse files Browse the repository at this point in the history
Во время работы над добавлением сваггера для эндпойнтов авторизации (#77) была добавлена определённая иерархия классов ошибок, но так как эта часть не являлась основной целью задачи, то эта иерархия была сделана на скорую руку и содержала определённые недостатки.

На данный момент у нас всё ещё нет четкого понимания, как именно мы хотим работать с ошибками, т.к. кажется, что это довольно сложная проблема, в которой необходимо учесть много нюансов.

Но несмотря на то, что на данный момент у нас ещё нет конечного варианта архитектуры ошибок, мы всё равно можем поправить хотя бы тот недостаток существующей архитектуры, который виден сейчас. А именно:

(Основная проблема из-за которой был инициирован этот рефакторинг) - Объекты ошибок могут иметь разные значения определённых полей, т.к. их определение доступно через конструктор, например:
```python
class NotFoundException(HTTPException):
    """Exception for 404 NOT FOUND error."""

    resource: str

    description = "Requested resource not found."
    details = "Requested resource doesn't exist or has been deleted."

    def __init__(self: Self, resource: str, *args: Any, **kwargs: Any) -> None:  # pragma: no cover
        """Initialize object."""
        self.resource = resource
        super().__init__(*args, **kwargs)
```
Это значит, что в разных местах кода, или даже в разных эндпойнтах, могут подниматься ошибки с разным текстом поля `details`:
```python
raise NotFoundException(resource="foo", details="foo not found")

raise NotFoundException(resource="bar", details="bar not found")
```
И в случае если нам надо будет поменять текст ошибки для ресурса "foo", то необходимо будет использовать полнотекстовый поиск для нахождения в коде всех строк, содержащих `"foo"`.

Вместо этого, удобнее было бы иметь конкретный класс ошибки под каждую сущность, без возможности переопределения аттрибутов, например:
```python
class FooNotFoundException(NotFoundException):
    resource = "foo"

    description = "not found"
    details = "foo not found"

class BarNotFoundException(NotFoundException):
    resource = "bar"

    description = "not found"
    details = "bar not found"
```
таким образом в коде получится:
```python
raise FooNotFoundException()

raise BarFoundException()
```

Это позволит нам легко изменить текст ошибки для ресурса "foo", просто поменяв значение атрибута в классе.

Для решения этой проблемы, необходимо изменить архитектуру классов исключений так, чтобы она удовлетворяла следующим требованиям:

1. Базовые классы исключений находятся в `src/shared/exceptions.py` и определяют интерфейс исключений подклассов, для возможности удобной обработки разных исключений одного типа. Например, если в `src/shared/exceptions.py` определено исключение
   ```python
   class NotFoundException(HTTPException):
       """Exception for 404 NOT FOUND error."""

       resource: str

       description = "Requested resource not found."
       details = "Requested resource doesn't exist or has been deleted."
   ```
   это значит, что у всех подклассов класса `NotFoundException` должны быть определены поля `resource`, `description` и `details`. (К сожалению, python не предоставляет возможности обеспечить соблюдение этого требования, либо такая возможность не была найдена во время работы над этим тикетом, поэтому разработчикам придётся следить за его соблюдением самостоятельно). Соблюдение этого требования позволит нам обрабатывать все исключения одного и того же типа единообразно, например:
   ```python
   @app.exception_handler(NotFoundException)
   def handle_not_found_exception(request: Request, exception: NotFoundException) -> JSONResponse:
       # будет работать правильно как для FooNotFoundException так и для BarNotFoundException
       return JSONResponse(
           status_code=exception.status_code,
           content={
               "resource": exception.resource,
               "description": exception.description,
               "details": exception.details,
           },
       )
   ```

2. Все классы исключений, поднимаемых в коде, находятся в своём "feature-package", например - `FooNotFoundException` и `DuplicateFooException` находятся в `src/foo/exceptions.py` и наследуются от соответствующих им `NotFoundException` и `NotAllowedException` из модуля `src/shared/exceptions.py`.

3. Каждый базовый класс исключения содержит HTTP код, с которым будет отправлен ответ в случае если ошибка подобного класса (или подкласса этого класса) не будет обработана в роуте.

4. Все обработчики исключений верхнего уровня (т.е. обработчики исключений запускающиеся после того как исключение не было обработано в рамках роута) должны обрабатывать только исключения базовых типов - т.е. исключения из модуля `src/shared/exceptions.py`.

Таким образом в рамках этой задачи необходимо изменить иерархию классов ошибок для соответствия требованиям, описанным выше.
  • Loading branch information
birthdaysgift committed Nov 5, 2023
1 parent 045eace commit 8dcfb08
Show file tree
Hide file tree
Showing 10 changed files with 42 additions and 76 deletions.
3 changes: 1 addition & 2 deletions src/account/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

from src.account import schemas
from src.account.fields import Login
from src.auth import swagger as auth_swagger
from src.auth.security import get_token
from src.shared import swagger as shared_swagger

Expand All @@ -31,7 +30,7 @@
},
},
},
status.HTTP_401_UNAUTHORIZED: auth_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
response_model=schemas.AccountId,
Expand Down
12 changes: 0 additions & 12 deletions src/auth/exceptions.py

This file was deleted.

10 changes: 5 additions & 5 deletions src/auth/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from fastapi.security import HTTPAuthorizationCredentials
from pydantic import PositiveInt

from src.auth import schemas, swagger
from src.auth import schemas
from src.auth.security import get_token
from src.shared import swagger as shared_swagger

Expand Down Expand Up @@ -90,7 +90,7 @@ async def create_account(
},
},
},
status.HTTP_401_UNAUTHORIZED: swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down Expand Up @@ -160,7 +160,7 @@ async def create_tokens(
},
},
},
status.HTTP_401_UNAUTHORIZED: swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down Expand Up @@ -192,7 +192,7 @@ async def refresh_tokens(
status.HTTP_204_NO_CONTENT: {
"description": "All tokens for particular device are removed. User has been signed out from this device.",
},
status.HTTP_401_UNAUTHORIZED: swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand All @@ -214,7 +214,7 @@ async def remove_device_tokens(
status.HTTP_204_NO_CONTENT: {
"description": "All account tokens are removed. User has been signed out from all devices.",
},
status.HTTP_401_UNAUTHORIZED: swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down
6 changes: 1 addition & 5 deletions src/auth/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,12 @@
from src.auth.fields import Password
from src.profile.fields import Description, Name
from src.profile.schemas import Profile
from src.shared.schemas import HTTPError, Schema
from src.shared.schemas import Schema


DictT = TypeVar("DictT", bound=dict[str, Any])


class NotAuthenticatedResponse(HTTPError): # noqa: N818
"""Schema for 401 UNAUTHORIZED response."""


class _NewAccount(Schema):
"""Account data which is going to be created during sign up process."""

Expand Down
23 changes: 0 additions & 23 deletions src/auth/swagger.py

This file was deleted.

13 changes: 6 additions & 7 deletions src/friendship/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from fastapi.security import HTTPAuthorizationCredentials
from pydantic import PositiveInt

from src.auth import swagger as auth_swagger
from src.auth.security import get_token
from src.friendship import schemas
from src.shared import swagger as shared_swagger
Expand Down Expand Up @@ -58,7 +57,7 @@
},
},
},
status.HTTP_401_UNAUTHORIZED: auth_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down Expand Up @@ -91,7 +90,7 @@ async def get_friends(
},
},
},
status.HTTP_401_UNAUTHORIZED: auth_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
},
response_model=schemas.FriendshipRequest,
Expand All @@ -115,7 +114,7 @@ async def create_friendship_request(
status.HTTP_204_NO_CONTENT: {
"description": "Friendship request has been removed (same as cancelled).",
},
status.HTTP_401_UNAUTHORIZED: auth_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down Expand Up @@ -153,7 +152,7 @@ async def cancel_friendship_request(
},
},
},
status.HTTP_401_UNAUTHORIZED: auth_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down Expand Up @@ -186,7 +185,7 @@ async def accept_friendship_request(
},
},
},
status.HTTP_401_UNAUTHORIZED: auth_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down Expand Up @@ -237,7 +236,7 @@ async def reject_friendship_request() -> None:
},
},
},
status.HTTP_401_UNAUTHORIZED: auth_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down
5 changes: 2 additions & 3 deletions src/profile/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from fastapi.security import HTTPAuthorizationCredentials
from pydantic import PositiveInt

from src.auth import swagger as auth_swagger
from src.auth.security import get_token
from src.profile import schemas
from src.shared import swagger as shared_swagger
Expand All @@ -34,7 +33,7 @@
},
},
},
status.HTTP_401_UNAUTHORIZED: auth_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down Expand Up @@ -66,7 +65,7 @@ def get_profile(
},
},
},
status.HTTP_401_UNAUTHORIZED: auth_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_401_UNAUTHORIZED: shared_swagger.responses[status.HTTP_401_UNAUTHORIZED],
status.HTTP_403_FORBIDDEN: shared_swagger.responses[status.HTTP_403_FORBIDDEN],
status.HTTP_404_NOT_FOUND: shared_swagger.responses[status.HTTP_404_NOT_FOUND],
},
Expand Down
30 changes: 11 additions & 19 deletions src/shared/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,23 @@

from __future__ import annotations

from typing import TYPE_CHECKING


if TYPE_CHECKING:
from typing import Any, Self
from fastapi import status


class HTTPException(Exception): # noqa: N818
"""Base class for all app exceptions."""

description = "Unknown error occured."
details = "Please contact backend maintenance team."
status_code = status.HTTP_500_INTERNAL_SERVER_ERROR


def __init__(self: Self, details: str = "") -> None: # pragma: no cover
"""Initialize object."""
self.details = details
super().__init__()
class NotAuthenticatedException(HTTPException):
"""Exception for 401 UNAUTHORIZED error."""

description = "Request initiator is not authenticated."
details = "Your credentials or tokens are invalid or missing."
status_code = status.HTTP_401_UNAUTHORIZED


class NotFoundException(HTTPException):
Expand All @@ -28,11 +28,7 @@ class NotFoundException(HTTPException):

description = "Requested resource not found."
details = "Requested resource doesn't exist or has been deleted."

def __init__(self: Self, resource: str, *args: Any, **kwargs: Any) -> None: # pragma: no cover
"""Initialize object."""
self.resource = resource
super().__init__(*args, **kwargs)
status_code = status.HTTP_404_NOT_FOUND


class NotAllowedException(HTTPException):
Expand All @@ -42,8 +38,4 @@ class NotAllowedException(HTTPException):

description = "Requested action not allowed."
details = "Provided tokens or credentials don't grant you enough access rights."

def __init__(self: Self, action: str, *args: Any, **kwargs: Any) -> None: # pragma: no cover
"""Initialize object."""
self.action = action
super().__init__(*args, **kwargs)
status_code = status.HTTP_403_FORBIDDEN
4 changes: 4 additions & 0 deletions src/shared/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ class HTTPError(Schema):
details: str


class NotAuthenticatedResponse(HTTPError): # noqa: N818
"""Schema for 401 UNAUTHORIZED response."""


class NotFoundResponse(HTTPError): # noqa: N818
"""Schema for 404 NOT FOUND error."""

Expand Down
12 changes: 12 additions & 0 deletions src/shared/swagger.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,18 @@


responses = { # pylint: disable=consider-using-namedtuple-or-dataclass
status.HTTP_401_UNAUTHORIZED: {
"description": "Provided tokens or credentials are invalid or missing.",
"model": schemas.NotAuthenticatedResponse,
"content": {
"application/json": {
"example": {
"description": exceptions.NotAuthenticatedException.description,
"details": exceptions.NotAuthenticatedException.details,
},
},
},
},
status.HTTP_403_FORBIDDEN: {
"description": "Provided tokens or credentials don't grant you enough access rights.",
"model": schemas.NotAllowedResponse,
Expand Down

0 comments on commit 8dcfb08

Please sign in to comment.