Fix to #29638 - GroupBy generates invalid SQL when using custom database function

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -1094,6 +1094,9 @@
   <data name="StoredProcedureUnmapped" xml:space="preserve">
     <value>The entity type '{entityType}' was configured to use some stored procedures and is not mapped to any table. An entity type that isn't mapped to a table must be mapped to insert, update and delete stored procedures.</value>
   </data>
+  <data name="TableExpressionBaseWithoutCloningLogic" xml:space="preserve">
+    <value>Expression type '{expressionType}' does not implement proper cloning logic. Every expression derived from '{tableExpressionBase}' must implement '{clonableTableExpressionBase}' interface or have it's cloning logic added to the '{cloningExpressionVisitor}' inside '{selectExpression}'.</value>
+  </data>
   <data name="TableNotMappedEntityType" xml:space="preserve">
     <value>The entity type '{entityType}' is not mapped to the store object '{table}'.</value>
   </data>

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

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Diagnostics.CodeAnalysis;
+using Microsoft.EntityFrameworkCore.Metadata.Internal;
 
 namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 
@@ -1008,7 +1009,60 @@ private sealed class CloningExpressionVisitor : ExpressionVisitor
                 return newTpcTable;
             }
 
-            return expression is IClonableTableExpressionBase cloneable ? cloneable.Clone() : base.Visit(expression);
+            if (expression is TableValuedFunctionExpression tableValuedFunctionExpression)
+            {
+                var newArguments = new SqlExpression[tableValuedFunctionExpression.Arguments.Count];
+                for (var i = 0; i < newArguments.Length; i++)
+                {
+                    newArguments[i] = (SqlExpression)Visit(tableValuedFunctionExpression.Arguments[i]);
+                }
+
+                var newTableValuedFunctionExpression = new TableValuedFunctionExpression(
+                    tableValuedFunctionExpression.StoreFunction,
+                    newArguments)
+                {
+                    Alias = tableValuedFunctionExpression.Alias
+                };
+
+                foreach (var annotation in tableValuedFunctionExpression.GetAnnotations())
+                {
+                    newTableValuedFunctionExpression.AddAnnotation(annotation.Name, annotation.Value);
+                }
+
+                return newTableValuedFunctionExpression;
+            }
+
+            if (expression is IClonableTableExpressionBase cloneable)
+            {
+                return cloneable.Clone();
+            }
+
+            // join and set operations are fine, because they contain other TableExpressionBases inside, that will get cloned
+            // and therefore set expression's Update function will generate a new instance.
+            if (expression is CrossJoinExpression
+                or InnerJoinExpression
+                or LeftJoinExpression
+                or CrossApplyExpression
+                or OuterApplyExpression
+                or ExceptExpression
+                or IntersectExpression
+                or UnionExpression)
+            {
+                return base.Visit(expression);
+            }
+
+            if (expression is TableExpressionBase)
+            {
+                throw new InvalidOperationException(
+                    RelationalStrings.TableExpressionBaseWithoutCloningLogic(
+                        expression.GetType().Name,
+                        nameof(TableExpressionBase),
+                        nameof(IClonableTableExpressionBase),
+                        nameof(CloningExpressionVisitor),
+                        nameof(SelectExpression))); ;
+            }
+
+            return base.Visit(expression);
         }
     }
 

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

@@ -2133,6 +2133,52 @@ orderby t.ProductId
         }
     }
 
+    [ConditionalFact]
+    public virtual void TVF_with_navigation_in_projection_groupby_aggregate()
+    {
+        using (var context = CreateContext())
+        {
+            var query = context.Orders
+                .Where(c => !context.Set<TopSellingProduct>().Select(x => x.ProductId).Contains(25))
+                .Select(x => new { x.Customer.FirstName, x.Customer.LastName })
+                .GroupBy(x => new { x.LastName })
+                .Select(x => new { x.Key.LastName, SumOfLengths = x.Sum(xx => xx.FirstName.Length) })
+                .ToList();
+
+            Assert.Equal(3, query.Count);
+            var orderedResult = query.OrderBy(x => x.LastName).ToList();
+            Assert.Equal("One", orderedResult[0].LastName);
+            Assert.Equal(24, orderedResult[0].SumOfLengths);
+            Assert.Equal("Three", orderedResult[1].LastName);
+            Assert.Equal(8, orderedResult[1].SumOfLengths);
+            Assert.Equal("Two", orderedResult[2].LastName);
+            Assert.Equal(16, orderedResult[2].SumOfLengths);
+        }
+    }
+
+    [ConditionalFact]
+    public virtual void TVF_with_argument_being_a_subquery_with_navigation_in_projection_groupby_aggregate()
+    {
+        using (var context = CreateContext())
+        {
+            var query = context.Orders
+                .Where(c => !context.GetOrdersWithMultipleProducts(context.Customers.OrderBy(x => x.Id).FirstOrDefault().Id).Select(x => x.CustomerId).Contains(25))
+                .Select(x => new { x.Customer.FirstName, x.Customer.LastName })
+                .GroupBy(x => new { x.LastName })
+                .Select(x => new { x.Key.LastName, SumOfLengths = x.Sum(xx => xx.FirstName.Length) })
+                .ToList();
+
+            Assert.Equal(3, query.Count);
+            var orderedResult = query.OrderBy(x => x.LastName).ToList();
+            Assert.Equal("One", orderedResult[0].LastName);
+            Assert.Equal(24, orderedResult[0].SumOfLengths);
+            Assert.Equal("Three", orderedResult[1].LastName);
+            Assert.Equal(8, orderedResult[1].SumOfLengths);
+            Assert.Equal("Two", orderedResult[2].LastName);
+            Assert.Equal(16, orderedResult[2].SumOfLengths);
+        }
+    }
+
     [ConditionalFact]
     public virtual void TVF_backing_entity_type_mapped_to_view()
     {

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

@@ -8240,6 +8240,17 @@ public virtual Task DateTimeOffset_to_unix_time_seconds(bool async)
                     .FirstOrDefault() == null));
     }
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Set_operator_with_navigation_in_projection_groupby_aggregate(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<Gear>()
+                .Where(x => ss.Set<Gear>().Concat(ss.Set<Gear>()).Select(x => x.Nickname).Contains("Marcus"))
+                .Select(x => new { x.Squad.Name, x.CityOfBirth.Location })
+                .GroupBy(x => new { x.Name })
+                .Select(x => new { x.Key.Name, SumOfLengths = x.Sum(xx => xx.Location.Length) }));
+
     protected GearsOfWarContext CreateContext()
         => Fixture.CreateContext();
 

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

@@ -10016,6 +10016,43 @@ FROM [SquadMissions] AS [s0]
 ORDER BY [g].[Nickname], [g].[SquadId], [s].[Id], [s1].[SquadId]");
     }
 
+    public override async Task Set_operator_with_navigation_in_projection_groupby_aggregate(bool async)
+    {
+        await base.Set_operator_with_navigation_in_projection_groupby_aggregate(async);
+
+        AssertSql(
+"""
+SELECT [s].[Name], (
+    SELECT COALESCE(SUM(CAST(LEN([c].[Location]) AS int)), 0)
+    FROM [Gears] AS [g2]
+    INNER JOIN [Squads] AS [s0] ON [g2].[SquadId] = [s0].[Id]
+    INNER JOIN [Cities] AS [c] ON [g2].[CityOfBirthName] = [c].[Name]
+    WHERE EXISTS (
+        SELECT 1
+        FROM (
+            SELECT [g3].[Nickname], [g3].[SquadId], [g3].[AssignedCityName], [g3].[CityOfBirthName], [g3].[Discriminator], [g3].[FullName], [g3].[HasSoulPatch], [g3].[LeaderNickname], [g3].[LeaderSquadId], [g3].[Rank]
+            FROM [Gears] AS [g3]
+            UNION ALL
+            SELECT [g4].[Nickname], [g4].[SquadId], [g4].[AssignedCityName], [g4].[CityOfBirthName], [g4].[Discriminator], [g4].[FullName], [g4].[HasSoulPatch], [g4].[LeaderNickname], [g4].[LeaderSquadId], [g4].[Rank]
+            FROM [Gears] AS [g4]
+        ) AS [t0]
+        WHERE [t0].[Nickname] = N'Marcus') AND ([s].[Name] = [s0].[Name] OR ([s].[Name] IS NULL AND [s0].[Name] IS NULL))) AS [SumOfLengths]
+FROM [Gears] AS [g]
+INNER JOIN [Squads] AS [s] ON [g].[SquadId] = [s].[Id]
+WHERE EXISTS (
+    SELECT 1
+    FROM (
+        SELECT [g0].[Nickname], [g0].[SquadId], [g0].[AssignedCityName], [g0].[CityOfBirthName], [g0].[Discriminator], [g0].[FullName], [g0].[HasSoulPatch], [g0].[LeaderNickname], [g0].[LeaderSquadId], [g0].[Rank]
+        FROM [Gears] AS [g0]
+        UNION ALL
+        SELECT [g1].[Nickname], [g1].[SquadId], [g1].[AssignedCityName], [g1].[CityOfBirthName], [g1].[Discriminator], [g1].[FullName], [g1].[HasSoulPatch], [g1].[LeaderNickname], [g1].[LeaderSquadId], [g1].[Rank]
+        FROM [Gears] AS [g1]
+    ) AS [t]
+    WHERE [t].[Nickname] = N'Marcus')
+GROUP BY [s].[Name]
+""");
+    }
+
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 }

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

