From 752820cb0ed02eaab9c7e7a73292aeeefc5b974b Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Wed, 16 Oct 2019 17:30:02 +0200 Subject: [PATCH] Correct nullability for set operations Fixes #18135 --- .../Query/SqlExpressions/SelectExpression.cs | 40 +++++++++++++++---- .../SimpleQueryCosmosTest.SetOperations.cs | 1 + .../Query/ComplexNavigationsQueryTestBase.cs | 16 ++++++++ .../ComplexNavigationsWeakQueryTestBase.cs | 4 ++ .../SimpleQueryTestBase.SetOperations.cs | 11 +++++ .../ComplexNavigationsQuerySqlServerTest.cs | 18 +++++++++ .../SimpleQuerySqlServerTest.SetOperations.cs | 12 ++++++ 7 files changed, 94 insertions(+), 8 deletions(-) diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index f3ceee93a7d..05a2f41b716 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -527,10 +527,19 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi ?? (innerColumn1 as ColumnExpression)?.Name ?? "c"); - var innerProjection = new ProjectionExpression(innerColumn1, alias); - select1._projection.Add(innerProjection); - select2._projection.Add(new ProjectionExpression(innerColumn2, alias)); - _projectionMapping[joinedMapping.Key] = new ColumnExpression(innerProjection, setExpression); + var innerProjection1 = new ProjectionExpression(innerColumn1, alias); + var innerProjection2 = new ProjectionExpression(innerColumn2, alias); + select1._projection.Add(innerProjection1); + select2._projection.Add(innerProjection2); + var outerProjection = new ColumnExpression(innerProjection1, setExpression); + + if (isNullableProjection(innerProjection1) + || isNullableProjection(innerProjection2)) + { + outerProjection = outerProjection.MakeNullable(); + } + + _projectionMapping[joinedMapping.Key] = outerProjection; continue; } @@ -575,10 +584,17 @@ ColumnExpression addSetOperationColumnProjections( SelectExpression select2, ColumnExpression column2) { var alias = generateUniqueAlias(column1.Name); - var innerProjection = new ProjectionExpression(column1, alias); - select1._projection.Add(innerProjection); - select2._projection.Add(new ProjectionExpression(column2, alias)); - var outerProjection = new ColumnExpression(innerProjection, setExpression); + var innerProjection1 = new ProjectionExpression(column1, alias); + var innerProjection2 = new ProjectionExpression(column2, alias); + select1._projection.Add(innerProjection1); + select2._projection.Add(innerProjection2); + var outerProjection = new ColumnExpression(innerProjection1, setExpression); + if (isNullableProjection(innerProjection1) + || isNullableProjection(innerProjection2)) + { + outerProjection = outerProjection.MakeNullable(); + } + if (select1._identifier.Contains(column1)) { _identifier.Add(outerProjection); @@ -598,6 +614,14 @@ string generateUniqueAlias(string baseAlias) return currentAlias; } + + static bool isNullableProjection(ProjectionExpression projectionExpression) + => projectionExpression.Expression switch + { + ColumnExpression columnExpression => columnExpression.IsNullable, + SqlConstantExpression sqlConstantExpression => sqlConstantExpression.Value == null, + _ => true, + }; } private ColumnExpression GenerateOuterColumn(SqlExpression projection, string alias = null) diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.SetOperations.cs b/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.SetOperations.cs index e1578dd920b..2e52ae5cdde 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.SetOperations.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.SetOperations.cs @@ -40,6 +40,7 @@ public override void Include_Union_different_includes_throws() { } public override Task SubSelect_Union(bool isAsync) => Task.CompletedTask; public override Task Client_eval_Union_FirstOrDefault(bool isAsync) => Task.CompletedTask; public override Task GroupBy_Select_Union(bool isAsync) => Task.CompletedTask; + public override Task Union_over_columns_with_different_nullability(bool isAsync) => Task.CompletedTask; public override Task Union_over_different_projection_types(bool isAsync, string leftType, string rightType) => Task.CompletedTask; } } diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs index f57912a7a08..5993c508f69 100644 --- a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs @@ -5709,5 +5709,21 @@ public virtual Task Null_check_different_structure_does_not_remove_null_checks(b ? null : l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2.OneToOne_Optional_FK3.Name) == "L4 01")); } + + [ConditionalFact] + public virtual void Union_over_entities_with_different_nullability() + { + using var ctx = CreateContext(); + + var query = ctx.Set() + .GroupJoin(ctx.Set(), l1 => l1.Id, l2 => l2.Level1_Optional_Id, (l1, l2s) => new { l1, l2s }) + .SelectMany(g => g.l2s.DefaultIfEmpty(), (g, l2) => new { g.l1, l2 }) + .Concat(ctx.Set().GroupJoin(ctx.Set(), l2 => l2.Level1_Optional_Id, l1 => l1.Id, (l2, l1s) => new { l2, l1s }) + .SelectMany(g => g.l1s.DefaultIfEmpty(), (g, l1) => new { l1, g.l2 }) + .Where(e => e.l1.Equals(null))) + .Select(e => e.l1.Id); + + var result = query.ToList(); + } } } diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs index a360a1032d6..4cb29354172 100644 --- a/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs @@ -169,6 +169,10 @@ public override void Member_pushdown_with_collection_navigation_in_the_middle() { } + public override void Union_over_entities_with_different_nullability() + { + } + [ConditionalTheory(Skip = "Issue#16752")] public override Task Include_inside_subquery(bool isAsync) { diff --git a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs index 57e9d05bf13..ff43b6a1e50 100644 --- a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs +++ b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs @@ -444,6 +444,17 @@ public virtual Task GroupBy_Select_Union(bool isAsync) .Select(g => new { CustomerID = g.Key, Count = g.Count() }))); } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Union_over_columns_with_different_nullability(bool isAsync) + { + return AssertQuery( + isAsync, ss => ss.Set() + .Select(c => "NonNullableConstant") + .Concat(ss.Set() + .Select(c => (string)null))); + } + [ConditionalTheory] #pragma warning disable xUnit1016 // MemberData must reference a public member [MemberData(nameof(GetSetOperandTestCases))] diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs index a05dae94343..3920cfa25a1 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs @@ -4357,6 +4357,24 @@ ELSE [l2].[Name] END IS NOT NULL"); } + public override void Union_over_entities_with_different_nullability() + { + base.Union_over_entities_with_different_nullability(); + + AssertSql( + @"SELECT [t].[Id] +FROM ( + SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id], [l0].[Id] AS [Id0], [l0].[Date] AS [Date0], [l0].[Level1_Optional_Id], [l0].[Level1_Required_Id], [l0].[Name] AS [Name0], [l0].[OneToMany_Optional_Inverse2Id], [l0].[OneToMany_Optional_Self_Inverse2Id], [l0].[OneToMany_Required_Inverse2Id], [l0].[OneToMany_Required_Self_Inverse2Id], [l0].[OneToOne_Optional_PK_Inverse2Id], [l0].[OneToOne_Optional_Self2Id] + FROM [LevelOne] AS [l] + LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Optional_Id] + UNION ALL + SELECT [l2].[Id], [l2].[Date], [l2].[Name], [l2].[OneToMany_Optional_Self_Inverse1Id], [l2].[OneToMany_Required_Self_Inverse1Id], [l2].[OneToOne_Optional_Self1Id], [l1].[Id] AS [Id0], [l1].[Date] AS [Date0], [l1].[Level1_Optional_Id], [l1].[Level1_Required_Id], [l1].[Name] AS [Name0], [l1].[OneToMany_Optional_Inverse2Id], [l1].[OneToMany_Optional_Self_Inverse2Id], [l1].[OneToMany_Required_Inverse2Id], [l1].[OneToMany_Required_Self_Inverse2Id], [l1].[OneToOne_Optional_PK_Inverse2Id], [l1].[OneToOne_Optional_Self2Id] + FROM [LevelTwo] AS [l1] + LEFT JOIN [LevelOne] AS [l2] ON [l1].[Level1_Optional_Id] = [l2].[Id] + WHERE [l2].[Id] IS NULL +) AS [t]"); + } + private void AssertSql(params string[] expected) { Fixture.TestSqlLoggerFactory.AssertBaseline(expected); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.SetOperations.cs b/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.SetOperations.cs index a645afd26e7..bd1b4326c89 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.SetOperations.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.SetOperations.cs @@ -380,6 +380,18 @@ FROM [Customers] AS [c0] GROUP BY [c0].[CustomerID]"); } + public override async Task Union_over_columns_with_different_nullability(bool isAsync) + { + await base.Union_over_columns_with_different_nullability(isAsync); + + AssertSql( + @"SELECT N'NonNullableConstant' AS [c] +FROM [Customers] AS [c] +UNION ALL +SELECT NULL AS [c] +FROM [Customers] AS [c0]"); + } + public override async Task Union_over_different_projection_types(bool isAsync, string leftType, string rightType) { await base.Union_over_different_projection_types(isAsync, leftType, rightType);