Fix to #29638 - GroupBy generates invalid SQL when using custom datab…

…ase function

Problem is that CloningExpressionVisitor doesn't have proper handling for TableValuedFunctionExpression, and therefore goes through default expression visitor pattern (visit all children, check if there are any changes, if there are return new, if not return the same). Since there are no changes, the same instance is returned from cloning, and causes the problem.
Fix is to add proper handling of TVFExpression in the CloningExpressionVisitor so that it produces a proper copy. Also added defensive check that throws if we encounter TableExpressionBase that doesn't implement the cloning logic, so that this never happens again.

Fixes #29638

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);
 }