@@ -13202,6 +13202,67 @@ FROM [SquadMissions] AS [s0]
 """);
     }
 
+    public override async Task Set_operator_with_navigation_in_projection_groupby_aggregate(bool async)
+    {
+        await base.Set_operator_with_navigation_in_projection_groupby_aggregate(async);
+
+        AssertSql(
+"""
+SELECT [s].[Name], (
+    SELECT COALESCE(SUM(CAST(LEN([c].[Location]) AS int)), 0)
+    FROM (
+        SELECT [g0].[Nickname], [g0].[SquadId], [g0].[AssignedCityName], [g0].[CityOfBirthName], [g0].[FullName], [g0].[HasSoulPatch], [g0].[LeaderNickname], [g0].[LeaderSquadId], [g0].[Rank], N'Gear' AS [Discriminator]
+        FROM [Gears] AS [g0]
+        UNION ALL
+        SELECT [o0].[Nickname], [o0].[SquadId], [o0].[AssignedCityName], [o0].[CityOfBirthName], [o0].[FullName], [o0].[HasSoulPatch], [o0].[LeaderNickname], [o0].[LeaderSquadId], [o0].[Rank], N'Officer' AS [Discriminator]
+        FROM [Officers] AS [o0]
+    ) AS [t3]
+    INNER JOIN [Squads] AS [s0] ON [t3].[SquadId] = [s0].[Id]
+    INNER JOIN [Cities] AS [c] ON [t3].[CityOfBirthName] = [c].[Name]
+    WHERE EXISTS (
+        SELECT 1
+        FROM (
+            SELECT [g1].[Nickname], [g1].[SquadId], [g1].[AssignedCityName], [g1].[CityOfBirthName], [g1].[FullName], [g1].[HasSoulPatch], [g1].[LeaderNickname], [g1].[LeaderSquadId], [g1].[Rank], N'Gear' AS [Discriminator]
+            FROM [Gears] AS [g1]
+            UNION ALL
+            SELECT [o1].[Nickname], [o1].[SquadId], [o1].[AssignedCityName], [o1].[CityOfBirthName], [o1].[FullName], [o1].[HasSoulPatch], [o1].[LeaderNickname], [o1].[LeaderSquadId], [o1].[Rank], N'Officer' AS [Discriminator]
+            FROM [Officers] AS [o1]
+            UNION ALL
+            SELECT [g2].[Nickname], [g2].[SquadId], [g2].[AssignedCityName], [g2].[CityOfBirthName], [g2].[FullName], [g2].[HasSoulPatch], [g2].[LeaderNickname], [g2].[LeaderSquadId], [g2].[Rank], N'Gear' AS [Discriminator]
+            FROM [Gears] AS [g2]
+            UNION ALL
+            SELECT [o2].[Nickname], [o2].[SquadId], [o2].[AssignedCityName], [o2].[CityOfBirthName], [o2].[FullName], [o2].[HasSoulPatch], [o2].[LeaderNickname], [o2].[LeaderSquadId], [o2].[Rank], N'Officer' AS [Discriminator]
+            FROM [Officers] AS [o2]
+        ) AS [t4]
+        WHERE [t4].[Nickname] = N'Marcus') AND ([s].[Name] = [s0].[Name] OR ([s].[Name] IS NULL AND [s0].[Name] IS NULL))) AS [SumOfLengths]
+FROM (
+    SELECT [g].[SquadId]
+    FROM [Gears] AS [g]
+    UNION ALL
+    SELECT [o].[SquadId]
+    FROM [Officers] AS [o]
+) AS [t]
+INNER JOIN [Squads] AS [s] ON [t].[SquadId] = [s].[Id]
+WHERE EXISTS (
+    SELECT 1
+    FROM (
+        SELECT [g3].[Nickname], [g3].[SquadId], [g3].[AssignedCityName], [g3].[CityOfBirthName], [g3].[FullName], [g3].[HasSoulPatch], [g3].[LeaderNickname], [g3].[LeaderSquadId], [g3].[Rank], N'Gear' AS [Discriminator]
+        FROM [Gears] AS [g3]
+        UNION ALL
+        SELECT [o3].[Nickname], [o3].[SquadId], [o3].[AssignedCityName], [o3].[CityOfBirthName], [o3].[FullName], [o3].[HasSoulPatch], [o3].[LeaderNickname], [o3].[LeaderSquadId], [o3].[Rank], N'Officer' AS [Discriminator]
+        FROM [Officers] AS [o3]
+        UNION ALL
+        SELECT [g4].[Nickname], [g4].[SquadId], [g4].[AssignedCityName], [g4].[CityOfBirthName], [g4].[FullName], [g4].[HasSoulPatch], [g4].[LeaderNickname], [g4].[LeaderSquadId], [g4].[Rank], N'Gear' AS [Discriminator]
+        FROM [Gears] AS [g4]
+        UNION ALL
+        SELECT [o4].[Nickname], [o4].[SquadId], [o4].[AssignedCityName], [o4].[CityOfBirthName], [o4].[FullName], [o4].[HasSoulPatch], [o4].[LeaderNickname], [o4].[LeaderSquadId], [o4].[Rank], N'Officer' AS [Discriminator]
+        FROM [Officers] AS [o4]
+    ) AS [t0]
+    WHERE [t0].[Nickname] = N'Marcus')
+GROUP BY [s].[Name]
+""");
+    }
+
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 }

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

@@ -11358,6 +11358,56 @@ FROM [SquadMissions] AS [s0]
 """);
     }
 
