From 7e87508ac535d851a5b1105df7fc65f9cca88328 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Sat, 27 Jun 2026 17:00:31 +0100 Subject: [PATCH] Make the #30915 nullability marker survive a subsequent join MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #26873 Builds on #30915 (a synthesized marker column that materializes a left-joined non-entity projection as `null` on a no-match). #30915 covered a single left join; this makes the marker survive a *subsequent* join. - **Root cause:** a second `AddJoin` remaps the outer shaper and rebuilds the `New`/`MemberInit` node a prior join recorded as a marker key, orphaning the reference-keyed marker — so the gate never fires and the no-match row throws *"Nullable object must have a value"*. - **Fix:** capture the nodes rebuilt by the outer-shaper remap (`RebuiltNodes`) and re-key each affected marker onto its new node with a rebound binding. Localized to `SelectExpression` / `ProjectionMemberRemappingExpressionVisitor`. - **Fail-safe, no regressions:** gated on `_nonEntityNullabilityMarkers is not null`, a pure no-op for queries without a #30915 marker. If a marker's projection member was pruned, the re-key is skipped and behavior degrades to the prior throw — never an incorrect result or a new crash. - **Generalizes:** the re-key composes across multiple sequential joins and through nested anonymous wrappers. - **Tests:** flipped `Second_join_after_then_whole_object` and `Two_left_joined_nonentity_objects_second_marker_orphaned` to assert correct results; added three-sequential-joins (re-key composition) and nested-wrapper characterization tests. SQLite + SQL Server, zero baseline drift. Sub-case of the umbrella #22517. Remaining follow-ups — GroupBy-after (#28119), plain-inner/no-pushdown, value-type/`Nullable`, set-operations, server-side null checks, RightJoin/outer-nullable, InMemory parity — stay deferred and remain pinned by characterization tests. --- .../SqlExpressions/SelectExpression.Helper.cs | 28 ++++ .../Query/SqlExpressions/SelectExpression.cs | 31 +++- ...HocMiscellaneousQueryRelationalTestBase.cs | 141 ++++++++++++++++-- .../AdHocMiscellaneousQuerySqlServerTest.cs | 77 ++++++++++ .../AdHocMiscellaneousQuerySqliteTest.cs | 77 ++++++++++ 5 files changed, 344 insertions(+), 10 deletions(-) diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs index c7432403fa1..b0b5767699f 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs @@ -54,6 +54,12 @@ private sealed class ProjectionMemberRemappingExpressionVisitor( Dictionary projectionMemberMappings) : ExpressionVisitor { + // #30915: tracks New/MemberInit nodes rebuilt by this visitor (old instance → new instance). Used by the caller + // to re-key any _nonEntityNullabilityMarkers entries whose key was one of the rebuilt nodes, so that a previously- + // recorded marker key does not go stale when an outer-shaper remap creates a fresh node instance. + public Dictionary RebuiltNodes { get; } + = new Dictionary(ReferenceEqualityComparer.Instance); + protected override Expression VisitExtension(Expression expression) { if (expression is ProjectionBindingExpression projectionBindingExpression) @@ -70,6 +76,28 @@ protected override Expression VisitExtension(Expression expression) return base.VisitExtension(expression); } + + protected override Expression VisitNew(NewExpression node) + { + var visited = (NewExpression)base.VisitNew(node); + if (!ReferenceEquals(visited, node)) + { + RebuiltNodes[node] = visited; + } + + return visited; + } + + protected override Expression VisitMemberInit(MemberInitExpression node) + { + var visited = (MemberInitExpression)base.VisitMemberInit(node); + if (!ReferenceEquals(visited, node)) + { + RebuiltNodes[node] = visited; + } + + return visited; + } } private sealed class ProjectionMemberToIndexConvertingExpressionVisitor( diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index 00f99d953f8..7dc0320abc9 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -3054,7 +3054,36 @@ private Expression AddJoin( projectionMapping[remappedProjectionMember] = MakeNullable(expression, outerNullable); } - outerShaper = new ProjectionMemberRemappingExpressionVisitor(this, mapping).Visit(outerShaper); + var outerRemapper = new ProjectionMemberRemappingExpressionVisitor(this, mapping); + outerShaper = outerRemapper.Visit(outerShaper); + + // #30915: the outer remap above rebuilds any New/MemberInit node whose projection bindings changed; a + // node previously recorded as a nullability-marker key is now a stale instance. Re-key each such marker + // onto its rebuilt node, re-binding the marker value through the same remap so it still resolves. + // Only this (outer client-eval == false) branch is covered; the other AddJoin branches that remap the + // outer shaper are intentionally not re-keyed (no reachable repro). If one is ever hit with a live prior + // marker, the fail-safe holds: the gate simply does not fire and behavior falls back to the prior throw + // rather than producing an incorrect result. Tracked with the #30915 follow-ups. + if (_nonEntityNullabilityMarkers is not null) + { + foreach (var oldNode in _nonEntityNullabilityMarkers.Keys.ToList()) + { + // Re-key only when the key node was rebuilt AND the marker binding still resolves through this + // remap. The marker column is not referenced by the shaper tree, so its projection member could in + // principle have been pruned from _projectionMapping while the key node survived; in that case + // skip the re-key rather than letting outerRemapper.Visit hit the throwing indexer + // (ProjectionMemberRemappingExpressionVisitor.VisitExtension). Skipping preserves the fail-safe: + // the gate does not fire and behavior degrades to the prior throw, never a KeyNotFoundException. + if (outerRemapper.RebuiltNodes.TryGetValue(oldNode, out var newNode) + && _nonEntityNullabilityMarkers[oldNode] is ProjectionBindingExpression { ProjectionMember: { } markerMember } + && mapping.ContainsKey(markerMember)) + { + var reboundBinding = outerRemapper.Visit(_nonEntityNullabilityMarkers[oldNode]); + RemapNonEntityNullabilityMarker(oldNode, newNode, reboundBinding); + } + } + } + mapping.Clear(); foreach (var (projectionMember, expression) in innerSelect._projectionMapping) diff --git a/test/EFCore.Relational.Specification.Tests/Query/AdHocMiscellaneousQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/AdHocMiscellaneousQueryRelationalTestBase.cs index 514f1cb7f32..16f0c4817fd 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/AdHocMiscellaneousQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/AdHocMiscellaneousQueryRelationalTestBase.cs @@ -738,9 +738,25 @@ from countInfo in g.DefaultIfEmpty() .Join(context.Statuses, e => e.PickupStatusId, s2 => s2.PickupStatusId, (e, s2) => new { s2.PickupStatusId, e.countInfo }) .OrderBy(e => e.PickupStatusId); - var ex = await Assert.ThrowsAsync(() => query.ToListAsync()); - Assert.Contains("Nullable object must have a value", ex.Message); - // #30915 TODO: currently throws on base; flip to assert results if/when fixed. + var result = await query.ToListAsync(); + + Assert.Equal(3, result.Count); + + // status 1 -> matched, Count 2 + Assert.Equal(1, result[0].PickupStatusId); + Assert.NotNull(result[0].countInfo); + Assert.Equal(1, result[0].countInfo.pickupStatusId); + Assert.Equal(2, result[0].countInfo.Count); + + // status 2 -> no match on the left join; whole non-entity object is null even after the second join + Assert.Equal(2, result[1].PickupStatusId); + Assert.Null(result[1].countInfo); + + // status 3 -> matched, Count 1 + Assert.Equal(3, result[2].PickupStatusId); + Assert.NotNull(result[2].countInfo); + Assert.Equal(3, result[2].countInfo.pickupStatusId); + Assert.Equal(1, result[2].countInfo.Count); } [Fact] // plain inner with no aggregate -> no pushdown @@ -1211,12 +1227,119 @@ public virtual async Task Two_left_joined_nonentity_objects_second_marker_orphan .LeftJoin(categories, e => e.PickupStatusId, c => c.pickupStatusId, (e, second) => new { e.PickupStatusId, e.first, second }) .OrderBy(e => e.PickupStatusId); - // Two sequential non-entity nullable joins: the FIRST object's marker must pass through the - // SECOND join's outer-shaper remap. It currently does not (Blocker-1). - var ex = await Assert.ThrowsAsync(() => query.ToListAsync()); - Assert.Contains("Nullable object must have a value", ex.Message); - // #30915 TODO: Blocker-1 (graph-anchored keying); when fixed, flip to assert results and - // verify two distinct marker aliases. + var result = await query.ToListAsync(); + + Assert.Equal(3, result.Count); + + // status 1 -> both joins matched, Count 2 + Assert.Equal(1, result[0].PickupStatusId); + Assert.NotNull(result[0].first); + Assert.Equal(1, result[0].first.pickupStatusId); + Assert.Equal(2, result[0].first.Count); + Assert.NotNull(result[0].second); + Assert.Equal(1, result[0].second.pickupStatusId); + Assert.Equal(2, result[0].second.Count); + + // status 2 -> neither join matched; both whole non-entity objects are null. + // The first object's marker passes through the second join's outer-shaper remap, + // and the second object carries its own marker — two distinct marker aliases in SQL. + Assert.Equal(2, result[1].PickupStatusId); + Assert.Null(result[1].first); + Assert.Null(result[1].second); + + // status 3 -> both joins matched, Count 1 + Assert.Equal(3, result[2].PickupStatusId); + Assert.NotNull(result[2].first); + Assert.Equal(3, result[2].first.pickupStatusId); + Assert.Equal(1, result[2].first.Count); + Assert.NotNull(result[2].second); + Assert.Equal(3, result[2].second.pickupStatusId); + Assert.Equal(1, result[2].second.Count); + } + + [Fact] + public virtual async Task Three_sequential_joins_marker_survives_two_remaps() + { + // The marker, recorded on the non-entity inner-shaper node from the first DefaultIfEmpty, must + // survive TWO subsequent AddJoin outer-shaper remaps (re-key composes): one when the second + // .Join rebuilds the outer shaper and again when the third .Join does the same. + var contextFactory = await InitializeNonSharedTest(seed: Seed30915); + using var context = contextFactory.CreateDbContext(); + + var categories = context.Requests + .GroupBy(r => r.PickupStatusId, (k, els) => new { pickupStatusId = k, Count = els.Count() }); + + var query = (from s in context.Statuses + join c in categories on s.PickupStatusId equals c.pickupStatusId into g + from countInfo in g.DefaultIfEmpty() + select new { s.PickupStatusId, countInfo }) + .Join(context.Statuses, e => e.PickupStatusId, s2 => s2.PickupStatusId, (e, s2) => new { s2.PickupStatusId, e.countInfo }) + .Join(context.Statuses, e => e.PickupStatusId, s3 => s3.PickupStatusId, (e, s3) => new { s3.PickupStatusId, e.countInfo }) + .OrderBy(e => e.PickupStatusId); + + var result = await query.ToListAsync(); + + Assert.Equal(3, result.Count); + + // status 1 -> matched, Count 2 + Assert.Equal(1, result[0].PickupStatusId); + Assert.NotNull(result[0].countInfo); + Assert.Equal(1, result[0].countInfo.pickupStatusId); + Assert.Equal(2, result[0].countInfo.Count); + + // status 2 -> no match on the left join; whole non-entity object is null even after TWO subsequent joins + Assert.Equal(2, result[1].PickupStatusId); + Assert.Null(result[1].countInfo); + + // status 3 -> matched, Count 1 + Assert.Equal(3, result[2].PickupStatusId); + Assert.NotNull(result[2].countInfo); + Assert.Equal(3, result[2].countInfo.pickupStatusId); + Assert.Equal(1, result[2].countInfo.Count); + } + + [Fact] + public virtual async Task Marker_object_nested_in_outer_wrapper_across_second_join() + { + // The marker-carrying countInfo is nested inside an outer anonymous wrapper object constructed + // by the second .Join. RebuiltNodes must capture the nested rebuild so the marker still gates + // countInfo correctly after the join rebuilds the wrapping node. + var contextFactory = await InitializeNonSharedTest(seed: Seed30915); + using var context = contextFactory.CreateDbContext(); + + var categories = context.Requests + .GroupBy(r => r.PickupStatusId, (k, els) => new { pickupStatusId = k, Count = els.Count() }); + + var query = (from s in context.Statuses + join c in categories on s.PickupStatusId equals c.pickupStatusId into g + from countInfo in g.DefaultIfEmpty() + select new { s.PickupStatusId, countInfo }) + .Join(context.Statuses, e => e.PickupStatusId, s2 => s2.PickupStatusId, (e, s2) => new { s2.PickupStatusId, wrapper = new { e.countInfo } }) + .OrderBy(e => e.PickupStatusId); + + var result = await query.ToListAsync(); + + Assert.Equal(3, result.Count); + + // status 1 -> matched, wrapper is always non-null (constructed on match side), countInfo inside is non-null + Assert.Equal(1, result[0].PickupStatusId); + Assert.NotNull(result[0].wrapper); + Assert.NotNull(result[0].wrapper.countInfo); + Assert.Equal(1, result[0].wrapper.countInfo.pickupStatusId); + Assert.Equal(2, result[0].wrapper.countInfo.Count); + + // status 2 -> no match on the left join; wrapper itself is non-null (it is always constructed), + // but wrapper.countInfo is null because the marker gates the inner non-entity object to null + Assert.Equal(2, result[1].PickupStatusId); + Assert.NotNull(result[1].wrapper); + Assert.Null(result[1].wrapper.countInfo); + + // status 3 -> matched, wrapper is non-null, countInfo inside is non-null + Assert.Equal(3, result[2].PickupStatusId); + Assert.NotNull(result[2].wrapper); + Assert.NotNull(result[2].wrapper.countInfo); + Assert.Equal(3, result[2].wrapper.countInfo.pickupStatusId); + Assert.Equal(1, result[2].wrapper.countInfo.Count); } #endregion diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs index 938c822160c..ee4c895ceaf 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs @@ -3123,5 +3123,82 @@ ORDER BY [s0].[PickupStatusId] """); } + public override async Task Second_join_after_then_whole_object() + { + await base.Second_join_after_then_whole_object(); + + AssertSql( + """ +SELECT [s0].[PickupStatusId], [r0].[pickupStatusId], [r0].[Count], [r0].[marker] +FROM [Statuses] AS [s] +LEFT JOIN ( + SELECT [r].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker] + FROM [Requests] AS [r] + GROUP BY [r].[PickupStatusId] +) AS [r0] ON [s].[PickupStatusId] = [r0].[pickupStatusId] +INNER JOIN [Statuses] AS [s0] ON [s].[PickupStatusId] = [s0].[PickupStatusId] +ORDER BY [s0].[PickupStatusId] +"""); + } + + public override async Task Two_left_joined_nonentity_objects_second_marker_orphaned() + { + await base.Two_left_joined_nonentity_objects_second_marker_orphaned(); + + AssertSql( + """ +SELECT [s].[PickupStatusId], [r0].[pickupStatusId], [r0].[Count], [r0].[marker], [r2].[pickupStatusId], [r2].[Count], [r2].[marker] +FROM [Statuses] AS [s] +LEFT JOIN ( + SELECT [r].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker] + FROM [Requests] AS [r] + GROUP BY [r].[PickupStatusId] +) AS [r0] ON [s].[PickupStatusId] = [r0].[pickupStatusId] +LEFT JOIN ( + SELECT [r1].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker] + FROM [Requests] AS [r1] + GROUP BY [r1].[PickupStatusId] +) AS [r2] ON [s].[PickupStatusId] = [r2].[pickupStatusId] +ORDER BY [s].[PickupStatusId] +"""); + } + + public override async Task Three_sequential_joins_marker_survives_two_remaps() + { + await base.Three_sequential_joins_marker_survives_two_remaps(); + + AssertSql( + """ +SELECT [s1].[PickupStatusId], [r0].[pickupStatusId], [r0].[Count], [r0].[marker] +FROM [Statuses] AS [s] +LEFT JOIN ( + SELECT [r].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker] + FROM [Requests] AS [r] + GROUP BY [r].[PickupStatusId] +) AS [r0] ON [s].[PickupStatusId] = [r0].[pickupStatusId] +INNER JOIN [Statuses] AS [s0] ON [s].[PickupStatusId] = [s0].[PickupStatusId] +INNER JOIN [Statuses] AS [s1] ON [s0].[PickupStatusId] = [s1].[PickupStatusId] +ORDER BY [s1].[PickupStatusId] +"""); + } + + public override async Task Marker_object_nested_in_outer_wrapper_across_second_join() + { + await base.Marker_object_nested_in_outer_wrapper_across_second_join(); + + AssertSql( + """ +SELECT [s0].[PickupStatusId], [r0].[pickupStatusId], [r0].[Count], [r0].[marker] +FROM [Statuses] AS [s] +LEFT JOIN ( + SELECT [r].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker] + FROM [Requests] AS [r] + GROUP BY [r].[PickupStatusId] +) AS [r0] ON [s].[PickupStatusId] = [r0].[pickupStatusId] +INNER JOIN [Statuses] AS [s0] ON [s].[PickupStatusId] = [s0].[PickupStatusId] +ORDER BY [s0].[PickupStatusId] +"""); + } + #endregion } diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/AdHocMiscellaneousQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/AdHocMiscellaneousQuerySqliteTest.cs index 6b087811dde..0be8968567c 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/AdHocMiscellaneousQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/AdHocMiscellaneousQuerySqliteTest.cs @@ -686,5 +686,82 @@ ORDER BY "s0"."PickupStatusId" """); } + public override async Task Second_join_after_then_whole_object() + { + await base.Second_join_after_then_whole_object(); + + AssertSql( + """ +SELECT "s0"."PickupStatusId", "r0"."pickupStatusId", "r0"."Count", "r0"."marker" +FROM "Statuses" AS "s" +LEFT JOIN ( + SELECT "r"."PickupStatusId" AS "pickupStatusId", COUNT(*) AS "Count", 1 AS "marker" + FROM "Requests" AS "r" + GROUP BY "r"."PickupStatusId" +) AS "r0" ON "s"."PickupStatusId" = "r0"."pickupStatusId" +INNER JOIN "Statuses" AS "s0" ON "s"."PickupStatusId" = "s0"."PickupStatusId" +ORDER BY "s0"."PickupStatusId" +"""); + } + + public override async Task Two_left_joined_nonentity_objects_second_marker_orphaned() + { + await base.Two_left_joined_nonentity_objects_second_marker_orphaned(); + + AssertSql( + """ +SELECT "s"."PickupStatusId", "r0"."pickupStatusId", "r0"."Count", "r0"."marker", "r2"."pickupStatusId", "r2"."Count", "r2"."marker" +FROM "Statuses" AS "s" +LEFT JOIN ( + SELECT "r"."PickupStatusId" AS "pickupStatusId", COUNT(*) AS "Count", 1 AS "marker" + FROM "Requests" AS "r" + GROUP BY "r"."PickupStatusId" +) AS "r0" ON "s"."PickupStatusId" = "r0"."pickupStatusId" +LEFT JOIN ( + SELECT "r1"."PickupStatusId" AS "pickupStatusId", COUNT(*) AS "Count", 1 AS "marker" + FROM "Requests" AS "r1" + GROUP BY "r1"."PickupStatusId" +) AS "r2" ON "s"."PickupStatusId" = "r2"."pickupStatusId" +ORDER BY "s"."PickupStatusId" +"""); + } + + public override async Task Three_sequential_joins_marker_survives_two_remaps() + { + await base.Three_sequential_joins_marker_survives_two_remaps(); + + AssertSql( + """ +SELECT "s1"."PickupStatusId", "r0"."pickupStatusId", "r0"."Count", "r0"."marker" +FROM "Statuses" AS "s" +LEFT JOIN ( + SELECT "r"."PickupStatusId" AS "pickupStatusId", COUNT(*) AS "Count", 1 AS "marker" + FROM "Requests" AS "r" + GROUP BY "r"."PickupStatusId" +) AS "r0" ON "s"."PickupStatusId" = "r0"."pickupStatusId" +INNER JOIN "Statuses" AS "s0" ON "s"."PickupStatusId" = "s0"."PickupStatusId" +INNER JOIN "Statuses" AS "s1" ON "s0"."PickupStatusId" = "s1"."PickupStatusId" +ORDER BY "s1"."PickupStatusId" +"""); + } + + public override async Task Marker_object_nested_in_outer_wrapper_across_second_join() + { + await base.Marker_object_nested_in_outer_wrapper_across_second_join(); + + AssertSql( + """ +SELECT "s0"."PickupStatusId", "r0"."pickupStatusId", "r0"."Count", "r0"."marker" +FROM "Statuses" AS "s" +LEFT JOIN ( + SELECT "r"."PickupStatusId" AS "pickupStatusId", COUNT(*) AS "Count", 1 AS "marker" + FROM "Requests" AS "r" + GROUP BY "r"."PickupStatusId" +) AS "r0" ON "s"."PickupStatusId" = "r0"."pickupStatusId" +INNER JOIN "Statuses" AS "s0" ON "s"."PickupStatusId" = "s0"."PickupStatusId" +ORDER BY "s0"."PickupStatusId" +"""); + } + #endregion }