Skip to content

feat(migrations): foreign defaults are drift on every door (#628) - #634

Merged
0x054 merged 5 commits into
feat/pinch-migration-needsfrom
sdd/628-foreign-defaults-are-drift
Oct 11, 2026
Merged

0x054 merged 5 commits into
feat/pinch-migration-needsfrom
sdd/628-foreign-defaults-are-drift

Conversation

@0x054

@0x054 0x054 commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Closes #628

What

Someone sets a default by hand that no model declares:

ALTER TABLE "author" ALTER COLUMN "slug" SET DEFAULT '';

Every door now sees it:

$ ferro migrate drift
drift against 0001_create_author:
  author.slug has default '' in the database, none declared
  • baseline refuses it with the same line, exits 4 and records nothing.
  • connect(migrate_destructive=True) drops it, and the PassReport lists ALTER TABLE "author" ALTER COLUMN "slug" DROP DEFAULT.
  • migrate_updates leaves it and warns, naming both fixes.
  • Alembic autogenerate writes op.alter_column('author', 'slug', server_default=None, …). Its downgrade() raises with the reason: no declared side holds the default, so there is nothing to put back.

Ferro's own two defaults never show up as drift:

  • On Postgres, a serial key's nextval(...), recognised by the sequence the column owns (pg_get_serial_sequence) and never by matching its text. A renamed table's sequence therefore still counts as ferro's.
  • On SQLite, a literal default on a NOT NULL column, whatever the literal. This is the default ADR-0034 keeps when the pass adds a required column.

On SQLite an expression default or any default on a nullable column is drift. The pass cannot rebuild a table, so it warns, names the fix (a hand rebuild, or a ferro migrate new … --sql-step) and changes nothing.

The successful-baseline sentence now reads: baseline checked what ferro migrate drift checks: objects ferro does not own (functions, triggers) were not compared.

How it works: the live read now carries each column's default (LiveColumn.column_default) and whether the column owns a sequence (owns_sequence). The live IR copies these into SchemaColumn.default and autoincrement. Inside the one planner, foreign_default(column, dialect) decides whether a default is foreign and plan_from_ir plans MigrationOp::DropDefault { table, column, was }. The op's verdict is Native on Postgres and Rebuild on SQLite, with drops_data=false. Plan::render renders it once. A plan between two declared snapshots never carries the op, so every generated file is unchanged.

Verify

Ran the ticket's Verify block on 7b9fe27: passed (cargo: 419 + 354 tests; pytest: 998 passed, 20 skipped, 1 xfailed, which is the existing C2 finding; prek clean).
TDD: RED uv run pytest --db-backends=sqlite,postgres tests/test_migrate_defaults.py -x failed as expected (drift printed DropDefault on author, because the drift module had no renderer for the new op yet). GREEN, same command: 16 passed, 10 skipped (dialect-specific cases).
The full suite (-n 8) had 1 failure, test_session_settings_operations.py::test_every_operation_shape_is_scoped[postgres]. It passes when run alone and does not touch defaults; it looks load-sensitive.
Verify block corrected: no

Notes for the reviewer

  • Which way the down goes. The ticket says the verdict is "Irreversible toward a declared target". I implemented it the way the down actually runs. The up plans live → models and its DropDefault is Native/Rebuild. The down (plan_down, used by the bridge's downgrade()) plans models → live, and the live side's foreign default comes back as the same op with Execution::Irreversible(reason) through Side::expresses. This is the same pattern as RemoveEnumLabel toward a live type. Two declared snapshots never plan it at all.
  • Live defaults never reach a backfill. A live column's default is now catalog text, so Side::writable_values / writable_ir strip it wherever a statement could write a value: the renderer, the verdict's needs_values, and the bridge's has_twin. A down that restores a dropped required column therefore still demands values, as before. This is pinned in a_live_default_is_never_a_value_a_down_writes.
  • The drift line's literal. was folds the cast Postgres adds to a literal for display: ''::character varying becomes ''. Any other text is kept as the catalog prints it.
  • Two new names in the lowering crate. ReportKind::ForeignDefault { was } is the migrate_updates warning (recurring, answered by a DropDefault). InPlaceChange::DropDefault is the SQLite rebuild warning.
  • Casebook. The new case A2-foreign-default-on-the-before-side carries a Case.hand_edit. Pins (e) defaults, (f) and (g) run it on the database they build at the before side. Pin (e) has a third database that the pass reconciles under migrate_destructive. FOREIGN_DEFAULTS_LEFT names the one default SQLite keeps (it warns about it). No new FINDINGS entry. A new test checks that no casebook case's generated plan carries DropDefault.

Outside Files owned

  • step_class.rs compile fix (6ead977): dropped on the rebase onto feat(schema): retired enum labels (#627) #633, which carries the same line.

  • crates/ferro-migrate/src/run_plan/step_class.rs also gets one match arm, DropDefault → Expand. The match is exhaustive, and the op is never in a generated step anyway.

  • crates/ferro-ddl-lowering/src/lib.rs: the report kind, the in-place change and their two sentences (foreign_default_warning, sqlite_drop_default_report). Every warning lives in this crate (ADR-0052).

  • crates/ferro-migrate/src/revision.rs: DropDefault is Alembic's own op (has_twin), and the bridge reads the target's writable values.

  • crates/ferro-migrate/src/render.rs was owned. generate/rebuild.rs needed no change, because the generator never meets the op.

  • src/ferro/pass_report.py: one docstring word, adding ForeignDefault to the kinds list.

  • Issue status: Closes #628 above (on the integration branch, the orchestrator closes it after merge).

Fix report (review round 1)

New head 2821853, rebased onto origin/feat/pinch-migration-needs.

  • Rebase. The rebase dropped the compile-fix commit as empty against feat(schema): retired enum labels (#627) #633. src/live_ir.rs had no conflict.
  • E0063 fixed. The planner's test helper foreign_default_tests::column now has enum_retired_labels: Default::default(). That literal was the only SchemaColumn literal this PR adds, and src/live_ir.rs already has the field from feat(schema): retired enum labels (#627) #633.
    • cargo check -p ferro-migrate --all-targets and cargo check --all-targets are clean.
    • cargo test -p ferro-migrate: 419 passed.
  • rustfmt-only hunks dropped. src/codec.rs, codec_plan.rs, operations.rs, query.rs, schema.rs, state.rs and migrate.rs are back to their base contents. prek run --all-files passes without them. The cargo fmt --check diffs left in those files were already on the base.
  • Test name. The "up or pass" test is now two tests: test_a_fresh_tables_key_sequence_is_never_drift_where_up_built_it (up, then drift exits 0) and …_where_the_pass_built_it (pass, then baseline records).
  • Verify block on 2821853, each exit status 0:
    • maturin develop
    • cargo test -p ferro-migrate: 419 passed
    • cargo test --no-default-features --features testing: 354 passed
    • pytest: 1001 passed, 20 skipped, 1 xfailed
    • prek run --all-files

🤖 Generated with Claude Code

https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR

@0x054 0x054 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec: PASS

  • Minor · src/codec.rs, src/codec_plan.rs, src/operations.rs, src/query.rs, src/schema.rs, src/state.rs (plus the non-default hunks in src/introspect.rs, src/migrate.rs, src/live_ir.rs) at 7b9fe27 · Extra · Every hunk in these six files is rustfmt reflow; none of them strips a default. The PR body's "Outside Files owned" list leaves them out. The cause is real: the base is not fmt-clean (rustfmt --check on src/state.rs at c5b9e72 exits 1), and Verify's prek run --all-files runs the fmt hook, so Verify forces the churn. The ticket's Files owned does not list these files, so the PR body should declare them as formatter output.
  • Minor · tests/test_migrate_defaults.py test_a_serial_keys_sequence_is_never_drift_on_an_up_or_pass_built_database at 7b9fe27 · The name promises an up-built database, but the test builds only with the pass. AC2's up-built and renamed-table side is still pinned: pin (e) runs ferro migrate drift on the migrated database of B3-rename-a-model, which passed on Postgres. The name overstates what the test covers.

Checked against the ticket:

  • AC1: drift line and exit 4, baseline refusal, destructive DROP DEFAULT in the PassReport, updates warning, and autogenerate's alter_column(server_default=None) with a downgrade() that raises "set outside ferro" are each covered on Postgres in test_migrate_defaults.py.
  • AC2: owns_sequence is pg_get_serial_sequence(format('%I.%I', …)) IS NOT NULL (src/introspect.rs). The planner reads only autoincrement. No code matches on nextval text, and a Rust test shows the same nextval text without ownership is foreign.
  • AC3: foreign_default gives SQLite nullable || !is_sqlite_literal. SQLite's backfill defaults (literal_default_value, render_json_backfill_default → '…') are always literals, so a pass-built file is never flagged.
  • AC4: Side::foreign_default returns None for a declared side, and writable_ir is the identity there. A new test walks every casebook case and finds no DropDefault. The generator and generate/ are untouched.
  • AC5: NOT_COMPARED and the adopting howto changed. The howto's "When baseline says no" names both fixes and the SQLite hand rebuild.
  • AC6: the verdict is decided once in verdict()/sqlite_rebuilds. Goldens assert Native (PG) and Rebuild (SQLite), drops_data=false, and an Irreversible(reason) down on both dialects. They assert the outcome, not the implementation.
  • AC7: case A2-foreign-default-on-the-before-side runs through pins (a)–(g) on both dialects. FINDINGS has no new entry. FOREIGN_DEFAULTS_LEFT names only the SQLite default the narrowing keeps.
  • **Narrows:** is honoured: on SQLite the pass emits a SqliteInPlace warning naming ferro migrate new … --sql-step and runs no DDL.
  • Declared out-of-scope files: ferro-ddl-lowering/src/lib.rs (ADR-0052 home for sentences), revision.rs (has_twin and the target's writable values), run_plan/step_class.rs (#633's literal line plus the exhaustive-match arm) and the pass_report.py docstring. I accept all four.
  • I-1 item 11: the enum-types and checks families and translate.py dispatch are untouched apart from the new DropDefault twin branch.

Cannot verify:

  • test_session_settings_operations::test_every_operation_shape_is_scoped[postgres] is flaky under -n 8, per the worker. That file is outside Verify and outside this diff, and I did not reproduce it.

Quality: FAIL

  • Important · crates/ferro-migrate/src/plan.rs:4628 at 7b9fe27 · The new test helper foreign_default_tests::column builds a SchemaColumn without enum_retired_labels. That field became required when #633 landed on the integration branch (682c30b). I merged origin/feat/pinch-migration-needs locally, unpushed, into the PR head, and cargo test -p ferro-migrate --no-run fails: error[E0063]: missing field 'enum_retired_labels' in initializer of 'ferro_schema_ir::SchemaColumn' at plan.rs:4629 in the merged tree. On the same merged tree, cargo test --no-default-features --features testing --no-run and cargo check --all-targets compile clean. The textual merge is clean, so merging this PR as it stands breaks cargo test -p ferro-migrate on the integration branch. Fix: rebase onto feat/pinch-migration-needs and add enum_retired_labels: Default::default() to the literal. The retired: false line in step_class.rs then drops out, as the PR body says.

Checked against the laws:

  • Live defaults never reach a written value. render_from reads writable_ir() on both sides, verdict() reads target.writable_values() (so needs_values / demands_values / fails_on_rows see no catalog text), and revision::written reads plan.target.writable_ir() (so has_twin sees none). emit.rs's backfill reads only those stripped models or declared ones. a_live_default_is_never_a_value_a_down_writes pins that a down restoring a dropped required column still demands_values. Pin (e) column_defaults and KEPT_DEFAULTS are unchanged and green.
  • Concern (1): the up is Native/Rebuild, and the down, planned models → live, carries Execution::Irreversible(reason) through Side::expresses, the same shape as RemoveEnumLabel. The reason reaches Alembic's downgrade() as raise RuntimeError(…set outside ferro…). This matches the ticket's prose, and the AC phrase names the same direction.

Verify: passed on 7b9fe27

  • uv run maturin develop: exit 0
  • cargo test -p ferro-migrate: 419 passed, exit 0
  • cargo test --no-default-features --features testing: 354 passed, exit 0
  • uv run pytest … (the 10 files): 998 passed, 20 skipped, 1 xfailed, exit 0
  • uv run prek run --all-files: exit 0, tree clean after

The worker's sdd-verify note is present on HEAD.

0x054 and others added 5 commits October 11, 2026 14:05
…drops foreign ones

The live read now carries each column's server default and whether it owns
a sequence. The one planner plans a DropDefault for every foreign default
(a live default ferro did not leave): native on Postgres, a rebuild on
SQLite, under migrate_destructive only, and reported otherwise; its down
toward a live database is irreversible with its reason.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
…eign defaults

drift prints '<table>.<column> has default <literal> in the database, none
declared'; baseline refuses it and its success sentence now says only that
objects ferro does not own were not compared; the Alembic bridge writes
alter_column(server_default=None) with an irreversible downgrade.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
…-migrate guide

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
The planner's test column gains enum_retired_labels; the rustfmt-only hunks
in files this change does not touch are dropped; the serial-sequence test
is split into its up-built and pass-built halves.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR

@0x054 0x054 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: 7b9fe27 → 2821853 (rebased onto 682c30b)

Spec: PASS

  • Minor · formatter-only files left undeclared · ADDRESSED · src/codec.rs, src/codec_plan.rs, src/operations.rs, src/query.rs, src/schema.rs, src/state.rs and src/migrate.rs are no longer in git diff 682c30b 2821853. The PR diff now touches 21 files, all owned or declared.
  • Minor · the "up or pass" test name · ADDRESSED · It is now two tests:
    • tests/test_migrate_defaults.py:292 test_a_fresh_tables_key_sequence_is_never_drift_where_up_built_it builds the database with ferro migrate up and asserts that drift exits 0 with "no drift".
    • tests/test_migrate_defaults.py:304 …_where_the_pass_built_it builds it with the pass (built_by_the_pass), then checks drift and baseline.

Quality: PASS

  • Important · foreign_default_tests::column was missing enum_retired_labels · ADDRESSED · crates/ferro-migrate/src/plan.rs:4648 at 2821853 now sets enum_retired_labels: Default::default(). Because of the rebase, the retired: false line in step_class.rs comes from the base and no longer from this PR.
  • Merge preview, re-run: I merged origin/feat/pinch-migration-needs locally without pushing. It is now at cee6a19, which adds #635 since the rebase. The merge was clean, and each of these compiled with no errors: cargo check --all-targets, cargo test -p ferro-migrate --no-run and cargo test --no-default-features --features testing --no-run. I then ran git merge --abort.
  • The fix diff (8927cff..2821853) adds no new breakage. It holds only the one field, the reverted formatting and the split test.

Verify: passed on 2821853

  • uv run maturin develop: exit 0
  • cargo test -p ferro-migrate: 419 passed, exit 0
  • cargo test --no-default-features --features testing: 354 passed, exit 0
  • uv run pytest (the 10 files): 1001 passed, 20 skipped, 1 xfailed, exit 0
  • uv run prek run --all-files: exit 0, tree clean after

The sdd-verify note on HEAD reads "verified 2026-10-11T18:23:08Z".

@0x054
0x054 merged commit c9e3f61 into feat/pinch-migration-needs Oct 11, 2026
6 checks passed
@0x054
0x054 deleted the sdd/628-foreign-defaults-are-drift branch October 11, 2026 18:36
0x054 added a commit that referenced this pull request Oct 12, 2026
…efaults and DropDefault runs on every door (#628) (#634)

`ferro migrate drift` and `ferro migrate baseline` see column defaults. The live read carries each column's server default on both dialects, and the one planner plans a `DropDefault` op for every foreign default, a live default ferro did not leave. `drift` prints `<table>.<column> has default <literal> in the database, none declared`; `baseline` refuses it and its success sentence now says only that objects ferro does not own were not compared; the Postgres pass drops it under `migrate_destructive` and reports it; the SQLite pass warns naming the migrations door (the pass never rebuilds a table); the Alembic bridge writes `alter_column(server_default=None)` with an irreversible downgrade. Ferro's own defaults are never drift: the serial sequence a Postgres column owns, and a literal on a `NOT NULL` SQLite column. A casebook case pins every door.

Closes #628

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant