Skips V03's index rename on SQLite - #142
Merged
Merged
Conversation
StatifierRouter.Migrations.V03 renamed the subscription index with ALTER INDEX IF EXISTS on every adapter. SQLite has no ALTER INDEX, so a SQLite host whose router migration walks through V03 failed with a syntax error near "INDEX" from 0.8.0 on. On Ecto.Adapters.SQLite3 both directions now do nothing: SQLite keeps an identifier whole, so V02's index there already holds its whole name and there is no cut name to repair. Every other adapter runs the rename as before; the Postgres migration tests pass unchanged. Adds ecto_sqlite3 as a test-only dependency, a SQLite test repo and StatifierRouter.SQLiteMigrationsTest, which walks V01 to V03 up and down on SQLite and reads the index name back. ADR-0007 gets a dated foot Note; a changelog fragment records the fix. Refs: sr-zb9m
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.
Fixes migration V03 on SQLite. Refs sr-zb9m.
The bug
StatifierRouter.Migrations.V03renames the subscription table's unique index withALTER INDEX IF EXISTS ... RENAME TO ...in both directions, on every adapter. SQLite has noALTER INDEX, so a SQLite host whose router migration walks through V03 (any uncappedStatifierRouter.Migrations.up/1) fails with a syntax error near "INDEX", from 0.8.0 on. V01 and V02 run on SQLite; the new test reproduces the failure on the base commit.The fix
On
Ecto.Adapters.SQLite3,V03.up/1andV03.down/1do nothing (the privaterename_index/3readsrepo().__adapter__()). V03 exists to repair a name Postgres cut to 63 bytes; SQLite keeps an identifier whole, so there V02 already left the index under its whole 70-byte name and there is nothing to repair. The test reads that name back fromsqlite_masterafter V02. The index keeps V02's name on SQLite; the package's queries name the index's columns, never its name (StatifierRouter.subscribe/3'sconflict_target). Every other adapter runs exactly the statement it ran before, and the Postgres migration tests (StatifierRouter.IndexNamesTest,StatifierRouter.MigrationsTest) pass unchanged: neither file is in this diff.Tests
test/support/sqlite_repo.ex:StatifierRouter.SQLiteRepo, an Ecto repo onEcto.Adapters.SQLite3, started per test with a temporary database file.test/statifier_router/sqlite_migrations_test.exs: V01 and V02, then V03 up and down, then V02 down, asserting the subscription index's name, uniqueness and column order after each step; and the uncappedup/1anddown/1a host's first migration writes. Not:isolated: it never touches Postgres.Sabotage, one row per check, each restored byte-equal from a copy before the next:
v03.extrue(the old unconditional rename)down/1running theALTER INDEXdirectlyIndexNamesTestred in three tests, both SQLite tests redDependencies
ecto_sqlite3 ~> 0.22,only: :test, besidepostgrex. No runtime dependency changes. The wholemix.lockchange is four added entries and nothing else moved:ecto_sqlite30.25.0,exqlite0.41.0,elixir_make0.10.0 andcc_precompiler0.1.11.Records and changelog
ALTER INDEX IF EXISTS, which no longer holds on SQLite. The Note decides nothing and removes no line (git diff origin/main -- docs/adr/is additions only); each claim in it citesStatifierRouter.Migrations.V03or the new test by name.changelog.d/sr-zb9m.mdunder### Fixed.Provenance
One engineering choice: the rename is skipped on SQLite only, not run on Postgres only. SQLite is the adapter observed failing and checked to keep the name whole; on any other adapter the migration sends what it sent before, so nothing changes where nothing was checked.
Gate
Full
mix qualityon this head: Format, Compile, Isolated tests, Doc links, Dependencies, Credo, Docs, Tests (422 of 422 passed, 97.7% coverage) and Dialyzer all passed; Doctor, Gettext and Sobelow skipped as not installed.