Add Alembic migrations for database schema management with SQLModel #14
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughIntroduces Alembic-based database migrations replacing manual table creation. Adds modular database infrastructure (base.py, session.py, migrations.py), Alembic configuration, migration files, and updates application startup to automatically execute migrations on boot. Changes
Sequence DiagramsequenceDiagram
participant App as Application Startup
participant Lifespan as Lifespan (on_startup)
participant Migrations as app.db.migrations
participant Alembic as Alembic Engine
participant DB as SQLite Database
rect rgb(220, 240, 255)
Note over App,DB: New Alembic-Based Flow
end
App->>Lifespan: start_up()
activate Lifespan
Lifespan->>Migrations: run_migrations()
activate Migrations
Migrations->>Alembic: Build config from alembic.ini
Alembic->>DB: Connect to database
DB-->>Alembic: Connection established
Alembic->>DB: Query current revision
DB-->>Alembic: Return alembic_version
Alembic->>DB: Execute pending migrations<br/>(e.g., create tables, indexes)
DB-->>Alembic: Schema updated
Alembic-->>Migrations: Migration complete
deactivate Migrations
Migrations-->>Lifespan: return (no errors)
Lifespan->>Lifespan: Continue startup<br/>(cleanup jobs, init workers)
deactivate Lifespan
Lifespan-->>App: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: This PR introduces substantial architectural changes across multiple subsystems—new modular database infrastructure (base.py, session.py, migrations.py), comprehensive Alembic configuration and migration setup, significant refactoring of app/db/models.py to use ModelBase and CRUD helpers, integration into startup lifecycle, and updated test fixtures. While many changes follow consistent patterns (e.g., model migration to ModelBase), the heterogeneity of concerns (ORM structure, migration framework, lifecycle integration, schema design, documentation, tests) and logic density in migrations.py and models.py require distinct reasoning for each area. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Co-authored-by: Zzackllack <149129501+Zzackllack@users.noreply.github.com>
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
anibridge-docs | c3d2370 | Nov 18 2025, 03:07 PM |
Co-authored-by: Zzackllack <149129501+Zzackllack@users.noreply.github.com>
Co-authored-by: Zzackllack <149129501+Zzackllack@users.noreply.github.com>
Co-authored-by: Zzackllack <149129501+Zzackllack@users.noreply.github.com>
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
docs/DATABASE_QUICKSTART.md (1)
76-87: Fix markdown formatting for code blocks.The ASCII art diagrams (directory structure and workflow) should have language specifiers for proper rendering, and code blocks should be surrounded by blank lines per markdown best practices.
Apply these changes:
-``` +```text app/db/ ├── base.py # ModelBase for all table models-``` +```text ┌─────────────────┐ │ Modify models │Also ensure blank lines surround all fenced code blocks (Lines 151, as flagged by markdownlint).
Also applies to: 97-122
alembic/MIGRATIONS.md (1)
1-219: Fix markdown formatting issues.Multiple markdown formatting issues were flagged by markdownlint: headings should be surrounded by blank lines, and fenced code blocks should have language specifiers and be surrounded by blank lines. While these are minor, fixing them improves documentation quality and consistency.
Key fixes needed:
- Add blank lines around headings (Lines 9, 20, 27, 33, 40, 47, 56, etc.)
- Add blank lines around fenced code blocks (Lines 35, 42, 48, 67, 74, etc.)
- Add language specifiers to code blocks (Lines 48, 191)
Example:
## Key Concepts + - **Migrations**: Version-controlled database schema changesapp/db/__init__.py (1)
20-29: Simplify the all construction.Line 29's comprehension logic is complex and fragile. The exclusion list
["base", "session", "models", "migrations"]must be maintained manually.Consider explicit listing for clarity:
__all__ = [ "ModelBase", "engine", "get_session", "dispose_engine", "DATABASE_URL", "run_migrations", "get_current_revision", "check_migrations_status", -] + [name for name in dir() if not name.startswith("_") and name not in ["base", "session", "models", "migrations"]] + # Explicitly list exports from models for clarity + "Job", + "EpisodeAvailability", + "ClientTask", + # ... other model exports +]This makes the public API explicit and maintainable.
requirements.runtime.txt (1)
7-7: Update Alembic to use the latest stable version constraint.The new
alembicdependency lacks version constraints, which can lead to unexpected breaking changes and supply-chain risks. Pinning versions improves reproducibility and security. The latest stable version is 1.17.0 (released Oct 11, 2025).-alembic +alembic>=1.17.0,<2.0.0Apply this pattern across all dependencies for better supply-chain security.
app/db/session.py (1)
21-29: Clarify the comment about check_same_thread.The comment at lines 22-23 states that
check_same_thread=Falseallows SQLite to be used with async code, but this is misleading. The setting actually allows SQLite connections to be used across multiple threads, which is necessary for the multi-threaded nature of FastAPI (even in sync endpoints). The async vs sync distinction is not the primary reason for this configuration.Apply this diff to improve the comment:
# Create SQLAlchemy engine with SQLite-specific configuration -# - check_same_thread=False: Allow SQLite to be used with async code +# - check_same_thread=False: Allow SQLite connections to be used across multiple threads # - NullPool: Ensure connections are closed when sessions end (important for SQLite) engine = create_engine(app/db/migrations.py (1)
85-106: Consider logging exceptions at error level for better diagnostics.Similar to
get_current_revision(), this function catches broad exceptions and returns an error dictionary. While it does include the error message in the return value (which is better), logging at warning level may cause these issues to be overlooked in production.Apply this diff:
except Exception as e: - logger.warning(f"Failed to check migration status: {e}") + logger.error(f"Failed to check migration status: {e}", exc_info=True) return {app/db/models.py (1)
357-365: Consider simplifying the boolean comparison.In SQLAlchemy/SQLModel queries, boolean columns can be used directly in WHERE clauses without explicit
== Truecomparison.Apply this diff:
select(EpisodeAvailability).where( (EpisodeAvailability.slug == slug) & (EpisodeAvailability.season == season) & (EpisodeAvailability.episode == episode) & (EpisodeAvailability.site == site) - & (EpisodeAvailability.available == True) + & EpisodeAvailability.available )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
MIGRATION_SUMMARY.md(1 hunks)alembic.ini(1 hunks)alembic/MIGRATIONS.md(1 hunks)alembic/README(1 hunks)alembic/env.py(1 hunks)alembic/script.py.mako(1 hunks)alembic/versions/397fa0304f9f_initial_migration_with_existing_schema.py(1 hunks)app/core/lifespan.py(1 hunks)app/db/__init__.py(1 hunks)app/db/base.py(1 hunks)app/db/migrations.py(1 hunks)app/db/models.py(3 hunks)app/db/session.py(1 hunks)docs/DATABASE.md(1 hunks)docs/DATABASE_QUICKSTART.md(1 hunks)docs/EXAMPLE_NEW_MODEL.md(1 hunks)requirements.runtime.txt(1 hunks)tests/conftest.py(2 hunks)tests/test_migrations.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T21:03:52.730Z
Learnt from: CR
PR: Zzackllack/AniBridge#0
File: AGENTS.md:0-0
Timestamp: 2025-10-10T21:03:52.730Z
Learning: Applies to app/db/**/*.py : Define persistence models with SQLModel under app/db and keep CRUD/helpers colocated
Applied to files:
app/db/__init__.py
🪛 LanguageTool
docs/EXAMPLE_NEW_MODEL.md
[style] ~317-~317: Consider using a different verb for a more formal wording.
Context: ...plied migrations** - Create new ones to fix issues 7. **Document complex migrations...
(FIX_RESOLVE)
alembic/MIGRATIONS.md
[style] ~160-~160: Consider using a different verb for a more formal wording.
Context: ...sting migrations** - Create new ones to fix issues 6. **Document complex migrations...
(FIX_RESOLVE)
docs/DATABASE.md
[style] ~396-~396: Consider using a different verb for a more formal wording.
Context: ...migrations** - Create new migrations to fix issues 5. **Document complex migrations...
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.18.1)
docs/DATABASE_QUICKSTART.md
76-76: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
151-151: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
alembic/MIGRATIONS.md
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
20-20: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
33-33: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
35-35: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
42-42: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
47-47: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
48-48: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
56-56: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
67-67: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
74-74: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
79-79: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
109-109: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
113-113: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
120-120: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
122-122: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
124-124: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
127-127: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
129-129: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
134-134: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
136-136: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
158-158: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
159-159: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
165-165: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
166-166: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
191-191: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
202-202: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
docs/DATABASE.md
38-38: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
117-117: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
117-117: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
233-233: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
254-254: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
254-254: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
MIGRATION_SUMMARY.md
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
20-20: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
33-33: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
35-35: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
42-42: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
47-47: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
48-48: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
56-56: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
67-67: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
74-74: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
79-79: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
109-109: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
113-113: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
120-120: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
122-122: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
124-124: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
127-127: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
129-129: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
134-134: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
136-136: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
158-158: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
159-159: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
165-165: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
166-166: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
191-191: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
202-202: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
🪛 Ruff (0.14.1)
app/db/__init__.py
18-18: from .models import * used; unable to detect undefined names
(F403)
tests/test_migrations.py
35-35: Unused function argument: client
(ARG001)
52-52: Unused function argument: client
(ARG001)
68-68: Possible SQL injection vector through string-based query construction
(S608)
73-73: Unused function argument: client
(ARG001)
89-89: Possible SQL injection vector through string-based query construction
(S608)
94-94: Unused function argument: client
(ARG001)
141-141: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
164-164: Unused function argument: client
(ARG001)
app/db/session.py
72-72: Do not catch blind exception: Exception
(BLE001)
76-76: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
alembic/env.py
48-48: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
app/db/migrations.py
27-30: Avoid specifying long messages outside the exception class
(TRY003)
80-80: Do not catch blind exception: Exception
(BLE001)
97-100: Consider moving this statement to an else block
(TRY300)
101-101: Do not catch blind exception: Exception
(BLE001)
109-114: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: anibridge-docs
🔇 Additional comments (30)
alembic/README (1)
1-1: LGTM!This is standard Alembic boilerplate. The detailed migration documentation is appropriately placed in
alembic/MIGRATIONS.mdand other docs.tests/conftest.py (2)
40-43: LGTM!Adding the new database modules to the reset list ensures clean test isolation. This prevents stale router instances and ensures migrations run with a fresh state.
61-66: LGTM!Replacing direct table creation with
run_migrations()aligns test setup with the production startup flow. This ensures tests validate the actual migration-based schema setup.docs/DATABASE_QUICKSTART.md (1)
1-183: LGTM!Comprehensive quick-start guide that effectively introduces the Alembic-based migration workflow. The structure is clear, examples are practical, and troubleshooting guidance is helpful.
docs/EXAMPLE_NEW_MODEL.md (1)
1-360: LGTM!Excellent step-by-step guide demonstrating the complete workflow for adding a new model. The inclusion of CRUD operations, testing, rollback verification, and data migration examples provides comprehensive coverage for developers.
alembic/MIGRATIONS.md (1)
87-95: LGTM!Documenting automatic migration execution on startup is crucial. This clearly explains how the application ensures schema consistency without manual intervention.
app/db/base.py (2)
19-36: LGTM!The
ModelBaseclass provides a clean abstraction for all table models with proper documentation and an example. The type ignore comment is appropriate for the non-standard registry parameter.
12-16: The review comment is incorrect—Python's standard import caching prevents the production issue described.The code is working as intended. Python modules are cached in
sys.modulesafter first import, so in productionapp.db.baseis imported once and the_registryis created once. The comment "Each import of this module creates a fresh registry" accurately describes test behavior, not production behavior.In tests,
tests/conftest.py(lines 35-58) explicitly deletesapp.db.basefromsys.modulesto force a fresh import for each test, which is the intended design to avoid SQLAlchemy warnings about duplicate class names. This is correct and necessary for test isolation.No dynamic imports or module reloading of
app.db.baseexist in production code—the only module reloading happens in the test fixture, which is intentional.Likely an incorrect or invalid review comment.
alembic.ini (1)
84-90: LGTM!The comment correctly notes that the database URL is dynamically configured in env.py (line 44) from app.db.session.DATABASE_URL, ensuring consistency with the application's runtime configuration. The placeholder here serves as documentation.
app/core/lifespan.py (1)
109-124: LGTM! Migrations run before database access.The startup sequence is correct: migrations run first (lines 112-117), then dangling jobs are cleaned up (lines 120-123). This ensures the schema is up-to-date before any database operations. Error handling properly logs and re-raises migration failures to prevent startup with an inconsistent schema.
alembic/script.py.mako (1)
1-28: LGTM!This is a standard Alembic migration script template with proper structure, type hints, and docstrings. The template correctly handles revision identifiers and provides upgrade/downgrade functions.
alembic/env.py (2)
46-50: LGTM! Model import is necessary for metadata registration.Line 48 imports all models to ensure they are registered with ModelBase.metadata, which is required for Alembic's autogenerate support. The noqa comment is unnecessary (as noted by static analysis) but harmless. The target_metadata is correctly set to ModelBase.metadata.
70-82: LGTM! Batch mode correctly enabled for SQLite.Both offline and online migration modes correctly set
render_as_batch=True(lines 77, 102), which is essential for SQLite's limited ALTER TABLE support. This ensures migrations can add/remove columns and modify constraints.Also applies to: 91-107
docs/DATABASE.md (1)
1-426: Excellent comprehensive database documentation!This documentation provides thorough coverage of:
- Architecture and components
- Model schemas with detailed field descriptions
- Complete migration workflow (automatic, manual, and testing)
- Development and production deployment procedures
- Troubleshooting guidance and best practices
The documentation will be invaluable for developers working with the database layer.
tests/test_migrations.py (5)
9-23: LGTM! Comprehensive utility function coverage.Tests verify that all migration utilities are importable and callable, ensuring the public API is properly exposed.
25-50: LGTM! Migration status verification is thorough.Tests verify both Alembic configuration existence and migration status, ensuring the migration infrastructure is properly initialized and tracking versions correctly.
52-92: LGTM! Schema validation is comprehensive.Tests verify that migrations create all expected tables and indexes. The SQL queries use SQLite's metadata tables (sqlite_master) with literal table/index names from the codebase, not user input, so the static analysis SQL injection warnings are false positives.
94-147: LGTM! CRUD operations validated post-migration.Tests exercise all model CRUD functions (Job, EpisodeAvailability, ClientTask) to ensure models work correctly after migrations. The hardcoded
/tmppath is just test data, not a security concern.
149-226: LGTM! Excellent coverage of edge cases and infrastructure.Tests verify:
- Migration idempotency (safe to run multiple times)
- Alembic version tracking table
- Database URL configuration
- ModelBase metadata registration
- Session lifecycle management
- Engine disposal
This comprehensive test suite ensures the migration infrastructure is robust and reliable.
app/db/migrations.py (2)
16-38: LGTM!The function correctly locates and validates the Alembic configuration, with clear error messaging when the configuration file is missing.
41-63: LGTM!The migration execution logic is correct and properly handles errors by logging and re-raising, which is appropriate for startup-critical code.
MIGRATION_SUMMARY.md (1)
1-214: Comprehensive documentation for the migration integration.The summary provides clear guidance on the Alembic migration workflow, benefits, and deployment steps. The content aligns well with the code changes reviewed.
app/db/models.py (8)
1-31: LGTM!The module docstring is comprehensive and provides clear guidance for adding new models with Alembic migrations. The imports are clean and appropriate.
36-60: LGTM!The datetime helper functions correctly handle timezone-aware datetime objects, which is essential for consistent timestamp handling across the application.
66-81: LGTM!The Job model is well-structured with appropriate indexes on frequently queried fields (status, source_site, timestamps) and sensible defaults.
85-119: LGTM!The EpisodeAvailability model effectively implements a semi-cache pattern with a composite primary key and the
is_freshproperty correctly validates cache staleness based on TTL configuration.
122-142: LGTM!The ClientTask model appropriately maps client-side torrent hashes to internal jobs with proper indexing for common query patterns.
145-158: LGTM!The comment block clearly documents the transition from manual table creation to Alembic-managed migrations and provides helpful workflow guidance.
219-232: LGTM!The cleanup logic correctly marks interrupted jobs as failed on application restart. The
type: ignorecomment on line 221 is necessary due to SQLModel's incomplete type annotations for the.in_()method.
161-467: LGTM!The CRUD helper functions are well-implemented with:
- Comprehensive docstrings documenting parameters and return values
- Consistent error handling and logging patterns
- Proper session management and commit/refresh cycles
- Appropriate upsert logic for availability and client task operations
| with op.batch_alter_table('clienttask', schema=None) as batch_op: | ||
| batch_op.create_index(batch_op.f('ix_clienttask_added_on'), ['added_on'], unique=False) | ||
| batch_op.create_index(batch_op.f('ix_clienttask_hash'), ['hash'], unique=False) | ||
| batch_op.create_index(batch_op.f('ix_clienttask_job_id'), ['job_id'], unique=False) | ||
| batch_op.create_index(batch_op.f('ix_clienttask_site'), ['site'], unique=False) | ||
| batch_op.create_index(batch_op.f('ix_clienttask_state'), ['state'], unique=False) |
There was a problem hiding this comment.
Remove redundant index on primary key column.
Line 43 creates an index on the hash column, which is already the primary key. SQLite automatically creates an index for primary key columns, making this explicit index redundant and wasteful of storage space.
Apply this diff to remove the redundant index:
with op.batch_alter_table('clienttask', schema=None) as batch_op:
batch_op.create_index(batch_op.f('ix_clienttask_added_on'), ['added_on'], unique=False)
- batch_op.create_index(batch_op.f('ix_clienttask_hash'), ['hash'], unique=False)
batch_op.create_index(batch_op.f('ix_clienttask_job_id'), ['job_id'], unique=False)
batch_op.create_index(batch_op.f('ix_clienttask_site'), ['site'], unique=False)
batch_op.create_index(batch_op.f('ix_clienttask_state'), ['state'], unique=False)Also update the downgrade function to remove the corresponding drop_index call:
with op.batch_alter_table('clienttask', schema=None) as batch_op:
batch_op.drop_index(batch_op.f('ix_clienttask_state'))
batch_op.drop_index(batch_op.f('ix_clienttask_site'))
batch_op.drop_index(batch_op.f('ix_clienttask_job_id'))
- batch_op.drop_index(batch_op.f('ix_clienttask_hash'))
batch_op.drop_index(batch_op.f('ix_clienttask_added_on'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with op.batch_alter_table('clienttask', schema=None) as batch_op: | |
| batch_op.create_index(batch_op.f('ix_clienttask_added_on'), ['added_on'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_clienttask_hash'), ['hash'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_clienttask_job_id'), ['job_id'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_clienttask_site'), ['site'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_clienttask_state'), ['state'], unique=False) | |
| with op.batch_alter_table('clienttask', schema=None) as batch_op: | |
| batch_op.create_index(batch_op.f('ix_clienttask_added_on'), ['added_on'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_clienttask_job_id'), ['job_id'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_clienttask_site'), ['site'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_clienttask_state'), ['state'], unique=False) |
🤖 Prompt for AI Agents
In alembic/versions/397fa0304f9f_initial_migration_with_existing_schema.py
around lines 41 to 46, remove the redundant index creation for the primary key
column 'hash' (the batch_op.create_index call on 'hash') and in the
corresponding downgrade function remove the matching batch_op.drop_index call so
we don't create/drop an unnecessary index on the PK column; keep the other index
create/drop calls intact.
| with op.batch_alter_table('job', schema=None) as batch_op: | ||
| batch_op.create_index(batch_op.f('ix_job_created_at'), ['created_at'], unique=False) | ||
| batch_op.create_index(batch_op.f('ix_job_id'), ['id'], unique=False) | ||
| batch_op.create_index(batch_op.f('ix_job_source_site'), ['source_site'], unique=False) | ||
| batch_op.create_index(batch_op.f('ix_job_status'), ['status'], unique=False) | ||
| batch_op.create_index(batch_op.f('ix_job_updated_at'), ['updated_at'], unique=False) | ||
|
|
There was a problem hiding this comment.
Remove redundant index on primary key column.
Line 82 creates an index on the id column, which is already the primary key. This is redundant as SQLite automatically indexes primary key columns.
Apply this diff to remove the redundant index:
with op.batch_alter_table('job', schema=None) as batch_op:
batch_op.create_index(batch_op.f('ix_job_created_at'), ['created_at'], unique=False)
- batch_op.create_index(batch_op.f('ix_job_id'), ['id'], unique=False)
batch_op.create_index(batch_op.f('ix_job_source_site'), ['source_site'], unique=False)
batch_op.create_index(batch_op.f('ix_job_status'), ['status'], unique=False)
batch_op.create_index(batch_op.f('ix_job_updated_at'), ['updated_at'], unique=False)Also update the downgrade function:
with op.batch_alter_table('job', schema=None) as batch_op:
batch_op.drop_index(batch_op.f('ix_job_updated_at'))
batch_op.drop_index(batch_op.f('ix_job_status'))
batch_op.drop_index(batch_op.f('ix_job_source_site'))
- batch_op.drop_index(batch_op.f('ix_job_id'))
batch_op.drop_index(batch_op.f('ix_job_created_at'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with op.batch_alter_table('job', schema=None) as batch_op: | |
| batch_op.create_index(batch_op.f('ix_job_created_at'), ['created_at'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_job_id'), ['id'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_job_source_site'), ['source_site'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_job_status'), ['status'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_job_updated_at'), ['updated_at'], unique=False) | |
| with op.batch_alter_table('job', schema=None) as batch_op: | |
| batch_op.create_index(batch_op.f('ix_job_created_at'), ['created_at'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_job_source_site'), ['source_site'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_job_status'), ['status'], unique=False) | |
| batch_op.create_index(batch_op.f('ix_job_updated_at'), ['updated_at'], unique=False) |
🤖 Prompt for AI Agents
In alembic/versions/397fa0304f9f_initial_migration_with_existing_schema.py
around lines 80 to 86, the migration creates an explicit index on the primary
key column `id` (line 82) which is redundant; remove the
batch_op.create_index(...) call that targets ['id'] from the upgrade block and
also remove the corresponding drop_index call for the `id` index from the
downgrade block so the migration does not attempt to create or drop an index on
the primary key.
| from .base import ModelBase | ||
| from .session import engine, get_session, dispose_engine, DATABASE_URL | ||
| from .migrations import run_migrations, get_current_revision, check_migrations_status | ||
| from .models import * # re-export full surface for backwards compatibility |
There was a problem hiding this comment.
🧩 Analysis chain
Wildcard import prevents static analysis.
The from .models import * prevents tools from detecting undefined names (Ruff F403). While this maintains backward compatibility, it makes the public API surface unclear and prevents static analysis.
Consider explicitly listing exports from models:
-from .models import * # re-export full surface for backwards compatibility
+from .models import (
+ Job,
+ EpisodeAvailability,
+ ClientTask,
+ create_job,
+ get_job,
+ # ... other exports
+)Verify which symbols from models.py need to be re-exported:
🏁 Script executed:
#!/bin/bash
# Find all public symbols in models.py
ast-grep --pattern 'class $CLASS($$$):
$$$' app/db/models.py
ast-grep --pattern 'def $FUNC($$$):
$$$' app/db/models.pyLength of output: 4306
Now I need to check the complete contents of the relevant files to understand what's currently being exported:
Now I need to look at the actual files to complete the verification:
🏁 Script executed:
#!/bin/bash
# Get complete content of models.py and __init__.py to verify what's actually defined/imported
echo "=== app/db/models.py content (first 200 lines) ==="
head -200 app/db/models.py
echo ""
echo "=== app/db/__init__.py content ==="
cat app/db/__init__.py
echo ""
echo "=== Search for functions (def statements) in models.py ==="
grep -n "^def " app/db/models.py || echo "No module-level functions found"
echo ""
echo "=== Search for imports from app.db in codebase ==="
rg -n "from app\.db import|from \.\.db import|from \.db import" --type py | head -30Length of output: 10226
Replace wildcard import with explicit exports—incomplete list in suggested fix.
The concern is valid: the wildcard import prevents static analysis tools (Ruff F403) from tracking the public API. The current __init__.py has an incomplete dynamic __all__ that doesn't explicitly list what's being re-exported from models.py.
The suggested fix is correct but needs to include all CRUD functions actually used throughout the codebase. Complete list of exports from models.py:
-from .models import * # re-export full surface for backwards compatibility
+from .models import (
+ Job,
+ EpisodeAvailability,
+ ClientTask,
+ JobStatus,
+ create_job,
+ get_job,
+ update_job,
+ cleanup_dangling_jobs,
+ upsert_availability,
+ get_availability,
+ list_available_languages_cached,
+ upsert_client_task,
+ get_client_task,
+ delete_client_task,
+)Then simplify __all__ to explicitly list all re-exported names instead of using dynamic dir() filtering.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from .models import * # re-export full surface for backwards compatibility | |
| from .models import ( | |
| Job, | |
| EpisodeAvailability, | |
| ClientTask, | |
| JobStatus, | |
| create_job, | |
| get_job, | |
| update_job, | |
| cleanup_dangling_jobs, | |
| upsert_availability, | |
| get_availability, | |
| list_available_languages_cached, | |
| upsert_client_task, | |
| get_client_task, | |
| delete_client_task, | |
| ) |
🧰 Tools
🪛 Ruff (0.14.1)
18-18: from .models import * used; unable to detect undefined names
(F403)
| def get_current_revision() -> str: | ||
| """Get the current database schema revision. | ||
| Returns: | ||
| str: The current revision hash, or "base" if no migrations have been applied | ||
| """ | ||
| try: | ||
| from alembic.runtime.migration import MigrationContext | ||
| from app.db.session import engine | ||
|
|
||
| with engine.connect() as connection: | ||
| context = MigrationContext.configure(connection) | ||
| current = context.get_current_revision() | ||
| return current if current else "base" | ||
| except Exception as e: | ||
| logger.warning(f"Failed to get current revision: {e}") | ||
| return "unknown" |
There was a problem hiding this comment.
🧩 Analysis chain
Silently swallowing exceptions may hide configuration or connectivity issues.
The function catches all exceptions and returns "unknown" without providing visibility into what went wrong. While this prevents crashes in status-checking code, it makes debugging connection or configuration problems significantly harder.
Consider these improvements:
- Log the full exception at error level (not just warning)
- Add more context to help diagnose the root cause
- Consider whether certain exception types should be re-raised
Apply this diff:
except Exception as e:
- logger.warning(f"Failed to get current revision: {e}")
+ logger.error(f"Failed to get current revision: {e}", exc_info=True)
return "unknown"Note on imports: The function-local imports of MigrationContext and engine may be intentional to avoid circular import issues with app.db.session. If not needed for that reason, consider moving them to module level.
🏁 Script executed:
#!/bin/bash
# Locate and display the function in question
fd -n "migrations.py" -path "*/app/db/*" --exec cat {}Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for usages of get_current_revision to understand context
rg "get_current_revision" -n -B2 -A2 --type=pyLength of output: 3841
🏁 Script executed:
#!/bin/bash
# Check app.db.session for potential circular imports
fd -n "session.py" -path "*/app/db/*" --exec cat {}Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for other exception handling patterns in migrations.py
fd -n "migrations.py" -path "*/app/db/*" --exec rg "except.*:" {} -n -A3Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check imports at top of migrations.py
fd -n "migrations.py" -path "*/app/db/*" --exec head -30 {}Length of output: 230
🏁 Script executed:
#!/bin/bash
# Get full migrations.py file (fixed fd syntax)
fd "migrations.py" --path "*/app/db/*" --exec cat {}Length of output: 215
🏁 Script executed:
#!/bin/bash
# Get full session.py file
fd "session.py" --path "*/app/db/*" --exec cat {}Length of output: 215
🏁 Script executed:
#!/bin/bash
# Check what check_migrations_status does with get_current_revision
rg "check_migrations_status" -n -B2 -A15 --type=pyLength of output: 4729
🏁 Script executed:
#!/bin/bash
# Look for any imports of engine or session in migrations.py
rg "from app.db.session|from .session|import.*engine" -n -B3 -A3 --type=pyLength of output: 7143
🏁 Script executed:
#!/bin/bash
# Check full session.py for any imports from migrations or app.db
rg "from app.db|from .migrations|from .models|from \.db" app/db/session.py -n -A5 -B5Length of output: 403
🏁 Script executed:
#!/bin/bash
# Show the complete get_current_revision function
rg "def get_current_revision" -A20 --type=py app/db/migrations.pyLength of output: 730
Returning "unknown" violates the API contract expected by tests—this is a critical issue.
The tests explicitly assert that "unknown" should never be returned:
test_migration_statusline 43:assert status["current_revision"] != "unknown"test_migration_statusline 49:assert revision != "unknown"
Silently catching all exceptions and returning "unknown" breaks these expectations and masks real database connectivity or configuration problems. This function is part of the public API and has contractual guarantees that callers depend on.
Additionally, the function-local imports of MigrationContext and engine (lines 73-74) are unnecessary—session.py has no imports from migrations or app.db, so there is no circular import risk. Move these to module level.
Apply this diff:
+from alembic.runtime.migration import MigrationContext
+from app.db.session import engine
+
def get_current_revision() -> str:
"""Get the current database schema revision.
Returns:
str: The current revision hash, or "base" if no migrations have been applied
"""
try:
- from alembic.runtime.migration import MigrationContext
- from app.db.session import engine
-
with engine.connect() as connection:
context = MigrationContext.configure(connection)
current = context.get_current_revision()
return current if current else "base"
except Exception as e:
- logger.warning(f"Failed to get current revision: {e}")
+ logger.error(f"Failed to get current revision: {e}", exc_info=True)
return "unknown"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.1)
80-80: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In app/db/migrations.py around lines 66-82, remove the broad try/except that
swallows all errors and returns "unknown" (which violates tests); instead move
the imports for MigrationContext and engine to module level, call
MigrationContext.configure(connection) and get_current_revision(), return "base"
when current is falsy, and if an exception occurs log the error then re-raise it
(do not return "unknown") so callers/tests see the real failure.
Overview
This PR integrates Alembic for professional database schema management with the existing SQLModel implementation in AniBridge. The changes enable automatic, version-controlled database migrations while maintaining 100% backward compatibility with existing functionality.
Problem Statement
Previously, the application managed database schema through manual
create_db_and_tables()calls and custom migration logic (e.g.,_migrate_episode_availability_table()). This approach had several limitations:Solution
Implemented Alembic as the database migration system with the following key features:
1. Automatic Migration Execution
Migrations now run automatically on application startup, ensuring the database schema is always up-to-date:
2. SQLModel Integration
Alembic's autogenerate feature uses SQLModel metadata to detect model changes:
3. Clean Architecture
Reorganized the database package for better separation of concerns:
app/db/base.py- Base model class with isolated metadata registryapp/db/session.py- Database engine and session managementapp/db/models.py- Table models and CRUD functions (refactored)app/db/migrations.py- Migration utilities and auto-execution4. Developer Workflow
Adding a new model is now straightforward:
Changes
New Files (11)
Database modules:
app/db/base.py- ModelBase class for all table modelsapp/db/session.py- Engine and session configurationapp/db/migrations.py- Migration utilitiesAlembic configuration:
alembic.ini- Alembic configurationalembic/env.py- Environment setup with SQLModel integrationalembic/versions/397fa0304f9f_*.py- Initial migration capturing current schemaDocumentation:
docs/DATABASE.md- Complete database management guide (10KB)alembic/MIGRATIONS.md- Migration workflow details (5KB)docs/EXAMPLE_NEW_MODEL.md- Step-by-step example (9KB)docs/DATABASE_QUICKSTART.md- Quick reference (3KB)Testing:
tests/test_migrations.py- 12 new tests for migration functionalityModified Files (5)
app/db/models.py- Refactored to use new base class, removed manual migration codeapp/db/__init__.py- Updated exports for new structureapp/core/lifespan.py- Integrated automatic migration executiontests/conftest.py- Updated to use migrations instead of manual table creationrequirements.runtime.txt- Added alembic dependencyTesting
46 tests passing (34 original + 12 new):
New migration tests cover:
All existing tests continue to pass without modification.
Documentation
Created comprehensive documentation (28KB total):
docs/DATABASE.md) - Architecture, models, workflow, deploymentalembic/MIGRATIONS.md) - Common operations, best practicesdocs/EXAMPLE_NEW_MODEL.md) - Adding UserPreferences modeldocs/DATABASE_QUICKSTART.md) - Common tasks, troubleshootingBackward Compatibility
✅ 100% compatible with existing functionality:
Existing databases work seamlessly - the initial migration captures the current schema.
Production Deployment
Zero-change deployment - migrations run automatically on startup:
For existing databases, the initial migration creates the alembic_version table and marks the current schema as migrated.
Benefits
Example: Current Schema
The initial migration includes all three existing models:
All indexes and constraints are preserved exactly as before.
Migration Verification
Tested migration workflow:
Summary
This PR provides a professional, production-ready database migration system for AniBridge. It follows modern best practices, is fully documented, comprehensively tested, and maintains complete backward compatibility while enabling easy future extension.
Ready to merge! ✅
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
https://api.github.com/repos/phoenixthrush/AniWorld-Downloader/releases/latestpython -m pytest tests/ -v --tb=short(http block)python -m pytest tests/test_models.py -v --tb=short(http block)python -m pytest tests/test_migrations.py -v --tb=short(http block)https://api.github.com/repos/zzackllack/AniBridge/releases/latestpython -m app.main(http block)https://api.github.com/repos/zzackllack/AniBridge/tagspython -m app.main(http block)s.topython -m pytest tests/ -v --tb=short(dns block)python -m pytest tests/ -v(dns block)python -m pytest tests/ -v --tb=line(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests