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

…21990)

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.
Also making sure that identifying columns can be correctly propagated during pushdown and joining - if they are not we mark them as such (by removing identifying columns altogether), so that we can throw exception when these columns are actually needed.

Fixes #15873
Fixes #20184

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -337,6 +337,9 @@
   <data name="InsertDataOperationValuesCountMismatch" xml:space="preserve">
     <value>The number of values ({valuesCount}) doesn't match the number of columns ({columnsCount}) for the data insertion operation on '{table}'. Provide the same number of values and columns.</value>
   </data>
+  <data name="InsufficientInformationToIdentifyOuterElementOfCollectionJoin" xml:space="preserve">
+    <value>Not enough information to uniquely identify outer element in correlated collection scenario. This can happen when trying to correlate on keyless entity or when using 'Distinct' or 'GroupBy' operations without projecting all of the key columns.</value>
+  </data>
   <data name="InvalidCommandTimeout" xml:space="preserve">
     <value>The specified CommandTimeout value is not valid. It must be a positive number.</value>
   </data>
@@ -592,6 +595,9 @@
   <data name="MissingConcurrencyColumn" xml:space="preserve">
     <value>Entity type '{entityType}' doesn't contain a property mapped to the store-generated concurrency token column '{missingColumn}' that is used by another entity type sharing the table '{table}'. Add a store-generated property mapped to the same column to '{entityType}'. It can be in shadow state.</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>
   <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>
@@ -649,9 +655,6 @@
   <data name="PendingAmbientTransaction" xml:space="preserve">
     <value>This connection was used with an ambient transaction. The original ambient transaction needs to be completed before this connection can be used outside of it.</value>
   </data>
-  <data name="ProjectingCollectionOnKeylessEntityNotSupported" xml:space="preserve">
-    <value>Projecting collection correlated with keyless entity is not supported.</value>
-  </data>
   <data name="ProjectionMappingCountMismatch" xml:space="preserve">
     <value>Different projection mapping count in set operation.</value>
   </data>

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

@@ -1101,36 +1101,52 @@ 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))
                 {
                     _identifier.Add((outerColumn, identifier.Comparer));
                 }
                 else if (!IsDistinct
-                    && GroupBy.Count == 0)
+                    && GroupBy.Count == 0
+                    || (GroupBy.Contains(identifier.Column)))
                 {
                     outerColumn = subquery.GenerateOuterColumn(identifier.Column);
                     _identifier.Add((outerColumn, identifier.Comparer));
                 }
+                else
+                {
+                    // if we can't propagate any identifier - clear them all instead
+                    // when adding collection join we detect this and throw appropriate exception
+                    _identifier.Clear();
+                    break;
+                }
             }
 
             var childIdentifiers = _childIdentifiers.ToList();
             _childIdentifiers.Clear();
-            // TODO: See issue#15873
+
             foreach (var identifier in childIdentifiers)
             {
                 if (projectionMap.TryGetValue(identifier.Column, out var outerColumn))
                 {
                     _childIdentifiers.Add((outerColumn, identifier.Comparer));
                 }
                 else if (!IsDistinct
-                    && GroupBy.Count == 0)
+                    && GroupBy.Count == 0
+                    || (GroupBy.Contains(identifier.Column)))
                 {
                     outerColumn = subquery.GenerateOuterColumn(identifier.Column);
                     _childIdentifiers.Add((outerColumn, identifier.Comparer));
                 }
+                else
+                {
+                    // if we can't propagate any identifier - clear them all instead
+                    // when adding collection join we detect this and throw appropriate exception
+                    _childIdentifiers.Clear();
+                    break;
+                }
             }
 
             var pendingCollections = _pendingCollections.ToList();
@@ -1361,6 +1377,12 @@ public Expression ApplyCollectionJoin(
             var innerSelectExpression = _pendingCollections[collectionIndex];
             _pendingCollections[collectionIndex] = null;
 
+            if (_identifier.Count == 0
+                || innerSelectExpression._identifier.Count == 0)
+            {
+                throw new InvalidOperationException(RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin);
+            }
+
             if (splitQuery)
             {
                 var containsReferenceToOuter = new SelectExpressionCorrelationFindingExpressionVisitor(this)
@@ -1389,6 +1411,8 @@ public Expression ApplyCollectionJoin(
                 var parentIdentifier = GetIdentifierAccessor(identifierFromParent).Item1;
                 innerSelectExpression.ApplyProjection();
 
+                ValidateIdentifyingProjection(innerSelectExpression);
+
                 for (var i = 0; i < identifierFromParent.Count; i++)
                 {
                     AppendOrdering(new OrderingExpression(identifierFromParent[i].Column, ascending: true));
@@ -1483,16 +1507,14 @@ public Expression ApplyCollectionJoin(
             else
             {
                 var parentIdentifierList = _identifier.Except(_childIdentifiers).ToList();
-                if (parentIdentifierList.Count == 0)
-                {
-                    throw new InvalidOperationException(RelationalStrings.ProjectingCollectionOnKeylessEntityNotSupported);
-                }
 
                 var (parentIdentifier, parentIdentifierValueComparers) = GetIdentifierAccessor(parentIdentifierList);
                 var (outerIdentifier, outerIdentifierValueComparers) = GetIdentifierAccessor(_identifier);
                 var innerClientEval = innerSelectExpression.Projection.Count > 0;
                 innerSelectExpression.ApplyProjection();
 
+                ValidateIdentifyingProjection(innerSelectExpression);
+
                 if (collectionIndex == 0)
                 {
                     foreach (var identifier in parentIdentifierList)
@@ -1614,6 +1636,24 @@ public Expression ApplyCollectionJoin(
 
                 return result;
             }
+
+            static void ValidateIdentifyingProjection(SelectExpression selectExpression)
+            {
+                if (selectExpression.IsDistinct
+                    || selectExpression.GroupBy.Count > 0)
+                {
+                    var innerSelectProjectionExpressions = selectExpression._projection.Select(p => p.Expression).ToList();
+                    foreach (var innerSelectIdentifier in selectExpression._identifier)
+                    {
+                        if (!innerSelectProjectionExpressions.Contains(innerSelectIdentifier.Column)
+                            && (selectExpression.GroupBy.Count == 0
+                                || !selectExpression.GroupBy.Contains(innerSelectIdentifier.Column)))
+
+                            throw new InvalidOperationException(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery(
+                                innerSelectIdentifier.Column.Table.Alias + "." + innerSelectIdentifier.Column.Name));
+                    }
+                }
+            }
         }
 
         private sealed class EntityShaperNullableMarkingExpressionVisitor : ExpressionVisitor
@@ -2247,14 +2287,24 @@ private void AddJoin(
                     .Remap(joinPredicate);
             }
 
-            if (joinType == JoinType.LeftJoin
-                || joinType == JoinType.OuterApply)
+            if (_identifier.Count > 0
+                && innerSelectExpression._identifier.Count > 0)
             {
-                _identifier.AddRange(innerSelectExpression._identifier.Select(e => (e.Column.MakeNullable(), e.Comparer)));
+                if (joinType == JoinType.LeftJoin
+                    || joinType == JoinType.OuterApply)
+                {
+                    _identifier.AddRange(innerSelectExpression._identifier.Select(e => (e.Column.MakeNullable(), e.Comparer)));
+                }
+                else
+                {
+                    _identifier.AddRange(innerSelectExpression._identifier);
+                }
             }
-            else
+            else if (innerSelectExpression._identifier.Count == 0)
             {
-                _identifier.AddRange(innerSelectExpression._identifier);
+                // if the subquery that is joined to can't be uniquely identified
+                // then the entire join should also not be marked as non-identifiable
+                _identifier.Clear();
             }
 
             var innerTable = innerSelectExpression.Tables.Single();

test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs

@@ -1143,9 +1143,11 @@ public override Task Projecting_multiple_collection_with_same_constant_works(boo
         }
 
         [ConditionalTheory(Skip = "Issue#17246")]
-        public override Task Projecting_after_navigation_and_distinct_works_correctly(bool async)
+        public override async Task Projecting_after_navigation_and_distinct_throws(bool isAsync)
         {
-            return base.Projecting_after_navigation_and_distinct_works_correctly(async);
+            await base.Projecting_after_navigation_and_distinct_throws(isAsync);
+
+            AssertSql(" ");
         }
 
         public override Task Reverse_without_explicit_ordering_throws(bool async)

test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs

@@ -482,6 +482,14 @@ public override async Task Query_on_collection_entry_works_for_owned_collection(
             AssertSql(" ");
         }
 
+        [ConditionalTheory(Skip = "issue #17246")]
+        public override async Task Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(bool isAsync)
+        {
+            await base.Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(isAsync);
+
+            AssertSql(" ");
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

test/EFCore.InMemory.FunctionalTests/Query/OwnedQueryInMemoryTest.cs

@@ -1,7 +1,9 @@
 // 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.Threading.Tasks;
 using Microsoft.EntityFrameworkCore.TestUtilities;
+using Xunit;
 using Xunit.Abstractions;
 
 namespace Microsoft.EntityFrameworkCore.Query
@@ -14,6 +16,12 @@ public OwnedQueryInMemoryTest(OwnedQueryInMemoryFixture fixture, ITestOutputHelp
             //TestLoggerFactory.TestOutputHelper = testOutputHelper;
         }
 
+        [ConditionalTheory(Skip = "issue #19742")]
+        public override Task Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(bool async)
+        {
+            return base.Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(async);
+        }
+
         public class OwnedQueryInMemoryFixture : OwnedQueryFixtureBase
         {
             protected override ITestStoreFactory TestStoreFactory

test/EFCore.Relational.Specification.Tests/Query/GearsOfWarQueryRelationalTestBase.cs

@@ -1,7 +1,13 @@
 // 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 Microsoft.EntityFrameworkCore.TestUtilities;
+using Xunit;
 
 namespace Microsoft.EntityFrameworkCore.Query
 {
@@ -13,6 +19,41 @@ protected GearsOfWarQueryRelationalTestBase(TFixture fixture)
         {
         }
 
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
+        {
+            var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => 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())))).Message;
+
+            Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("w.Id"), message);
+        }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
+        {
+            var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => 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() })
+                    })))).Message;
+
+            Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("s.MissionId"), message);
+        }
+
         protected virtual bool CanExecuteQueryString
             => false;