fix: add ondelete flag to fk in flow version table#12059
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughChanges modify database schema to make user_id optional in FlowVersion by allowing NULL values, update the Pydantic read schema accordingly, and refactor TraceBase.flow_id to use explicit SQLModel sa_column configuration with cascade delete behavior instead of the foreign_key argument. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (3 passed)
✨ 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 |
|
✅ Migration Validation Passed All migrations follow the Expand-Contract pattern correctly. |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (42.83%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #12059 +/- ##
==========================================
+ Coverage 37.58% 37.59% +0.01%
==========================================
Files 1623 1623
Lines 79603 79604 +1
Branches 11971 11971
==========================================
+ Hits 29917 29928 +11
+ Misses 48027 48017 -10
Partials 1659 1659
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/base/langflow/alembic/versions/7d327cfafab6_add_flow_history_table.py (1)
23-45:⚠️ Potential issue | 🟠 MajorDo not rely on mutating an existing revision to fix upgraded environments.
Changing nullable behavior inside revision
7d327cfafab6only affects DBs that have not applied this revision yet. Already-migrated environments will not re-run it, so the mismatch can persist. Add a new forward migration that altersflow_version.user_idnullability/FK explicitly.Suggested follow-up migration (new revision)
+"""make flow_version.user_id nullable and enforce SET NULL FK""" + +from alembic import op +import sqlalchemy as sa + +revision = "NEW_REVISION_ID" +down_revision = "7d327cfafab6" +branch_labels = None +depends_on = None + +def upgrade() -> None: + with op.batch_alter_table("flow_version") as batch_op: + batch_op.alter_column("user_id", existing_type=sa.Uuid(), nullable=True) + # Recreate FK with ondelete SET NULL if needed in your DB state. + # Use the actual FK name from your database naming convention. + +def downgrade() -> None: + with op.batch_alter_table("flow_version") as batch_op: + batch_op.alter_column("user_id", existing_type=sa.Uuid(), nullable=False) + # Recreate prior FK behavior if required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/base/langflow/alembic/versions/7d327cfafab6_add_flow_history_table.py` around lines 23 - 45, The current revision only creates flow_version.user_id as nullable=True in upgrade() which won't change DBs that already ran this migration; add a new Alembic revision that explicitly alters the existing column and FK on upgraded environments: in the new migration's upgrade() use op.alter_column("flow_version", "user_id", existing_type=sa.Uuid(), nullable=True) and update the foreign key constraint to ensure ondelete="SET NULL" (drop the existing FK constraint and create a new one referencing user.id with ondelete="SET NULL"); include a corresponding downgrade() that reverts the nullability and restores the prior FK behavior; target symbols: flow_version, user_id, upgrade(), and the FK constraint on user_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/backend/base/langflow/alembic/versions/7d327cfafab6_add_flow_history_table.py`:
- Around line 23-45: The current revision only creates flow_version.user_id as
nullable=True in upgrade() which won't change DBs that already ran this
migration; add a new Alembic revision that explicitly alters the existing column
and FK on upgraded environments: in the new migration's upgrade() use
op.alter_column("flow_version", "user_id", existing_type=sa.Uuid(),
nullable=True) and update the foreign key constraint to ensure ondelete="SET
NULL" (drop the existing FK constraint and create a new one referencing user.id
with ondelete="SET NULL"); include a corresponding downgrade() that reverts the
nullability and restores the prior FK behavior; target symbols: flow_version,
user_id, upgrade(), and the FK constraint on user_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19262dad-8676-4605-8ede-a6665f3e740c
📒 Files selected for processing (3)
src/backend/base/langflow/alembic/versions/7d327cfafab6_add_flow_history_table.pysrc/backend/base/langflow/services/database/models/flow_version/model.pysrc/backend/base/langflow/services/database/models/traces/model.py
* Add ondelete flag to fk in flow version table * set uuid nullable * Fix alembic chain for cascade migration in release branch * Add merge migration * correct down path * use randomly generated reivison id * Add expand * update doc * update merge migration points
Fixes model / database mismatch. Add ondelete flag to fk in flow version table
Summary by CodeRabbit