Repository navigation
chore(db): squash the 34 EF migrations into a single InitialCreate (#245) - #348
Merged
Merged
Conversation
) Replaces every migration from 20260627123409_InitialCreate through 20260801054132_AddBaseReferenceDataAndMustChangePassword with one InitialCreate generated from the current model. Twelve of the retired migrations carried raw SQL written for mid-history data states — backfills that touch 0 rows on an empty database but still execute on every fresh deploy and every Testcontainers run. Safe exactly once, and only now: no production database exists, so no __EFMigrationsHistory anywhere needs baselining. DEV DATABASES MUST BE DROPPED AND RECREATED — the old history rows no longer match any migration in the assembly, so MigrateOnStartup fails: docker compose -f deploy/docker-compose.dev.yml down -v docker compose -f deploy/docker-compose.dev.yml up -d Equivalence proof (not "the tests pass"): both migration sets were applied to throwaway Postgres 18.4 containers and compared. - pg_dump --schema-only diff: identical except (a) physical column order, now model order instead of the order columns happened to be ADDed over three weeks, and (b) 20 vestigial column DEFAULTs left behind by AddColumn(defaultValue:) backfills. A canonical information_schema/pg_catalog comparison (columns, types, nullability, indexes, constraints, identity/generated, collations, functions, triggers, views, extensions) diffs to exactly those 20 defaults and nothing else. - The defaults were never in the EF model — AppDbContextModelSnapshot.cs is byte-identical before and after — so EF always wrote those columns explicitly and never relied on them, and no hand-written SQL in the repo inserts into those tables. - Seeded reference data (1 account, 4 roles, 10 egg grades, 6 unit conversions): row counts and full contents identical. - dotnet ef migrations has-pending-model-changes: clean. - `dotnet Cluckwork.Api.dll migrate` applies it end to end against a throwaway Postgres and is a no-op on a second run. Two things in the new migration are hand-carried and could not be regenerated, both caught by the dump diff: - The four lower("Name") expression unique indexes (IX_EggGrades_AccountId_FarmId_LowerName, IX_ExpenseCategories_NameCi, UX_InventoryItems_Account_Farm_LowerName, IX_Products_AccountId_LowerName). EF cannot model a functional index; regeneration silently dropped four uniqueness constraints. - #283's static base reference data raw SQL, kept verbatim (still not HasData: these rows are user-mutable, so model-owned seed data would later emit UpdateData/DeleteData over a farm's own edits). Both now have guards: MigrationSecurityReviewTests checks the migration source, BaseReferenceDataMigrationTests checks the resulting database (including that a case-only-different duplicate is rejected). Removing one index from the migration fails both. Retired with the history they tested: - MigrationUpgradePathTests's two upgrade-from-the-old-seeder fixtures — they migrate to "the migration before AddBaseReferenceData…", which no longer exists. Its virgin-database fixture survives as BaseReferenceDataMigrationTests. - LegacyIdempotencyRowMigrationTests — pins a row shaped by the pre-#307 schema, reachable only by migrating to 20260730053303_AddSimulationSeedState. IdempotencyMiddleware's empty-RequestHash shim is now unreachable (its comment says it goes away with this squash). Left in place as an inert guard and flagged instead of deleted, so this commit changes no behaviour.
Owner
Author
|
@codex please review this |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Brings in #350 (#269, transient-DB resilience). Two things needed hands: - MigrationUpgradePathTests.cs: modify/delete conflict. This branch deletes it (the squash abolishes its premise — there is no longer a starting state to upgrade FROM); main only threaded #350's new DatabaseResilienceOptions parameter through its BuildContext. No coverage is lost, so the delete stands. - BaseReferenceDataMigrationTests.cs: auto-merged CLEANLY and broke the build. It is the file this branch created to carry the one surviving fixture out of the deleted test, so it still called the two-argument PostgresDbContextConfigurator.Configure that #350 widened. Patched to the three-argument form, exactly as #350 patched the file it could see. Re-verified on the merged tree, since a squash that drifts from the model is the failure worth catching here: `has-pending-model-changes` reports no changes, the migration count is still 1, build is 0 warnings / 0 errors, and the full suite is 1140 green (283 domain + 114 application + 743 integration).
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 #245.
Replaces the 34 migrations with one
InitialCreate. This has to happen before the first production deploy — once a real database exists, history can no longer be collapsed. Every database today (dev, CI, Testcontainers, simulation) is disposable, which is the property that makes it safe.Every existing dev / demo / simulation database must be dropped and recreated.
MigrateOnStartupwill fail against the old 34-row__EFMigrationsHistory. For the compose dev DB:docker compose -f deploy/docker-compose.dev.yml down -v && up -d.The trap this nearly walked into
The first regeneration silently dropped four unique indexes that EF cannot express in the model —
lower("Name")functional indexes created as raw SQL in mid-history migrations:IX_EggGrades_AccountId_FarmId_LowerNameIX_ExpenseCategories_NameCiUX_InventoryItems_Account_Farm_LowerNameIX_Products_AccountId_LowerNameLosing them would have let a farm create "Layer Mash" beside "layer mash", quietly breaking a case-insensitive-uniqueness guarantee three entity configurations depend on. Nothing in the build or the test suite would have caught it, because the model was still correct — only the database wasn't.
They are carried into
InitialCreateverbatim and now guarded two ways: a source-level assertion (Docker-free) and a real-database test that inserts'SMALL'beside the seeded'Small'and requires the rejection. Mutation-checked: deleting the EggGrades index turns both red; restoring turns them green.Equivalence evidence
The deliverable is a schema diff, not a green suite — tests pass against a subtly wrong schema.
A canonical structural comparison (column name-wise, order-insensitive: types, lengths, nullability, indexes, constraints incl. FK delete behaviour, identity, collations, functions, triggers, views, extensions) between a database built from the 34 migrations and one built from
InitialCreateis identical except for column defaults.Seeded reference data diff is zero lines — 1 account, 4 roles, 10 egg grades, 6 unit conversions, identical ids/names/sort orders/
EggsPerUnit/Version.__EFMigrationsHistorygoes 34 rows → 1, which is the point.AppDbContextModelSnapshot.csis byte-identical to main, which independently proves the model didn't move — including all 19IsConcurrencyTokendeclarations (Version, and thexminuse inIdentityProvider.RevokeRefreshTokenAsyncfrom #176).The diff is not literally zero, and I'd rather say so
Two categories remain, both accepted deliberately:
CREATE TABLE. The old order was an artifact of the sequence columns wereAddColumned over three weeks. Postgres attaches no semantics to it, and noSELECT *or column-list-freeINSERTexists insrc/ortests/.DEFAULTclauses, residue ofAddColumn(defaultValue:)used to backfill pre-existing rows. They were never in the EF model — the byte-identical snapshot proves it — so EF always wrote those columns explicitly. Several are actively misleading (SalesOrderItems.ProductId DEFAULT '00000000-…'on an FK). Reproducing them would mean hand-addingdefaultValue:to a regenerated migration, deliberately recreating model/DB drift. The one default the model does declare,refresh_tokens.RevokedByGrace, survives.Seeded data stays raw SQL, not
HasDataThe squash retires the original reason (back-filling existing installs), but a permanent one remains:
HasDatarows become part of the model, and all four tables are user-mutable through the app (renaming the account,PUT /api/v1/egg-grades/{id}, retuningEggsPerUnit). A later model-diff would emitUpdateData/DeleteDatareverting a farm's own edits.Tests retired — unavoidable, not cleanup
MigrationUpgradePathTests— two fixtures migrate to "the migration beforeAddBaseReferenceDataAndMustChangePassword", which no longer exists; their premise is the starting state this PR abolishes. Third fixture survives, expanded, asBaseReferenceDataMigrationTests.LegacyIdempotencyRowMigrationTests— hardcodes an intermediate migration target and hand-inserts a pre-Make API write idempotency atomic across replicas #307-shaped row. Neither is reachable now.MigrationSecurityReviewTestsretargeted from migration name to SQL content, so a future migration can't move the statements out from under it.Verification
has-pending-model-changes→ "No changes have been made to the model since the last migration."migrateCLI verb end-to-end against a throwaway Postgres, Production env: exit 0, correct seed counts, and a second run is a clean no-op.Follow-up, deliberately not done here
IdempotencyMiddleware's empty-RequestHashshim is now unreachable dead code (its own comment said it "goes away once #245's squash retires every pre-#307 row"). Left in place because removing it is a request-path behaviour change that would make this squash harder to review. Worth a one-line PR.