Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential Improvements #1

Open
ThirVondukr opened this issue Aug 20, 2022 · 9 comments
Open

Potential Improvements #1

ThirVondukr opened this issue Aug 20, 2022 · 9 comments

Comments

@ThirVondukr
Copy link

First of all - it's an amazing set of best practices, I also want to share some things that I use:

Project Structure

src could be added to PYTHONPATH to avoid
prefixing every app import with src, IDEs like PyCharm also support that.

Models also could be stored in same package, it's easier to import your models and make sure all modules were executed when you generate your migrations:

src/
  db/
    models/
      __init__.py
      comments.py
      posts.py
      tags.py
    base.py
    dependencies.py
# __init__.py
from . commends import Comment
from . posts import Post
from . tags import Tag

__all__ = ["Comment", "Post", "Tag"]
# Anywhere in the code
from db.models import Post

Continuous Integration

Absolutely use CI in gitlab/github to automate your tests and linters!

Dependency Management

Use poetry instead of requirements.txt, it's awesome!

Custom base model from day 0

This could also be used to enable orm_mode
and set up custom alias_generator if client (for example a JS app) requires it.

Use Starlette's Config object Pydantic BaseSettings!

Pydantic has its own class to manage environment variables:

class AppSettings(BaseSettings):
    class Config:
        env_prefix = "app_"

    domain: str
@zhanymkanov
Copy link
Owner

Hey, @ThirVondukr

Thank you for your kind words, I truly appreciate them!

  1. Although I find this suggestion very handy, I am not sure to advocate implicit solutions like PYTHONPATH modifications or __init__ imports. We did put some models there when it was appropriate though, just not an "every package" practice I believe.
  2. I agree, that CI is very important, but it feels like that recommendation doesn't fall into FastAPI best practices in particular.
  3. poetry is great, but I wish it would be easier to use it in docker containers :)
  4. Agree, I will add these suggestions.
  5. Agree, too many people were pointing it out. My key point was not to use other libraries but already installed ones though.

@ThirVondukr
Copy link
Author

Storing db models in same place is just a personal preference, but I feel like it's very usable in small applications/microservices.

You can use poetry in docker, either by exporting your dependencies as requirements.txt in a multi-stage build or directly:

FROM python:3.10-slim as build
ENV POETRY_VERSION=1.1.14
RUN pip install poetry==$POETRY_VERSION

COPY ./pyproject.toml ./poetry.lock ./
RUN poetry export -f requirements.txt -o requirements.txt


FROM python:3.10-slim as final

WORKDIR /app
ENV PYTHONPATH=$PYTHONPATH:src
COPY --from=build requirements.txt .
RUN pip install --upgrade pip && pip install -r requirements.txt --no-cache-dir

COPY ./src ./src
COPY alembic.ini ./

...

@zhanymkanov
Copy link
Owner

Yep, agree. We actually do store all DB models in one folder in a small service we have for transcoding.

Well, your example with multi-stage looks pretty neat and clean! I didn't like the examples I have seen online, but will definitely try your example.

@ThirVondukr
Copy link
Author

I'd say there's also a "problem" with using sqlalchemy commits with fastapi.
Usually you should use .flush() method to flush sql to database if you need a server-side generated data such as primary keys, you still can use nested transactions, but you don't need to in most cases.
If you use a typical get_session dependency and commit inside of it then commit would actually happen after the response is sent, which can lead to client receiving 200/201 for example but data won't actually be saved:

async def get_session() -> AsyncIterable[AsyncSession]:
    async with async_sessionmaker.begin() as session:
        yield session

This could be avoided by either

  1. Committing manually in each request
  2. Using another framework/library to do DI for you
  3. (Best Option) Using a middleware to commit session in it

I think that also could be covered in this repo

@pistocop
Copy link

Storing db models in same place is just a personal preference, but I feel like it's very usable in small applications/microservices.

You can use poetry in docker, either by exporting your dependencies as requirements.txt in a multi-stage build or directly:

FROM python:3.10-slim as build
ENV POETRY_VERSION=1.1.14
RUN pip install poetry==$POETRY_VERSION

