From 356b6775fede5d309639c963ff93fc64f38618ee Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Tue, 16 Jun 2026 14:22:07 +0100 Subject: [PATCH] Fix GroupBy().Select(entity) followed by a second projection (#31209, #26748) - Member-mode ReplaceProjection left stale _clientProjections, producing an invalid mixed projection state. - Made the member-mode overload clear _clientProjections, mirroring the index-mode overload. - Fixes member Select / second GroupBy composed on top of GroupBy(k).Select(g => g.First()). - Enables 4 previously-skipped tests and adds 4 regression tests; InMemory's separate limitation stays skipped. - Verified across ~27k query tests (SQLite + SqlServer) with zero regressions. A SelectExpression holds its projection as either a projection-member mapping or a client-projection (index) list, never both. The two ReplaceProjection overloads were asymmetric: the index-mode overload cleared both representations, but the member-mode overload cleared only the member mapping. So a member-mode Select following an index-mode Select left stale client projections behind, and ApplyProjection then took the index path over a member-based shaper and threw. This change makes the member-mode overload clear the client-projection list too, restoring the invariant. This is a partial follow-up to #38140. It covers the member-Select / second-GroupBy family on top of the "entity per group" idiom. InMemory has an independent limitation in the same area and remains skipped. ## The original issue #31209 (and the older, more general #26748) collect a large family of GroupBy translation failures, all sharing one shape: a GroupBy whose Select projects an entire entity per group (the "latest row per group" idiom), with a further operator composed on top: db.Set() .GroupBy(o => o.CustomerID) .Select(g => g.OrderByDescending(x => x.Date).First()) // one whole entity per group . // the trigger Reporters hit it through many surface forms - a second Select projecting a member, a final Where/OrderBy, a result-selector GroupBy, a second GroupBy, FirstOrDefault instead of First. The original repros ended in a second GroupBy (#31209) or in Count/Select/OrderBy over a composite (anonymous-type) key (#26748). The symptom was a confusing KeyNotFoundException: The given key 'EmptyProjectionMember' was not present in the dictionary, thrown deep in projection post-processing rather than at the offending operator, which is why the threads accumulated years of dead-end workarounds (FromSqlRaw round-trips, query-tag interceptors, ToList().AsQueryable()). #38140 fixed part of this by cloning the subquery SelectExpression in RelationalSqlTranslatingExpressionVisitor.BindProperty, which resolved the Where/OrderBy/ anonymous-type and Count cases. It explicitly left the member-Select-after case unsolved, and after that fix the remaining cases surfaced a different error, InvalidOperationException: Nullable object must have a value. ## What is actually being fixed Root cause is an invariant violation in SelectExpression. A projection is represented as exactly one of: - _projectionMapping - keyed by ProjectionMember (member mode), or - _clientProjections - an index list (client/index mode) GetProjection reads one or the other depending on whether the binding carries a ProjectionMember or an Index; the two are never meant to coexist. ReplaceProjection has two overloads, and they were asymmetric: - ReplaceProjection(IReadOnlyList) clears _projectionMapping AND _clientProjections - ReplaceProjection(IReadOnlyDictionary<...>) cleared ONLY _projectionMapping <-- bug Sequence that breaks: 1. .Select(g => g.First()) falls back to index-based binding -> populates _clientProjections. 2. .Select(r => r.EmployeeID) translates in member mode -> repopulates _projectionMapping via the Dictionary overload, but leaves _clientProjections stale and non-empty. 3. ApplyProjection sees _clientProjections.Count > 0, takes the index path, and the ClientProjectionRemappingExpressionVisitor walks the member-based shaper (Index == null) -> "Nullable object must have a value" (was EmptyProjectionMember before #38140). The fix makes the Dictionary overload also clear _clientProjections and _aliasForClientProjections, making it symmetric with the List overload and restoring the one-representation invariant. ## Challenges - Misleading symptom. The exception is thrown in post-processing (ApplyProjection), far from the Select that creates the bad state, and it had already mutated once (EmptyProjectionMember -> Nullable-must-have-a-value) across #38140, so the stack trace points away from the cause. - Two plausible fix sites. The contributor on #38140 pursued the upstream NavigationExpandingExpressionVisitor.ProcessSelect path (making the GroupBy Select return a pending selector) and found it cascades into broad regressions requiring a redesign of pending- selector handling. The actual defect is downstream, in the relational SelectExpression, where a one-line symmetry restoration suffices. - Shared, hot code path. ReplaceProjection(IReadOnlyDictionary) is called from ~7 sites across the relational, SqlServer, and Sqlite providers. The change had to be proven safe everywhere, not just for the reported query. ## Outcome Production change: clear _clientProjections / _aliasForClientProjections in the member-mode ReplaceProjection overload. The clear is a no-op whenever the list is already empty (the normal case), so the only behavioral change is in the previously-invalid mixed state. Tests: - Un-skipped GroupBy_Select_Entire_Entity_Select and _Where_Select. - Added regression tests: GroupBy_Select_Entire_Entity_FirstOrDefault_Where, GroupBy_ResultSelector_Entire_Entity_Where (result-selector entry point), GroupBy_Select_Entire_Entity_GroupBy (the literal #31209 repro), GroupBy_Select_Entire_Entity_composite_key_Select (the literal #26748 repro). - Added SqlServer AssertSql baselines for all of the above (clean correlated-subquery / nested GROUP BY SQL). - InMemory keeps these skipped - it has a separate limitation in the same area. ## Review and validation Three independent reviewers analysed the change for blast radius, cross-shape consequences, and coverage gaps; their proposed experiments were then run. - Blast radius: all 7 callers of the member-mode overload traced. Six provably have an empty _clientProjections at call time (fresh select, just-cleared, or member-mode by construction), so the clear is a literal no-op; the seventh is the bug's target. - One genuine behavioral delta - the set-operation guard (SetOperationsNotAllowedAfterClientEvaluation). Verified empirically: * GroupBy(k).Select(g => g.First()).Select(o => o.OrderID).Union(...) previously threw spuriously and now returns correct results - corrective. * GroupBy(k).Select(g => g.First()).Union(...) (no intervening member Select) still correctly throws - the fix does not suppress the legitimate guard. - Structural invariant confirmed: when a kept value depends on a collection / correlated subquery / JSON node / entity held in _clientProjections, member-mode translation bails to the index fallback (List overload), so the Dictionary clear never fires destructively. No silent drop of collections, includes, JSON, or split-query metadata. - Pre-existing limitation, not a regression: projecting a navigation off the picked entity (GroupBy(k).Select(g => g.First()).Select(r => r.Customer.City)) fails as "ProjectionBindingExpression: 0 could not be translated". Confirmed by reverting the fix that it fails identically with and without this change. Separate, deeper issue. - #26748 cross-check: its Count and OrderBy repros were already resolved by #38140; its remaining member-Select repro (composite/anonymous key) failed with the exact "Nullable object must have a value" error and is resolved by this change. Verified by removing the fix and re-running. - Regression suites: ~12.1k SQLite and ~14.9k SqlServer query tests pass with zero failures; the NorthwindGroupBy class is green on SQLite, SqlServer, and InMemory. Fixes #31209. Fixes #26748. Follow-up to #38140. --- .../Query/SqlExpressions/SelectExpression.cs | 8 ++ .../NorthwindGroupByQueryInMemoryTest.cs | 24 ++++ .../Query/NorthwindGroupByQueryTestBase.cs | 39 +++++- .../NorthwindGroupByQuerySqlServerTest.cs | 117 +++++++++++++++++- 4 files changed, 184 insertions(+), 4 deletions(-) diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index 6718c5d3619..22724d93a0b 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -1548,6 +1548,14 @@ static IReadOnlyList GetMappedKeyProperties(IKey key) public void ReplaceProjection(IReadOnlyDictionary projectionMapping) { _projectionMapping.Clear(); + + // A projection is represented either as a projection-member mapping or as a client-projection list, never both + // (see GetProjection). When switching to a member mapping, any client projections left over from a previous + // projection (e.g. a prior Select that fell back to index-based binding) must be cleared, otherwise the stale + // client projections take precedence in ApplyProjection and the member-based shaper fails to remap. See #31209. + _clientProjections.Clear(); + _aliasForClientProjections.Clear(); + foreach (var (projectionMember, expression) in projectionMapping) { Check.DebugAssert( diff --git a/test/EFCore.InMemory.FunctionalTests/Query/NorthwindGroupByQueryInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/Query/NorthwindGroupByQueryInMemoryTest.cs index f9a1f8ff177..18be4614e3d 100644 --- a/test/EFCore.InMemory.FunctionalTests/Query/NorthwindGroupByQueryInMemoryTest.cs +++ b/test/EFCore.InMemory.FunctionalTests/Query/NorthwindGroupByQueryInMemoryTest.cs @@ -84,4 +84,28 @@ public override Task GroupBy_Select_Entire_Entity_Order(bool async) [Theory(Skip = "Issue#31209")] public override Task GroupBy_Select_Entire_Entity_Where(bool async) => base.GroupBy_Select_Entire_Entity_Where(async); + + [Theory(Skip = "Issue#31209")] + public override Task GroupBy_Select_Entire_Entity_Select(bool async) + => base.GroupBy_Select_Entire_Entity_Select(async); + + [Theory(Skip = "Issue#31209")] + public override Task GroupBy_Select_Entire_Entity_Where_Select(bool async) + => base.GroupBy_Select_Entire_Entity_Where_Select(async); + + [Theory(Skip = "Issue#31209")] + public override Task GroupBy_Select_Entire_Entity_FirstOrDefault_Where(bool async) + => base.GroupBy_Select_Entire_Entity_FirstOrDefault_Where(async); + + [Theory(Skip = "Issue#31209")] + public override Task GroupBy_ResultSelector_Entire_Entity_Where(bool async) + => base.GroupBy_ResultSelector_Entire_Entity_Where(async); + + [Theory(Skip = "Issue#31209")] + public override Task GroupBy_Select_Entire_Entity_GroupBy(bool async) + => base.GroupBy_Select_Entire_Entity_GroupBy(async); + + [Theory(Skip = "Issue#31209")] + public override Task GroupBy_Select_Entire_Entity_composite_key_Select(bool async) + => base.GroupBy_Select_Entire_Entity_composite_key_Select(async); } diff --git a/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs index 559dc517251..4bfdbaf25ff 100644 --- a/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs @@ -1957,7 +1957,7 @@ public virtual Task GroupBy_Select_Entire_Entity_Where(bool async) // #31209 .Select(a => a.First()) .Where(x => x.EmployeeID == 6u)); - [Theory(Skip = "Issue#31209"), MemberData(nameof(IsAsyncData))] + [Theory, MemberData(nameof(IsAsyncData))] public virtual Task GroupBy_Select_Entire_Entity_Where_Select(bool async) // #31209 => AssertQuery( async, @@ -1966,7 +1966,7 @@ public virtual Task GroupBy_Select_Entire_Entity_Where_Select(bool async) // #31 .Where(x => x.OrderID > 10) .Select(r => r.EmployeeID)); - [Theory(Skip = "Issue#31209"), MemberData(nameof(IsAsyncData))] + [Theory, MemberData(nameof(IsAsyncData))] public virtual Task GroupBy_Select_Entire_Entity_Select(bool async) // #31209 => AssertQuery( async, @@ -1995,6 +1995,41 @@ public virtual Task GroupBy_Select_Anonymous_Type_With_Entire_Entity(bool async) Item = g.OrderByDescending(x => x.OrderDate).FirstOrDefault(), }).Where(x => x.Item != null)); + [Theory, MemberData(nameof(IsAsyncData))] + public virtual Task GroupBy_Select_Entire_Entity_FirstOrDefault_Where(bool async) // #31209 + => AssertQuery( + async, + ss => ss.Set().GroupBy(o => o.CustomerID) + .Select(g => g.OrderByDescending(o => o.OrderDate).FirstOrDefault()) + .Where(r => r.EmployeeID == 5u)); + + [Theory, MemberData(nameof(IsAsyncData))] + public virtual Task GroupBy_ResultSelector_Entire_Entity_Where(bool async) // #31209 + => AssertQuery( + async, + ss => ss.Set().GroupBy( + o => o.CustomerID, + (k, es) => es.OrderByDescending(o => o.OrderDate).First()) + .Where(r => r.EmployeeID == 6u)); + + [Theory, MemberData(nameof(IsAsyncData))] + public virtual Task GroupBy_Select_Entire_Entity_GroupBy(bool async) // #31209 + => AssertQuery( + async, + ss => ss.Set().GroupBy(o => new { o.CustomerID, o.EmployeeID }) + .Select(g => g.First()) + .GroupBy(o => o.EmployeeID) + .Select(g2 => new { g2.Key, Count = g2.Count() }), + elementSorter: e => e.Key); + + [Theory, MemberData(nameof(IsAsyncData))] + public virtual Task GroupBy_Select_Entire_Entity_composite_key_Select(bool async) // #26748 + => AssertQuery( + async, + ss => ss.Set().GroupBy(o => new { o.CustomerID, o.EmployeeID }) + .Select(g => g.First()) + .Select(p => p.OrderID)); + [Theory, MemberData(nameof(IsAsyncData))] public virtual Task GroupBy_aggregate_join_with_group_result(bool async) => AssertQuery( diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs index 3f37ac5a1c8..459367a3c05 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs @@ -3085,14 +3085,127 @@ public override async Task GroupBy_Select_Entire_Entity_Where_Select(bool async) { await base.GroupBy_Select_Entire_Entity_Where_Select(async); - AssertSql(); + AssertSql( +""" +SELECT ( + SELECT TOP(1) [o1].[EmployeeID] + FROM [Orders] AS [o1] + WHERE [o].[OrderID] = [o1].[OrderID]) +FROM [Orders] AS [o] +GROUP BY [o].[OrderID] +HAVING ( + SELECT TOP(1) [o0].[OrderID] + FROM [Orders] AS [o0] + WHERE [o].[OrderID] = [o0].[OrderID]) > 10 +"""); } public override async Task GroupBy_Select_Entire_Entity_Select(bool async) { await base.GroupBy_Select_Entire_Entity_Select(async); - AssertSql(); + AssertSql( +""" +SELECT ( + SELECT TOP(1) [o0].[EmployeeID] + FROM [Orders] AS [o0] + WHERE [o].[OrderID] = [o0].[OrderID]) +FROM [Orders] AS [o] +GROUP BY [o].[OrderID] +"""); + } + + public override async Task GroupBy_Select_Entire_Entity_FirstOrDefault_Where(bool async) + { + await base.GroupBy_Select_Entire_Entity_FirstOrDefault_Where(async); + + AssertSql( +""" +SELECT [o4].[OrderID], [o4].[CustomerID], [o4].[EmployeeID], [o4].[OrderDate] +FROM ( + SELECT [o].[CustomerID] + FROM [Orders] AS [o] + GROUP BY [o].[CustomerID] + HAVING ( + SELECT TOP(1) [o1].[EmployeeID] + FROM [Orders] AS [o1] + WHERE [o].[CustomerID] = [o1].[CustomerID] OR ([o].[CustomerID] IS NULL AND [o1].[CustomerID] IS NULL) + ORDER BY [o1].[OrderDate] DESC) = 5 +) AS [o2] +LEFT JOIN ( + SELECT [o3].[OrderID], [o3].[CustomerID], [o3].[EmployeeID], [o3].[OrderDate] + FROM ( + SELECT [o0].[OrderID], [o0].[CustomerID], [o0].[EmployeeID], [o0].[OrderDate], ROW_NUMBER() OVER(PARTITION BY [o0].[CustomerID] ORDER BY [o0].[OrderDate] DESC) AS [row] + FROM [Orders] AS [o0] + ) AS [o3] + WHERE [o3].[row] <= 1 +) AS [o4] ON [o2].[CustomerID] = [o4].[CustomerID] +"""); + } + + public override async Task GroupBy_ResultSelector_Entire_Entity_Where(bool async) + { + await base.GroupBy_ResultSelector_Entire_Entity_Where(async); + + AssertSql( +""" +SELECT [o4].[OrderID], [o4].[CustomerID], [o4].[EmployeeID], [o4].[OrderDate] +FROM ( + SELECT [o].[CustomerID] + FROM [Orders] AS [o] + GROUP BY [o].[CustomerID] + HAVING ( + SELECT TOP(1) [o1].[EmployeeID] + FROM [Orders] AS [o1] + WHERE [o].[CustomerID] = [o1].[CustomerID] OR ([o].[CustomerID] IS NULL AND [o1].[CustomerID] IS NULL) + ORDER BY [o1].[OrderDate] DESC) = 6 +) AS [o2] +LEFT JOIN ( + SELECT [o3].[OrderID], [o3].[CustomerID], [o3].[EmployeeID], [o3].[OrderDate] + FROM ( + SELECT [o0].[OrderID], [o0].[CustomerID], [o0].[EmployeeID], [o0].[OrderDate], ROW_NUMBER() OVER(PARTITION BY [o0].[CustomerID] ORDER BY [o0].[OrderDate] DESC) AS [row] + FROM [Orders] AS [o0] + ) AS [o3] + WHERE [o3].[row] <= 1 +) AS [o4] ON [o2].[CustomerID] = [o4].[CustomerID] +"""); + } + + public override async Task GroupBy_Select_Entire_Entity_GroupBy(bool async) + { + await base.GroupBy_Select_Entire_Entity_GroupBy(async); + + AssertSql( +""" +SELECT [o2].[Key], COUNT(*) AS [Count] +FROM ( + SELECT ( + SELECT TOP(1) [o1].[EmployeeID] + FROM [Orders] AS [o1] + WHERE ([o0].[CustomerID] = [o1].[CustomerID] OR ([o0].[CustomerID] IS NULL AND [o1].[CustomerID] IS NULL)) AND ([o0].[EmployeeID] = [o1].[EmployeeID] OR ([o0].[EmployeeID] IS NULL AND [o1].[EmployeeID] IS NULL))) AS [Key] + FROM ( + SELECT [o].[CustomerID], [o].[EmployeeID] + FROM [Orders] AS [o] + GROUP BY [o].[CustomerID], [o].[EmployeeID] + ) AS [o0] +) AS [o2] +GROUP BY [o2].[Key] +"""); + } + + public override async Task GroupBy_Select_Entire_Entity_composite_key_Select(bool async) + { + await base.GroupBy_Select_Entire_Entity_composite_key_Select(async); + + AssertSql( +""" +SELECT ( + SELECT TOP(1) [o0].[OrderID] + FROM [Orders] AS [o0] + WHERE ([o].[CustomerID] = [o0].[CustomerID] OR ([o].[CustomerID] IS NULL AND [o0].[CustomerID] IS NULL)) AND ([o].[EmployeeID] = [o0].[EmployeeID] OR ([o].[EmployeeID] IS NULL AND [o0].[EmployeeID] IS NULL))) +FROM [Orders] AS [o] +GROUP BY [o].[CustomerID], [o].[EmployeeID] +"""); } public override async Task GroupBy_Select_Entire_Entity_Order(bool async)