Fix to #15873 - Query: Identifying columns in the case of distinct

Added validation step for AddCollectionJoin which checks that if subquery contains Distinct or GroupBy, the projection contains all identifying columns needed to correctly bucket the results during materialization.

Fixes #15873

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -735,4 +735,7 @@
   <data name="MissingOrderingInSqlExpression" xml:space="preserve">
     <value>Reverse could not be translated to the server because there is no ordering on the server side.</value>
   </data>
+  <data name="MissingIdentifyingProjectionInDistinctGroupBySubquery" xml:space="preserve">
+    <value>Collection subquery that uses 'Distinct' or 'Group By' operations must project key columns of all of it's tables. Missing column: {column}. Either add column(s) to the projection or rewrite query to not use 'GroupBy'/'Distinct' operation.</value>
+  </data>
 </root>

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

@@ -10,6 +10,7 @@
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.ChangeTracking;
 using Microsoft.EntityFrameworkCore.Diagnostics;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Metadata.Internal;
 using Microsoft.EntityFrameworkCore.Utilities;
@@ -1066,7 +1067,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
 
             var identifiers = _identifier.ToList();
             _identifier.Clear();
-            // TODO: See issue#15873
+
             foreach (var identifier in identifiers)
             {
                 if (projectionMap.TryGetValue(identifier.Column, out var outerColumn))
@@ -1083,7 +1084,7 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
 
             var childIdentifiers = _childIdentifiers.ToList();
             _childIdentifiers.Clear();
-            // TODO: See issue#15873
+
             foreach (var identifier in childIdentifiers)
             {
                 if (projectionMap.TryGetValue(identifier.Column, out var outerColumn))
@@ -1427,6 +1428,20 @@ public Expression ApplyCollectionJoin(
                 var innerClientEval = innerSelectExpression.Projection.Count > 0;
                 innerSelectExpression.ApplyProjection();
 
+                if (innerSelectExpression.IsDistinct
+                    || innerSelectExpression.GroupBy.Count > 0)
+                {
+                    var innerSelectProjectionExpressions = innerSelectExpression._projection.Select(p => p.Expression).ToList();
+                    foreach (var innerSelectIdentifier in innerSelectExpression._identifier)
+                    {
+                        if (!innerSelectProjectionExpressions.Contains(innerSelectIdentifier.Column))
+                        {
+                            throw new InvalidOperationException(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery(
+                                innerSelectIdentifier.Column.Table.Alias + "." + innerSelectIdentifier.Column.Name));
+                        }
+                    }
+                }
+
                 if (collectionIndex == 0)
                 {
                     foreach (var identifier in parentIdentifierList)

test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs

@@ -5517,5 +5517,27 @@ public virtual Task GroupBy_aggregate_where_required_relationship_2(bool async)
                         .Select(g => new { g.Key, Max = g.Max(e => e.Id) })
                         .Where(x => x.Max < 2 || x.Max > 2));
         }
+
+        //[ConditionalTheory]
+        //[MemberData(nameof(IsAsyncData))]
+        //public virtual Task Fubarson(bool async)
+        //{
+
+        //    using var ctx = CreateContext();
+        //    var q = ctx.LevelThree.Select(x => new { x, collection = x.OneToMany_Optional3.SelectMany(xx => xx.OneToMany_Required_Inverse4.OneToMany_Optional_Inverse3.Name).ToList() }).ToList();
+
+
+        //    return AssertQuery(
+        //        async,
+        //        ss => ss.Set<Level2>()
+        //            .Select(l2 => l2.OneToMany_Optional2
+        //                .SelectMany(l3 => l3.OneToMany_Optional3
+        //                    .Select(l4 => l4.OneToMany_Optional_Inverse4))
+        //                .Distinct()
+        //                .OrderBy(l3 => l3.Id)
+        //                .ToList()),
+        //        assertOrder: true,
+        //        elementAsserter: (e, a) => AssertCollection(e, a));
+        //}
     }
 }

test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs

@@ -7764,6 +7764,35 @@ from w in ss.Set<Weapon>().Where(x => x.OwnerFullName != g.FullName).OrderBy(x =
                 });
         }
 
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
+        {
+            return AssertQuery(
+                async,
+                ss => ss.Set<Gear>()
+                    .OrderBy(g => g.Nickname)
+                    .Select(g => g.Weapons.SelectMany(x => x.Owner.AssignedCity.BornGears)
+                    .Select(x => (bool?)x.HasSoulPatch).Distinct().ToList()));
+        }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
+        {
+            return AssertQuery(
+                async,
+                ss => ss.Set<Mission>()
+                    .Select(m => new
+                    {
+                        m.Id,
+                        grouping = m.ParticipatingSquads
+                            .Select(ps => ps.SquadId)
+                            .GroupBy(s => s)
+                            .Select(g => new { g.Key, Count = g.Count() })
+                    }));
+        }
+
         protected GearsOfWarContext CreateContext() => Fixture.CreateContext();
 
         protected virtual void ClearLog()

test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs

@@ -1,8 +1,10 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Linq;
 using System.Threading.Tasks;
+using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.TestModels.GearsOfWarModel;
 using Xunit;
 using Xunit.Abstractions;
@@ -7136,6 +7138,22 @@ ORDER BY [w].[Id]
 ORDER BY [g].[Nickname], [t].[Id]");
         }
 
+        public override async Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
+        {
+            var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => base.Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(async))).Message;
+
+            Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("w.Id"), message);
+        }
+
+        public override async Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
+        {
+            var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => base.Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(async))).Message;
+
+            Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("s.MissionId"), message);
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
     }

test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs

@@ -5260,6 +5260,33 @@ FROM [Orders] AS [o]
 FROM [Customers] AS [c]");
         }
 
+        public override async Task SelectMany_correlated_subquery_hard(bool async)
+        {
+            await base.SelectMany_correlated_subquery_hard(async);
+
+            AssertSql(
+                @"@__p_0='91'
+
+SELECT [t0].[City] AS [c1], [t1].[City], [t1].[City0] AS [c1]
+FROM (
+    SELECT DISTINCT [t].[City]
+    FROM (
+        SELECT TOP(@__p_0) [c].[City], [c].[CustomerID]
+        FROM [Customers] AS [c]
+    ) AS [t]
+) AS [t0]
+CROSS APPLY (
+    SELECT TOP(9) [e].[City], [t0].[City] AS [City0], [e].[EmployeeID]
+    FROM [Employees] AS [e]
+    WHERE ([t0].[City] = [e].[City]) OR ([t0].[City] IS NULL AND [e].[City] IS NULL)
+) AS [t1]
+CROSS APPLY (
+    SELECT TOP(9) [t0].[City], [e0].[EmployeeID]
+    FROM [Employees] AS [e0]
+    WHERE ([t1].[City] = [e0].[City]) OR ([t1].[City] IS NULL AND [e0].[City] IS NULL)
+) AS [t2]");
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs

@@ -1,8 +1,10 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Linq;
 using System.Threading.Tasks;
+using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.TestModels.GearsOfWarModel;
 using Xunit;
 using Xunit.Abstractions;
@@ -8569,6 +8571,22 @@ LEFT JOIN [Officers] AS [o] ON ([g].[Nickname] = [o].[Nickname]) AND ([g].[Squad
 ORDER BY [c].[Location]");
         }
 
+        public override async Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
+        {
+            var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => base.Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(async))).Message;
+
+            Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("w.Id"), message);
+        }
+
+        public override async Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
+        {
+            var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => base.Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(async))).Message;
+
+            Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("s.MissionId"), message);
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
     }