COPY ./pyproject.toml ./poetry.lock ./
RUN poetry export -f requirements.txt -o requirements.txt


FROM python:3.10-slim as final

WORKDIR /app
ENV PYTHONPATH=$PYTHONPATH:src
COPY --from=build requirements.txt .
RUN pip install --upgrade pip && pip install -r requirements.txt --no-cache-dir

COPY ./src ./src
COPY alembic.ini ./

...

I agree with the usage of poetry in combination with docker multi stage, this behavior is also suggested by official documentation.

Another thing I usually do is to create a install-poetry.sh script with the poetry version specified inside, in this way there is only one point of trust used by both Docker and the Developer.

Then simply include it into the docker first stage:

RUN bash install-poetry.sh
RUN poetry export -f requirements.txt --output requirements.txt --without-hashes

@b-bokma
Copy link

b-bokma commented Nov 25, 2022

First of all - it's an amazing set of best practices, I also want to share some things that I use:

Project Structure

src could be added to PYTHONPATH to avoid prefixing every app import with src, IDEs like PyCharm also support that.

Models also could be stored in same package, it's easier to import your models and make sure all modules were executed when you generate your migrations:

src/
  db/
    models/
      __init__.py
      comments.py
      posts.py
      tags.py
    base.py
    dependencies.py
# __init__.py
from . commends import Comment
from . posts import Post
from . tags import Tag

__all__ = ["Comment", "Post", "Tag"]
# Anywhere in the code
from db.models import Post

Continuous Integration

Absolutely use CI in gitlab/github to automate your tests and linters!

Dependency Management

Use poetry instead of requirements.txt, it's awesome!

Custom base model from day 0

This could also be used to enable orm_mode and set up custom alias_generator if client (for example a JS app) requires it.

Use Starlette's Config object Pydantic BaseSettings!

Pydantic has its own class to manage environment variables:

class AppSettings(BaseSettings):
    class Config:
        env_prefix = "app_"

    domain: str

I am using the method described here as a way to split secrets over different environments with pydantic settings:
https://rednafi.github.io/digressions/python/2020/06/03/python-configs.html

@ThirVondukr
Copy link
Author

ThirVondukr commented Nov 25, 2022

@b-bokma I'm currently using kubernetes and it's really easy to store your k8s secrets and configmaps in same format as your pydantic models:

class DatabaseSettings(BaseSettings):
    class Config:
        env_prefix = "database_"

    driver: str = "postgresql+asyncpg"
    sync_driver = "postgresql+psycopg2"
    name: str
    username: str
    password: str
    host: str
apiVersion: apps/v1
kind: Deployment
metadata:
  ...
spec:
  ...
  template:
    ...
    spec:
      ...
      containers:
        - name: fastapi
          ...
          envFrom:
            - secretRef:
                name: app-database
              prefix: database_

This also helps if you need to use same secret for different applications
So we have a different sets of secrets in different namespaces at this moment 😉

@zhanymkanov zhanymkanov pinned this issue Feb 24, 2023
@zhanymkanov zhanymkanov unpinned this issue Feb 24, 2023
birthdaysgift pushed a commit to week-password/wlss-backend that referenced this issue Nov 15, 2023
После того как была разработана архитектура эндпойнтов авторизации (#77), необходимо приступить к реализации этих эндпойнтов. Т.к. процессы аутентификации и авторизации требуют наличия зарегистрированного в системе Аккаунта, то в первую очередь нужно реализовать эндпойнт регистрации (`src/auth/routes.py :: create_account`).

Планируемая архитектура бд:

**Account**

- id (primary key)
- login (unique)
- email (unique)
- created_at
- updated_at

**PasswordHash**

- account_id (foreign key, primary key)
- value

**Profile**

- account_id (foreign key, primary key)
- avatar (nullable, unique)
- description (nullable)
- name

В рамках этой задачи необходимо написать реализацию для эндпойнта регистрации.

---

В процессе реализации были приняты следующие решения:

- **Использование foreign key в качестве primary key для таблиц, имеющих связь один к одному.**

  Например, Профиль пользователя всегда связан с одним и только одним Аккаунтом, таким образом в качестве primary key для таблицы Профиля будет использоваться foreign key на айди записи в таблице Аккаунта. Это позволит нам обеспечить соблюдение некоторых инвариантов доменной модели на уровне бд.

- **Отказ от использования `default` и `onupdate` в колонках даты и времени.**

  Это решение было принято в качестве следствия из разработанного подхода работы с бд, а именно: "бд **должна** автоматизировать проверку как можно большего количества **ограничений**, но при этом бд **не должна** автоматизировать какое-либо **изменение** данных. Следование этому принципу позволит нам, во-первых, сохранить бизнес логику изменения данных на уровне кода приложения, т.е. сохранить её в явном виде, а не спрятать в бд в виде триггеров и процедур. А во-вторых, обеспечить поддержание целостности данных при попытке внесения изменений в обход кода нашего приложения (например, при проведении миграций).

- **Расположение логики обработчиков ошибок в `src/app.py`.**

  Это решение является временным компромиссом и, возможно, будет изменено во время будущего рефакторинга. Мы приняли это решение в связи с тем, что fastapi позволяет привязывать обработчики ошибок только к экземпляру приложения, но не к отдельным роутам.

- **Отказ от инкапсуляции логики создания зависимых моделей в методах основной модели.**

  Например, при создании Аккаунта всегда должен создаваться соответствующий аккаунту Профиль, но на данных момент эндпойнт создания Аккаунта не содержит в себе вызов эндпойнта создания Профиля. Это связано с тем, что после реализации MVP, мы планируем провести рефакторинг кода, связанного с внесением изменений в бд и созданием объектов модели, с помощью внедрения DDD репозиториев, фабрик и агрегатов. Наша "упрощенная" текущая реализация скорее всего позволит перейти на новый подход более безболезненно (по крайней мере сейчас так кажется).

- **Сохранение таймзоны в бд и использование timezone-aware объектов даты и времени.**

  Это позволит нам всегда точно знать, что в нашей бд все записи даты и времени добавленные/измененные нашим приложением были сделаны с учетом таймзоны.

- **Использование `.flush()` вместо `.commit()` в методах изменяющих данные в бд.**

  Все методы вносящие изменения в данные бд будут использовать `.flush()`, чтобы в нашем коде, в рамках сессии мы могли видеть данные в изменённом виде, но при этом `.commit()` будет вызываться только в тирдауне `get_db`, позволяя нам фиксировать изменения атомарно в рамках всего эндпойнта, а не в рамках вызовов отдельных методов. У это подхода тоже есть серьезный недостаток - в связи с особенностями работы `Dependency` в fastapi, применение изменений в бд будет происходить уже после того как ответ 200 будет отправлен (см [комментарий](zhanymkanov/fastapi-best-practices#1 (comment))). Но т.к. для решения этой проблемы необходимо проводить отдельный ресёрч, было принято решение исправить это в рамках [другой](#142) задачи.
birthdaysgift pushed a commit to week-password/wlss-backend that referenced this issue Nov 15, 2023
После того как была разработана архитектура эндпойнтов авторизации (#77), необходимо приступить к реализации этих эндпойнтов. Т.к. процессы аутентификации и авторизации требуют наличия зарегистрированного в системе Аккаунта, то в первую очередь нужно реализовать эндпойнт регистрации (`src/auth/routes.py :: create_account`).

Планируемая архитектура бд:

**Account**

- id (primary key)
- login (unique)
- email (unique)
- created_at
- updated_at

**PasswordHash**

- account_id (foreign key, primary key)
- value

**Profile**

- account_id (foreign key, primary key)
- avatar (nullable, unique)
- description (nullable)
- name

В рамках этой задачи необходимо написать реализацию для эндпойнта регистрации.

---

В процессе реализации были приняты следующие решения:

- **Использование foreign key в качестве primary key для таблиц, имеющих связь один к одному.**

  Например, Профиль пользователя всегда связан с одним и только одним Аккаунтом, таким образом в качестве primary key для таблицы Профиля будет использоваться foreign key на айди записи в таблице Аккаунта. Это позволит нам обеспечить соблюдение некоторых инвариантов доменной модели на уровне бд.

- **Отказ от использования `default` и `onupdate` в колонках даты и времени.**

  Это решение было принято в качестве следствия из разработанного подхода работы с бд, а именно: "бд **должна** автоматизировать проверку как можно большего количества **ограничений**, но при этом бд **не должна** автоматизировать какое-либо **изменение** данных. Следование этому принципу позволит нам, во-первых, сохранить бизнес логику изменения данных на уровне кода приложения, т.е. сохранить её в явном виде, а не спрятать в бд в виде триггеров и процедур. А во-вторых, обеспечить поддержание целостности данных при попытке внесения изменений в обход кода нашего приложения (например, при проведении миграций).

- **Расположение логики обработчиков ошибок в `src/app.py`.**

  Это решение является временным компромиссом и, возможно, будет изменено во время будущего рефакторинга. Мы приняли это решение в связи с тем, что fastapi позволяет привязывать обработчики ошибок только к экземпляру приложения, но не к отдельным роутам.

- **Отказ от инкапсуляции логики создания зависимых моделей в методах основной модели.**

  Например, при создании Аккаунта всегда должен создаваться соответствующий аккаунту Профиль, но на данных момент эндпойнт создания Аккаунта не содержит в себе вызов эндпойнта создания Профиля. Это связано с тем, что после реализации MVP, мы планируем провести рефакторинг кода, связанного с внесением изменений в бд и созданием объектов модели, с помощью внедрения DDD репозиториев, фабрик и агрегатов. Наша "упрощенная" текущая реализация скорее всего позволит перейти на новый подход более безболезненно (по крайней мере сейчас так кажется).

- **Сохранение таймзоны в бд и использование timezone-aware объектов даты и времени.**

  Это позволит нам всегда точно знать, что в нашей бд все записи даты и времени добавленные/измененные нашим приложением были сделаны с учетом таймзоны.

- **Использование `.flush()` вместо `.commit()` в методах изменяющих данные в бд.**

  Все методы вносящие изменения в данные бд будут использовать `.flush()`, чтобы в нашем коде, в рамках сессии мы могли видеть данные в изменённом виде, но при этом `.commit()` будет вызываться только в тирдауне `get_db`, позволяя нам фиксировать изменения атомарно в рамках всего эндпойнта, а не в рамках вызовов отдельных методов. У это подхода тоже есть серьезный недостаток - в связи с особенностями работы `Dependency` в fastapi, применение изменений в бд будет происходить уже после того как ответ 200 будет отправлен (см [комментарий](zhanymkanov/fastapi-best-practices#1 (comment))). Но т.к. для решения этой проблемы необходимо проводить отдельный ресёрч, было принято решение исправить это в рамках [другой](#142) задачи.
@PashaDem
Copy link

Hey, @ThirVondukr

Thank you for your kind words, I truly appreciate them!

  1. Although I find this suggestion very handy, I am not sure to advocate implicit solutions like PYTHONPATH modifications or __init__ imports. We did put some models there when it was appropriate though, just not an "every package" practice I believe.
  2. I agree, that CI is very important, but it feels like that recommendation doesn't fall into FastAPI best practices in particular.
  3. poetry is great, but I wish it would be easier to use it in docker containers :)
  4. Agree, I will add these suggestions.
  5. Agree, too many people were pointing it out. My key point was not to use other libraries but already installed ones though.

I don't understand why not to use poetry right in docker?
It is possible and very convenient to use the groups of dependencies provided by poetry?

The workflow is the following:

  1. install poetry tool by pip (the problem can only be in the size of this tool)
  2. copy pyproject.toml and poetry.lock files
  3. set env variable POETRY_VIRTUALENVS_CREATE to false, not to create env in docker container
  4. run poetry install --only main --no-root

@ThirVondukr
Copy link
Author

@PashaDem Because I personally don't think you need an extra dependency/tool inside of your docker container, if you need it for something - add it to your docker image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants