Skip to content

fix(core): let a rebuildable Invalid delta through AssertPatchingIsValid - #542

Merged
jeremydmiller merged 1 commit into
masterfrom
fix-538-rebuildable-invalid
Sep 2, 2026
Merged

jeremydmiller merged 1 commit into
masterfrom
fix-538-rebuildable-invalid

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Closes #538.

The gap

SchemaMigration.AssertPatchingIsValid threw for SchemaPatchDifference.Invalid on every AutoCreate except All, without asking whether the delta could actually apply the change. Both apply paths directly below it do ask:

  • SchemaMigration.WriteAllUpdates
  • Migrator.WriteUpdate

Both honour ISchemaObjectDeltaWithRebuild.CanRebuildInPlace and call WriteUpdate rather than drop-and-create — which is what #477 added so a rebuild would copy the data instead of discarding it. The gate was stricter than the thing it gates.

The fix

var invalids = _deltas
    .Where(x => x.Difference == SchemaPatchDifference.Invalid)
    .Where(x => x is not ISchemaObjectDeltaWithRebuild { CanRebuildInPlace: true })
    .ToArray();

if (invalids.Any())
{
    throw new SchemaMigrationException(autoCreate, invalids);
}

The open question, decided: CreateOnly still refuses

A rebuild recreates a table that is already there, which is an update however you look at it. This needs no special case — the delta is still Invalid, so the migration's Difference is still not Create, and the existing CreateOnly branch throws. Pinned by a test in both suites rather than left to fall out silently.

The doc said the opposite, and was wrong on its own terms

ISchemaObjectDeltaWithRebuild's XML doc claimed a rebuild always requires AutoCreate.All, because a rebuild that also drops a column takes that column's data with it.

That reasoning does not survive contact with the code. AssertPatchingIsValid could only express it by refusing every rebuildable delta, including the great majority that drop nothing — a column type change, a foreign key, a primary key. And measured against a real SQLite database: the ordinary Update path already emits ALTER TABLE … DROP COLUMN and is already permitted under CreateOrUpdate. So the strict rule was not buying the protection it claimed; it refused the same data loss only when the dropped column happened to sit in a key, and refused three harmless changes to do it.

The doc is rewritten to record the decision and the measurement, and keeps the point that is still worth knowing: a rebuild copies every row, so on a large table it is a very different proposition from an ALTER.

Reproduction

The issue cites Weasel.Sqlite.Tests/Tables/numeric_upgrade_rebuild.cs, which does not exist on master — so the reproduction is built here from scratch, in the 9.28.0 upgrade shape: a table created when numeric mapped to REAL, against a model that now asks for NUMERIC. Confirmed failing before the change:

Shouldly.ShouldAssertException : `migration.AssertPatchingIsValid(AutoCreate.CreateOrUpdate)`
should not throw but threw Weasel.Core.SchemaMigrationException
with message "Cannot derive schema migrations for Weasel.Sqlite.Tables.TableDelta AutoCreate.CreateOrUpdate"

Tests

Weasel.Core.Tests/Migrations/rebuildable_invalid_deltas_pass_the_gate.cs — the rule is a Core rule, so it is tested at Core with fakes: permitted under All/CreateOrUpdate, refused under CreateOnly, still refused when CanRebuildInPlace is false, still refused for a delta not implementing the interface, and a mixed migration refused with only the genuinely stuck delta named in the message.

Weasel.Sqlite.Tests/Tables/rebuildable_invalid_is_permitted.cs — end to end against a real database, since SQLite is the only provider implementing the interface today. Asserts the permission is worth something: the migration it lets through converges to None and the row survives with its value.

Full Weasel.Core.Tests (327) and Weasel.Sqlite.Tests (523) green.

Docs

docs/release-9-28.md documented this as a known break with "stay on 9.27 until the guard is fixed". Rewritten for 9.28.1, keeping the workaround for anyone already on 9.28.0, and noting that the gap was never specific to numeric — it affected any SQLite column type change, foreign key change or primary key change.

🤖 Generated with Claude Code

…lid (#538)

weasel#477 taught both apply paths -- SchemaMigration.WriteAllUpdates and
Migrator.WriteUpdate -- to honour ISchemaObjectDeltaWithRebuild.CanRebuildInPlace
and rebuild the object rather than drop and recreate it. The gate above them was
not taught the same thing, so AssertPatchingIsValid refused every Invalid delta
below AutoCreate.All without asking whether the delta could actually carry the
change out. It rejected migrations the machinery it guards would have applied
correctly, with the data intact.

Reachable in 9.28.0 through weasel#533: a table Weasel created from a model
declaring numeric or decimal has a REAL column and the model now asks for
NUMERIC, so the first migration after upgrading threw under CreateOrUpdate.

CreateOnly still refuses -- a rebuild recreates a table that is already there,
which is an update -- and that falls out of the existing CreateOnly branch
without a special case, since the delta is still Invalid.

The interface doc said the opposite, that a rebuild always requires All because
a rebuild which drops a column takes that column's data. Measured: SQLite's
ordinary Update path already emits ALTER TABLE ... DROP COLUMN under
CreateOrUpdate, so the strict rule was not buying that protection -- it only
refused the same loss when the column happened to sit in a key. Doc updated to
match, along with the 9.28 upgrade guide.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jakobt

jakobt commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Would be good to get #539 before this because a table rebuild can in some cases fail halfway through and live database in unfortunate state.

@jeremydmiller
jeremydmiller merged commit 66469fd into master Sep 2, 2026
18 checks passed
@jeremydmiller
jeremydmiller deleted the fix-538-rebuildable-invalid branch September 2, 2026 10:27
@jeremydmiller jeremydmiller mentioned this pull request Sep 2, 2026
jeremydmiller added a commit that referenced this pull request Sep 2, 2026
Clears 9.28's known upgrade break, makes the SQLite table rebuild safe against a
database that has foreign keys, and adds two capabilities.

- #542 closes #538. AssertPatchingIsValid no longer refuses a delta that can rebuild in
  place, which is what made a numeric or decimal SQLite column throw on 9.28 under every
  AutoCreate except All. CreateOrUpdate permits a rebuild; CreateOnly still refuses.
- #539. The SQLite rebuild runs the way SQLite documents it -- enforcement suspended, one
  transaction, foreign_key_check before the commit -- instead of four autocommitting
  statements that wedged the database when the table was referenced. Also fixes an
  AUTOINCREMENT table reissuing a used id, and a view breaking the rename.
- #540. Mutually referencing tables can be created from scratch. A migration holds back
  only the keys whose target it creates later, so a schema that never had the problem
  generates byte-for-byte identical DDL.
- #543 closes #541. FullTextIndexDefinition can index a pre-built tsvector, making
  setweight() weighting reachable for JasperFx/marten#5298.
- #545. SQLite delete-all names the schema instead of letting SQLite resolve by search
  order, which put temp first.

Minor rather than patch: #543 and #540 add API, and #539 and #542 change what an existing
migration does.

Ships with one known issue, #546: on SQLite, resetting identity against a schema that has
no AUTOINCREMENT table fails with "no such table: <schema>.sqlite_sequence". The
single-schema form of this predates 9.29.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

AssertPatchingIsValid refuses a rebuildable Invalid delta that the apply path can handle

2 participants