From caf852f3863715e8f6f4fe422eadbebdeb0fac23 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Miller" Date: Thu, 30 Jul 2026 09:38:51 -0500 Subject: [PATCH] fix(#399): compare columns through the virtual Equals in MatchesForDelta MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression from #373. Routing every column through MatchesForDelta replaced a null comparer — under which ItemDelta fell back to the VIRTUAL Equals(object) — with a call to Equals(actual) that binds at compile time to the protected, non-virtual Equals(TableColumn) overload. Any subclass override of Equals(object) was silently bypassed. That override is a real extension seam: Marten's RevisionColumn uses it to declare that an integer mt_version tolerates a column an earlier release already migrated to bigint, instead of emitting a lossy narrowing cast (marten#4614 / #4742). With the override skipped, the column landed in Columns.Different, so the table was classified as needing an Update while the writer still emitted nothing for it — AssertDatabaseMatchesConfiguration threw with an empty change set. Bisected to 9.18.0; 9.17.0 is clean. Both the PostgreSQL and SQL Server implementations now compare through the virtual member. No behaviour change for columns that do not override Equals(object), since the base override delegates to Equals(TableColumn). Co-Authored-By: Claude Opus 5 (1M context) --- .../Tables/TableDeltaTests.cs | 66 +++++++++++++++++++ src/Weasel.Postgresql/Tables/TableColumn.cs | 15 ++++- src/Weasel.SqlServer/Tables/TableColumn.cs | 7 +- 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/Weasel.Postgresql.Tests/Tables/TableDeltaTests.cs b/src/Weasel.Postgresql.Tests/Tables/TableDeltaTests.cs index 2958d148..3e7b0b78 100644 --- a/src/Weasel.Postgresql.Tests/Tables/TableDeltaTests.cs +++ b/src/Weasel.Postgresql.Tests/Tables/TableDeltaTests.cs @@ -38,6 +38,72 @@ public void invalid_if_any_new_columns_cannot_be_modified() delta.Difference.ShouldBe(SchemaPatchDifference.Invalid); } + // weasel#399: consumers declare "this wider actual column is acceptable" by overriding the + // virtual Equals(object) on their own TableColumn subclass — that is the seam Marten uses so + // an integer mt_version tolerates a column an earlier release already migrated to bigint + // (marten#4614/#4742). Computed-column detection (weasel#373) routed every column through + // MatchesForDelta, where a bare Equals(actual) binds to the protected, NON-virtual + // Equals(TableColumn) overload and skips the override. The column then landed in + // Columns.Different, so the table was classified Update while the writer emitted nothing — + // AssertDatabaseMatchesConfiguration threw with an empty change set. + [Fact] + public void a_subclass_equality_override_decides_whether_a_column_matches() + { + var expected = new Table("people"); + expected.AddColumn("id").AsPrimaryKey(); + expected.AddColumn(new ToleratesWiderColumn("mt_version", "integer")); + + var actual = new Table("people"); + actual.AddColumn("id").AsPrimaryKey(); + actual.AddColumn(new TableColumn("mt_version", "bigint")); + + var delta = new TableDelta(expected, actual); + + delta.Columns.Matched.Select(x => x.Name).ShouldContain("mt_version"); + delta.Columns.Different.ShouldBeEmpty(); + delta.Difference.ShouldBe(SchemaPatchDifference.None); + } + + [Fact] + public void a_subclass_equality_override_that_rejects_still_reports_a_difference() + { + var expected = new Table("people"); + expected.AddColumn("id").AsPrimaryKey(); + expected.AddColumn(new ToleratesWiderColumn("mt_version", "integer")); + + // uuid is NOT the tolerated type, so the override falls through to the base comparison + var actual = new Table("people"); + actual.AddColumn("id").AsPrimaryKey(); + actual.AddColumn(new TableColumn("mt_version", "uuid")); + + var delta = new TableDelta(expected, actual); + + delta.Columns.Different.Select(x => x.Expected.Name).ShouldContain("mt_version"); + } + + // Mirrors Marten's RevisionColumn: an integer column that accepts an on-disk bigint rather + // than emitting a lossy narrowing cast. + public class ToleratesWiderColumn: TableColumn + { + public ToleratesWiderColumn(string name, string type): base(name, type) + { + } + + public override bool Equals(object? obj) + { + if (obj is TableColumn actual + && string.Equals(Name, actual.Name, StringComparison.OrdinalIgnoreCase) + && actual.RawType().Equals("bigint", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + return base.Equals(obj); + } + + public override int GetHashCode() => base.GetHashCode(); + } + public class CannotAddColumn: TableColumn { public CannotAddColumn(string name, string type): base(name, type) diff --git a/src/Weasel.Postgresql/Tables/TableColumn.cs b/src/Weasel.Postgresql/Tables/TableColumn.cs index a6a9b871..9242ba54 100644 --- a/src/Weasel.Postgresql/Tables/TableColumn.cs +++ b/src/Weasel.Postgresql/Tables/TableColumn.cs @@ -131,7 +131,7 @@ internal bool MatchesForDelta(TableColumn actual, bool detectDrift) { // the generation expression is the column's identity — defaults and // nullability drift don't apply to generated columns - return Equals(actual) && HasSameComputedDefinition(actual); + return equalsVirtual(actual) && HasSameComputedDefinition(actual); } if (actual.ComputedExpression.IsNotEmpty()) @@ -141,7 +141,7 @@ internal bool MatchesForDelta(TableColumn actual, bool detectDrift) return true; } - return Equals(actual) && (!detectDrift || HasSameDefaultAndNullability(actual)); + return equalsVirtual(actual) && (!detectDrift || HasSameDefaultAndNullability(actual)); } internal bool HasSameComputedDefinition(TableColumn actual) @@ -172,6 +172,17 @@ internal void WriteDriftCorrections(Table parent, TableColumn actual, TextWriter } } + // weasel#399: MatchesForDelta must compare through the VIRTUAL Equals(object). Calling + // Equals(actual) there binds at compile time to the protected, non-virtual + // Equals(TableColumn) overload below, which silently bypasses any subclass override of + // Equals(object) — the seam consumers use to declare a wider actual column acceptable + // (Marten's integer mt_version tolerating an existing bigint, marten#4614/#4742). Before + // computed-column detection (weasel#373) the no-drift path passed a null comparer and + // ItemDelta fell back to the virtual Equals, so such columns compared equal; routing every + // comparison through MatchesForDelta changed that silently and classified the table as + // needing an update while emitting no SQL — an empty-change-set assert failure. + private bool equalsVirtual(TableColumn actual) => Equals((object)actual); + protected bool Equals(TableColumn other) { // Name comparison is case-insensitive: an expected case-preserved column diff --git a/src/Weasel.SqlServer/Tables/TableColumn.cs b/src/Weasel.SqlServer/Tables/TableColumn.cs index 713fa1c8..a4aa526e 100644 --- a/src/Weasel.SqlServer/Tables/TableColumn.cs +++ b/src/Weasel.SqlServer/Tables/TableColumn.cs @@ -114,9 +114,14 @@ internal bool MatchesForDelta(TableColumn actual, bool detectDrift) return true; } - return Equals(actual) && (!detectDrift || HasSameDefaultAndNullability(actual)); + return equalsVirtual(actual) && (!detectDrift || HasSameDefaultAndNullability(actual)); } + // weasel#399: compare through the VIRTUAL Equals(object) so a subclass override + // participates. A bare Equals(actual) binds to the protected, non-virtual + // Equals(TableColumn) overload and silently bypasses it. See the PostgreSQL twin. + private bool equalsVirtual(TableColumn actual) => Equals((object)actual); + internal bool HasSameComputedDefinition(TableColumn actual) { return actual.ComputedExpression.IsNotEmpty() &&