From 986b194fe243b1e797e16be5d2e9edb39489078a Mon Sep 17 00:00:00 2001 From: cat fortman Date: Wed, 22 Jul 2026 21:52:45 -0300 Subject: [PATCH 1/2] Add MigrationOps.Core.Tests xUnit project with coverage for existing 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 --- MigrationOps.Core.Tests/AddPlanEntryTests.cs | 131 +++++++++++++++++ .../DetermineDatabaseFromTagsTests.cs | 56 +++++++ .../EnsureCreateOrAlterStatementTests.cs | 69 +++++++++ .../ExtractChecksumFromScriptTests.cs | 55 +++++++ .../GetMigrationFileStatusesTests.cs | 137 ++++++++++++++++++ .../MigrationOps.Core.Tests.csproj | 25 ++++ .../ParseTagsFromFileTests.cs | 55 +++++++ .../ShouldApplyScriptTests.cs | 31 ++++ MigrationOps.Core.Tests/TestHelpers.cs | 70 +++++++++ .../Services/MigrationService.cs | 7 +- MigrationOps.Core/MigrationOps.Core.csproj | 6 + MigrationOps.sln | 42 ++++++ 12 files changed, 682 insertions(+), 2 deletions(-) create mode 100644 MigrationOps.Core.Tests/AddPlanEntryTests.cs create mode 100644 MigrationOps.Core.Tests/DetermineDatabaseFromTagsTests.cs create mode 100644 MigrationOps.Core.Tests/EnsureCreateOrAlterStatementTests.cs create mode 100644 MigrationOps.Core.Tests/ExtractChecksumFromScriptTests.cs create mode 100644 MigrationOps.Core.Tests/GetMigrationFileStatusesTests.cs create mode 100644 MigrationOps.Core.Tests/MigrationOps.Core.Tests.csproj create mode 100644 MigrationOps.Core.Tests/ParseTagsFromFileTests.cs create mode 100644 MigrationOps.Core.Tests/ShouldApplyScriptTests.cs create mode 100644 MigrationOps.Core.Tests/TestHelpers.cs diff --git a/MigrationOps.Core.Tests/AddPlanEntryTests.cs b/MigrationOps.Core.Tests/AddPlanEntryTests.cs new file mode 100644 index 0000000..cbffb3c --- /dev/null +++ b/MigrationOps.Core.Tests/AddPlanEntryTests.cs @@ -0,0 +1,131 @@ +using MigrationOps.Core.MigrationFramework.Services; +using MigrationOps.Core.Models; + +namespace MigrationOps.Core.Tests +{ + // AddPlanEntry is the single place that turns a MigrationFileStatus diff into the status + // the dry-run report / --verify gate acts on. These tests exist mainly to lock in the one + // rule that matters most: a drifted (edited) migration must classify as Changed, while a + // drifted database object script (proc/view/etc., meant to be re-applied) classifies as + // WouldApply — mixing those up would either block legitimate object redeploys or let an + // edited migration slip through as an ordinary pending apply. + public class AddPlanEntryTests + { + private static readonly HashSet NoUnresolvedReported = new(StringComparer.OrdinalIgnoreCase); + + [Fact] + public void AlreadyAppliedStatusMapsToAlreadyApplied() + { + var plan = new DryRunPlan(); + var status = new MigrationFileStatus + { + FileName = "Foo.sql", + Tags = new List { "Db1" }, + IsApplied = true, + CurrentChecksum = "abc" + }; + + MigrationService.AddPlanEntry(plan, status, ScriptKind.Migration, "Db1", "Foo.sql", NoUnresolvedReported); + + var entry = Assert.Single(plan.Entries); + Assert.Equal(PlanEntryStatus.AlreadyApplied, entry.Status); + } + + [Fact] + public void DriftedMigrationMapsToChangedNotWouldApply() + { + using var dir = new TempDirectory(); + var filePath = dir.WriteFile("Foo.sql", "SELECT 1;"); + var plan = new DryRunPlan(); + var status = new MigrationFileStatus + { + FileName = "Foo.sql", + Tags = new List { "Db1" }, + HasDrift = true, + RecordedChecksum = "old", + CurrentChecksum = "new" + }; + + MigrationService.AddPlanEntry(plan, status, ScriptKind.Migration, "Db1", filePath, NoUnresolvedReported); + + var entry = Assert.Single(plan.Entries); + Assert.Equal(PlanEntryStatus.Changed, entry.Status); + Assert.Contains("recorded", entry.Detail); + Assert.NotNull(entry.ScriptText); + } + + [Fact] + public void DriftedDatabaseObjectScriptMapsToWouldApplyNotChanged() + { + using var dir = new TempDirectory(); + var filePath = dir.WriteFile("Foo.sql", "CREATE OR ALTER VIEW dbo.V AS SELECT 1;"); + var plan = new DryRunPlan(); + var status = new MigrationFileStatus + { + FileName = "Foo.sql", + Tags = new List { "Db1" }, + HasDrift = true, + RecordedChecksum = "old", + CurrentChecksum = "new" + }; + + MigrationService.AddPlanEntry(plan, status, ScriptKind.DatabaseObject, "Db1", filePath, NoUnresolvedReported); + + var entry = Assert.Single(plan.Entries); + Assert.Equal(PlanEntryStatus.WouldApply, entry.Status); + Assert.Contains("updated", entry.Detail); + } + + [Fact] + public void NewPendingFileMapsToWouldApply() + { + using var dir = new TempDirectory(); + var filePath = dir.WriteFile("Foo.sql", "SELECT 1;"); + var plan = new DryRunPlan(); + var status = new MigrationFileStatus + { + FileName = "Foo.sql", + Tags = new List { "Db1" }, + CurrentChecksum = "new" + }; + + MigrationService.AddPlanEntry(plan, status, ScriptKind.Migration, "Db1", filePath, NoUnresolvedReported); + + var entry = Assert.Single(plan.Entries); + Assert.Equal(PlanEntryStatus.WouldApply, entry.Status); + Assert.Contains("new", entry.Detail); + } + + [Fact] + public void ChecksumMissingMapsToValidationError() + { + var plan = new DryRunPlan(); + var status = new MigrationFileStatus + { + FileName = "Foo.sql", + Tags = new List { "Db1" }, + ChecksumMissing = true + }; + + MigrationService.AddPlanEntry(plan, status, ScriptKind.Migration, "Db1", "Foo.sql", NoUnresolvedReported); + + var entry = Assert.Single(plan.Entries); + Assert.Equal(PlanEntryStatus.ValidationError, entry.Status); + } + + [Fact] + public void TaglessFileIsReportedOnceAcrossMultipleDatabasesAsUnresolved() + { + var plan = new DryRunPlan(); + var status = new MigrationFileStatus { FileName = "Foo.sql", ValidationError = "no tags" }; + var reported = new HashSet(StringComparer.OrdinalIgnoreCase); + + MigrationService.AddPlanEntry(plan, status, ScriptKind.Migration, "Db1", "Foo.sql", reported); + MigrationService.AddPlanEntry(plan, status, ScriptKind.Migration, "Db2", "Foo.sql", reported); + + var entry = Assert.Single(plan.Entries); + Assert.Equal(PlanEntryStatus.ValidationError, entry.Status); + Assert.Equal("(unresolved)", entry.Database); + } + } +} diff --git a/MigrationOps.Core.Tests/DetermineDatabaseFromTagsTests.cs b/MigrationOps.Core.Tests/DetermineDatabaseFromTagsTests.cs new file mode 100644 index 0000000..e90ff39 --- /dev/null +++ b/MigrationOps.Core.Tests/DetermineDatabaseFromTagsTests.cs @@ -0,0 +1,56 @@ +using MigrationOps.Core.MigrationFramework.Services; + +namespace MigrationOps.Core.Tests +{ + public class DetermineDatabaseFromTagsTests + { + // DetermineDatabaseFromTags reads the "Databases" section of configuration, so each + // test points a MigrationService at a throwaway dbconfig.json with just the database + // names it needs (connection strings are never used by this method). + private static MigrationService CreateServiceWithDatabases(params string[] databaseNames) + { + var entries = string.Join(",", databaseNames.Select(n => $"\"{n}\": {{ \"ConnectionString\": \"unused\" }}")); + var json = $"{{ \"Databases\": {{ {entries} }} }}"; + + using var configFile = new TempFile(json, ".json"); + return new MigrationService(configFile.Path); + } + + [Fact] + public void ReturnsMatchingDatabaseName() + { + var service = CreateServiceWithDatabases("Db1", "Db2"); + + Assert.Equal("Db1", service.DetermineDatabaseFromTags(new List { "Db1" })); + } + + [Fact] + public void MatchesConfiguredDatabaseCaseInsensitively() + { + var service = CreateServiceWithDatabases("Db1"); + + // Note: the match is case-insensitive, but the method returns the tag as written + // in the file, not the configured key's casing. That's safe downstream only + // because IConfiguration's own indexer (used by GetConnectionString) is itself + // case-insensitive - this test documents current behavior rather than asserting + // it is the ideal API. + Assert.Equal("db1", service.DetermineDatabaseFromTags(new List { "db1" })); + } + + [Fact] + public void ReturnsFirstTagThatMatchesAConfiguredDatabase() + { + var service = CreateServiceWithDatabases("Db1", "Db2"); + + Assert.Equal("Db2", service.DetermineDatabaseFromTags(new List { "UnknownTag", "Db2" })); + } + + [Fact] + public void ThrowsWhenNoTagMatchesAConfiguredDatabase() + { + var service = CreateServiceWithDatabases("Db1", "Db2"); + + Assert.Throws(() => service.DetermineDatabaseFromTags(new List { "Db3" })); + } + } +} diff --git a/MigrationOps.Core.Tests/EnsureCreateOrAlterStatementTests.cs b/MigrationOps.Core.Tests/EnsureCreateOrAlterStatementTests.cs new file mode 100644 index 0000000..60f9d54 --- /dev/null +++ b/MigrationOps.Core.Tests/EnsureCreateOrAlterStatementTests.cs @@ -0,0 +1,69 @@ +using MigrationOps.Core.MigrationFramework.Services; + +namespace MigrationOps.Core.Tests +{ + public class EnsureCreateOrAlterStatementTests + { + [Fact] + public void AllowsCreateOrAlterAfterHeaderComments() + { + var script = "-- Tags: Db1\n-- Checksum: abc\nCREATE OR ALTER PROCEDURE dbo.Foo AS SELECT 1;"; + + var exception = Record.Exception(() => MigrationService.EnsureCreateOrAlterStatement(script, "Foo.sql")); + + Assert.Null(exception); + } + + [Fact] + public void AllowsBlankLinesBeforeTheStatement() + { + var script = "-- Tags: Db1\n\n\nCREATE OR ALTER VIEW dbo.V AS SELECT 1;"; + + var exception = Record.Exception(() => MigrationService.EnsureCreateOrAlterStatement(script, "V.sql")); + + Assert.Null(exception); + } + + [Fact] + public void IsCaseInsensitive() + { + var script = "-- Tags: Db1\ncreate or alter function dbo.F() returns int as begin return 1 end"; + + var exception = Record.Exception(() => MigrationService.EnsureCreateOrAlterStatement(script, "F.sql")); + + Assert.Null(exception); + } + + [Fact] + public void ThrowsWhenFirstStatementIsPlainCreate() + { + var script = "-- Tags: Db1\nCREATE PROCEDURE dbo.Foo AS SELECT 1;"; + + var ex = Assert.Throws( + () => MigrationService.EnsureCreateOrAlterStatement(script, "Foo.sql")); + + Assert.Contains("Foo.sql", ex.Message); + Assert.Contains("CREATE OR ALTER", ex.Message); + } + + [Fact] + public void ThrowsForEmptyScript() + { + var ex = Assert.Throws( + () => MigrationService.EnsureCreateOrAlterStatement("", "Empty.sql")); + + Assert.Contains("empty", ex.Message); + } + + [Fact] + public void ThrowsWhenScriptIsOnlyHeaderComments() + { + var script = "-- Tags: Db1\n-- Checksum: abc\n-- just a trailing comment, no statement"; + + var ex = Assert.Throws( + () => MigrationService.EnsureCreateOrAlterStatement(script, "OnlyComments.sql")); + + Assert.Contains("empty", ex.Message); + } + } +} diff --git a/MigrationOps.Core.Tests/ExtractChecksumFromScriptTests.cs b/MigrationOps.Core.Tests/ExtractChecksumFromScriptTests.cs new file mode 100644 index 0000000..b41b68b --- /dev/null +++ b/MigrationOps.Core.Tests/ExtractChecksumFromScriptTests.cs @@ -0,0 +1,55 @@ +using MigrationOps.Core.MigrationFramework.Services; + +namespace MigrationOps.Core.Tests +{ + public class ExtractChecksumFromScriptTests + { + private readonly MigrationService _service = TestMigrationService.Create(); + + [Fact] + public void ExtractsChecksumValue() + { + var script = "-- Tags: Db1\n-- Checksum: abc123\nSELECT 1;"; + + Assert.Equal("abc123", _service.ExtractChecksumFromScript(script)); + } + + [Fact] + public void TrimsWhitespaceAroundChecksum() + { + var script = "-- Checksum: abc123 \nSELECT 1;"; + + Assert.Equal("abc123", _service.ExtractChecksumFromScript(script)); + } + + [Fact] + public void HandlesWindowsLineEndings() + { + var script = "-- Tags: Db1\r\n-- Checksum: winval\r\nSELECT 1;"; + + Assert.Equal("winval", _service.ExtractChecksumFromScript(script)); + } + + [Fact] + public void HandlesUnixLineEndings() + { + var script = "-- Tags: Db1\n-- Checksum: unixval\nSELECT 1;"; + + Assert.Equal("unixval", _service.ExtractChecksumFromScript(script)); + } + + [Fact] + public void ThrowsWhenNoChecksumLinePresent() + { + var script = "-- Tags: Db1\nSELECT 1;"; + + Assert.Throws(() => _service.ExtractChecksumFromScript(script)); + } + + [Fact] + public void ThrowsForEmptyScript() + { + Assert.Throws(() => _service.ExtractChecksumFromScript("")); + } + } +} diff --git a/MigrationOps.Core.Tests/GetMigrationFileStatusesTests.cs b/MigrationOps.Core.Tests/GetMigrationFileStatusesTests.cs new file mode 100644 index 0000000..04757ea --- /dev/null +++ b/MigrationOps.Core.Tests/GetMigrationFileStatusesTests.cs @@ -0,0 +1,137 @@ +using MigrationOps.Core.MigrationFramework.Services; +using MigrationOps.Core.Models; + +namespace MigrationOps.Core.Tests +{ + public class GetMigrationFileStatusesTests + { + private readonly MigrationService _service = TestMigrationService.Create(); + + [Fact] + public void MarksFileAppliedWhenChecksumMatchesSuccessfulHistory() + { + using var dir = new TempDirectory(); + dir.WriteFile("20260101-001-Foo.sql", "-- Tags: Db1\n-- Checksum: abc\nSELECT 1;"); + + var history = new List + { + new() { MigrationName = "20260101-001-Foo.sql", Checksum = "abc", Success = true, AppliedOn = DateTime.UtcNow } + }; + + var status = Assert.Single(_service.GetMigrationFileStatuses(dir.Path, "Db1", history)); + + Assert.True(status.IsApplied); + Assert.False(status.HasDrift); + } + + [Fact] + public void MarksFileAsDriftedWhenFileChecksumDiffersFromLastSuccessfulApply() + { + using var dir = new TempDirectory(); + dir.WriteFile("20260101-001-Foo.sql", "-- Tags: Db1\n-- Checksum: new-checksum\nSELECT 1;"); + + var history = new List + { + new() { MigrationName = "20260101-001-Foo.sql", Checksum = "old-checksum", Success = true, AppliedOn = DateTime.UtcNow } + }; + + var status = Assert.Single(_service.GetMigrationFileStatuses(dir.Path, "Db1", history)); + + Assert.False(status.IsApplied); + Assert.True(status.HasDrift); + Assert.Equal("old-checksum", status.RecordedChecksum); + Assert.Equal("new-checksum", status.CurrentChecksum); + } + + [Fact] + public void MarksFileAsPendingWhenNotYetInHistory() + { + using var dir = new TempDirectory(); + dir.WriteFile("20260101-001-Foo.sql", "-- Tags: Db1\n-- Checksum: abc\nSELECT 1;"); + + var status = Assert.Single(_service.GetMigrationFileStatuses(dir.Path, "Db1", new List())); + + Assert.False(status.IsApplied); + Assert.False(status.HasDrift); + } + + [Fact] + public void IgnoresFailedHistoryRowsSoRetryIsNotMistakenForDrift() + { + using var dir = new TempDirectory(); + dir.WriteFile("20260101-001-Foo.sql", "-- Tags: Db1\n-- Checksum: abc\nSELECT 1;"); + + var history = new List + { + new() { MigrationName = "20260101-001-Foo.sql", Checksum = "abc", Success = false, AppliedOn = DateTime.UtcNow } + }; + + var status = Assert.Single(_service.GetMigrationFileStatuses(dir.Path, "Db1", history)); + + Assert.False(status.IsApplied); + Assert.False(status.HasDrift); + } + + [Fact] + public void UsesTheMostRecentSuccessfulChecksumWhenHistoryHasMultipleRows() + { + using var dir = new TempDirectory(); + dir.WriteFile("20260101-001-Foo.sql", "-- Tags: Db1\n-- Checksum: second\nSELECT 1;"); + + var history = new List + { + new() { MigrationName = "20260101-001-Foo.sql", Checksum = "first", Success = true, AppliedOn = DateTime.UtcNow.AddMinutes(-10) }, + new() { MigrationName = "20260101-001-Foo.sql", Checksum = "second", Success = true, AppliedOn = DateTime.UtcNow } + }; + + var status = Assert.Single(_service.GetMigrationFileStatuses(dir.Path, "Db1", history)); + + Assert.True(status.IsApplied); + Assert.False(status.HasDrift); + } + + [Fact] + public void SkipsFilesNotTaggedForTheRequestedDatabase() + { + using var dir = new TempDirectory(); + dir.WriteFile("20260101-001-Foo.sql", "-- Tags: Db2\n-- Checksum: abc\nSELECT 1;"); + + Assert.Empty(_service.GetMigrationFileStatuses(dir.Path, "Db1", new List())); + } + + [Fact] + public void ReportsChecksumMissingWhenFileHasNoChecksumHeaderYet() + { + using var dir = new TempDirectory(); + dir.WriteFile("20260101-001-Foo.sql", "-- Tags: Db1\nSELECT 1;"); + + var status = Assert.Single(_service.GetMigrationFileStatuses(dir.Path, "Db1", new List())); + + Assert.True(status.ChecksumMissing); + } + + [Fact] + public void ReportsValidationErrorWhenFileHasNoTagsHeader() + { + using var dir = new TempDirectory(); + dir.WriteFile("20260101-001-Foo.sql", "-- Checksum: abc\nSELECT 1;"); + + var status = Assert.Single(_service.GetMigrationFileStatuses(dir.Path, "Db1", new List())); + + Assert.NotNull(status.ValidationError); + } + + [Fact] + public void TaglessFileIsReportedRegardlessOfWhichDatabaseIsRequested() + { + using var dir = new TempDirectory(); + dir.WriteFile("20260101-001-Foo.sql", "-- Checksum: abc\nSELECT 1;"); + + var statusForDb1 = Assert.Single(_service.GetMigrationFileStatuses(dir.Path, "Db1", new List())); + var statusForDb2 = Assert.Single(_service.GetMigrationFileStatuses(dir.Path, "Db2", new List())); + + Assert.NotNull(statusForDb1.ValidationError); + Assert.NotNull(statusForDb2.ValidationError); + } + } +} diff --git a/MigrationOps.Core.Tests/MigrationOps.Core.Tests.csproj b/MigrationOps.Core.Tests/MigrationOps.Core.Tests.csproj new file mode 100644 index 0000000..da71d2b --- /dev/null +++ b/MigrationOps.Core.Tests/MigrationOps.Core.Tests.csproj @@ -0,0 +1,25 @@ + + + + net8.0 + enable + enable + false + + + + + + + + + + + + + + + + + + diff --git a/MigrationOps.Core.Tests/ParseTagsFromFileTests.cs b/MigrationOps.Core.Tests/ParseTagsFromFileTests.cs new file mode 100644 index 0000000..a8ddc96 --- /dev/null +++ b/MigrationOps.Core.Tests/ParseTagsFromFileTests.cs @@ -0,0 +1,55 @@ +using MigrationOps.Core.MigrationFramework.Services; + +namespace MigrationOps.Core.Tests +{ + public class ParseTagsFromFileTests + { + [Fact] + public void ParsesCommaSeparatedTags() + { + using var file = new TempFile("-- Tags: Db1, Db2\n-- Checksum: abc\nSELECT 1;"); + + var tags = MigrationService.ParseTagsFromFile(file.Path); + + Assert.Equal(new[] { "Db1", "Db2" }, tags); + } + + [Fact] + public void TrimsWhitespaceAroundTags() + { + using var file = new TempFile("-- Tags: Db1 , Db2 \nSELECT 1;"); + + var tags = MigrationService.ParseTagsFromFile(file.Path); + + Assert.Equal(new[] { "Db1", "Db2" }, tags); + } + + [Fact] + public void UsesFirstTagsLineWhenMultiplePresent() + { + using var file = new TempFile("-- Tags: Db1\n-- Tags: Db2\nSELECT 1;"); + + var tags = MigrationService.ParseTagsFromFile(file.Path); + + Assert.Equal(new[] { "Db1" }, tags); + } + + [Fact] + public void ThrowsWhenNoTagsCommentPresent() + { + using var file = new TempFile("-- Checksum: abc\nSELECT 1;"); + + var ex = Assert.Throws(() => MigrationService.ParseTagsFromFile(file.Path)); + + Assert.Contains(Path.GetFileName(file.Path), ex.Message); + } + + [Fact] + public void ThrowsForEmptyFile() + { + using var file = new TempFile(""); + + Assert.Throws(() => MigrationService.ParseTagsFromFile(file.Path)); + } + } +} diff --git a/MigrationOps.Core.Tests/ShouldApplyScriptTests.cs b/MigrationOps.Core.Tests/ShouldApplyScriptTests.cs new file mode 100644 index 0000000..5c61020 --- /dev/null +++ b/MigrationOps.Core.Tests/ShouldApplyScriptTests.cs @@ -0,0 +1,31 @@ +using MigrationOps.Core.MigrationFramework.Services; + +namespace MigrationOps.Core.Tests +{ + public class ShouldApplyScriptTests + { + [Fact] + public void MatchesExactTag() + { + Assert.True(MigrationService.ShouldApplyScript(new List { "Db1", "Db2" }, "Db1")); + } + + [Fact] + public void IsCaseInsensitive() + { + Assert.True(MigrationService.ShouldApplyScript(new List { "db1" }, "DB1")); + } + + [Fact] + public void ReturnsFalseWhenTagNotPresent() + { + Assert.False(MigrationService.ShouldApplyScript(new List { "Db2" }, "Db1")); + } + + [Fact] + public void ReturnsFalseForEmptyTagList() + { + Assert.False(MigrationService.ShouldApplyScript(new List(), "Db1")); + } + } +} diff --git a/MigrationOps.Core.Tests/TestHelpers.cs b/MigrationOps.Core.Tests/TestHelpers.cs new file mode 100644 index 0000000..81a3dc7 --- /dev/null +++ b/MigrationOps.Core.Tests/TestHelpers.cs @@ -0,0 +1,70 @@ +using MigrationOps.Core.MigrationFramework.Services; + +namespace MigrationOps.Core.Tests +{ + // MigrationService's parameterless constructor resolves Configurations/dbconfig.json + // relative to the current directory and calls IConfigurationBuilder.SetBasePath on it, + // which throws DirectoryNotFoundException if that folder doesn't exist (as it doesn't + // under the test project's bin output). Tests that just need *a* MigrationService + // instance to call an instance method on (and don't care about its configuration) + // should go through here instead of `new MigrationService()`. + internal static class TestMigrationService + { + public static MigrationService Create() + { + using var configFile = new TempFile("{}", ".json"); + return new MigrationService(configFile.Path); + } + } + + + // Writes content to a uniquely-named temp file so tests can exercise file-based parsing + // (ParseTagsFromFile, ExtractChecksumFromScript, etc.) without checking in .sql fixtures + // that the pre-commit hook would otherwise expect a "-- Tags:"/checksum header on. + internal sealed class TempFile : IDisposable + { + public string Path { get; } + + public TempFile(string content, string extension = ".sql") + { + Path = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"{Guid.NewGuid()}{extension}"); + File.WriteAllText(Path, content); + } + + public void Dispose() + { + if (File.Exists(Path)) + { + File.Delete(Path); + } + } + } + + // Same idea as TempFile but for tests (e.g. GetMigrationFileStatuses) that scan a whole + // migrations directory. + internal sealed class TempDirectory : IDisposable + { + public string Path { get; } + + public TempDirectory() + { + Path = System.IO.Path.Combine(System.IO.Path.GetTempPath(), Guid.NewGuid().ToString()); + Directory.CreateDirectory(Path); + } + + public string WriteFile(string fileName, string content) + { + var filePath = System.IO.Path.Combine(Path, fileName); + File.WriteAllText(filePath, content); + return filePath; + } + + public void Dispose() + { + if (Directory.Exists(Path)) + { + Directory.Delete(Path, recursive: true); + } + } + } +} diff --git a/MigrationOps.Core/MigrationFramework/Services/MigrationService.cs b/MigrationOps.Core/MigrationFramework/Services/MigrationService.cs index a53f853..9753eaf 100644 --- a/MigrationOps.Core/MigrationFramework/Services/MigrationService.cs +++ b/MigrationOps.Core/MigrationFramework/Services/MigrationService.cs @@ -642,7 +642,9 @@ public DryRunPlan BuildDryRunPlan(string scriptsRootDirectory, string migrations return plan; } - private static void AddPlanEntry(DryRunPlan plan, MigrationFileStatus status, ScriptKind kind, string database, string filePath, HashSet unresolvedReported) + // internal (not private) so MigrationOps.Core.Tests can exercise the classification + // logic directly, without needing a live database connection. + internal static void AddPlanEntry(DryRunPlan plan, MigrationFileStatus status, ScriptKind kind, string database, string filePath, HashSet unresolvedReported) { var unresolved = status.ValidationError != null && status.Tags.Count == 0; if (unresolved && !unresolvedReported.Add(status.FileName)) @@ -902,7 +904,8 @@ public string ExtractChecksumFromScript(string script) /// This enforces that the first executable statement (after the checksum/tags header /// comments) is a CREATE OR ALTER, rather than a plain CREATE that fails on redeploy. /// - private static void EnsureCreateOrAlterStatement(string script, string scriptName) + // internal (not private) so MigrationOps.Core.Tests can exercise it directly. + internal static void EnsureCreateOrAlterStatement(string script, string scriptName) { var lines = script.Split(new[] { "\r\n", "\n" }, StringSplitOptions.None); diff --git a/MigrationOps.Core/MigrationOps.Core.csproj b/MigrationOps.Core/MigrationOps.Core.csproj index c59d0a6..adfedb7 100644 --- a/MigrationOps.Core/MigrationOps.Core.csproj +++ b/MigrationOps.Core/MigrationOps.Core.csproj @@ -14,4 +14,10 @@ + + + + + diff --git a/MigrationOps.sln b/MigrationOps.sln index 8c6715b..a6eda41 100644 --- a/MigrationOps.sln +++ b/MigrationOps.sln @@ -9,24 +9,66 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MigrationOps.Core", "Migrat EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "MigrationOps.Dashboard", "MigrationOps.Dashboard\MigrationOps.Dashboard.csproj", "{E9ACDEDA-04C0-41DE-B638-54968D071E77}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "MigrationOps.Core.Tests", "MigrationOps.Core.Tests\MigrationOps.Core.Tests.csproj", "{693BDB4C-3494-4BBA-8E86-06CDE4E70F22}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU + Debug|x64 = Debug|x64 + Debug|x86 = Debug|x86 Release|Any CPU = Release|Any CPU + Release|x64 = Release|x64 + Release|x86 = Release|x86 EndGlobalSection GlobalSection(ProjectConfigurationPlatforms) = postSolution {467AE79D-B8B1-4EB9-BA75-97618F264645}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {467AE79D-B8B1-4EB9-BA75-97618F264645}.Debug|Any CPU.Build.0 = Debug|Any CPU + {467AE79D-B8B1-4EB9-BA75-97618F264645}.Debug|x64.ActiveCfg = Debug|Any CPU + {467AE79D-B8B1-4EB9-BA75-97618F264645}.Debug|x64.Build.0 = Debug|Any CPU + {467AE79D-B8B1-4EB9-BA75-97618F264645}.Debug|x86.ActiveCfg = Debug|Any CPU + {467AE79D-B8B1-4EB9-BA75-97618F264645}.Debug|x86.Build.0 = Debug|Any CPU {467AE79D-B8B1-4EB9-BA75-97618F264645}.Release|Any CPU.ActiveCfg = Release|Any CPU {467AE79D-B8B1-4EB9-BA75-97618F264645}.Release|Any CPU.Build.0 = Release|Any CPU + {467AE79D-B8B1-4EB9-BA75-97618F264645}.Release|x64.ActiveCfg = Release|Any CPU + {467AE79D-B8B1-4EB9-BA75-97618F264645}.Release|x64.Build.0 = Release|Any CPU + {467AE79D-B8B1-4EB9-BA75-97618F264645}.Release|x86.ActiveCfg = Release|Any CPU + {467AE79D-B8B1-4EB9-BA75-97618F264645}.Release|x86.Build.0 = Release|Any CPU {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Debug|Any CPU.Build.0 = Debug|Any CPU + {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Debug|x64.ActiveCfg = Debug|Any CPU + {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Debug|x64.Build.0 = Debug|Any CPU + {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Debug|x86.ActiveCfg = Debug|Any CPU + {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Debug|x86.Build.0 = Debug|Any CPU {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Release|Any CPU.ActiveCfg = Release|Any CPU {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Release|Any CPU.Build.0 = Release|Any CPU + {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Release|x64.ActiveCfg = Release|Any CPU + {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Release|x64.Build.0 = Release|Any CPU + {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Release|x86.ActiveCfg = Release|Any CPU + {15A2975F-2184-45B7-93C9-8FB0DDE38F7D}.Release|x86.Build.0 = Release|Any CPU {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Debug|Any CPU.Build.0 = Debug|Any CPU + {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Debug|x64.ActiveCfg = Debug|Any CPU + {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Debug|x64.Build.0 = Debug|Any CPU + {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Debug|x86.ActiveCfg = Debug|Any CPU + {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Debug|x86.Build.0 = Debug|Any CPU {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Release|Any CPU.ActiveCfg = Release|Any CPU {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Release|Any CPU.Build.0 = Release|Any CPU + {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Release|x64.ActiveCfg = Release|Any CPU + {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Release|x64.Build.0 = Release|Any CPU + {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Release|x86.ActiveCfg = Release|Any CPU + {E9ACDEDA-04C0-41DE-B638-54968D071E77}.Release|x86.Build.0 = Release|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Debug|Any CPU.Build.0 = Debug|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Debug|x64.ActiveCfg = Debug|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Debug|x64.Build.0 = Debug|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Debug|x86.ActiveCfg = Debug|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Debug|x86.Build.0 = Debug|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Release|Any CPU.ActiveCfg = Release|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Release|Any CPU.Build.0 = Release|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Release|x64.ActiveCfg = Release|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Release|x64.Build.0 = Release|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Release|x86.ActiveCfg = Release|Any CPU + {693BDB4C-3494-4BBA-8E86-06CDE4E70F22}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE From 17ffb3acc8a197cc690496e4c49aeed97edd68d8 Mon Sep 17 00:00:00 2001 From: cat fortman Date: Wed, 22 Jul 2026 22:01:39 -0300 Subject: [PATCH 2/2] Refuse to re-apply a migration edited after it was already applied 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 --- .../DetectEditedMigrationTests.cs | 41 +++++++++++++++++ .../AppConstants/SqlStatements.cs | 10 ++++ .../Services/MigrationService.cs | 46 +++++++++++++++++++ 3 files changed, 97 insertions(+) create mode 100644 MigrationOps.Core.Tests/DetectEditedMigrationTests.cs diff --git a/MigrationOps.Core.Tests/DetectEditedMigrationTests.cs b/MigrationOps.Core.Tests/DetectEditedMigrationTests.cs new file mode 100644 index 0000000..1fd5ba2 --- /dev/null +++ b/MigrationOps.Core.Tests/DetectEditedMigrationTests.cs @@ -0,0 +1,41 @@ +using MigrationOps.Core.MigrationFramework.Services; + +namespace MigrationOps.Core.Tests +{ + // Covers the guard added for #29: editing an already-applied migration must be refused by + // `apply`, not just flagged by dry-run. DetectEditedMigration is the pure decision behind + // that guard - given the checksum of the migration's last *successful* apply (or null if + // it has never successfully applied) and the checksum of the file on disk, it decides + // whether it's safe to proceed. + public class DetectEditedMigrationTests + { + [Fact] + public void AllowsAnUnchangedAlreadyAppliedMigration() + { + var error = MigrationService.DetectEditedMigration("20260101-001-Foo.sql", recordedChecksum: "abc", currentChecksum: "abc"); + + Assert.Null(error); + } + + [Fact] + public void RejectsAMigrationEditedAfterItWasApplied() + { + var error = MigrationService.DetectEditedMigration("20260101-001-Foo.sql", recordedChecksum: "old-checksum", currentChecksum: "new-checksum"); + + Assert.NotNull(error); + Assert.Contains("20260101-001-Foo.sql", error); + Assert.Contains("immutable", error); + } + + [Fact] + public void AllowsAMigrationThatHasNeverSuccessfullyApplied() + { + // recordedChecksum is null both for a brand-new file and for one whose only history + // row is a failed attempt (Success = 0) - GetLatestSuccessfulMigrationChecksum only + // looks at Success = 1 rows, so a failed-then-fixed migration is allowed to re-run. + var error = MigrationService.DetectEditedMigration("20260101-001-Foo.sql", recordedChecksum: null, currentChecksum: "whatever"); + + Assert.Null(error); + } + } +} diff --git a/MigrationOps.Core/MigrationFramework/AppConstants/SqlStatements.cs b/MigrationOps.Core/MigrationFramework/AppConstants/SqlStatements.cs index 3945351..eb54e76 100644 --- a/MigrationOps.Core/MigrationFramework/AppConstants/SqlStatements.cs +++ b/MigrationOps.Core/MigrationFramework/AppConstants/SqlStatements.cs @@ -39,6 +39,16 @@ SELECT COUNT(1) FROM __MigrationHistory WHERE MigrationName = @MigrationName AND Checksum = @Checksum AND Success = 1"; + // SQL for finding the checksum of the most recent *successful* apply of a migration, + // by name alone (ignoring checksum) - used to detect a migration file edited after + // it was already applied, since HasBeenApplied's (name, checksum) match can't tell + // "never applied" apart from "applied under a different checksum". + public static readonly string SelectLatestSuccessfulMigrationChecksum = @" + SELECT TOP 1 Checksum + FROM __MigrationHistory + WHERE MigrationName = @MigrationName AND Success = 1 + ORDER BY AppliedOn DESC"; + // SQL for reading the full migration history, most recent first. public static readonly string SelectMigrationHistory = @" SELECT MigrationId, MigrationName, AppliedOn, Checksum, Success, ErrorMessage, DurationMs diff --git a/MigrationOps.Core/MigrationFramework/Services/MigrationService.cs b/MigrationOps.Core/MigrationFramework/Services/MigrationService.cs index 9753eaf..76975f2 100644 --- a/MigrationOps.Core/MigrationFramework/Services/MigrationService.cs +++ b/MigrationOps.Core/MigrationFramework/Services/MigrationService.cs @@ -206,6 +206,21 @@ private bool ApplyScriptFile(string file, ScriptKind kind, bool deferSqlFailures EnsureHistoryTable(connectionString, kind); + // Migrations are immutable once applied: HasBeenApplied only matches on + // (name, checksum), so an edited file would otherwise look "never applied" + // and get re-executed. Object scripts are exempt - re-applying an edited + // proc/view is the designed workflow. + if (kind == ScriptKind.Migration) + { + var recordedChecksum = GetLatestSuccessfulMigrationChecksum(connectionString, scriptName); + var editedError = DetectEditedMigration(scriptName, recordedChecksum, checksum); + + if (editedError != null) + { + throw new InvalidOperationException(editedError); + } + } + if (HasBeenApplied(connectionString, scriptName, checksum, kind)) { Console.WriteLine($"Skipping {scriptName} as it has already been applied to {currentDb}"); @@ -335,6 +350,37 @@ private void EnsureHistoryTable(string connectionString, ScriptKind kind) } } + private string? GetLatestSuccessfulMigrationChecksum(string connectionString, string scriptName) + { + using (var connection = new SqlConnection(connectionString)) + { + connection.Open(); + var command = new SqlCommand(SqlStatements.SelectLatestSuccessfulMigrationChecksum, connection); + command.Parameters.AddWithValue("@MigrationName", scriptName); + var result = command.ExecuteScalar(); + return result == null || result == DBNull.Value ? null : (string)result; + } + } + + /// + /// Pure decision function behind the "editing an applied migration is forbidden" guard: + /// given the checksum of the migration's last successful apply (null if it has never + /// successfully applied) and the checksum of the file on disk now, returns a descriptive + /// error if they've diverged, or null if it's safe to proceed (never applied, or applied + /// with this exact checksum already). + /// + internal static string? DetectEditedMigration(string scriptName, string? recordedChecksum, string currentChecksum) + { + if (recordedChecksum == null || recordedChecksum == currentChecksum) + { + return null; + } + + return $"Migration '{scriptName}' was already applied with checksum {ShortChecksum(recordedChecksum)} " + + $"but the file now has checksum {ShortChecksum(currentChecksum)}. Migrations are immutable once " + + "applied - create a new migration instead of editing this one."; + } + private bool HasBeenApplied(string connectionString, string scriptName, string checksum, ScriptKind kind) { var sql = kind == ScriptKind.Migration ? SqlStatements.CheckMigrationApplied : SqlStatements.CheckScriptApplied;