Fix migration bootstrap failures on fresh Postgress#995
Open
DevPatils wants to merge 1 commit into
Open
Conversation
DevPatils
requested review from
Pa04rth,
northdpole,
paoga87 and
robvanderveer
as code owners
July 24, 2026 16:27
Contributor
Summary by CodeRabbit
WalkthroughChangesMigration bootstrap fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
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
Comment on lines
+20
to
+34
| 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)) |
Contributor
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.
Comment on lines
+37
to
+42
| 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") |
Contributor
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"]) |
Contributor
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.
DevPatils
force-pushed
the
994-fix-migration-bootstrap-bugs
branch
from
July 24, 2026 16:54
dfa5de6 to
a55e380
Compare
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
Contributor
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
Comment on lines
+24
to
+34
| 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)) |
Contributor
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.
Comment on lines
+37
to
+42
| 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") |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Do not unconditionally drop columns this migration may not own.
Because upgrade() skips columns that already existed out-of-band, downgrade() cannot distinguish migration-created columns from pre-existing production columns. Rolling back can therefore delete existing metadata and break the ORM contract. Persist column ownership during upgrade and drop only columns created by this migration; otherwise make the downgrade intentionally non-destructive.
🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #994 —
flask db upgradefails when run against a genuinely fresh,empty Postgres database. Found and fixed three independent bugs in the
migration chain, each confirmed by reproducing the failure first, then
verifying the fix resolves it on the same fresh database.
The three bugs, and the fix for each
1. Duplicate constraint name across two tables (edited existing file)
migrations/versions/7bf4eac76958_add_rule_id_column.pycreated a uniqueconstraint named
uq_pairon bothcre_linksandcre_node_links.Postgres requires constraint/index names to be unique per schema, not per
table, so the second
ADD CONSTRAINTalways failed:psycopg2.errors.DuplicateTable: relation "uq_pair" already exists
Fix: renamed the second one to
uq_node_pair(bothupgrade()anddowngrade()updated to match).2. Two migration heads were never merged (new file)
9b1c2d3e4f50andab12cd34ef56both branch off the same parent revisionindependently — no merge revision was ever created.
flask db upgrade(without explicitly saying
heads) fails with "Multiple head revisions arepresent," and any future migration would need a merge point to build on
anyway.
Fix: added
a0c5734926c5_merge_divergent_embedding_metadata_heads.py,a standard Alembic merge revision reconciling both heads into one.
3. Missing column migration (new file)
application/database/db.pydefinesdocument_metadataas a real columnon both
NodeandCREmodels, but no migration anywhere ever created it— a fresh migration run crashes the first time any code queries it
(hit this during
--upstream_sync):psycopg2.errors.UndefinedColumn: column cre.document_metadata does not exist
Fix: added
0115af8843b4_add_missing_document_metadata_column_to_.py,which adds the column to both tables — defensively checking first whether
it already exists, since production likely has it applied out-of-band
already (see below).
Why this needed 2 new files instead of just editing the old ones
Migrations are an ordered, append-only chain — each one records "this
change happened, right after that one." You can safely correct an old
migration's content only when it never successfully ran anywhere (which
is true for bug #1 — it always crashed at that exact step, so nothing has
ever depended on its old, broken behavior).
Bugs #2 and #3 are different: they're not typos in an existing step,
they're entirely absent steps — a missing "merge these two paths" step,
and a missing "create this column" step. There's no existing file to
correct, because the instruction never existed. The only way to add a
step that never existed is a new migration, placed after everything that
currently exists. Retrofitting that logic into an old file would
misrepresent when the schema actually changed, and would silently break
for anyone who already has an older schema state.
Why this doesn't affect opencre.org today
All three bugs only block a from-scratch migration run. Production has
presumably accumulated manual/out-of-band schema patches over time (e.g.
the
document_metadatacolumn added directly rather than via migration)without those changes ever being captured back into a migration file —
a common way this kind of drift survives silently until someone attempts
a clean bootstrap. The defensive check in fix #3 specifically accounts for
this: it won't fail or duplicate work if the column already exists.
Test plan
main: confirmeduq_paircollision at migration7bf4eac76958flask db upgrade headscomplete, but confirmed via
flask db headsthat 2 heads stillremain, and via
\d cre/\d nodein psql thatdocument_metadatais still missing — proving all three fixes areneeded together
flask db upgrade headscompletescleanly end-to-end,
flask db headsshows exactly 1 head,document_metadatapresent on both tables--upstream_syncagainst the fully migrated database —completed successfully, loading real data (500+ CREs, 25+ standards)
with no further errors
Verification
Before Fix:
Migration.Bug.mp4
After Fix :
after.fix.mp4