+    public override async Task Set_operator_with_navigation_in_projection_groupby_aggregate(bool async)
+    {
+        await base.Set_operator_with_navigation_in_projection_groupby_aggregate(async);
+
+        AssertSql(
+"""
+SELECT [s].[Name], (
+    SELECT COALESCE(SUM(CAST(LEN([c].[Location]) AS int)), 0)
+    FROM [Gears] AS [g2]
+    LEFT JOIN [Officers] AS [o2] ON [g2].[Nickname] = [o2].[Nickname] AND [g2].[SquadId] = [o2].[SquadId]
+    INNER JOIN [Squads] AS [s0] ON [g2].[SquadId] = [s0].[Id]
+    INNER JOIN [Cities] AS [c] ON [g2].[CityOfBirthName] = [c].[Name]
+    WHERE EXISTS (
+        SELECT 1
+        FROM (
+            SELECT [g3].[Nickname], [g3].[SquadId], [g3].[AssignedCityName], [g3].[CityOfBirthName], [g3].[FullName], [g3].[HasSoulPatch], [g3].[LeaderNickname], [g3].[LeaderSquadId], [g3].[Rank], CASE
+                WHEN [o3].[Nickname] IS NOT NULL THEN N'Officer'
+            END AS [Discriminator]
+            FROM [Gears] AS [g3]
+            LEFT JOIN [Officers] AS [o3] ON [g3].[Nickname] = [o3].[Nickname] AND [g3].[SquadId] = [o3].[SquadId]
+            UNION ALL
+            SELECT [g4].[Nickname], [g4].[SquadId], [g4].[AssignedCityName], [g4].[CityOfBirthName], [g4].[FullName], [g4].[HasSoulPatch], [g4].[LeaderNickname], [g4].[LeaderSquadId], [g4].[Rank], CASE
+                WHEN [o4].[Nickname] IS NOT NULL THEN N'Officer'
+            END AS [Discriminator]
+            FROM [Gears] AS [g4]
+            LEFT JOIN [Officers] AS [o4] ON [g4].[Nickname] = [o4].[Nickname] AND [g4].[SquadId] = [o4].[SquadId]
+        ) AS [t0]
+        WHERE [t0].[Nickname] = N'Marcus') AND ([s].[Name] = [s0].[Name] OR ([s].[Name] IS NULL AND [s0].[Name] IS NULL))) AS [SumOfLengths]
+FROM [Gears] AS [g]
+INNER JOIN [Squads] AS [s] ON [g].[SquadId] = [s].[Id]
+WHERE EXISTS (
+    SELECT 1
+    FROM (
+        SELECT [g0].[Nickname], [g0].[SquadId], [g0].[AssignedCityName], [g0].[CityOfBirthName], [g0].[FullName], [g0].[HasSoulPatch], [g0].[LeaderNickname], [g0].[LeaderSquadId], [g0].[Rank], CASE
+            WHEN [o0].[Nickname] IS NOT NULL THEN N'Officer'
+        END AS [Discriminator]
+        FROM [Gears] AS [g0]
+        LEFT JOIN [Officers] AS [o0] ON [g0].[Nickname] = [o0].[Nickname] AND [g0].[SquadId] = [o0].[SquadId]
+        UNION ALL
+        SELECT [g1].[Nickname], [g1].[SquadId], [g1].[AssignedCityName], [g1].[CityOfBirthName], [g1].[FullName], [g1].[HasSoulPatch], [g1].[LeaderNickname], [g1].[LeaderSquadId], [g1].[Rank], CASE
+            WHEN [o1].[Nickname] IS NOT NULL THEN N'Officer'
+        END AS [Discriminator]
+        FROM [Gears] AS [g1]
+        LEFT JOIN [Officers] AS [o1] ON [g1].[Nickname] = [o1].[Nickname] AND [g1].[SquadId] = [o1].[SquadId]
+    ) AS [t]
+    WHERE [t].[Nickname] = N'Marcus')
+GROUP BY [s].[Name]
+""");
+    }
+
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 }

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

