From ebf82f6b70420c87860ac783c0b4e4b65e5b12a3 Mon Sep 17 00:00:00 2001 From: maumar Date: Tue, 12 Sep 2023 16:16:08 -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 | 14 ++-- .../OwnedEntityQueryRelationalTestBase.cs | 64 +++++++++++++++++++ .../Query/OwnedEntityQuerySqlServerTest.cs | 32 +++++++++- 3 files changed, 104 insertions(+), 6 deletions(-) diff --git a/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs index 3a96b93fb13..53ac4ec78cd 100644 --- a/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs @@ -1791,19 +1791,23 @@ bool TryRewriteEntityEquality([NotNullWhen(true)] out Expression? result) if (allNonPrincipalSharedNonPkProperties.Count != 0 && allNonPrincipalSharedNonPkProperties.All(p => p.IsNullable)) { - var atLeastOneNonNullValueInNullablePropertyCondition = allNonPrincipalSharedNonPkProperties + // if we don't have any required properties to properly check the nullability, + // we rely on optional ones (somewhat unreliably) + // - if entity is to be null, all the properties must be null + // - if the entity is to be not null, at least one property must be not null + var 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)); + .Aggregate((l, r) => nodeType == ExpressionType.Equal ? Expression.AndAlso(l, r) : Expression.OrElse(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 804c9ea3db0..12c52e09b30 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] """); }