Fix migration bootstrap failures on fresh Postgres - #995
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThe migration conditionally adds nullable JSON ChangesMigration bootstrap fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@migrations/versions/0115af8843b4_add_missing_document_metadata_column_to_.py`:
- Around line 37-42: Update the migration’s upgrade/downgrade ownership handling
so columns detected as pre-existing and skipped by upgrade are not dropped
during downgrade. Persist which document_metadata columns this revision created,
then make downgrade drop only those owned columns while preserving out-of-band
columns and their data.
- Around line 20-34: Update upgrade() to preserve existing metadata_json values
whenever document_metadata is absent: before or while adding the new column on
each affected table, rename or copy the populated metadata_json data into
document_metadata so ORM and backend reads retain it. Keep the existing
defensive checks for already-present document_metadata, and add a migration test
covering a schema with populated metadata_json and no document_metadata.
In `@migrations/versions/7bf4eac76958_add_rule_id_column.py`:
- Line 28: Align the constraint name used by the cre_node_links ORM model in
application/database/db.py with the migration’s uq_node_pair name, or
consistently rename both sides to another shared name. Ensure the persisted
schema and ORM metadata use exactly the same constraint identifier.
🪄 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.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: be6a969e-dde3-42ac-b2ee-995942a3b7dc
📒 Files selected for processing (3)
migrations/versions/0115af8843b4_add_missing_document_metadata_column_to_.pymigrations/versions/7bf4eac76958_add_rule_id_column.pymigrations/versions/a0c5734926c5_merge_divergent_embedding_metadata_heads.py
| def upgrade(): | ||
| # Defensive: some environments (e.g. production) already have this column | ||
| # applied out-of-band without a corresponding migration ever being | ||
| # committed, so this must not assume a clean "column doesn't exist" state. | ||
| inspector = inspect(op.get_bind()) | ||
| node_columns = {c["name"] for c in inspector.get_columns("node")} | ||
| cre_columns = {c["name"] for c in inspector.get_columns("cre")} | ||
|
|
||
| if "document_metadata" not in node_columns: | ||
| with op.batch_alter_table("node", schema=None) as batch_op: | ||
| batch_op.add_column(sa.Column("document_metadata", sa.JSON(), nullable=True)) | ||
|
|
||
| if "document_metadata" not in cre_columns: | ||
| with op.batch_alter_table("cre", schema=None) as batch_op: | ||
| batch_op.add_column(sa.Column("document_metadata", sa.JSON(), nullable=True)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migrations relevant files =="
fd -a '0115af8843b4_add_missing_document_metadata_column_to_\.py|metadata_json|document_metadata' . | sed 's#^\./##' | head -200
echo
echo "== migration file outline =="
if [ -f "migrations/versions/0115af8843b4_add_missing_document_metadata_column_to_.py" ]; then
wc -l migrations/versions/0115af8843b4_add_missing_document_metadata_column_to_.py
sed -n '1,220p' migrations/versions/0115af8843b4_add_missing_document_metadata_column_to_.py
fi
echo
echo "== benchmark import parity relevant lines =="
if [ -f "scripts/benchmark_import_parity.py" ]; then
sed -n '80,130p' scripts/benchmark_import_parity.py
fi
echo
echo "== search metadata_json/document_metadata usages =="
rg -n "document_metadata|metadata_json|metadata_json" . --glob '!*.pyc' --glob '!.git/**' | head -500Repository: OWASP/OpenCRE
Length of output: 8113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
migration = Path("migrations/versions/0115af8843b4_add_missing_document_metadata_column_to_.py").read_text()
benchmark = Path("scripts/benchmark_import_parity.py").read_text()
backend = Path("application/database/db.py").read_text()
checks = {
"migration_adds_without_rename_or_copy": {
"add_column_node": 'batch_op.add_column(sa.Column("document_metadata", sa.JSON(), nullable=True))' in migration,
"upgrade_prefers_exact_document_metadata_name": '"document_metadata" not in node_columns' in migration and '"document_metadata" not in cre_columns' in migration,
"no_metadata_json_rename": "metadata_json" in migration and "rename_column" in migration,
"no_metadata_json_copy": ("metadata_json" not in migration) or ("copy" not in migration.lower() and "rename" not in migration.lower()),
},
"benchmark_selects_document_metadata_when_present": {
"_document_metadata_select": (
'if "document_metadata" in cols:' in benchmark
and 'return "document_metadata"' in benchmark
and 'if "metadata_json" in cols:' in benchmark
and 'return "metadata_json AS document_metadata"' in benchmark
),
"canonicalized_result_key": '"document_metadata": _json_canonical(r["document_metadata"])' in benchmark,
},
"backend_orm_and_reads_use_document_metadata": {
"orm_column_maps_to_meta_json_name": 'sqla.Column("document_metadata", sqla.JSON, nullable=True)' in backend and 'metadata_json = sqla.Column' in backend,
"read_prefers_document_metadata_ORM_attr": ".metadata_json" in backend,
},
}
for group, claims in checks.items():
print("\n".join(f"{group}/{k}: {json.dumps(v)}" for k, v in claims.items()))
PYRepository: OWASP/OpenCRE
Length of output: 770
Preserve existing metadata_json before adding document_metadata.
This migration only checks for document_metadata, so a schema that has populated metadata_json will get a new empty document_metadata column. Because the ORM, backend reads, and benchmark parity select prefer document_metadata, existing metadata becomes invisible after migration. Rename or copy metadata_json values into document_metadata when document_metadata is absent, and add a migration test for that path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migrations/versions/0115af8843b4_add_missing_document_metadata_column_to_.py`
around lines 20 - 34, Update upgrade() to preserve existing metadata_json values
whenever document_metadata is absent: before or while adding the new column on
each affected table, rename or copy the populated metadata_json data into
document_metadata so ORM and backend reads retain it. Keep the existing
defensive checks for already-present document_metadata, and add a migration test
covering a schema with populated metadata_json and no document_metadata.
| def downgrade(): | ||
| with op.batch_alter_table("cre", schema=None) as batch_op: | ||
| batch_op.drop_column("document_metadata") | ||
|
|
||
| with op.batch_alter_table("node", schema=None) as batch_op: | ||
| batch_op.drop_column("document_metadata") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not unconditionally drop columns that this revision may not own.
Because upgrade() skips out-of-band columns, downgrade() can delete an existing document_metadata column and its data. Persist column ownership or make downgrade non-destructive for pre-existing columns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migrations/versions/0115af8843b4_add_missing_document_metadata_column_to_.py`
around lines 37 - 42, Update the migration’s upgrade/downgrade ownership
handling so columns detected as pre-existing and skipped by upgrade are not
dropped during downgrade. Persist which document_metadata columns this revision
created, then make downgrade drop only those owned columns while preserving
out-of-band columns and their data.
| with op.batch_alter_table("cre_node_links", schema=None) as batch_op: | ||
| batch_op.drop_constraint("uq_cre_node_link_pair", type_="unique") | ||
| batch_op.create_unique_constraint("uq_pair", ["cre", "node"]) | ||
| batch_op.create_unique_constraint("uq_node_pair", ["cre", "node"]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the ORM constraint name with this migration.
The upgrade now creates uq_node_pair, but application/database/db.py still declares the cre_node_links constraint as uq_cre_node_link_pair. Update the model (or choose a different migration name) so the persisted schema and ORM metadata agree; otherwise Alembic autogeneration/schema checks can report drift after upgrade.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migrations/versions/7bf4eac76958_add_rule_id_column.py` at line 28, Align the
constraint name used by the cre_node_links ORM model in
application/database/db.py with the migration’s uq_node_pair name, or
consistently rename both sides to another shared name. Ensure the persisted
schema and ORM metadata use exactly the same constraint identifier.
dfa5de6 to
a55e380
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py`:
- Around line 37-42: Make the migration’s downgrade non-destructive for
pre-existing columns: update upgrade() to persist whether each document_metadata
column was created by this migration, then have downgrade() consult that
ownership state and drop only migration-created columns. If ownership cannot be
persisted reliably, remove the unconditional drop behavior from downgrade()
rather than risking deletion of existing columns.
- Around line 24-34: Update the migration’s Node and CRE column handling to
preserve existing metadata_json values: when only metadata_json exists, rename
it to document_metadata (or copy its values before removing it), and when both
columns exist, transfer metadata_json values into document_metadata without
overwriting valid document_metadata data. Keep the migration safe when
document_metadata already exists and apply the same logic to both tables.
🪄 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.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fed044a-9ea5-4e8c-a825-59580e1f37d6
📒 Files selected for processing (1)
migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py
| inspector = inspect(op.get_bind()) | ||
| node_columns = {c["name"] for c in inspector.get_columns("node")} | ||
| cre_columns = {c["name"] for c in inspector.get_columns("cre")} | ||
|
|
||
| if "document_metadata" not in node_columns: | ||
| with op.batch_alter_table("node", schema=None) as batch_op: | ||
| batch_op.add_column(sa.Column("document_metadata", sa.JSON(), nullable=True)) | ||
|
|
||
| if "document_metadata" not in cre_columns: | ||
| with op.batch_alter_table("cre", schema=None) as batch_op: | ||
| batch_op.add_column(sa.Column("document_metadata", sa.JSON(), nullable=True)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching migration/scripts/db =="
git ls-files | rg '(^migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_|(^scripts/benchmark_import_parity\.py$|^application/database/db\.py$))' || true
echo
echo "== migration file =="
sed -n '1,140p' migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py
echo
echo "== scripts/benchmark_import_parity.py relevant =="
sed -n '1,180p' scripts/benchmark_import_parity.py
echo
echo "== application/database/db.py relevant =="
sed -n '1,180p' application/database/db.py
echo
echo "== search metadata_json/document_metadata references =="
rg -n '"?metadata_json"?\b|"?document_metadata"?\b' scripts application migrations -S || trueRepository: OWASP/OpenCRE
Length of output: 17258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search older migrations for document_metadata/metadata_json creation == =="
rg -n 'document_metadata|metadata_json|add_column.*metadata|create_table.*metadata' migrations scripts -S || true
echo
echo "== inspect alembic version/creates/migrations references =="
sed -n '1,140p' scripts/prod-docker-entrypoint.sh || true
echo
echo "== deterministic migration behavior probe =="
python3 - <<'PY'
def upgrade_columns_after_migration(existing_node, existing_cre):
node_columns = set(existing_node)
cre_columns = set(existing_cre)
if "document_metadata" not in node_columns:
node_columns = node_columns | {"document_metadata"}
if "document_metadata" not in cre_columns:
cre_columns = cre_columns | {"document_metadata"}
return {"node": sorted(node_columns), "cre": sorted(cre_columns)}
cases = [
(["metadata_json"], []),
(["metadata_json"], ["metadata_json"]),
(["document_metadata", "metadata_json"], ["document_metadata", "metadata_json"]),
]
for node, cre in cases:
result = upgrade_columns_after_migration(node, cre)
print({"existing_node": node, "existing_cre": cre}, "=>", result)
PY
echo
echo "== behavioral probe: data accessible from ORM-backed column if only legacy column is present? =="
python3 - <<'PY'
legacy_schema = {"document_metadata": None}
ORM_reads_column = legacy_schema.get("metadata_json")
print({"migration_result_schema": legacy_schema, "orm_attribute_value_from_document_metadata": ORM_reads_column})
PYRepository: OWASP/OpenCRE
Length of output: 3951
Preserve legacy metadata_json values before setting document_metadata.
The ORM for Node and CRE reads metadata_json mapped to the SQL column document_metadata, and benchmark parity also supports schemas that have metadata_json instead. This migration only adds an empty document_metadata column when metadata_json exists, so existing metadata becomes inaccessible. Rename/copy metadata_json to document_metadata for both tables, and handle the case where both columns already exist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py`
around lines 24 - 34, Update the migration’s Node and CRE column handling to
preserve existing metadata_json values: when only metadata_json exists, rename
it to document_metadata (or copy its values before removing it), and when both
columns exist, transfer metadata_json values into document_metadata without
overwriting valid document_metadata data. Keep the migration safe when
document_metadata already exists and apply the same logic to both tables.
|
Thanks for chasing down #994. Please land this before #1006 (that PR should rebase on top of this one). Fixes needed before merge
Once black is green, this is next in the migration queue; #1006 will rebase after it merges. |
Fixing |
DevPatils
changed the title
DevPatils
changed the title
DevPatils
changed the title
|
@northdpole check it once all the issues are resolved. |
|
Black looks fixed — thanks. Please rebase onto latest Reminder: this should land before #1006 (which will then rebase on top of this). |
Bugs 1 and 2 from OWASP#994 were already fixed upstream (uq_pair rename removal in a55e380, heads merged via c7d8e9f0a1b2's pgvector migration). This delivers the remaining fix: no migration anywhere created the document_metadata column that Node and CRE models require, causing a crash the moment any code queries it on a freshly-migrated database. Defensively checks for existing column first, since production may already have it applied out-of-band. Verified end-to-end on a fresh Postgres database (with pgvector): full migration chain completes, single head, document_metadata present on both tables. Addresses OWASP#994
6b643fa to
d6e7cf8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Thanks for the review! All three addressed:
Re-verified end-to-end on a genuinely fresh Postgres database (pgvector-enabled) after the rebase: full migration chain (16 revisions) runs clean from empty, single head, Please have a look @northdpole !!! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py (1)
42-47: 🗄️ Data Integrity & Integration | 🔴 CriticalDo not drop pre-existing columns during downgrade.
When
upgrade()finds an existingdocument_metadatacolumn, it skips creation.downgrade()still drops that column unconditionally from both tables. A rollback can therefore delete existing application data.Persist column ownership during
upgrade()and drop only columns created by this revision. If ownership cannot be persisted reliably, makedowngrade()non-destructive. This repeats the unresolved prior review finding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py` around lines 42 - 47, Make the migration’s upgrade/downgrade behavior ownership-aware: update upgrade() to persist whether this revision created each document_metadata column on cre and node, then have downgrade() drop only columns recorded as created by this revision and retain pre-existing columns. If creation ownership cannot be persisted reliably, make downgrade() non-destructive instead of unconditionally dropping document_metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py`:
- Around line 25-39: Update the migration logic for the node and cre tables to
reconcile metadata_json and document_metadata: rename metadata_json to
document_metadata when the legacy column is the only one present, and when both
exist copy legacy values only where document_metadata is null or empty without
overwriting valid values. Apply the same handling independently in both table
branches.
---
Duplicate comments:
In
`@migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py`:
- Around line 42-47: Make the migration’s upgrade/downgrade behavior
ownership-aware: update upgrade() to persist whether this revision created each
document_metadata column on cre and node, then have downgrade() drop only
columns recorded as created by this revision and retain pre-existing columns. If
creation ownership cannot be persisted reliably, make downgrade()
non-destructive instead of unconditionally dropping document_metadata.
🪄 Autofix
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.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: b87493d2-2d3e-4502-bdda-1450ffeea9ab
📒 Files selected for processing (1)
migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py
upgrade() is defensive (only adds document_metadata where missing), so downgrade() unconditionally dropping the column could destroy real data on any environment where the column pre-existed before this migration ran (e.g. production, applied out-of-band). There's no reliable way to distinguish 'this migration added it' from 'it already existed', so the safe choice is a no-op downgrade. Verified: upgrade -> downgrade -> upgrade cycle on a fresh database confirms the column survives the downgrade step correctly.
|
@northdpole all the coderabbit reviews are fixed, please review it . |
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Review — #995 fresh-Postgres document_metadata migration
CI green after re-running jobs stuck/cancelled in yesterday’s Actions outage. Scope is correctly narrowed to the remaining #994 bug (column missing on fresh migrate).
Looks good
- Defensive
inspect+ add-only-if-missing (safe for prod drift) sa.JSON()nullable oncre/node- Downgrade intentionally no-op — right call given out-of-band prod column risk
down_revision = d4e5f6a7b8c9is a real head onmain(Module B tables)
Non-blocking / follow-up
mainstill has two Alembic heads (d4e5f6a7b8c9andab12cd34ef56). This PR extends the d4e5 line; after merge you’ll still want a merge revision (or rebase) soflask db upgradewithoutheadsstays single-head.- Branch is ~14 commits behind
main— rebase before merge if convenient. - #1006 should rebase on top of this once landed (don’t ship two competing
document_metadatamigrations).
Approving.
|
CI recovered after the Actions outage (reran stuck Lint/Test). Ready from the review side aside from the optional rebase / dual-head follow-up noted in the approval. |
Thanks so much for the thorough review, and glad the defensive One small correction on the follow-up note: I checked |
Keep UniqueConstraint inside create_table for SQLite, drop the custom _migration_tracking helper (schema clash risk), restore the OWASP#995 document_metadata no-op downgrade contract, and limit embedding_vec downgrade to SQLite only.
Keep UniqueConstraint inside create_table for SQLite, drop the custom _migration_tracking helper (schema clash risk), restore the OWASP#995 document_metadata no-op downgrade contract, and limit embedding_vec downgrade to SQLite only.
Keep UniqueConstraint inside create_table for SQLite, drop the custom _migration_tracking helper (schema clash risk), restore the OWASP#995 document_metadata no-op downgrade contract, and limit embedding_vec downgrade to SQLite only.
2 participants
Fixes #994 —
flask db upgradefails when run against a genuinely fresh,empty Postgres database, because no migration anywhere creates the
document_metadatacolumn that theNodeandCREmodels require.Note on scope: #994 originally reported three bugs. Two of them
(the
uq_pairconstraint collision, and the two unmerged migrationheads) were already fixed upstream independently before this PR was
ready — see
a55e380(constraint fix) andc7d8e9f0a1b2(which mergesboth heads while adding pgvector support). This PR delivers the one
remaining bug: the missing
document_metadatacolumn.The bug
application/database/db.pydefinesdocument_metadataas a realcolumn on both
NodeandCRE(mapped viametadata_json), but nomigration anywhere creates it. A fresh migration run crashes the first
time any code queries it:
The fix
Adds
migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py,which creates the column on both tables — defensively checking whether
it already exists first, since production likely has it applied
out-of-band already (a common way this kind of drift survives — someone
patches the live schema directly without the change ever making it back
into a migration file).
Test plan
flask db upgrade headsend-to-end from empty — completes cleanlyflask db headsshows exactly 1 head (b5ac48010165)\d cre/\d nodein psql —document_metadatapresent on both tables
black --checkpasses on the new migration fileVerification
Before Fix:
Migration.Bug.mp4
After Fix :
after.fix.mp4