@@ -9926,6 +9926,43 @@ SELECT 1
 """);
     }
 
+    public override async Task Set_operator_with_navigation_in_projection_groupby_aggregate(bool async)
+    {
+        await base.Set_operator_with_navigation_in_projection_groupby_aggregate(async);
+
+        AssertSql(
+"""
+SELECT [s].[Name], (
+    SELECT COALESCE(SUM(CAST(LEN([c].[Location]) AS int)), 0)
+    FROM [Gears] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [g2]
+    INNER JOIN [Squads] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [s0] ON [g2].[SquadId] = [s0].[Id]
+    INNER JOIN [Cities] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [c] ON [g2].[CityOfBirthName] = [c].[Name]
+    WHERE EXISTS (
+        SELECT 1
+        FROM (
+            SELECT [g3].[Nickname], [g3].[SquadId], [g3].[AssignedCityName], [g3].[CityOfBirthName], [g3].[Discriminator], [g3].[FullName], [g3].[HasSoulPatch], [g3].[LeaderNickname], [g3].[LeaderSquadId], [g3].[PeriodEnd], [g3].[PeriodStart], [g3].[Rank]
+            FROM [Gears] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [g3]
+            UNION ALL
+            SELECT [g4].[Nickname], [g4].[SquadId], [g4].[AssignedCityName], [g4].[CityOfBirthName], [g4].[Discriminator], [g4].[FullName], [g4].[HasSoulPatch], [g4].[LeaderNickname], [g4].[LeaderSquadId], [g4].[PeriodEnd], [g4].[PeriodStart], [g4].[Rank]
+            FROM [Gears] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [g4]
+        ) AS [t0]
+        WHERE [t0].[Nickname] = N'Marcus') AND ([s].[Name] = [s0].[Name] OR ([s].[Name] IS NULL AND [s0].[Name] IS NULL))) AS [SumOfLengths]
+FROM [Gears] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [g]
+INNER JOIN [Squads] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [s] ON [g].[SquadId] = [s].[Id]
+WHERE EXISTS (
+    SELECT 1
+    FROM (
+        SELECT [g0].[Nickname], [g0].[SquadId], [g0].[AssignedCityName], [g0].[CityOfBirthName], [g0].[Discriminator], [g0].[FullName], [g0].[HasSoulPatch], [g0].[LeaderNickname], [g0].[LeaderSquadId], [g0].[PeriodEnd], [g0].[PeriodStart], [g0].[Rank]
+        FROM [Gears] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [g0]
+        UNION ALL
+        SELECT [g1].[Nickname], [g1].[SquadId], [g1].[AssignedCityName], [g1].[CityOfBirthName], [g1].[Discriminator], [g1].[FullName], [g1].[HasSoulPatch], [g1].[LeaderNickname], [g1].[LeaderSquadId], [g1].[PeriodEnd], [g1].[PeriodStart], [g1].[Rank]
+        FROM [Gears] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [g1]
+    ) AS [t]
+    WHERE [t].[Nickname] = N'Marcus')
+GROUP BY [s].[Name]
+""");
+    }
+
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 }