From 545de3a7566c3ddda9bf56a33b146fa491ee3045 Mon Sep 17 00:00:00 2001 From: maumar Date: Mon, 11 Jan 2021 19:36:45 -0800 Subject: [PATCH] Fix to #23761 - UseRelationalNulls causes subquery to include NOT IN (NULL, x) We do optimization which combines comparisons based on the same property into IN and NOT IN. Problem is that if the comparisons contain nulls, we add those into the list of IN values, generating queries like a IN (1, 2, NULL). In normal circumstances, during null semantics processing we extract these nulls and produce correct IS NULL / IS NOT NULL calls, however when useRelationalNulls is enabled, we don't run null semantics visitor and therefore don't "fix" the IN expressions. Fix is to only apply the initial optimization for c# null semantics. Fixes #23761 --- ...lExpressionSimplifyingExpressionVisitor.cs | 18 ++- ...RelationalQueryTranslationPostprocessor.cs | 6 +- .../Query/NullSemanticsQueryTestBase.cs | 128 +++++++++++++++++- .../Query/NullSemanticsQuerySqlServerTest.cs | 93 +++++++++++++ 4 files changed, 241 insertions(+), 4 deletions(-) diff --git a/src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs b/src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs index 3b539fbd934..67017bf9b58 100644 --- a/src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Internal/SqlExpressionSimplifyingExpressionVisitor.cs @@ -24,6 +24,7 @@ namespace Microsoft.EntityFrameworkCore.Query.Internal public class SqlExpressionSimplifyingExpressionVisitor : ExpressionVisitor { private readonly ISqlExpressionFactory _sqlExpressionFactory; + private readonly bool _useRelationalNulls; /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -31,9 +32,10 @@ public class SqlExpressionSimplifyingExpressionVisitor : ExpressionVisitor /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public SqlExpressionSimplifyingExpressionVisitor([NotNull] ISqlExpressionFactory sqlExpressionFactory) + public SqlExpressionSimplifyingExpressionVisitor([NotNull] ISqlExpressionFactory sqlExpressionFactory, bool useRelationalNulls) { _sqlExpressionFactory = sqlExpressionFactory; + _useRelationalNulls = useRelationalNulls; } /// @@ -264,6 +266,15 @@ private Expression SimplifySqlBinary(SqlBinaryExpression sqlBinaryExpression) leftValue = leftCandidateInfo.ConstantValue; rightValue = rightCandidateInfo.ConstantValue; + // for relational nulls we can't combine comparisons that contain null + // a != 1 && a != null would be converted to a NOT IN (1, null), which never returns any results + // we need to keep it in the original form so that a != null gets converted to a IS NOT NULL instead + // for c# null semantics it's fine because null semantics visitor extracts null back into proper null checks + if (_useRelationalNulls && (leftValue == null || rightValue == null)) + { + return sqlBinaryExpression.Update(left, right); + } + resultArray = ConstructCollection(leftValue, rightValue); } else if (leftConstantIsEnumerable && rightConstantIsEnumerable) @@ -284,6 +295,11 @@ private Expression SimplifySqlBinary(SqlBinaryExpression sqlBinaryExpression) ? rightCandidateInfo.ConstantValue : leftCandidateInfo.ConstantValue; + if (_useRelationalNulls && rightValue == null) + { + return sqlBinaryExpression.Update(left, right); + } + resultArray = AddToCollection((IEnumerable)leftValue, rightValue); } diff --git a/src/EFCore.Relational/Query/RelationalQueryTranslationPostprocessor.cs b/src/EFCore.Relational/Query/RelationalQueryTranslationPostprocessor.cs index fbc96760bca..9f03e3e79ab 100644 --- a/src/EFCore.Relational/Query/RelationalQueryTranslationPostprocessor.cs +++ b/src/EFCore.Relational/Query/RelationalQueryTranslationPostprocessor.cs @@ -4,6 +4,7 @@ using System; using System.Linq.Expressions; using JetBrains.Annotations; +using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Query.Internal; using Microsoft.EntityFrameworkCore.Utilities; @@ -14,6 +15,8 @@ namespace Microsoft.EntityFrameworkCore.Query /// public class RelationalQueryTranslationPostprocessor : QueryTranslationPostprocessor { + private readonly bool _useRelationalNulls; + /// /// Creates a new instance of the class. /// @@ -30,6 +33,7 @@ public RelationalQueryTranslationPostprocessor( Check.NotNull(queryCompilationContext, nameof(queryCompilationContext)); RelationalDependencies = relationalDependencies; + _useRelationalNulls = RelationalOptionsExtension.Extract(queryCompilationContext.ContextOptions).UseRelationalNulls; } /// @@ -45,7 +49,7 @@ public override Expression Process(Expression query) query = new CollectionJoinApplyingExpressionVisitor((RelationalQueryCompilationContext)QueryCompilationContext).Visit(query); query = new TableAliasUniquifyingExpressionVisitor().Visit(query); query = new SelectExpressionPruningExpressionVisitor().Visit(query); - query = new SqlExpressionSimplifyingExpressionVisitor(RelationalDependencies.SqlExpressionFactory).Visit(query); + query = new SqlExpressionSimplifyingExpressionVisitor(RelationalDependencies.SqlExpressionFactory, _useRelationalNulls).Visit(query); query = new RelationalValueConverterCompensatingExpressionVisitor(RelationalDependencies.SqlExpressionFactory).Visit(query); #pragma warning disable 618 diff --git a/test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs index d417c37a1bf..e0787d1498d 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs @@ -1530,13 +1530,137 @@ await AssertQuery( [ConditionalTheory] [MemberData(nameof(IsAsyncData))] - public virtual async Task False_compared_to_negated_is_null(bool async) + public virtual Task False_compared_to_negated_is_null(bool async) { - await AssertQuery( + return AssertQuery( async, ss => ss.Set().Where(e => false == (!(e.NullableStringA == null)))); } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Multiple_non_equality_comparisons_with_null_in_the_middle(bool async) + { + return AssertQuery( + async, + ss => ss.Set().Where(e => e.NullableIntA != 1 && e.NullableIntA != null && e.NullableIntA != 2)); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Multiple_non_equality_comparisons_including_null_comparison_work_for_relational_null_semantics(bool async) + { + var ctx = CreateContext(useRelationalNulls: true); + + var expected = ctx.Entities1.AsEnumerable().Where(e => e.NullableIntA != 1 && e.NullableIntA != null).ToList(); + ClearLog(); + var query = ctx.Entities1.Where(e => e.NullableIntA != 1 && e.NullableIntA != null); + + var result = async ? await query.ToListAsync() : query.ToList(); + Assert.Equal(expected.Count, result.Count); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Multiple_non_equality_comparisons_without_null_comparison_work_for_relational_null_semantics(bool async) + { + var ctx = CreateContext(useRelationalNulls: true); + + var expected = ctx.Entities1.AsEnumerable().Where(e => e.NullableIntA != 1 && e.NullableIntA != 2 && e.NullableIntA != null).ToList(); + ClearLog(); + var query = ctx.Entities1.Where(e => e.NullableIntA != 1 && e.NullableIntA != 2); + + var result = async ? await query.ToListAsync() : query.ToList(); + Assert.Equal(expected.Count, result.Count); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Multiple_equality_comparisons_including_null_comparison_work_for_relational_null_semantics(bool async) + { + var ctx = CreateContext(useRelationalNulls: true); + + var expected = ctx.Entities1.AsEnumerable().Where(e => e.NullableIntA == 1 || e.NullableIntA == null).ToList(); + ClearLog(); + var query = ctx.Entities1.Where(e => e.NullableIntA == 1 || e.NullableIntA == null); + + var result = async ? await query.ToListAsync() : query.ToList(); + Assert.Equal(expected.Count, result.Count); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Multiple_contains_calls_get_combined_into_one_for_relational_null_semantics(bool async) + { + var ctx = CreateContext(useRelationalNulls: true); + + var expected = ctx.Entities1.AsEnumerable().Where(e => new int?[] { 1, 2, 3 }.Contains(e.NullableIntA)).ToList(); + + ClearLog(); + var query = ctx.Entities1.Where(e => new int?[] { 1, null }.Contains(e.NullableIntA) + || new int?[] { 2, null, 3 }.Contains(e.NullableIntA)); + + var result = async ? await query.ToListAsync() : query.ToList(); + Assert.Equal(expected.Count, result.Count); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Multiple_negated_contains_calls_get_combined_into_one_for_relational_null_semantics(bool async) + { + var ctx = CreateContext(useRelationalNulls: true); + var query = ctx.Entities1.Where(e => !(new int?[] { 1, null }.Contains(e.NullableIntA)) + && !(new int?[] { 2, null, 3 }.Contains(e.NullableIntA))); + + var result = async ? await query.ToListAsync() : query.ToList(); + Assert.Empty(result); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Contains_with_comparison_dont_get_combined_for_relational_null_semantics(bool async) + { + var ctx = CreateContext(useRelationalNulls: true); + + var expected = ctx.Entities1.AsEnumerable().Where(e => new int?[] { 1, 2 }.Contains(e.NullableIntA) || e.NullableIntA == null).ToList(); + + ClearLog(); + var query = ctx.Entities1.Where(e => new int?[] { 1, 2 }.Contains(e.NullableIntA) || e.NullableIntA == null); + + var result = async ? await query.ToListAsync() : query.ToList(); + Assert.Equal(expected.Count, result.Count); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Negated_contains_with_comparison_dont_get_combined_for_relational_null_semantics(bool async) + { + var ctx = CreateContext(useRelationalNulls: true); + + var expected = ctx.Entities1.AsEnumerable().Where(e => !(new int?[] { 1, 2 }.Contains(e.NullableIntA)) && e.NullableIntA != null).ToList(); + + ClearLog(); + var query = ctx.Entities1.Where(e => e.NullableIntA != null && !(new int?[] { 1, 2 }.Contains(e.NullableIntA))); + + var result = async ? await query.ToListAsync() : query.ToList(); + Assert.Equal(expected.Count, result.Count); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Negated_contains_with_comparison_without_null_get_combined_for_relational_null_semantics(bool async) + { + var ctx = CreateContext(useRelationalNulls: true); + + var expected = ctx.Entities1.AsEnumerable().Where(e => !(new int?[] { 1, 2, 3, null }.Contains(e.NullableIntA))).ToList(); + + ClearLog(); + var query = ctx.Entities1.Where(e => e.NullableIntA != 3 && !(new int?[] { 1, 2 }.Contains(e.NullableIntA))); + + var result = async ? await query.ToListAsync() : query.ToList(); + Assert.Equal(expected.Count, result.Count); + } + private string NormalizeDelimitersInRawString(string sql) => Fixture.TestStore.NormalizeDelimitersInRawString(sql); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs index 8f8ca564791..35ff72ff350 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs @@ -1894,6 +1894,96 @@ FROM [Entities1] AS [e] WHERE [e].[NullableStringA] IS NULL"); } + public override async Task Multiple_non_equality_comparisons_with_null_in_the_middle(bool async) + { + await base.Multiple_non_equality_comparisons_with_null_in_the_middle(async); + + AssertSql( + @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC] +FROM [Entities1] AS [e] +WHERE [e].[NullableIntA] NOT IN (1, 2) AND [e].[NullableIntA] IS NOT NULL"); + } + + public override async Task Multiple_non_equality_comparisons_including_null_comparison_work_for_relational_null_semantics(bool async) + { + await base.Multiple_non_equality_comparisons_including_null_comparison_work_for_relational_null_semantics(async); + + AssertSql( + @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC] +FROM [Entities1] AS [e] +WHERE ([e].[NullableIntA] <> 1) AND [e].[NullableIntA] IS NOT NULL"); + } + + public override async Task Multiple_non_equality_comparisons_without_null_comparison_work_for_relational_null_semantics(bool async) + { + await base.Multiple_non_equality_comparisons_without_null_comparison_work_for_relational_null_semantics(async); + + AssertSql( + @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC] +FROM [Entities1] AS [e] +WHERE [e].[NullableIntA] NOT IN (1, 2)"); + } + + public override async Task Multiple_equality_comparisons_including_null_comparison_work_for_relational_null_semantics(bool async) + { + await base.Multiple_equality_comparisons_including_null_comparison_work_for_relational_null_semantics(async); + + AssertSql( + @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC] +FROM [Entities1] AS [e] +WHERE ([e].[NullableIntA] = 1) OR [e].[NullableIntA] IS NULL"); + } + + public override async Task Multiple_contains_calls_get_combined_into_one_for_relational_null_semantics(bool async) + { + await base.Multiple_contains_calls_get_combined_into_one_for_relational_null_semantics(async); + + AssertSql( + @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC] +FROM [Entities1] AS [e] +WHERE [e].[NullableIntA] IN (1, NULL, 2, 3)"); + } + + public override async Task Multiple_negated_contains_calls_get_combined_into_one_for_relational_null_semantics(bool async) + { + await base.Multiple_negated_contains_calls_get_combined_into_one_for_relational_null_semantics(async); + + AssertSql( + @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC] +FROM [Entities1] AS [e] +WHERE [e].[NullableIntA] NOT IN (1, NULL, 2, 3)"); + } + + public override async Task Contains_with_comparison_dont_get_combined_for_relational_null_semantics(bool async) + { + await base.Contains_with_comparison_dont_get_combined_for_relational_null_semantics(async); + + AssertSql( + @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC] +FROM [Entities1] AS [e] +WHERE [e].[NullableIntA] IN (1, 2) OR [e].[NullableIntA] IS NULL"); + } + + public override async Task Negated_contains_with_comparison_dont_get_combined_for_relational_null_semantics(bool async) + { + await base.Negated_contains_with_comparison_dont_get_combined_for_relational_null_semantics(async); + + AssertSql( + @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC] +FROM [Entities1] AS [e] +WHERE [e].[NullableIntA] IS NOT NULL AND [e].[NullableIntA] NOT IN (1, 2)"); + } + + public override async Task Negated_contains_with_comparison_without_null_get_combined_for_relational_null_semantics(bool async) + { + await base.Negated_contains_with_comparison_without_null_get_combined_for_relational_null_semantics(async); + + AssertSql( + @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC] +FROM [Entities1] AS [e] +WHERE [e].[NullableIntA] NOT IN (1, 2, 3)"); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); @@ -1911,5 +2001,8 @@ protected override NullSemanticsContext CreateContext(bool useRelationalNulls = return context; } + + protected override void ClearLog() + => Fixture.TestSqlLoggerFactory.Clear(); } }