Code review - Githubissues

BigDeepBlue commented 2 years ago

Слов будет много, но не пугайтесь :)

Тесты:

Тесты работать будут в контейнере, т.о. доступ тут в settings.py к переменным окружения будет и без load_dotenv(). А еще лучше использовать https://pydantic-docs.helpmanual.io/usage/settings/
В скриптах ожидания ES и Redis в тестах (tests/functional/utils/wait_for_es.py и tests/functional/utils/wait_for_redis.py) вместо вечного цикла, в который можно попасть при возникновении сложностей, лучше использовать backoff из прошлых спринтов. Он более функционален в сравнении с вечным циклом, теоретически он может сигнализировать команде поддержки после определенного количества попыток.
Вот этот импорт, наверное, лучше убрать из середины файла и разместить рядом с остальными импортами.
Данные для assert и для запросов (такие как тут и тут и тут и далее везде), не нужно явно указывать в коде тестов, правильнее парсить содержимое testdata, тогда при изменении тестовых данных не придется менять код самих тестов. Да и вообще удобно держать в одном месте.
Неиспользуемый импорт тут.

SOLID:

В сервисах fastapi/src/services/films.py, fastapi/src/services/genre.py и fastapi/src/services/person.py можно общую реализацию ряда методов вынести в базовый класс. Например: обратите внимание на метод get_by_id - он практически идентичен в каждом из этих сервисов. И проанализируйте остальные методы, может еще найдете что-либо общее.
В src/api/v1 много раз встречается вот такая конструкция в параметрах методов:

page_number: int = Query(1, alias='page[number]'), page_size: int = Query(50, alias='page[size]'),

Можно попробовать вынести эту пару в отдельный класс, какой-нибудь PaginatedParams, инжектить его в качестве зависимости в параметры методов и работать с ними как с объектом. Т.о. параметры пагинации будут в одном месте и при необходимости легко будет их изменить сразу и везде.
Вот в этом месте у вас "захардкожены" redis и ES. А что делать если завтра будет принято решение для кэширования использовать memcached или ES заменить на что-то другое. Эта задача решается использованием абстрактных классов.
Например таким:
```
  class AsyncCacheStorage(ABC):
      @abstractmethod
      async def get(self, key: str, **kwargs):
          pass

      @abstractmethod
      async def set(self, key: str, value: str, expire: int, **kwargs):
          pass
```
И тогда redis: Redis = Depends(get_redis) можно заменить на cache: AsyncCacheStorage = Depends(get_redis), главное чтобы используемый модуль кэширования реализовывал методы класса AsyncCacheStorage (в случае с кэшем это методы get и set, к счастью aioredis их уже реализует, так что даже адаптер писать не нужно). И если завтра решат перейти на memcached то нам останется лишь изменить cache: AsyncCacheStorage = Depends(get_memcached),. Тоже самое и с полнотекстовым поиском можно провернуть.

И просто к сведению, в качестве апофеоза SOLID подхода https://python-dependency-injector.ets-labs.org/examples/fastapi.html

BigDeepBlue commented 2 years ago

п. 1 SOLID интересно решили вопрос. Можно было наследовать FilmService, GenreService и PersonService от BaseDetail, тогда не пришлось бы его использовать как самостоятельный сервис.

BigDeepBlue commented 2 years ago

п.2 SOLID имелось в виде вот такое, например:

class PaginateModel(BaseModel):
    page_number: Optional[int]
    page_size: Optional[int]

async def parse_pagination(
    page_size: Optional[int] = Query(
        DEFAULT_PAGE_PARAMS['size'],
        alias='page[size]',
        description='Items amount on page',
    ),
    page_number: Optional[int] = Query(
        DEFAULT_PAGE_PARAMS['number'],
        alias='page[number]',
        description='Page number for pagination',
    ),
):
    return PaginateModel(
        page_number=max(int(page_number), 1), page_size=max(int(page_size), 1)
    )

И после этого можно:

@router.get(
    '/',
    response_model=List[Genre],
    summary='Genres list',
    description='List of all available genres with search and sorting',
    response_description='Genres list',
)
async def genres_list(
    pagination: PaginateModel = Depends(parse_pagination),
    service: GetListAndEntityService = Depends(get_genre_service),
) -> List[Genre]:
   .... и тут будет доступен объект `pagination` с его  `pagination.page_size` и `pagination.page_number`. И его можно передавать дальше в сервисы для работы.

iliazaraysky / Async_API_sprint_2

Code review #1