dkarpele / Async_API_sprint_1

Sprint 4, Async API
0 stars 0 forks source link

Code review #23

Closed BigDeepBlue closed 1 year ago

BigDeepBlue commented 1 year ago

Отличная работа!

  1. В FastAPI появилась более удобная альтернатива для обработки событий @app.on_event('startup') и @app.on_event('shutdown') https://fastapi.tiangolo.com/advanced/events/. Можно попробовать использовать.

  2. Конфиг src/core/config.py можно было оформить в виде класса:

      from pydantic import BaseSettings
    
      class Settings(BaseSettings):
          redis_host: str = Field(..., env='REDIS_HOST')
          class Config:
              env_file = '.env'
    
      settings = Settings()

    и затем использовать инстанс конфига setting в разных частях приложения. Плюс такого подхода в том, что происходит валидация типов переменных окружения при помощи pydantic. Также при создании инфраструктуры под тесты, можно будет наследоваться от этого класса и создать отдельный тестовый конфиг со своими переменными окружения. Подробнее https://pydantic-docs.helpmanual.io/usage/settings/ Дам пример:

    BaseSettings позволяет упростить все до:

            class Settings(BaseSettings):
                project_name: str = 'Some project name'
                redis_host: str = ...
                redis_port: int = ...
                elastic_host: str = Field(..., env='ELASTIC_HOST_NAME')
                elastic_port: int = Field(9200, env='ELASTIC_PORT')

    BaseSettings регистронезависимый и работает вот так:

    • присвоит в поле project_name значение переменной PROJECT_NAME, а если ее нет, то даст значение по умолчанию 'Some project name'
    • redis_host мы сделали обязательным полем (... - служебный символ), если не будет переменной окружения REDIS_HOST, то проект не заведется
    • elastic_host мы сделали обязательным и явно указали имя переменной окружения, т.к. она отличается от имени поля
    • elastic_port так же как и с хостом, но указали и значение по умолчанию
  3. Начиная с python 3.10 можно не использовать Optional из модуля typing. В новом синтаксисе можно использовать вот такую конструкцию: redis: Redis | None = None. Обратите на это внимание в файлах src/db

  4. Параметры пагинации в эндпоинтах необходимо валидировать, пользователь может прислать отрицательные значения. Про валидацию посмотрите тут https://fastapi.tiangolo.com/tutorial/path-params-numeric-validations/#number-validations-greater-than-or-equal это применимо и к Query

  5. Продолжая про пагинацию, в техническом задании было явно указано как эти параметры должны называться GET /api/v1/films?sort=-imdb_rating&page_size=50&page_number=1. В вашем случае достаточно будет добавить alias. https://fastapi.tiangolo.com/tutorial/query-params-str-validations/#alias-parameters

  6. Вы используете fastapi_pagination, но есть одна проблема. Эта библиотека не умеет работать с ElasticSearch https://uriyyo-fastapi-pagination.netlify.app/integrations/available-integrations/. А значит будет выбирать ВСЕ записи из ES и после этого разбивать их на страницы, что крайне скверно по памяти.

dkarpele commented 1 year ago

Hi @BigDeepBlue , please review.

BigDeepBlue commented 1 year ago

Сделайте поля класса Settings строчными (pep8 настоятельно рекомендует так именовать поля класса) и увидите, что все будет работать

BigDeepBlue commented 1 year ago

LGTM