Fix SQLite migration compatibility and idempotency issues - #1006
Fix SQLite migration compatibility and idempotency issues#1006Bornunique911 wants to merge 20 commits into
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:
Summary by CodeRabbit
WalkthroughThe migrations conditionally add and remove nullable metadata and embedding columns. They also conditionally create artifact ingestion tables, repair unique constraints, and drop tables in dependency order. ChangesMigration persistence changes
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py (1)
11-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
sa.inspect(conn)instead ofInspector.from_engine(conn).The unpinned runtime SQLAlchemy dependency can resolve to SQLAlchemy 2.x, where
Inspector.from_engine()is deprecated. Replace the duplicated calls in the referenced migrations with the supported inspection entry point.Proposed change
-from sqlalchemy.engine.reflection import Inspector ... - inspector = Inspector.from_engine(conn) + inspector = sa.inspect(conn)🤖 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/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py` around lines 11 - 21, Replace the deprecated Inspector.from_engine(conn) usage with sa.inspect(conn) in the column_exists helper in migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py (lines 11-21), migrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.py (lines 11-21), and migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py (lines 11-21). Preserve the existing column inspection behavior and remove any now-unused Inspector imports.
🤖 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/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py`:
- Around line 39-43: The downgrade in
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py at
lines 39-43 must remove document_metadata from both cre and node using the
existing SQLite batch-table migration pattern, replacing the no-op downgrade.
Apply the same reversible downgrade change in
migrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.py at
lines 31-33 to remove embedding_vec from embeddings, and add downgrade coverage
for both revisions.
In `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py`:
- Around line 24-71: Update the migration logic around artifact_ingest_event and
ingest_chunk so existing tables are inspected for
uq_artifact_ingest_event_run_artifact and uq_ingest_chunk_artifact_chunk. When
either constraint is missing, rebuild or otherwise alter the table to add it
before the migration completes; do not silently skip DDL, and fail the migration
if the constraint cannot be repaired.
---
Nitpick comments:
In `@migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py`:
- Around line 11-21: Replace the deprecated Inspector.from_engine(conn) usage
with sa.inspect(conn) in the column_exists helper in
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py (lines
11-21), migrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.py
(lines 11-21), and
migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py (lines
11-21). Preserve the existing column inspection behavior and remove any
now-unused Inspector imports.
🪄 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: 0b0c9beb-ffbf-47a6-a28f-116a34aae300
📒 Files selected for processing (3)
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.pymigrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.pymigrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py
…ints - Add downgrades to 055dbd9f8bfe and 967016ee10fa to drop added columns - Replace deprecated Inspector.from_engine(conn) with sa.inspect(conn) - In 9f1a2b3c4d5e, verify that existing tables have the required unique constraints; add them via batch_alter_table if missing - Ensure migration fails if constraints cannot be added Addresses PR review comments OWASP#1006
Before change on the main website opencre.org/node/standard/CWE :
After change locally on 127.0.0.1:5000/node/standard/CWE :
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py (1)
94-97: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTrack table ownership before dropping in
downgrade().
upgrade()may only add the missing unique constraints to existingartifact_ingest_eventoringest_chunktables, whiledowngrade()always runsop.drop_table()for both. If a pre-existing table was adopted instead of created here, downgrade destroys its rows and schema. Track ownership or downgrade only constraint-alterations made by this revision.🤖 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/9f1a2b3c4d5e_add_artifact_ingest_persistence.py` around lines 94 - 97, Update the migration’s upgrade/downgrade flow to track whether artifact_ingest_event and ingest_chunk were created by this revision versus merely adopted with added constraints. In downgrade(), drop only tables owned and created by this revision; for pre-existing tables, revert only the unique constraints added by this migration and preserve their rows and schema.
🤖 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/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py`:
- Around line 38-43: Align downgrade ownership checks with the conditional
upgrades: in
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.py lines
38-43, update downgrade() to drop document_metadata from cre and node only when
each column exists, preserving pre-existing columns; in
migrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.py lines
30-33, apply the same existence check before dropping embedding_vec from
embeddings. Add downgrade coverage for absent columns and populated pre-existing
columns at both sites.
In `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py`:
- Around line 57-61: Before the artifact_ingest_event batch rewrite and
create_unique_constraint operation, handle the ingest_chunk foreign key
explicitly: temporarily enable foreign-key enforcement, drop the child
constraint, and recreate it with its existing cascade behavior after the
rewrite. Add a regression test covering pre-existing artifact_ingest_event rows
and verifying the ingest_chunk relationship remains valid.
- Around line 57-61: Update the existing-table migration paths using
op.batch_alter_table for artifact_ingest_event and ingest_chunk so unnamed
UNIQUE constraints are preserved during SQLite table recreation. Copy or replace
each supported unnamed uniqueness rule before adding the named constraint,
reject unsupported legacy schemas, or use recreate="always" with explicit
table_args; ensure no existing uniqueness rule is silently dropped.
- Around line 22-26: Update constraint_exists to validate both the constraint
name and its column_names against the migration’s expected target columns,
returning true only for an exact definition match; if the name exists with
different columns, do not treat it as present so the migration can correct it.
Apply the same validation at the checks around the constraints named
uq_artifact_ingest_event_run_artifact and uq_ingest_chunk_artifact_chunk.
---
Outside diff comments:
In `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py`:
- Around line 94-97: Update the migration’s upgrade/downgrade flow to track
whether artifact_ingest_event and ingest_chunk were created by this revision
versus merely adopted with added constraints. In downgrade(), drop only tables
owned and created by this revision; for pre-existing tables, revert only the
unique constraints added by this migration and preserve their rows and schema.
🪄 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: 7d32bd9f-c55b-4fa9-8cb2-af49b08dfa50
📒 Files selected for processing (3)
migrations/versions/055dbd9f8bfe_add_document_metadata_to_cre_and_node.pymigrations/versions/967016ee10fa_add_embedding_vec_to_embeddings_for_.pymigrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Thanks for the SQLite migration work. Please wait for #995 to land first, then rebase this PR onto Why #995 firstBoth PRs add Fixes after rebase
Happy to re-review once rebased on #995. |
Thanks for the review – that makes sense. I'll wait for #995 to merge, then rebase this PR onto main. After rebasing, I'll:
Once I've rebased and cleaned up, I'll re‑request your review. Thanks for the guidance! |
#995 is rebased please check @northdpole ! |
|
#995 has merged. Please rebase this PR onto latest |
…ints - Add downgrades to 055dbd9f8bfe and 967016ee10fa to drop added columns - Replace deprecated Inspector.from_engine(conn) with sa.inspect(conn) - In 9f1a2b3c4d5e, verify that existing tables have the required unique constraints; add them via batch_alter_table if missing - Ensure migration fails if constraints cannot be added Addresses PR review comments OWASP#1006
c79da22 to
d4bea6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py (1)
32-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
print()calls from the migration.The PR objectives explicitly require migrations without
print()statements. Remove the four unconditional stdout writes. Keep migration diagnostics in the project’s standard logging path if diagnostics are required.🤖 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 32 - 41, Remove all four print() calls from the migration’s column-existence branches, including the messages for the cre and node tables. Preserve the existing column checks and op.add_column operations; use the project’s standard logging path only if migration diagnostics are required.
🤖 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 44-58: The downgrade function must not drop pre-existing
document_metadata columns, since column_exists only confirms presence, not
ownership. Make the migration ownership-aware by folding this revision into the
original `#995` migration or recording which columns upgrade() actually added,
then have downgrade() drop only those columns; add regression coverage for both
cre and node.
- Around line 31-38: Update the migration’s document_metadata column definitions
for both cre and node to use sa.JSON() instead of sa.Text(), matching the JSON
handling in the application. Remove the related migration print() calls while
preserving the existing column-existence checks and conditional additions.
---
Nitpick comments:
In
`@migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py`:
- Around line 32-41: Remove all four print() calls from the migration’s
column-existence branches, including the messages for the cre and node tables.
Preserve the existing column checks and op.add_column operations; use the
project’s standard logging path only if migration diagnostics are required.
🪄 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: e67e0d87-a4a7-4297-8073-46ddaafd26af
📒 Files selected for processing (1)
migrations/versions/b5ac48010165_add_missing_document_metadata_column_to_.py
…ta column in downgrade
…logic for document_metadata column
3 participants
Problem
Several recent migrations were not fully compatible with SQLite, causing errors when developers run
make migrate-upgradeor./scripts/update-cwe.sh:op.create_unique_constraint()afterCREATE TABLEfails withNo support for ALTER of constraints in SQLite dialect.Missing
document_metadatacolumn increandnodetables causesno such columnerrors.Missing
embedding_veccolumn inembeddingstable causesno such column: embeddings.embedding_vec.Some migrations are not idempotent, causing
table already existserrors on re-runs.Solution
9f1a2b3c4d5e– DefineUniqueConstraintinsideCREATE TABLE;addtable_existsguards.055dbd9f8bfe– Adddocument_metadatawith column existence checks (new migration).967016ee10fa– Addembedding_vecasTEXTfor SQLite with existence check (new migration).Testing
make migrate-upgrade→make upstream-sync→./scripts/update-cwe.shall succeed.✅ Existing database: re-runs are idempotent, skip already-created objects.
✅ No regressions for PostgreSQL.
Impact
Developers using SQLite can now run migrations and import data without manual workarounds.
Makes the project more contributor‑friendly for SQLite users.
Ready for review. Let me know if any adjustments are needed.