fix(migrate): auto-migrate pass ownership — column-before-index sequencing and FK on_delete reconciliation#326
Merged
Conversation
…n pass One auto-migrate pass that both adds columns to an existing table and declares a composite unique over them crashed at boot: the create pass fired the model's index DDL at the already-existing table before the reconciliation pass could add the referenced columns (Postgres: 'column does not exist'; SQLite silently built the index over a string constant via its double-quote fallback). The create pass now introspects live table names once and skips existing tables entirely — enforcing the documented contract that plain auto_migrate leaves existing tables untouched. Index reconciliation on existing tables already lives, correctly ordered, in migrate_updates. See ADR-0010; part of #324.
The live-side IR previously hardcoded foreign_keys to empty: no FK introspection existed, so a declared FK change on an existing column was invisible to the diff. Introduce LiveForeignKey and per-backend readers — pg_constraint on Postgres (constraint name, columns, target, confdeltype mapped to the declared action vocabulary) and PRAGMA foreign_key_list on SQLite (which exposes no constraint names) — and populate the old IR's foreign_keys from them. Multi-column live FKs are user-owned by construction (ferro only emits single-column FKs) and are not surfaced. No diff behavior change yet; the FK diff step lands next. Part of #325.
Changing a ForeignKey's on_delete on an existing model was silently ignored — the dangerous kind of miss: the model declares SET NULL, the database keeps CASCADE, and the first parent delete destroys children an operation documented as non-destructive. migrate_updates now diffs each declared FK against the live constraint on the same column. On Postgres, a ferro-owned (fk_-named) constraint whose definition drifts (on_delete, target) is rebuilt — DROP CONSTRAINT + ADD CONSTRAINT under the canonical name, inside the per-table transaction — and a declared FK missing entirely from an existing column is added. A drifting constraint ferro does not own is left untouched but warned about. On SQLite, which cannot alter table constraints, every FK drift warns loudly instead — never silence. New MigrationOp variants AddForeignKey / RebuildForeignKey; ownership predicate is_ferro_fk_name single-sourced in ferro-ddl-lowering.
…DR-0010 Document the auto-migrate pass-ownership decision (create pass owns only missing tables; the reconciliation pass is the sole authority for DDL against existing tables) as ADR-0010, with the new CONTEXT.md terms (create pass, reconciliation pass, ferro-owned artifact, constraint rebuild). Extend the migrate_updates coverage docs — guide table, architecture, FAQ, and the connect()/create_tables()/migrate() docstrings — with FK reconciliation semantics per backend.
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
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.
Closes #324. Closes #325.
Both issues are auto-migrate gaps found by Pinch's M7 CP0 scratch-verification spike. One PR, one commit per slice, per the agreed plan (grilled and confirmed with the owner; decisions recorded in ADR-0010 and CONTEXT.md).
#324 — create pass fired index DDL at existing tables
The reported "index before column" crash was not in the reconcile plan (which already orders
ADD COLUMNbeforeCREATE INDEX):internal_migratefirst runs the create pass, which executed every model's index DDL even whenCREATE TABLE IF NOT EXISTSno-opped on an existing table. A new composite unique referencing not-yet-added columns died right there — before the reconciliation pass could add them. On SQLite it was worse than a crash: the double-quoted-string fallback silently built the unique index over a string constant.Fix: the create pass introspects live table names once and skips existing tables entirely, enforcing the already-documented contract ("existing tables are left untouched unless
migrate_updatesis set"). Index reconciliation on existing tables stays where it already lives, correctly ordered, inmigrate_updates. Deliberate behavior change (ADR-0010): plainauto_migrateno longer creates new indexes on live tables — that undocumented convenience was the crash vector and had two competing owners of index creation.#325 — FK
on_deletedrift was silently ignoredLive FK constraints were never introspected at all (
live_columns_to_schema_irhardcodedforeign_keys: vec![]), soCASCADE → SET NULLon an existing model changed nothing and warned about nothing — and Pinch's "sever, never destroy" disconnect would cascade-delete user history on upgraded installs.Fix:
migrate_updatesnow introspects live FKs (pg_constraint/PRAGMA foreign_key_list) into the old IR and diffs each declared FK against the live constraint on its column:fk_-named) constraint with definition drift (on_delete, target) is rebuilt —DROP CONSTRAINT+ADD CONSTRAINTunder the canonical name, inside the existing per-table transaction. A declared FK missing entirely from an existing column is added. A drifting constraint ferro does not own is warned about, never altered (mirroring theidx_*/uq_*index-ownership policy).Verification
connect()seam (tests/test_auto_migrate.py), plus plan-level DDL goldens (tests/test_migrate_plan.py) and Rust diff/emission tests (crates/ferro-migrate/src/tests.rs).cargo test --no-default-features --features testing203 passed,just checkclean.fk_<table>_<col>_<to_table>name;test_alembic_autogen_after_migrate_updates_is_idempotentstays green.Docs
CONTEXT.mdgains four terms (create pass, reconciliation pass, ferro-owned artifact, constraint rebuild); ADR-0010 records the pass-ownership decision and its rejected alternatives; the migrations guide, architecture page, FAQ, andconnect()/create_tables()/migrate()docstrings document the new coverage per backend.