Fix/sql migration - #315
Conversation
Base.metadata.create_all() at app startup may already have created columns/tables/indexes from the SQLAlchemy models on existing deployments. Guard each ADD/CREATE op so re-running alembic upgrade on such a database no-ops instead of raising DuplicateColumn / DuplicateTable.
Move the duplicated table_exists / column_exists / index_exists / fk_exists checks (plus a new column_type_is) out of individual migration files and into a single schema_helpers module alongside env.py. env.py prepends the alembic directory to sys.path so versions can import it regardless of cwd. All call sites updated to the canonical (table, index) arg order.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces idempotent migration support by creating a centralized schema-inspection utility module, updating all Alembic migration files to import and use these helpers instead of local implementations, and making schema alterations conditional on object existence checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openrag/scripts/migrations/alembic/versions/f1a2b3c4d5e6_add_workspace_files_file_id_fk.py (1)
29-85:⚠️ Potential issue | 🟠 MajorGuard all schema-mutating operations in this Alembic migration for idempotency.
The
upgrade()function checks only whetherworkspace_files.file_idis already an integer (line 29), then proceeds with unguarded operations at lines 36, 55–64. Similarly, thedowngrade()function (lines 76–85) performs schema mutations without existence checks. A migration rerun after a partial failure will fail on duplicate columns, missing indices, or missing foreign keys.Add guards using inspector-based checks (
column_exists(),index_exists(),fk_exists()) and consider a unique-constraint helper foruix_workspace_file(created at lines 63 and 85 without guards). Guard both theupgrade()anddowngrade()paths entirely, as required by the coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/versions/f1a2b3c4d5e6_add_workspace_files_file_id_fk.py` around lines 29 - 85, The migration's upgrade() and downgrade() perform schema changes unguarded; wrap each schema-mutating step (DELETEs, add_column("file_fk"), UPDATEs, drop_index("ix_workspace_files_file_id"), drop_column("file_id"), alter_column, create_index, create_unique_constraint("uix_workspace_file"), create_foreign_key("fk_workspace_files_file_id")) with inspector-based existence checks (use the existing column_type_is() / add new helpers column_exists(), index_exists(), fk_exists()) so each operation only runs if the target object is absent/present as appropriate; do the same in downgrade() for add_column("file_str"), UPDATE from files, drop_column("file_id"), alter_column, create_index and create_unique_constraint("uix_workspace_file"), and ensure the unique constraint creation is guarded by a helper check to avoid duplicate-constraint errors on reruns or partial failures.
🧹 Nitpick comments (1)
openrag/scripts/migrations/alembic/env.py (1)
5-7: Remove the redundantsys.path.insert(0, ...).The
alembic.iniconfiguration already includesprepend_sys_path = .(line 19), which adds the alembic directory to the Python path. The explicitsys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))inenv.py:7is redundant and can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/scripts/migrations/alembic/env.py` around lines 5 - 7, Remove the redundant manual PYTHONPATH modification in env.py by deleting the sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) call (the explicit insertion near the top of the file) so the alembic directory is not added twice; rely on the existing alembic.ini setting (prepend_sys_path) to provide the path and verify imports that reference local modules (e.g., schema_helpers) still resolve after removing that line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/scripts/migrations/alembic/schema_helpers.py`:
- Around line 16-33: The helper functions column_exists, index_exists,
fk_exists, and column_type_is must each check table_exists(table) first and
return False immediately if the table doesn't exist; move the
inspect(op.get_bind()) calls and subsequent reflection logic inside that guard
so NoSuchTableError is avoided and the helpers remain idempotent. Specifically,
add an initial if not table_exists(table): return False to column_exists,
index_exists, fk_exists, and column_type_is, then perform the existing
Inspector.get_* calls only when the table exists.
---
Outside diff comments:
In
`@openrag/scripts/migrations/alembic/versions/f1a2b3c4d5e6_add_workspace_files_file_id_fk.py`:
- Around line 29-85: The migration's upgrade() and downgrade() perform schema
changes unguarded; wrap each schema-mutating step (DELETEs,
add_column("file_fk"), UPDATEs, drop_index("ix_workspace_files_file_id"),
drop_column("file_id"), alter_column, create_index,
create_unique_constraint("uix_workspace_file"),
create_foreign_key("fk_workspace_files_file_id")) with inspector-based existence
checks (use the existing column_type_is() / add new helpers column_exists(),
index_exists(), fk_exists()) so each operation only runs if the target object is
absent/present as appropriate; do the same in downgrade() for
add_column("file_str"), UPDATE from files, drop_column("file_id"), alter_column,
create_index and create_unique_constraint("uix_workspace_file"), and ensure the
unique constraint creation is guarded by a helper check to avoid
duplicate-constraint errors on reruns or partial failures.
---
Nitpick comments:
In `@openrag/scripts/migrations/alembic/env.py`:
- Around line 5-7: Remove the redundant manual PYTHONPATH modification in env.py
by deleting the sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
call (the explicit insertion near the top of the file) so the alembic directory
is not added twice; rely on the existing alembic.ini setting (prepend_sys_path)
to provide the path and verify imports that reference local modules (e.g.,
schema_helpers) still resolve after removing that line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0cae611-f4bb-4fd5-96dd-8c0130980a66
📒 Files selected for processing (10)
CLAUDE.mdopenrag/scripts/migrations/alembic/env.pyopenrag/scripts/migrations/alembic/schema_helpers.pyopenrag/scripts/migrations/alembic/versions/4add4d260575_initial_migration.pyopenrag/scripts/migrations/alembic/versions/a1b2c3d4e5f6_add_document_relationships.pyopenrag/scripts/migrations/alembic/versions/c224d4befe71_add_file_count_and_file_quota.pyopenrag/scripts/migrations/alembic/versions/cd642e4502d8_create_users_memberships_tables.pyopenrag/scripts/migrations/alembic/versions/e7f8a9b0c1d2_add_workspaces.pyopenrag/scripts/migrations/alembic/versions/f1a2b3c4d5e6_add_workspace_files_file_id_fk.pyopenrag/scripts/migrations/alembic/versions/f5b6c918f741_add_oidc_auth.py
…rd reference Postgres rejects 'JOIN workspaces w ON w.workspace_id = wf.workspace_id' inside the FROM clause of an UPDATE because the target table (wf) cannot be referenced from a from_item's JOIN ON. Move workspaces into the FROM list and put all join conditions in WHERE.
72dc572 to
c4b2f8f
Compare
1 participant
Summary
Fix the alembic migration chain for existing deployments where
Base.metadata.create_all()(called at app startup inPartitionFileManager.__init__) had already produced the current-model schema before alembic ever ran. Re-runningalembic upgrade headagainst such a database used to raiseDuplicateColumn/DuplicateTable; it now no-ops cleanly.Also fixes a Postgres-invalid
UPDATE ... FROM ... JOINthat referenced the update target inside afrom_item, and consolidates duplicated inspector helpers into a shared module.Changes
add_column,create_table,create_index,create_foreign_key(and the correspondingdowngradeops) with an existence check. Migrations touched:4add4d260575,cd642e4502d8,c224d4befe71,e7f8a9b0c1d2,f1a2b3c4d5e6,f5b6c918f741,a1b2c3d4e5f6. The String→Integer conversion inf1a2b3c4d5e6short-circuits whenworkspace_files.file_idis already INTEGER.schema_helpersmodule — newopenrag/scripts/migrations/alembic/schema_helpers.pyexposestable_exists,column_exists,index_exists,fk_exists,column_type_is.env.pyprepends the alembic dir tosys.pathso version files canfrom schema_helpers import ...regardless of cwd. All call sites use the canonical(table, index)arg order.f1a2b3c4d5e6— theUPDATE workspace_files wf SET file_fk = f.id FROM files f JOIN workspaces w ON w.workspace_id = wf.workspace_id WHERE ...was rejected by Postgres ("invalid reference to FROM-clause entry for table 'wf'"). Rewritten to put bothfilesandworkspacesin theFROMlist with all join conditions inWHERE.### Alembic Migration Idempotencysection inCLAUDE.mdexplaining the contract and required guards.Test plan
Reproduce the original failure mode and verify the fix on an upgraded deployment:
v1.1.7and bring up the stack soBase.metadata.create_all()bootstraps the schema (and stamps no alembic version, or stamps the baseline that shipped with that tag).fix/sql_migration).docker compose up -d rdb docker compose \ run --no-deps --build --rm \ --entrypoint "uv run alembic -c /app/openrag/scripts/migrations/alembic/alembic.ini upgrade head" \ openrag; docker compose downSummary by CodeRabbit
Documentation
Refactor