Refuse to re-apply an edited already-applied migration (closes #29)#32
Open
CatFortman wants to merge 2 commits into
Open
Refuse to re-apply an edited already-applied migration (closes #29)#32CatFortman wants to merge 2 commits into
CatFortman wants to merge 2 commits into
Conversation
…pure logic CI's `dotnet test` currently passes vacuously with zero tests. Adds unit coverage for the tag/checksum header parsing, the CREATE OR ALTER guard on database object scripts, and the dry-run classification logic (applied / pending / drifted-migration-vs-drifted-object-script / checksum-missing / tagless), which is the trickiest and most safety-critical logic in the repo. EnsureCreateOrAlterStatement and AddPlanEntry move from private to internal (with InternalsVisibleTo) so they're directly testable without a live SQL Server connection. Closes #18
CheckMigrationApplied matches on (name, checksum, Success=1), so an edited migration file produces a (name, new-checksum) pair with no history row - HasBeenApplied returns false and apply silently re-executes the modified migration against a database where the original already ran. Dry-run already classified this as CHANGED, but nothing stopped a plain `apply`. ApplyScriptFile now looks up the checksum of the migration's last *successful* apply by name alone before the existing (name, checksum) check. If one exists and differs from the file's current checksum, it throws immediately naming the migration, instead of running the edited SQL. Object scripts are unaffected - re-applying an edited proc/view is still the designed workflow. A migration whose only history is a failed attempt (Success = 0) is still allowed to apply after being fixed, since the guard only looks at Success = 1 rows. Verified live against LocalDB (see PR description): fresh apply, unchanged no-op re-run, edited-migration throw (DB left untouched, no duplicate history row), failed-then-fixed re-run, and an edited object script still re-applying normally. Closes #29
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
1 participant
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.
Summary
CheckMigrationAppliedmatches on(MigrationName, Checksum, Success = 1), so editing an already-applied migration produces a(name, new-checksum)pair that isn't in history.HasBeenAppliedreturned false andApplyScriptFilesilently re-executed the edited migration against a database where the original had already run. Dry-run already flagged this asCHANGED, but nothing stopped a plainapply.ApplyScriptFile(migrations only) now looks up the checksum of the migration's last successful apply by name alone (SqlStatements.SelectLatestSuccessfulMigrationChecksum) before the existing skip check. If a successful history row exists and its checksum differs from the file on disk, it throws immediately, naming the migration, instead of running the edited SQL.MigrationService.DetectEditedMigration) separate from the DB lookup, per the pattern established by Add a unit test project — CI currently runs 'dotnet test' against zero tests #18's test project.ScriptKind.Migration).Success = 0) is still allowed to apply/re-run after being fixed, since the guard only considersSuccess = 1rows.Depends on #31 (adds the test project this PR's new tests live in) — until that merges, this PR's diff includes both branches' commits; only the last commit here is new.
I intentionally did not add the optional
--force-changedescape hatch the issue mentions, to keep this PR scoped to the hard-stop fix; happy to follow up separately if wanted.Test plan
dotnet test MigrationOps.sln— 43/43 passing, including newDetectEditedMigrationTests(unchanged-applied allowed, edited-applied rejected, failed-then-fixed allowed)Migration '...' was already applied with checksum ... but the file now has checksum .... Migrations are immutable once applied - create a new migration instead of editing this one.; confirmed the edited SQL never ran (no new column) and no duplicate/second history row was written