From fbc493a4db3797ee03ba390d37f6b072d3e9318e Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Wed, 30 Nov 2022 11:35:21 +0100 Subject: [PATCH] Support interfaces and entities with owned types in ExecuteUpdate setter property lambda (#29672) Fixes #29618 Fixes #28727 (cherry picked from commit d4be66b49c7f73bfed0540ea4763becd56640fe6) --- ...yableMethodTranslatingExpressionVisitor.cs | 85 ++++++++++++++++++- .../InheritanceBulkUpdatesTestBase.cs | 21 +++++ .../NonSharedModelBulkUpdatesTestBase.cs | 19 +++++ .../TPTInheritanceBulkUpdatesTestBase.cs | 10 +++ .../InheritanceBulkUpdatesSqlServerTest.cs | 32 ++++++- .../NonSharedModelBulkUpdatesSqlServerTest.cs | 12 +++ .../TPCInheritanceBulkUpdatesSqlServerTest.cs | 29 ++++++- .../TPTInheritanceBulkUpdatesSqlServerTest.cs | 14 +++ .../InheritanceBulkUpdatesSqliteTest.cs | 29 ++++++- .../NonSharedModelBulkUpdatesSqliteTest.cs | 11 +++ .../TPCInheritanceBulkUpdatesSqliteTest.cs | 27 +++++- .../TPTInheritanceBulkUpdatesSqliteTest.cs | 19 ++++- 12 files changed, 299 insertions(+), 9 deletions(-) diff --git a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs index 29010cc544f..ce10009a52c 100644 --- a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs @@ -18,6 +18,9 @@ public class RelationalQueryableMethodTranslatingExpressionVisitor : QueryableMe private readonly ISqlExpressionFactory _sqlExpressionFactory; private readonly bool _subquery; + private static readonly bool QuirkEnabled28727 + = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue28727", out var enabled) && enabled; + /// /// Creates a new instance of the class. /// @@ -1178,11 +1181,25 @@ static Expression PruneOwnedIncludes(IncludeExpression includeExpression) foreach (var (propertyExpression, _) in propertyValueLambdaExpressions) { var left = RemapLambdaBody(source, propertyExpression); - left = left.UnwrapTypeConversion(out _); - if (!IsValidPropertyAccess(RelationalDependencies.Model, left, out var ese)) + + EntityShaperExpression? ese; + + if (QuirkEnabled28727) { - AddTranslationErrorDetails(RelationalStrings.InvalidPropertyInSetProperty(propertyExpression.Print())); - return null; + left = left.UnwrapTypeConversion(out _); + if (!IsValidPropertyAccess(RelationalDependencies.Model, left, out ese)) + { + AddTranslationErrorDetails(RelationalStrings.InvalidPropertyInSetProperty(propertyExpression.Print())); + return null; + } + } + else + { + if (!TryProcessPropertyAccess(RelationalDependencies.Model, ref left, out ese)) + { + AddTranslationErrorDetails(RelationalStrings.InvalidPropertyInSetProperty(propertyExpression.Print())); + return null; + } } if (entityShaperExpression is null) @@ -1382,6 +1399,66 @@ when methodCallExpression.Method.IsGenericMethod } } + // For property setter selectors in ExecuteUpdate, we support only simple member access, EF.Function, etc. + // We also unwrap casts to interface/base class (#29618), as well as IncludeExpressions (which occur when the target entity has + // owned entities, #28727). + static bool TryProcessPropertyAccess( + IModel model, + ref Expression expression, + [NotNullWhen(true)] out EntityShaperExpression? entityShaperExpression) + { + expression = expression.UnwrapTypeConversion(out _); + + if (expression is MemberExpression { Expression : not null } memberExpression + && Unwrap(memberExpression.Expression) is EntityShaperExpression ese) + { + expression = memberExpression.Update(ese); + entityShaperExpression = ese; + return true; + } + + if (expression is MethodCallExpression mce) + { + if (mce.TryGetEFPropertyArguments(out var source, out _) + && Unwrap(source) is EntityShaperExpression ese1) + { + if (source != ese1) + { + var rewrittenArguments = mce.Arguments.ToArray(); + rewrittenArguments[0] = ese1; + expression = mce.Update(mce.Object, rewrittenArguments); + } + + entityShaperExpression = ese1; + return true; + } + + if (mce.TryGetIndexerArguments(model, out var source2, out _) + && Unwrap(source2) is EntityShaperExpression ese2) + { + expression = mce.Update(ese2, mce.Arguments); + entityShaperExpression = ese2; + return true; + } + } + + entityShaperExpression = null; + return false; + + static Expression Unwrap(Expression expression) + { + expression = expression.UnwrapTypeConversion(out _); + + while (expression is IncludeExpression includeExpression) + { + expression = includeExpression.EntityExpression; + } + + return expression; + } + } + + // Old quirked implementation only static bool IsValidPropertyAccess( IModel model, Expression expression, diff --git a/test/EFCore.Relational.Specification.Tests/BulkUpdates/InheritanceBulkUpdatesTestBase.cs b/test/EFCore.Relational.Specification.Tests/BulkUpdates/InheritanceBulkUpdatesTestBase.cs index 08b871a55d8..684ef3b26cb 100644 --- a/test/EFCore.Relational.Specification.Tests/BulkUpdates/InheritanceBulkUpdatesTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/BulkUpdates/InheritanceBulkUpdatesTestBase.cs @@ -158,5 +158,26 @@ public virtual Task Update_where_keyless_entity_mapped_to_sql_query(bool async) s => s.SetProperty(e => e.Name, "Eagle"), rowsAffectedCount: 1)); + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Update_with_interface_in_property_expression(bool async) + => AssertUpdate( + async, + ss => ss.Set(), + e => e, + s => s.SetProperty(c => ((ISugary)c).SugarGrams, 0), + rowsAffectedCount: 1); + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Update_with_interface_in_EF_Property_in_property_expression(bool async) + => AssertUpdate( + async, + ss => ss.Set(), + e => e, + // ReSharper disable once RedundantCast + s => s.SetProperty(c => EF.Property((ISugary)c, nameof(ISugary.SugarGrams)), 0), + rowsAffectedCount: 1); + protected abstract void ClearLog(); } diff --git a/test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs b/test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs index d28aa0ef245..10b54dc9b41 100644 --- a/test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs @@ -90,6 +90,25 @@ public class OtherReference } #nullable enable + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Update_non_owned_property_on_entity_with_owned(bool async) + { + var contextFactory = await InitializeAsync( + onModelCreating: mb => + { + mb.Entity().OwnsOne(o => o.OwnedReference); + }); + + await AssertUpdate( + async, + contextFactory.CreateContext, + ss => ss.Set(), + s => s.SetProperty(o => o.Title, "SomeValue"), + rowsAffectedCount: 0); + } + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual async Task Delete_predicate_based_on_optional_navigation(bool async) diff --git a/test/EFCore.Relational.Specification.Tests/BulkUpdates/TPTInheritanceBulkUpdatesTestBase.cs b/test/EFCore.Relational.Specification.Tests/BulkUpdates/TPTInheritanceBulkUpdatesTestBase.cs index a3f21b1faed..1a4fe584e5e 100644 --- a/test/EFCore.Relational.Specification.Tests/BulkUpdates/TPTInheritanceBulkUpdatesTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/BulkUpdates/TPTInheritanceBulkUpdatesTestBase.cs @@ -56,4 +56,14 @@ public override Task Update_where_hierarchy_derived(bool async) => AssertTranslationFailed( RelationalStrings.ExecuteOperationOnTPT("ExecuteUpdate", "Kiwi"), () => base.Update_where_hierarchy_derived(async)); + + public override Task Update_with_interface_in_property_expression(bool async) + => AssertTranslationFailed( + RelationalStrings.ExecuteOperationOnTPT("ExecuteUpdate", "Coke"), + () => base.Update_with_interface_in_property_expression(async)); + + public override Task Update_with_interface_in_EF_Property_in_property_expression(bool async) + => AssertTranslationFailed( + RelationalStrings.ExecuteOperationOnTPT("ExecuteUpdate", "Coke"), + () => base.Update_with_interface_in_EF_Property_in_property_expression(async)); } diff --git a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/InheritanceBulkUpdatesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/InheritanceBulkUpdatesSqlServerTest.cs index 846cf5f392c..e7dfdc2033d 100644 --- a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/InheritanceBulkUpdatesSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/InheritanceBulkUpdatesSqlServerTest.cs @@ -5,10 +5,14 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates; public class InheritanceBulkUpdatesSqlServerTest : InheritanceBulkUpdatesTestBase { - public InheritanceBulkUpdatesSqlServerTest(InheritanceBulkUpdatesSqlServerFixture fixture) + public InheritanceBulkUpdatesSqlServerTest( + InheritanceBulkUpdatesSqlServerFixture fixture, + ITestOutputHelper testOutputHelper) + : base(fixture) { ClearLog(); + // Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper); } [ConditionalFact] @@ -205,6 +209,32 @@ public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool AssertExecuteUpdateSql(); } + public override async Task Update_with_interface_in_property_expression(bool async) + { + await base.Update_with_interface_in_property_expression(async); + + AssertExecuteUpdateSql( +""" +UPDATE [d] +SET [d].[SugarGrams] = 0 +FROM [Drinks] AS [d] +WHERE [d].[Discriminator] = N'Coke' +"""); + } + + public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async) + { + await base.Update_with_interface_in_EF_Property_in_property_expression(async); + + AssertExecuteUpdateSql( +""" +UPDATE [d] +SET [d].[SugarGrams] = 0 +FROM [Drinks] AS [d] +WHERE [d].[Discriminator] = N'Coke' +"""); + } + protected override void ClearLog() => Fixture.TestSqlLoggerFactory.Clear(); diff --git a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs index 12a75a08257..e800288c5cf 100644 --- a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs @@ -41,6 +41,18 @@ public override async Task Delete_aggregate_root_when_table_sharing_with_non_own AssertSql(); } + public override async Task Update_non_owned_property_on_entity_with_owned(bool async) + { + await base.Update_non_owned_property_on_entity_with_owned(async); + + AssertSql( +""" +UPDATE [o] +SET [o].[Title] = N'SomeValue' +FROM [Owner] AS [o] +"""); + } + public override async Task Delete_predicate_based_on_optional_navigation(bool async) { await base.Delete_predicate_based_on_optional_navigation(async); diff --git a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/TPCInheritanceBulkUpdatesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/TPCInheritanceBulkUpdatesSqlServerTest.cs index 51b28b3cad0..ca5b94fc7a7 100644 --- a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/TPCInheritanceBulkUpdatesSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/TPCInheritanceBulkUpdatesSqlServerTest.cs @@ -5,10 +5,13 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates; public class TPCInheritanceBulkUpdatesSqlServerTest : TPCInheritanceBulkUpdatesTestBase { - public TPCInheritanceBulkUpdatesSqlServerTest(TPCInheritanceBulkUpdatesSqlServerFixture fixture) + public TPCInheritanceBulkUpdatesSqlServerTest( + TPCInheritanceBulkUpdatesSqlServerFixture fixture, + ITestOutputHelper testOutputHelper) : base(fixture) { ClearLog(); + // Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper); } [ConditionalFact] @@ -176,6 +179,30 @@ FROM [Kiwi] AS [k] """); } + public override async Task Update_with_interface_in_property_expression(bool async) + { + await base.Update_with_interface_in_property_expression(async); + + AssertExecuteUpdateSql( +""" +UPDATE [c] +SET [c].[SugarGrams] = 0 +FROM [Coke] AS [c] +"""); + } + + public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async) + { + await base.Update_with_interface_in_EF_Property_in_property_expression(async); + + AssertExecuteUpdateSql( +""" +UPDATE [c] +SET [c].[SugarGrams] = 0 +FROM [Coke] AS [c] +"""); + } + public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool async) { await base.Update_where_keyless_entity_mapped_to_sql_query(async); diff --git a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqlServerTest.cs index 768158f916c..3edf38df652 100644 --- a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqlServerTest.cs @@ -144,6 +144,20 @@ public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool AssertExecuteUpdateSql(); } + public override async Task Update_with_interface_in_property_expression(bool async) + { + await base.Update_with_interface_in_property_expression(async); + + AssertExecuteUpdateSql(); + } + + public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async) + { + await base.Update_with_interface_in_EF_Property_in_property_expression(async); + + AssertExecuteUpdateSql(); + } + protected override void ClearLog() => Fixture.TestSqlLoggerFactory.Clear(); diff --git a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/InheritanceBulkUpdatesSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/InheritanceBulkUpdatesSqliteTest.cs index d3f45ba0239..98b92ec2dd7 100644 --- a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/InheritanceBulkUpdatesSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/InheritanceBulkUpdatesSqliteTest.cs @@ -5,10 +5,13 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates; public class InheritanceBulkUpdatesSqliteTest : InheritanceBulkUpdatesTestBase { - public InheritanceBulkUpdatesSqliteTest(InheritanceBulkUpdatesSqliteFixture fixture) + public InheritanceBulkUpdatesSqliteTest( + InheritanceBulkUpdatesSqliteFixture fixture, + ITestOutputHelper testOutputHelper) : base(fixture) { ClearLog(); + // Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper); } [ConditionalFact] @@ -196,6 +199,30 @@ public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool AssertExecuteUpdateSql(); } + public override async Task Update_with_interface_in_property_expression(bool async) + { + await base.Update_with_interface_in_property_expression(async); + + AssertExecuteUpdateSql( +""" +UPDATE "Drinks" AS "d" +SET "SugarGrams" = 0 +WHERE "d"."Discriminator" = 'Coke' +"""); + } + + public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async) + { + await base.Update_with_interface_in_EF_Property_in_property_expression(async); + + AssertExecuteUpdateSql( +""" +UPDATE "Drinks" AS "d" +SET "SugarGrams" = 0 +WHERE "d"."Discriminator" = 'Coke' +"""); + } + protected override void ClearLog() => Fixture.TestSqlLoggerFactory.Clear(); diff --git a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs index 1b58fe6fc8a..df20f2232fd 100644 --- a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs @@ -39,6 +39,17 @@ public override async Task Delete_aggregate_root_when_table_sharing_with_non_own AssertSql(); } + public override async Task Update_non_owned_property_on_entity_with_owned(bool async) + { + await base.Update_non_owned_property_on_entity_with_owned(async); + + AssertSql( +""" +UPDATE "Owner" AS "o" +SET "Title" = 'SomeValue' +"""); + } + public override async Task Delete_predicate_based_on_optional_navigation(bool async) { await base.Delete_predicate_based_on_optional_navigation(async); diff --git a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPCInheritanceBulkUpdatesSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPCInheritanceBulkUpdatesSqliteTest.cs index 1d4b44ab21d..08f9b13d8bb 100644 --- a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPCInheritanceBulkUpdatesSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPCInheritanceBulkUpdatesSqliteTest.cs @@ -5,10 +5,13 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates; public class TPCInheritanceBulkUpdatesSqliteTest : TPCInheritanceBulkUpdatesTestBase { - public TPCInheritanceBulkUpdatesSqliteTest(TPCInheritanceBulkUpdatesSqliteFixture fixture) + public TPCInheritanceBulkUpdatesSqliteTest( + TPCInheritanceBulkUpdatesSqliteFixture fixture, + ITestOutputHelper testOutputHelper) : base(fixture) { ClearLog(); + // Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper); } [ConditionalFact] @@ -177,6 +180,28 @@ public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool AssertExecuteUpdateSql(); } + public override async Task Update_with_interface_in_property_expression(bool async) + { + await base.Update_with_interface_in_property_expression(async); + + AssertExecuteUpdateSql( +""" +UPDATE "Coke" AS "c" +SET "SugarGrams" = 0 +"""); + } + + public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async) + { + await base.Update_with_interface_in_EF_Property_in_property_expression(async); + + AssertExecuteUpdateSql( +""" +UPDATE "Coke" AS "c" +SET "SugarGrams" = 0 +"""); + } + protected override void ClearLog() => Fixture.TestSqlLoggerFactory.Clear(); diff --git a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqliteTest.cs index 296affc98e6..198a330eca4 100644 --- a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqliteTest.cs @@ -5,10 +5,13 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates; public class TPTInheritanceBulkUpdatesSqliteTest : TPTInheritanceBulkUpdatesTestBase { - public TPTInheritanceBulkUpdatesSqliteTest(TPTInheritanceBulkUpdatesSqliteFixture fixture) + public TPTInheritanceBulkUpdatesSqliteTest( + TPTInheritanceBulkUpdatesSqliteFixture fixture, + ITestOutputHelper testOutputHelper) : base(fixture) { ClearLog(); + // Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper); } [ConditionalFact] @@ -142,6 +145,20 @@ public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool AssertExecuteUpdateSql(); } + public override async Task Update_with_interface_in_property_expression(bool async) + { + await base.Update_with_interface_in_property_expression(async); + + AssertExecuteUpdateSql(); + } + + public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async) + { + await base.Update_with_interface_in_EF_Property_in_property_expression(async); + + AssertExecuteUpdateSql(); + } + protected override void ClearLog() => Fixture.TestSqlLoggerFactory.Clear();