Fix/alembic migrations#30
Conversation
Руководство для ревьюераУдаляет значение по умолчанию для Диаграмма связей сущностей для новой таблицы bans и связанных изменений схемыerDiagram
users {
int id
enum user_role
string email
string phone
string telegram_id
}
bans {
int id
string reason
int user_id
int banned_by
int revoked_by
datetime banned_at
datetime expires_at
datetime revoked_at
}
magic_tokens {
int id
int user_id
}
users ||--o{ bans : has_bans
users ||--o{ bans : bans_banned_by
users ||--o{ bans : bans_revoked_by
users ||--o{ magic_tokens : has_magic_tokens
Изменения на уровне файлов
Подсказки и командыВзаимодействие с Sourcery
Настройка работыЗайдите в свою панель управления, чтобы:
Получение помощи
Original review guide in EnglishReviewer's GuideRemoves default SECRET_KEY from configuration and introduces a new Alembic migration to align the database schema with the current models, including a bans table, stricter field nullability, new constraints/indexes, and a user_role enum on users. Entity relationship diagram for new bans table and related schema changeserDiagram
users {
int id
enum user_role
string email
string phone
string telegram_id
}
bans {
int id
string reason
int user_id
int banned_by
int revoked_by
datetime banned_at
datetime expires_at
datetime revoked_at
}
magic_tokens {
int id
int user_id
}
users ||--o{ bans : has_bans
users ||--o{ bans : bans_banned_by
users ||--o{ bans : bans_revoked_by
users ||--o{ magic_tokens : has_magic_tokens
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Привет — я нашёл 3 проблемы и оставил несколько общих замечаний:
- Удаление значения по умолчанию для
SECRET_KEYтеперь приведёт к жёсткому падению, если переменная окружения отсутствует; подумайте о том, чтобы либо валидировать и выбрасывать понятную ошибку при старте, либо держать отдельное явное dev-only значение по умолчанию/конфиг, чтобы избежать непонятных ошибок. - Новый столбец Enum
user_roleдобавлен сnullable=Falseбез значения по умолчанию на стороне сервера, что может привести к ошибкам на существующих строках при миграции; рассмотрите вариант либо сделать его nullable на время перехода, либо задать явное значение по умолчанию (например, 'user') и выполнить backfill. - В этом PR удаляется
alembic.ini— если Alembic всё ещё используется, убедитесь, что удаление этого файла намеренное и что конфигурация доступна в другом месте (например, через переменные окружения или другой путь к конфигу).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Removing the default `SECRET_KEY` will now hard-fail if the env var is missing; consider either validating and raising a clear error on startup or keeping a separate explicit dev-only default/config to avoid confusing failures.
- The new `user_role` Enum column is added as `nullable=False` without a server default, which may fail on existing rows during migration; consider either making it nullable for the transition or setting an explicit default (e.g. 'user') and backfilling.
- `alembic.ini` is being deleted in this PR—if Alembic is still in use, double-check that this file removal is intentional and that configuration is available elsewhere (e.g., via env or another config path).
## Individual Comments
### Comment 1
<location path="src/config.py" line_range="11" />
<code_context>
DATABASE_URL = os.getenv("DATABASE_URL")
-SECRET_KEY = os.getenv("SECRET_KEY", "development-secret-key-i-love_coding")
+SECRET_KEY = os.getenv("SECRET_KEY")
ALGORITHM = os.getenv("ALGORITHM", "HS256")
ACCESS_TOKEN_EXPIRE_MINUTES = int(os.getenv("ACCESS_TOKEN_EXPIRE_MINUTES", 30))
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider failing fast or providing a clear error path when SECRET_KEY is missing from the environment.
With the default removed, `SECRET_KEY` becomes `None` if the env var is missing, which can cause unclear runtime failures. Please add an explicit startup check (or use a helper for required env vars) to fail fast when `SECRET_KEY` is not set.
</issue_to_address>
### Comment 2
<location path="alembic/versions/4972fe91500d_v0_0_1.py" line_range="56" />
<code_context>
+ op.alter_column('services', 'address',
+ existing_type=sa.VARCHAR(length=100),
+ nullable=False)
+ op.add_column('users', sa.Column('user_role', sa.Enum('user', 'admin', 'moderator'), nullable=False))
+ op.alter_column('users', 'email',
+ existing_type=sa.VARCHAR(length=50),
</code_context>
<issue_to_address>
**issue (bug_risk):** Adding a non-nullable column without a default will break on existing rows.
On a populated `users` table this migration will fail, since existing rows have no value for `user_role` and the column is added as `nullable=False` with no `server_default`. Please either (a) add it as nullable with a default, backfill existing rows (e.g. `'user'`), then make it non-nullable, or (b) add it with a `server_default` that you later drop.
</issue_to_address>
### Comment 3
<location path="alembic/versions/4972fe91500d_v0_0_1.py" line_range="92" />
<code_context>
+ op.alter_column('services', 'description',
+ existing_type=sa.VARCHAR(length=2000),
+ nullable=True)
+ op.drop_constraint(None, 'magic_tokens', type_='foreignkey')
+ op.drop_column('magic_tokens', 'user_id')
+ op.alter_column('appointments', 'comment',
</code_context>
<issue_to_address>
**issue (bug_risk):** Dropping a foreign key with a `None` name may fail; the constraint name should match the one created in `upgrade`.
In `upgrade`, this FK from `magic_tokens.user_id` to `users.id` is created without a name, so the database generates one. `op.drop_constraint` needs that real name; passing `None` will likely fail when the migration runs. Define an explicit constraint name in `create_foreign_key` and reuse it here (optionally via `op.f(...)` to honor naming conventions).
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- Removing the default
SECRET_KEYwill now hard-fail if the env var is missing; consider either validating and raising a clear error on startup or keeping a separate explicit dev-only default/config to avoid confusing failures. - The new
user_roleEnum column is added asnullable=Falsewithout a server default, which may fail on existing rows during migration; consider either making it nullable for the transition or setting an explicit default (e.g. 'user') and backfilling. alembic.iniis being deleted in this PR—if Alembic is still in use, double-check that this file removal is intentional and that configuration is available elsewhere (e.g., via env or another config path).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Removing the default `SECRET_KEY` will now hard-fail if the env var is missing; consider either validating and raising a clear error on startup or keeping a separate explicit dev-only default/config to avoid confusing failures.
- The new `user_role` Enum column is added as `nullable=False` without a server default, which may fail on existing rows during migration; consider either making it nullable for the transition or setting an explicit default (e.g. 'user') and backfilling.
- `alembic.ini` is being deleted in this PR—if Alembic is still in use, double-check that this file removal is intentional and that configuration is available elsewhere (e.g., via env or another config path).
## Individual Comments
### Comment 1
<location path="src/config.py" line_range="11" />
<code_context>
DATABASE_URL = os.getenv("DATABASE_URL")
-SECRET_KEY = os.getenv("SECRET_KEY", "development-secret-key-i-love_coding")
+SECRET_KEY = os.getenv("SECRET_KEY")
ALGORITHM = os.getenv("ALGORITHM", "HS256")
ACCESS_TOKEN_EXPIRE_MINUTES = int(os.getenv("ACCESS_TOKEN_EXPIRE_MINUTES", 30))
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider failing fast or providing a clear error path when SECRET_KEY is missing from the environment.
With the default removed, `SECRET_KEY` becomes `None` if the env var is missing, which can cause unclear runtime failures. Please add an explicit startup check (or use a helper for required env vars) to fail fast when `SECRET_KEY` is not set.
</issue_to_address>
### Comment 2
<location path="alembic/versions/4972fe91500d_v0_0_1.py" line_range="56" />
<code_context>
+ op.alter_column('services', 'address',
+ existing_type=sa.VARCHAR(length=100),
+ nullable=False)
+ op.add_column('users', sa.Column('user_role', sa.Enum('user', 'admin', 'moderator'), nullable=False))
+ op.alter_column('users', 'email',
+ existing_type=sa.VARCHAR(length=50),
</code_context>
<issue_to_address>
**issue (bug_risk):** Adding a non-nullable column without a default will break on existing rows.
On a populated `users` table this migration will fail, since existing rows have no value for `user_role` and the column is added as `nullable=False` with no `server_default`. Please either (a) add it as nullable with a default, backfill existing rows (e.g. `'user'`), then make it non-nullable, or (b) add it with a `server_default` that you later drop.
</issue_to_address>
### Comment 3
<location path="alembic/versions/4972fe91500d_v0_0_1.py" line_range="92" />
<code_context>
+ op.alter_column('services', 'description',
+ existing_type=sa.VARCHAR(length=2000),
+ nullable=True)
+ op.drop_constraint(None, 'magic_tokens', type_='foreignkey')
+ op.drop_column('magic_tokens', 'user_id')
+ op.alter_column('appointments', 'comment',
</code_context>
<issue_to_address>
**issue (bug_risk):** Dropping a foreign key with a `None` name may fail; the constraint name should match the one created in `upgrade`.
In `upgrade`, this FK from `magic_tokens.user_id` to `users.id` is created without a name, so the database generates one. `op.drop_constraint` needs that real name; passing `None` will likely fail when the migration runs. Define an explicit constraint name in `create_foreign_key` and reuse it here (optionally via `op.f(...)` to honor naming conventions).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Coverage Report for CI Build 26742049364Coverage decreased (-0.2%) to 30.2%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions10 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
1 similar comment
Coverage Report for CI Build 26742049364Coverage decreased (-0.2%) to 30.2%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions10 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
Coverage Report for CI Build 26742351520Coverage decreased (-0.2%) to 30.167%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions9 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
* fix: Delete alembic.ini * chore: add alembic.ini into .gitignore * chore: update alembic scheme * fix: Remove the backup key from config.py * fix: Fix the SECRET_KEY test environment variable * fix(auth): Fixing a missing SECRET_KEY error in .env * fix(tests): Fix E401 [*] Multiple imports on one line * feat(config): Add a check for the absence of DATABSE_URL in .env * Add a database engine to lifespan's fastapi * refactor: Move session factory to lifespan --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com>
* delete cloudflared.deb" * feat(models): Start implementing the role system, add a ban table, and fix date fields by adding the timezone=True property to store time zone information. Fix and add missing relationships * chore(.gitignore): remove tracking of different caches * refactor(tg_link): rename MagicTokens to MagicToken Co-authored-by: Cursor <cursoragent@cursor.com> * fix(bot): send login-link URL on start when telegram is not linked Co-authored-by: Cursor <cursoragent@cursor.com> * test(auth): add JWT unit tests Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(models): Improve the readability of field checks in the bans table * feat(models): Add validation of the revoked_at field at the database level * feat(models): Add checks for the expires_at and revoked_at fields, and also fix the issue of multiple bans for one user simultaneously, while preserving the ban history. * refactor(models): Rename MagicTokens to MagicToken across the codebase for consistency * fix(modedls): Add foreigkey to the appointments table relationship * Fix/backround clear magic tokens (#17) * delete cloudflared.deb" * feat(models): Start implementing the role system, add a ban table, and fix date fields by adding the timezone=True property to store time zone information. Fix and add missing relationships * chore(.gitignore): remove tracking of different caches * refactor(tg_link): rename MagicTokens to MagicToken Co-authored-by: Cursor <cursoragent@cursor.com> * fix(bot): send login-link URL on start when telegram is not linked Co-authored-by: Cursor <cursoragent@cursor.com> * test(auth): add JWT unit tests Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(models): Improve the readability of field checks in the bans table * feat(models): Add validation of the revoked_at field at the database level * feat(models): Add checks for the expires_at and revoked_at fields, and also fix the issue of multiple bans for one user simultaneously, while preserving the ban history. * refactor(models): Rename MagicTokens to MagicToken across the codebase for consistency * fix(modedls): Add foreigkey to the appointments table relationship * fix(tasks): Fix database issues with the background task for clearing magic tokens --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> * feat(ci): Add Docker image building and pushing to the ghcr registry (#18) * feat(ci): Add Docker image building and pushing to the ghcr registry * feat(docker-compose): Add watchtower to query the registry and download dcoker images from there. * feat(ci): Add a commit hash tag to Docker images * chore: remove old SSH deploy workflow (#19) * Fix/put things in order (#20) * fix: Remove the get_db file from infrastructure and remove logging of all SQL queries to reduce the IO load. * chore(jwt): Remove dprecated definition of the current time * fix(compose): Fix port forwarding to localhost only (#21) Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * fix(compose): Fix the error of accessing the database outside the local network (#22) Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * Revert "fix(compose): Fix the error of accessing the database outside the local network (#22)" (#23) This reverts commit 73920aa. * Fix/magic link bot auth (#24) * fix(auth): Fix a vulnerability in the process of linking a Telegram bot to an account by adding X-Bot-Secret to the header * chore(auth): Improve logging of the create_telegram_magic_link function * fix(auth): Fix X-Bot-Secret authentication. Eliminates timig attack. * chore(requirements): update requrements.txt list * fix(bot): Add sending of the X-Bot-Secret header from the Telegram bot * fix(config): Remove os.environ, which causes an error in tests * Update comment src/presentation/api/v1/auth/tg_link.py Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * docs(services): Fix logging and doc string errors --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * Feature/docker security improvements (#25) * feat(docker): Isolate services by separating docker networks * feat(docker): Add a system user app. Remove buffering and .pyc, __pycache__ files. * feat(docker): Remove root privileges. Add cap_drop to disable root privileges. Enable write access to the /tmp folder. * feat(logger): Remove logging to a file * feat(docker): Write a user creation command in the api container and transfer the user change to the bot container * docs: Correct syntax errors --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * fix: Add && to the docker image build command (#27) Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * Feature/heartbit endpoint (#28) * feat: Add a health endpoint to check if the API is online * feat(docker): Add cleaning of outdated Docker images * fix: Change your HealthCheck internet connection to avoid being blocked by RateLimiting --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * feat: Bring the architecture to a clean archeticture. In the structur… (#29) * feat: Bring the architecture to a clean archeticture. In the structure, repeat the project structure for easier navigation. Separate the models into different files. * fix(models): Fix circular import * fix(models): Fix circular import * fix(models): correct error texts in models --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * Fix/alembic migrations (#30) * fix: Delete alembic.ini * chore: add alembic.ini into .gitignore * chore: update alembic scheme * fix: Remove the backup key from config.py * fix: Fix the SECRET_KEY test environment variable * fix(auth): Fixing a missing SECRET_KEY error in .env * fix(tests): Fix E401 [*] Multiple imports on one line * feat(config): Add a check for the absence of DATABSE_URL in .env * Add a database engine to lifespan's fastapi * refactor: Move session factory to lifespan --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * fix(migrations): Add alembic.ini file (#31) Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * refactor: refactor: simplify ban uniqueness constraint and database session handling * feat(tasks): add background task for revoking expired bans --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
* delete cloudflared.deb" * feat(models): Start implementing the role system, add a ban table, and fix date fields by adding the timezone=True property to store time zone information. Fix and add missing relationships * chore(.gitignore): remove tracking of different caches * refactor(tg_link): rename MagicTokens to MagicToken Co-authored-by: Cursor <cursoragent@cursor.com> * fix(bot): send login-link URL on start when telegram is not linked Co-authored-by: Cursor <cursoragent@cursor.com> * test(auth): add JWT unit tests Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(models): Improve the readability of field checks in the bans table * feat(models): Add validation of the revoked_at field at the database level * feat(models): Add checks for the expires_at and revoked_at fields, and also fix the issue of multiple bans for one user simultaneously, while preserving the ban history. * refactor(models): Rename MagicTokens to MagicToken across the codebase for consistency * fix(modedls): Add foreigkey to the appointments table relationship * Fix/backround clear magic tokens (#17) * delete cloudflared.deb" * feat(models): Start implementing the role system, add a ban table, and fix date fields by adding the timezone=True property to store time zone information. Fix and add missing relationships * chore(.gitignore): remove tracking of different caches * refactor(tg_link): rename MagicTokens to MagicToken Co-authored-by: Cursor <cursoragent@cursor.com> * fix(bot): send login-link URL on start when telegram is not linked Co-authored-by: Cursor <cursoragent@cursor.com> * test(auth): add JWT unit tests Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(models): Improve the readability of field checks in the bans table * feat(models): Add validation of the revoked_at field at the database level * feat(models): Add checks for the expires_at and revoked_at fields, and also fix the issue of multiple bans for one user simultaneously, while preserving the ban history. * refactor(models): Rename MagicTokens to MagicToken across the codebase for consistency * fix(modedls): Add foreigkey to the appointments table relationship * fix(tasks): Fix database issues with the background task for clearing magic tokens --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> * feat(ci): Add Docker image building and pushing to the ghcr registry (#18) * feat(ci): Add Docker image building and pushing to the ghcr registry * feat(docker-compose): Add watchtower to query the registry and download dcoker images from there. * feat(ci): Add a commit hash tag to Docker images * chore: remove old SSH deploy workflow (#19) * Fix/put things in order (#20) * fix: Remove the get_db file from infrastructure and remove logging of all SQL queries to reduce the IO load. * chore(jwt): Remove dprecated definition of the current time * fix(compose): Fix port forwarding to localhost only (#21) Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * fix(compose): Fix the error of accessing the database outside the local network (#22) Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * Revert "fix(compose): Fix the error of accessing the database outside the local network (#22)" (#23) This reverts commit 73920aa. * Fix/magic link bot auth (#24) * fix(auth): Fix a vulnerability in the process of linking a Telegram bot to an account by adding X-Bot-Secret to the header * chore(auth): Improve logging of the create_telegram_magic_link function * fix(auth): Fix X-Bot-Secret authentication. Eliminates timig attack. * chore(requirements): update requrements.txt list * fix(bot): Add sending of the X-Bot-Secret header from the Telegram bot * fix(config): Remove os.environ, which causes an error in tests * Update comment src/presentation/api/v1/auth/tg_link.py Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * docs(services): Fix logging and doc string errors --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * Feature/docker security improvements (#25) * feat(docker): Isolate services by separating docker networks * feat(docker): Add a system user app. Remove buffering and .pyc, __pycache__ files. * feat(docker): Remove root privileges. Add cap_drop to disable root privileges. Enable write access to the /tmp folder. * feat(logger): Remove logging to a file * feat(docker): Write a user creation command in the api container and transfer the user change to the bot container * docs: Correct syntax errors --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * fix: Add && to the docker image build command (#27) Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * Feature/heartbit endpoint (#28) * feat: Add a health endpoint to check if the API is online * feat(docker): Add cleaning of outdated Docker images * fix: Change your HealthCheck internet connection to avoid being blocked by RateLimiting --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * feat: Bring the architecture to a clean archeticture. In the structur… (#29) * feat: Bring the architecture to a clean archeticture. In the structure, repeat the project structure for easier navigation. Separate the models into different files. * fix(models): Fix circular import * fix(models): Fix circular import * fix(models): correct error texts in models --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * Fix/alembic migrations (#30) * fix: Delete alembic.ini * chore: add alembic.ini into .gitignore * chore: update alembic scheme * fix: Remove the backup key from config.py * fix: Fix the SECRET_KEY test environment variable * fix(auth): Fixing a missing SECRET_KEY error in .env * fix(tests): Fix E401 [*] Multiple imports on one line * feat(config): Add a check for the absence of DATABSE_URL in .env * Add a database engine to lifespan's fastapi * refactor: Move session factory to lifespan --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * fix(migrations): Add alembic.ini file (#31) Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> * refactor: refactor: simplify ban uniqueness constraint and database session handling * feat(tasks): add background task for revoking expired bans * chore(migrations): Upate migration scheme * chore: update alembic scheme --------- Co-authored-by: Squ1reX <maksimhripinkov658@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Summary by Sourcery
Добавлено начальное Alembic‑миграции для изменений схемы и усилена безопасность конфигурации.
Новые возможности:
bansи связанные с ней ограничения и индексы.user_roleв таблицуusersи установлена связьmagic_tokensсusersчерез внешний ключ.Улучшения:
appointments.comment, поля вservicesиusers.email.users.email,users.phoneиusers.telegram_id, чтобы использовать уникальность на уровне индексов вместо явных ограничений.SECRET_KEYтеперь должен задаваться через переменные окружения.Сборка:
Original summary in English
Summary by Sourcery
Add initial Alembic migration for schema changes and tighten configuration security.
New Features:
Enhancements:
Build: