From b68348d396de1a5fcd96ad8332eedb09c44c608b Mon Sep 17 00:00:00 2001 From: maumar Date: Thu, 14 Sep 2023 11:57:32 -0700 Subject: [PATCH] Fix to #30996 - Incorrect translation of comparison of current value with owned type default value In optional dependent table sharing scenario, when comparing the dependent to null we first look for any required properties (at least one of them needs to be null), and if we don't have any required properties we look at optional ones (even though this is somewhat ambiguous). Error was that we did similar check as with required properties (i.e. at least one of them must be null, but what we should be doing is checking that all of them are null. Fixes #30996 --- ...lationalSqlTranslatingExpressionVisitor.cs | 37 ++++++++--- .../OwnedEntityQueryRelationalTestBase.cs | 64 +++++++++++++++++++ .../Query/OwnedEntityQuerySqlServerTest.cs | 32 +++++++++- 3 files changed, 122 insertions(+), 11 deletions(-) diff --git a/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs index 6bd690a1ad2..7b94b87815a 100644 --- a/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs @@ -20,6 +20,9 @@ namespace Microsoft.EntityFrameworkCore.Query; /// public class RelationalSqlTranslatingExpressionVisitor : ExpressionVisitor { + private static readonly bool UseOldBehavior30996 + = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue30996", out var enabled30996) && enabled30996; + private const string RuntimeParameterPrefix = QueryCompilationContext.QueryParameterPrefix + "entity_equality_"; private static readonly List SingleResultMethodInfos = new() @@ -1671,19 +1674,33 @@ private bool TryRewriteEntityEquality( if (allNonPrincipalSharedNonPkProperties.Count != 0 && allNonPrincipalSharedNonPkProperties.All(p => p.IsNullable)) { - var atLeastOneNonNullValueInNullablePropertyCondition = allNonPrincipalSharedNonPkProperties - .Select( - p => Infrastructure.ExpressionExtensions.CreateEqualsExpression( - CreatePropertyAccessExpression(nonNullEntityReference, p), - Expression.Constant(null, p.ClrType.MakeNullable()), - nodeType != ExpressionType.Equal)) - .Aggregate((l, r) => nodeType == ExpressionType.Equal ? Expression.OrElse(l, r) : Expression.AndAlso(l, r)); + Expression? optionalPropertiesCondition; + if (!UseOldBehavior30996) + { + optionalPropertiesCondition = allNonPrincipalSharedNonPkProperties + .Select( + p => Infrastructure.ExpressionExtensions.CreateEqualsExpression( + CreatePropertyAccessExpression(nonNullEntityReference, p), + Expression.Constant(null, p.ClrType.MakeNullable()), + nodeType != ExpressionType.Equal)) + .Aggregate((l, r) => nodeType == ExpressionType.Equal ? Expression.AndAlso(l, r) : Expression.OrElse(l, r)); + } + else + { + optionalPropertiesCondition = allNonPrincipalSharedNonPkProperties + .Select( + p => Infrastructure.ExpressionExtensions.CreateEqualsExpression( + CreatePropertyAccessExpression(nonNullEntityReference, p), + Expression.Constant(null, p.ClrType.MakeNullable()), + nodeType != ExpressionType.Equal)) + .Aggregate((l, r) => nodeType == ExpressionType.Equal ? Expression.OrElse(l, r) : Expression.AndAlso(l, r)); + } condition = condition == null - ? atLeastOneNonNullValueInNullablePropertyCondition + ? optionalPropertiesCondition : nodeType == ExpressionType.Equal - ? Expression.OrElse(condition, atLeastOneNonNullValueInNullablePropertyCondition) - : Expression.AndAlso(condition, atLeastOneNonNullValueInNullablePropertyCondition); + ? Expression.OrElse(condition, optionalPropertiesCondition) + : Expression.AndAlso(condition, optionalPropertiesCondition); } if (condition != null) diff --git a/test/EFCore.Relational.Specification.Tests/Query/OwnedEntityQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/OwnedEntityQueryRelationalTestBase.cs index 54e78c7f789..f559e7266ea 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/OwnedEntityQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/OwnedEntityQueryRelationalTestBase.cs @@ -284,6 +284,64 @@ public virtual async Task Owned_entity_with_all_null_properties_entity_equality_ }); } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Owned_entity_with_all_null_properties_in_compared_to_null_in_conditional_projection(bool async) + { + var contextFactory = await InitializeAsync(seed: c => c.Seed()); + + using var context = contextFactory.CreateContext(); + var query = context.RotRutCases + .AsNoTracking() + .OrderBy(e => e.Id) + .Select(e => e.Rot == null ? null : new RotDto { MyApartmentNo = e.Rot.ApartmentNo, MyServiceType = e.Rot.ServiceType }); + + var result = async + ? await query.ToListAsync() + : query.ToList(); + + Assert.Collection( + result, + t => + { + Assert.Equal("1", t.MyApartmentNo); + Assert.Equal(1, t.MyServiceType); + }, + t => + { + Assert.Null(t); + }); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Owned_entity_with_all_null_properties_in_compared_to_non_null_in_conditional_projection(bool async) + { + var contextFactory = await InitializeAsync(seed: c => c.Seed()); + + using var context = contextFactory.CreateContext(); + var query = context.RotRutCases + .AsNoTracking() + .OrderBy(e => e.Id) + .Select(e => e.Rot != null ? new RotDto { MyApartmentNo = e.Rot.ApartmentNo, MyServiceType = e.Rot.ServiceType } : null); + + var result = async + ? await query.ToListAsync() + : query.ToList(); + + Assert.Collection( + result, + t => + { + Assert.Equal("1", t.MyApartmentNo); + Assert.Equal(1, t.MyServiceType); + }, + t => + { + Assert.Null(t); + }); + } + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual async Task Owned_entity_with_all_null_properties_property_access_when_not_containing_another_owned_entity(bool async) @@ -364,6 +422,12 @@ public class Rot public string ApartmentNo { get; set; } } + public class RotDto + { + public int? MyServiceType { get; set; } + public string MyApartmentNo { get; set; } + } + public class Rut { public int? Value { get; set; } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/OwnedEntityQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/OwnedEntityQuerySqlServerTest.cs index cd9b9197536..5d739e31b39 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/OwnedEntityQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/OwnedEntityQuerySqlServerTest.cs @@ -166,7 +166,37 @@ public override async Task Owned_entity_with_all_null_properties_entity_equality """ SELECT [r].[Id], [r].[Rot_ApartmentNo], [r].[Rot_ServiceType] FROM [RotRutCases] AS [r] -WHERE ([r].[Rot_ApartmentNo] IS NOT NULL) AND ([r].[Rot_ServiceType] IS NOT NULL) +WHERE ([r].[Rot_ApartmentNo] IS NOT NULL) OR ([r].[Rot_ServiceType] IS NOT NULL) +"""); + } + + public override async Task Owned_entity_with_all_null_properties_in_compared_to_null_in_conditional_projection(bool async) + { + await base.Owned_entity_with_all_null_properties_in_compared_to_null_in_conditional_projection(async); + + AssertSql( +""" +SELECT CASE + WHEN ([r].[Rot_ApartmentNo] IS NULL) AND ([r].[Rot_ServiceType] IS NULL) THEN CAST(1 AS bit) + ELSE CAST(0 AS bit) +END, [r].[Rot_ApartmentNo], [r].[Rot_ServiceType] +FROM [RotRutCases] AS [r] +ORDER BY [r].[Id] +"""); + } + + public override async Task Owned_entity_with_all_null_properties_in_compared_to_non_null_in_conditional_projection(bool async) + { + await base.Owned_entity_with_all_null_properties_in_compared_to_non_null_in_conditional_projection(async); + + AssertSql( +""" +SELECT CASE + WHEN ([r].[Rot_ApartmentNo] IS NOT NULL) OR ([r].[Rot_ServiceType] IS NOT NULL) THEN CAST(1 AS bit) + ELSE CAST(0 AS bit) +END, [r].[Rot_ApartmentNo], [r].[Rot_ServiceType] +FROM [RotRutCases] AS [r] +ORDER BY [r].[Id] """); }