From 42ffccb20538f4845496d49d9b78644e3395f032 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ricardo=20Gro=C3=9F?= Date: Sun, 25 Oct 2026 19:59:41 +0100 Subject: [PATCH] fix: keep a comparer out of SQL by what it compares, not its declared type (#452) #385 rewrote comparer.Compare(x, y) into RangeComparerCall.RunInMemory only when the declared parameter type was a registered range or range-set type. A comparer declared over IRange, the non-generic IComparer (Comparer.Create held as one) and a comparer of value sets declared some other type, so EF Core still read the call as the natural order: Where(cmp.Compare(r.Period, p) == 0) became WHERE "Period" = @p and selected one row where memory selects three. The rewriter now looks at the operands under their conversions to the declared type (and a sealed variant through its union), and RunInMemory gains an overload for the non-generic IComparer. The test comparers are captured locals: EF Core leaves Compare on a constant comparer untranslated in any case, so a static field would pass with or without the fix. Found-By: pre-release sweep of release/10.x (differential lens) Defect-Class: declared-type dispatch Co-Authored-By: Claude Opus 5.5 --- docs/efcore.md | 2 +- docs/migration.md | 4 +- .../Query/RangeComparerCall.cs | 14 +++-- .../ValueRangesQueryExpressionInterceptor.cs | 51 +++++++++++++++++-- .../LowerBoundComparerQueryTests.cs | 47 ++++++++++++++++- .../ClientSideOnlyTranslationTests.cs | 18 +++++++ test/oracles/claims.json | 2 +- 7 files changed, 125 insertions(+), 13 deletions(-) diff --git a/docs/efcore.md b/docs/efcore.md index 592beb0..133d066 100644 --- a/docs/efcore.md +++ b/docs/efcore.md @@ -926,7 +926,7 @@ translation, and in the final projection EF runs them in memory instead: | The value set indexer, `set[0]` | Subscripting reads the stored array, which is canonical only for rows this library wrote ([Rows written by other tools](#rows-written-by-other-tools)). PostgreSQL arrays also start at 1, and an index past the end is `NULL` where memory throws. | | `Values` on a value set | It is the loaded array itself. In a query, use the set's own members, such as `Contains` and `Count`. | | `Count` and the indexer on a `RangeSet` | Not translated. PostgreSQL has no function for either, and each would be a subquery over `unnest(x)`. | -| `Compare` on `RangeLowerBoundComparer`, `RangeSet.LowerBoundComparer` and any other `IComparer` of ranges or sets | EF Core reads every `Compare(x, y)` as the natural order of `x` and `y`, whatever the comparer. In SQL the lower-bound comparer would compare in the range order. For the range order in SQL, call `x.CompareTo(y)`. | +| `Compare` on `RangeLowerBoundComparer`, `RangeSet.LowerBoundComparer` and any other `IComparer` of ranges or sets | EF Core reads every `Compare(x, y)` as the natural order of `x` and `y`, whatever the comparer. In SQL the lower-bound comparer would compare in the range order. What counts is what you compare, not the type the comparer is declared over: an `IComparer>`, a non-generic `IComparer` and a comparer of value sets run in memory too. For the range order in SQL, call `x.CompareTo(y)`. | | `From` and `Parse` on any range or set | They build a value in .NET. Given values your code passes in, EF Core runs them before the query and sends the result as a parameter. | | `ToInstantRange`, `ToInterval`, `ToLocalDateRange`, `ToDateInterval` and `ToYearMonthRange` (NodaTime package) | They convert in .NET: between NodaTime's `Interval` or `DateInterval` and a range type, or between `LocalDateRange` and `YearMonthRange`. | diff --git a/docs/migration.md b/docs/migration.md index 77b5ac6..f27f940 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -994,7 +994,9 @@ in SQL in that order. Now it runs in memory only, like the other members under | `Select(r => comparer.Compare(r.Period, p))`, for a NULL `Period` | throws (`InvalidOperationException`) | `-1`, computed in memory | `Comparer.Default.Compare` runs in memory too. Once the comparer is a query parameter, a -query can't tell one comparer from another. +query can't tell one comparer from another. So do comparers declared over another type, when what they +compare is a range or a set: an `IComparer>`, a non-generic `IComparer` (such as +`Comparer.Create(…)` held as one) and an `IComparer`. - **Search your queries for `Compare(`.** Each call on a comparer of ranges or sets in a `Where` or an `OrderBy` now throws, so running the query finds it. diff --git a/src/CodoMetis.ValueRanges.EFCore.PostgreSQL/Query/RangeComparerCall.cs b/src/CodoMetis.ValueRanges.EFCore.PostgreSQL/Query/RangeComparerCall.cs index 48aa2a1..4289f93 100644 --- a/src/CodoMetis.ValueRanges.EFCore.PostgreSQL/Query/RangeComparerCall.cs +++ b/src/CodoMetis.ValueRanges.EFCore.PostgreSQL/Query/RangeComparerCall.cs @@ -3,7 +3,8 @@ namespace CodoMetis.ValueRanges.EntityFrameworkCore.PostgreSQL.Query; /// -/// The form an 's Compare over two ranges or two sets takes in a query +/// The form an 's or 's Compare +/// over a range, a range set or a value set takes in a query /// once has seen it: a call no translator knows, so /// it runs in memory or not at all (#385). /// @@ -17,7 +18,14 @@ namespace CodoMetis.ValueRanges.EntityFrameworkCore.PostgreSQL.Query; internal static class RangeComparerCall { /// 's own . - public static int RunInMemory(IComparer comparer, TRange x, TRange y) => comparer.Compare(x, y); + public static int RunInMemory(IComparer comparer, T x, T y) => comparer.Compare(x, y); - internal static readonly MethodInfo RunInMemoryMethod = typeof(RangeComparerCall).GetMethod(nameof(RunInMemory))!; + /// 's own . + public static int RunInMemory(System.Collections.IComparer comparer, object? x, object? y) => comparer.Compare(x, y); + + internal static readonly MethodInfo RunInMemoryMethod = + typeof(RangeComparerCall).GetMethods().Single(method => method is { Name: nameof(RunInMemory), IsGenericMethodDefinition: true }); + + internal static readonly MethodInfo RunInMemoryNonGenericMethod = + typeof(RangeComparerCall).GetMethod(nameof(RunInMemory), [typeof(System.Collections.IComparer), typeof(object), typeof(object)])!; } diff --git a/src/CodoMetis.ValueRanges.EFCore.PostgreSQL/Query/ValueRangesQueryExpressionInterceptor.cs b/src/CodoMetis.ValueRanges.EFCore.PostgreSQL/Query/ValueRangesQueryExpressionInterceptor.cs index d8a20c2..425fe40 100644 --- a/src/CodoMetis.ValueRanges.EFCore.PostgreSQL/Query/ValueRangesQueryExpressionInterceptor.cs +++ b/src/CodoMetis.ValueRanges.EFCore.PostgreSQL/Query/ValueRangesQueryExpressionInterceptor.cs @@ -1,4 +1,5 @@ using System.Linq.Expressions; +using System.Reflection; using CodoMetis.ValueRanges.EntityFrameworkCore.PostgreSQL.Internal; using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Diagnostics; @@ -362,8 +363,8 @@ protected override Expression VisitMethodCall(MethodCallExpression node) } /// - /// Turns comparer.Compare(x, y) over two ranges or two sets into - /// , which EF Core cannot translate: a filter or an + /// Turns comparer.Compare(x, y) over a range, a range set or a value set into + /// , which EF Core cannot translate: a filter or an /// ordering then fails translation, and a final projection runs the comparer in memory (#385). /// /// @@ -382,6 +383,14 @@ protected override Expression VisitMethodCall(MethodCallExpression node) /// range or set type is kept out of SQL, included: CompareTo /// is the call that runs in SQL in the range order. /// + /// + /// The operands decide, not the type Compare declares. A comparer declared over + /// , the non-generic (which + /// is, held as one) and a comparer of value sets each declare a type + /// does not hold, and each still compared in the range order in SQL + /// (#452). So a call is rewritten when either operand, under the conversions to the declared type, is a + /// range, a range set or a value set. + /// /// private sealed class RangeComparerRewriter : ExpressionVisitor { @@ -392,10 +401,42 @@ protected override Expression VisitMethodCall(MethodCallExpression node) && method.ReturnType == typeof(int) && method.GetParameters() is [{ ParameterType: var compared }, { ParameterType: var other }] && compared == other - && RangeTypeRegistry.TryGetByClrType(compared, out _, out _) - && typeof(IComparer<>).MakeGenericType(compared).IsAssignableFrom(comparer.Type) - ? Expression.Call(RangeComparerCall.RunInMemoryMethod.MakeGenericMethod(compared), Visit(comparer), Visit(x), Visit(y)) + && (IsRangeOrSet(x) || IsRangeOrSet(y)) + && RunInMemory(comparer.Type, compared) is { } runInMemory + ? Expression.Call(runInMemory, Visit(comparer), Visit(x), Visit(y)) : base.VisitMethodCall(node); + + private static MethodInfo? RunInMemory(Type comparerType, Type compared) + => typeof(IComparer<>).MakeGenericType(compared).IsAssignableFrom(comparerType) + ? RangeComparerCall.RunInMemoryMethod.MakeGenericMethod(compared) + : compared == typeof(object) && typeof(System.Collections.IComparer).IsAssignableFrom(comparerType) + ? RangeComparerCall.RunInMemoryNonGenericMethod + : null; + + // The operand and every conversion it passes through on its way to the declared parameter type: + // a column converted to IRange, object or IComparable is still a range. + private static bool IsRangeOrSet(Expression operand) + { + while (!IsRangeOrSetType(operand.Type)) + { + if (operand is not UnaryExpression { NodeType: ExpressionType.Convert or ExpressionType.ConvertChecked or ExpressionType.TypeAs } conversion) + return false; + + operand = conversion.Operand; + } + + return true; + } + + // A sealed variant is a range through its union. + private static bool IsRangeOrSetType(Type type) + { + for (var current = Nullable.GetUnderlyingType(type) ?? type; current is not null; current = current.BaseType) + if (RangeTypeRegistry.TryGetByClrType(current, out _, out _) || SetTypeRegistry.TryGetByClrType(current, out _)) + return true; + + return false; + } } private sealed class RangeSetOperatorRewriter : ExpressionVisitor diff --git a/test/CodoMetis.ValueRanges.EFCore.PostgreSQL.IntegrationTests/LowerBoundComparerQueryTests.cs b/test/CodoMetis.ValueRanges.EFCore.PostgreSQL.IntegrationTests/LowerBoundComparerQueryTests.cs index 4074498..395f4b3 100644 --- a/test/CodoMetis.ValueRanges.EFCore.PostgreSQL.IntegrationTests/LowerBoundComparerQueryTests.cs +++ b/test/CodoMetis.ValueRanges.EFCore.PostgreSQL.IntegrationTests/LowerBoundComparerQueryTests.cs @@ -1,4 +1,5 @@ using System.Linq.Expressions; +using CodoMetis.ValueRanges.Core; using CodoMetis.ValueRanges.EFCore.PostgreSQL.IntegrationTests.Composition; using Microsoft.EntityFrameworkCore; @@ -15,7 +16,10 @@ namespace CodoMetis.ValueRanges.EFCore.PostgreSQL.IntegrationTests; /// Where(… == 0) selected one row of the three. The comparer is reached directly, through /// , and through , which a query /// cannot tell apart once it is a parameter; goes the same way. A NULL -/// column sorts first, as the comparer says. +/// column sorts first, as the comparer says. The operands decide, not the type Compare declares: a +/// comparer over , the non-generic and a +/// comparer of value sets each still compared in the range order (#452). By count, {z} is equal to +/// rows 1 and 2, where SQL's = selected none. /// [TestClass] public sealed class LowerBoundComparerQueryTests @@ -33,6 +37,19 @@ public sealed class LowerBoundComparerQueryTests null ]; + private static readonly StringSet?[] Tags = [StringSet.From(["a"]), StringSet.From(["b"]), StringSet.From(["a", "b"]), StringSet.Empty, null]; + + private static readonly IComparer> OverInterface = + Comparer>.Create((x, y) => RangeLowerBoundComparer.Instance.Compare((DateRange?)x, (DateRange?)y)); + + private static readonly System.Collections.IComparer Created = Comparer.Create(RangeLowerBoundComparer.Instance.Compare); + + private static readonly System.Collections.IComparer OwnNonGeneric = new LowerBoundObjectComparer(); + + private static readonly IComparer ByCount = Comparer.Create((x, y) => (x?.Count ?? -1).CompareTo(y?.Count ?? -1)); + + private static readonly StringSet OneTag = StringSet.From(["z"]); + [TestMethod] public async Task Compare_InAProjection_AnswersAsTheComparerDoes() { @@ -60,6 +77,13 @@ async Task Agree(string label, Expression> compare) await Agree("IComparer.Compare", r => viaInterface.Compare(r.Period, p)); await Agree("RangeSet.LowerBoundComparer.Compare", r => RangeSet.LowerBoundComparer.Compare(p, r.Period)); await Agree("Comparer.Default.Compare", r => Comparer.Default.Compare(r.Period, p)); + + // As locals, so each comparer is a query parameter, as in the filter test. + var (overInterface, created, ownNonGeneric, byCount, oneTag) = (OverInterface, Created, OwnNonGeneric, ByCount, OneTag); + await Agree("IComparer>.Compare", r => overInterface.Compare(r.Period, p)); + await Agree("Comparer.Create as IComparer", r => created.Compare(r.Period, p)); + await Agree("own IComparer", r => ownNonGeneric.Compare(r.Period, p)); + await Agree("IComparer.Compare", r => byCount.Compare(r.Tags, oneTag)); } [TestMethod] @@ -71,12 +95,24 @@ public async Task Compare_InAFilterOrAnOrdering_FailsTranslation() IComparer viaInterface = RangeLowerBoundComparer.Instance; var p = P; + // As locals, so each comparer is a query parameter. EF Core leaves a Compare on a constant comparer + // (a static field) untranslated in any case, and the range order needs a parameter (#452). + var (overInterface, created, ownNonGeneric, byCount, oneTag) = (OverInterface, Created, OwnNonGeneric, ByCount, OneTag); + IQueryable[] queries = [ context.Rows.Where(r => RangeLowerBoundComparer.Instance.Compare(r.Period, p) == 0), context.Rows.Where(r => viaInterface.Compare(r.Period, p) == 0), context.Rows.Where(r => RangeSet.LowerBoundComparer.Compare(r.Period, p) <= 0), context.Rows.OrderBy(r => RangeLowerBoundComparer.Instance.Compare(r.Period, p)), + context.Rows.Where(r => overInterface.Compare(r.Period, p) == 0), + context.Rows.OrderBy(r => overInterface.Compare(r.Period, p)), + context.Rows.Where(r => created.Compare(r.Period, p) == 0), + context.Rows.OrderBy(r => created.Compare(r.Period, p)), + context.Rows.Where(r => ownNonGeneric.Compare(r.Period, p) == 0), + context.Rows.OrderBy(r => ownNonGeneric.Compare(r.Period, p)), + context.Rows.Where(r => byCount.Compare(r.Tags, oneTag) == 0), + context.Rows.OrderBy(r => byCount.Compare(r.Tags, oneTag)), ]; foreach (var query in queries) @@ -92,7 +128,7 @@ private static async Task Seed() var connectionString = await CompositionDatabase.Create("lower_bound_comparer"); await using var seed = new ComparerContext(connectionString); await seed.Database.EnsureCreatedAsync(); - seed.Rows.AddRange(Periods.Select((period, index) => new ComparerRow { Id = index + 1, Period = period })); + seed.Rows.AddRange(Periods.Select((period, index) => new ComparerRow { Id = index + 1, Period = period, Tags = Tags[index] })); await seed.SaveChangesAsync(); return connectionString; } @@ -102,6 +138,13 @@ public sealed class ComparerRow public int Id { get; set; } public DateRange? Period { get; set; } + + public StringSet? Tags { get; set; } + } + + private sealed class LowerBoundObjectComparer : System.Collections.IComparer + { + public int Compare(object? x, object? y) => RangeLowerBoundComparer.Instance.Compare((DateRange?)x, (DateRange?)y); } private sealed class ComparerContext(string connectionString) : DbContext diff --git a/test/CodoMetis.ValueRanges.EFCore.PostgreSQL.Tests/ClientSideOnlyTranslationTests.cs b/test/CodoMetis.ValueRanges.EFCore.PostgreSQL.Tests/ClientSideOnlyTranslationTests.cs index 2c38f52..77f7946 100644 --- a/test/CodoMetis.ValueRanges.EFCore.PostgreSQL.Tests/ClientSideOnlyTranslationTests.cs +++ b/test/CodoMetis.ValueRanges.EFCore.PostgreSQL.Tests/ClientSideOnlyTranslationTests.cs @@ -1,3 +1,4 @@ +using CodoMetis.ValueRanges.Core; using Microsoft.EntityFrameworkCore; namespace CodoMetis.ValueRanges.EFCore.PostgreSQL.Tests; @@ -34,8 +35,25 @@ public sealed class ClientSideOnlyTranslationTests ["RangeSet.LowerBoundComparer.Compare", (Func>)(db => db.Bookings.Where(b => RangeSet.LowerBoundComparer.Compare(b.Period, June) > 0))], ["IComparer.Compare", (Func>)(db => db.Bookings.Where(b => Comparer>.Default.Compare(b.BlockedDays, RangeSet.Empty) == 0))], ["OrderBy(RangeLowerBoundComparer.Compare)", (Func>)(db => db.Bookings.OrderBy(b => RangeLowerBoundComparer.Instance.Compare(b.Seats, Int32Range.Infinite)))], + // The operands decide, not the type Compare declares (#452). Each comparer is a captured local, so a + // query parameter: EF Core leaves Compare on a constant comparer untranslated in any case. + ["IComparer>.Compare", (Func>)(db => { var comparer = OverInterface; return db.Bookings.Where(b => comparer.Compare(b.Period, June) == 0); })], + ["OrderBy(IComparer>.Compare)", (Func>)(db => { var comparer = OverInterface; return db.Bookings.OrderBy(b => comparer.Compare(b.Period, June)); })], + ["IComparer.Compare", (Func>)(db => { var comparer = NonGeneric; return db.Bookings.Where(b => comparer.Compare(b.Period, June) == 0); })], + ["OrderBy(IComparer.Compare)", (Func>)(db => { var comparer = NonGeneric; return db.Bookings.OrderBy(b => comparer.Compare(b.Period, June)); })], + ["IComparer.Compare", (Func>)(db => { var comparer = OverValueSets; return db.Bookings.Where(b => comparer.Compare(b.Tags, OneTag) == 0); })], + ["OrderBy(IComparer.Compare)", (Func>)(db => { var comparer = OverValueSets; return db.Bookings.OrderBy(b => comparer.Compare(b.Tags, OneTag)); })], ]; + private static readonly IComparer> OverInterface = + Comparer>.Create((x, y) => RangeLowerBoundComparer.Instance.Compare((DateRange)x, (DateRange)y)); + + private static readonly System.Collections.IComparer NonGeneric = Comparer.Create(RangeLowerBoundComparer.Instance.Compare); + + private static readonly IComparer OverValueSets = Comparer.Create((x, y) => x.Count.CompareTo(y.Count)); + + private static readonly StringSet OneTag = StringSet.From(["z"]); + private static readonly DateRange June = DateRange.Month(2026, 6); [TestMethod] diff --git a/test/oracles/claims.json b/test/oracles/claims.json index 9c12361..d711a6b 100644 --- a/test/oracles/claims.json +++ b/test/oracles/claims.json @@ -583,7 +583,7 @@ "id": "E31-range-client-side-only-operations", "source": "docs/efcore.md#what-runs-where", "quote": "**Runs in memory only.** These compute the right answer in memory. In a predicate they fail translation, and in the final projection EF runs them in memory instead:", - "claim": "Length (on every range type but Int32Range and Int64Range), Values(), Values(step), Split, Clamp (on every range type but Int32Range, Int64Range and DecimalRange), Shift, Expand, ToRangeSet()/ToInt32Set(), the set indexer and Compare on RangeLowerBoundComparer, RangeSet.LowerBoundComparer and any IComparer of ranges or sets each throw an EF translation exception inside a Where (an OrderBy for the comparer) and evaluate on the client with the in-memory answer inside a Select.", + "claim": "Length (on every range type but Int32Range and Int64Range), Values(), Values(step), Split, Clamp (on every range type but Int32Range, Int64Range and DecimalRange), Shift, Expand, ToRangeSet()/ToInt32Set(), the set indexer and Compare on RangeLowerBoundComparer, RangeSet.LowerBoundComparer and any IComparer of ranges or sets (decided by the operands, so also a comparer declared over IRange, the non-generic IComparer and a comparer of value sets) each throw an EF translation exception inside a Where (an OrderBy for the comparer) and evaluate on the client with the in-memory answer inside a Select.", "oracle": "exception", "surfaces": [ "*Range.Length",