Skip to content

Commit

Permalink
Fix to #29638 - GroupBy generates invalid SQL when using custom datab…
Browse files Browse the repository at this point in the history
…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.

Fixes #29638
  • Loading branch information
maumar committed Apr 5, 2023
1 parent 13db6c9 commit 44ae8d4
Show file tree
Hide file tree
Showing 8 changed files with 322 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -1008,6 +1009,29 @@ private sealed class CloningExpressionVisitor : ExpressionVisitor
return newTpcTable;
}

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

newTableValuedFunctionExpression.Alias = tableValuedFunctionExpression.Alias;
foreach (var annotation in tableValuedFunctionExpression.GetAnnotations())
{
newTableValuedFunctionExpression.AddAnnotation(annotation.Name, annotation.Value);
}

return newTableValuedFunctionExpression;
}

// set operators are fine, because they contain other TableExpressionBases inside, that will get cloned
// and therefore set expression's Update function will generate a new instance.
return expression is IClonableTableExpressionBase cloneable ? cloneable.Clone() : base.Visit(expression);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
11 changes: 11 additions & 0 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,62 @@ ORDER BY [g].[ProductId]
""");
}

public override void TVF_with_navigation_in_projection_groupby_aggregate()
{
base.TVF_with_navigation_in_projection_groupby_aggregate();

AssertSql(
"""
SELECT [c].[LastName], (
SELECT COALESCE(SUM(CAST(LEN([c1].[FirstName]) AS int)), 0)
FROM [Orders] AS [o0]
INNER JOIN [Customers] AS [c0] ON [o0].[CustomerId] = [c0].[Id]
INNER JOIN [Customers] AS [c1] ON [o0].[CustomerId] = [c1].[Id]
WHERE NOT (EXISTS (
SELECT 1
FROM [dbo].[GetTopTwoSellingProducts]() AS [g0]
WHERE [g0].[ProductId] = 25)) AND ([c].[LastName] = [c0].[LastName] OR ([c].[LastName] IS NULL AND [c0].[LastName] IS NULL))) AS [SumOfLengths]
FROM [Orders] AS [o]
INNER JOIN [Customers] AS [c] ON [o].[CustomerId] = [c].[Id]
WHERE NOT (EXISTS (
SELECT 1
FROM [dbo].[GetTopTwoSellingProducts]() AS [g]
WHERE [g].[ProductId] = 25))
GROUP BY [c].[LastName]
""");
}

public override void TVF_with_argument_being_a_subquery_with_navigation_in_projection_groupby_aggregate()
{
base.TVF_with_argument_being_a_subquery_with_navigation_in_projection_groupby_aggregate();

AssertSql(
"""
SELECT [c0].[LastName], (
SELECT COALESCE(SUM(CAST(LEN([c2].[FirstName]) AS int)), 0)
FROM [Orders] AS [o0]
INNER JOIN [Customers] AS [c1] ON [o0].[CustomerId] = [c1].[Id]
INNER JOIN [Customers] AS [c2] ON [o0].[CustomerId] = [c2].[Id]
WHERE NOT (EXISTS (
SELECT 1
FROM [dbo].[GetOrdersWithMultipleProducts]((
SELECT TOP(1) [c3].[Id]
FROM [Customers] AS [c3]
ORDER BY [c3].[Id])) AS [g0]
WHERE [g0].[CustomerId] = 25)) AND ([c0].[LastName] = [c1].[LastName] OR ([c0].[LastName] IS NULL AND [c1].[LastName] IS NULL))) AS [SumOfLengths]
FROM [Orders] AS [o]
INNER JOIN [Customers] AS [c0] ON [o].[CustomerId] = [c0].[Id]
WHERE NOT (EXISTS (
SELECT 1
FROM [dbo].[GetOrdersWithMultipleProducts]((
SELECT TOP(1) [c].[Id]
FROM [Customers] AS [c]
ORDER BY [c].[Id])) AS [g]
WHERE [g].[CustomerId] = 25))
GROUP BY [c0].[LastName]
""");
}

public override void TVF_backing_entity_type_mapped_to_view()
{
base.TVF_backing_entity_type_mapped_to_view();
Expand Down

0 comments on commit 44ae8d4

Please # to comment.