Support interfaces and entities with owned types in ExecuteUpdate setter property lambda (dotnet#29672)

roji · roji · commit fbc493a4db37 · 2022-12-01T09:21:41.000+01:00
Fixes dotnet#29618 Fixes dotnet#28727 (cherry picked from commit d4be66b)
diff --git 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;
+
     /// <summary>
     ///     Creates a new instance of the <see cref="QueryableMethodTranslatingExpressionVisitor" /> class.
     /// </summary>
@@ -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
@@ -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<Coke>(),
+            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<Coke>(),
+            e => e,
+            // ReSharper disable once RedundantCast
+            s => s.SetProperty(c => EF.Property<int>((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
@@ -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<Context28671>(
+            onModelCreating: mb =>
+            {
+                mb.Entity<Owner>().OwnsOne(o => o.OwnedReference);
+            });
+
+        await AssertUpdate(
+            async,
+            contextFactory.CreateContext,
+            ss => ss.Set<Owner>(),
+            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
@@ -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
@@ -5,10 +5,14 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates;
 
 public class InheritanceBulkUpdatesSqlServerTest : InheritanceBulkUpdatesTestBase<InheritanceBulkUpdatesSqlServerFixture>
 {
-    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
@@ -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
@@ -5,10 +5,13 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates;
 
 public class TPCInheritanceBulkUpdatesSqlServerTest : TPCInheritanceBulkUpdatesTestBase<TPCInheritanceBulkUpdatesSqlServerFixture>
 {
-    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
@@ -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
@@ -5,10 +5,13 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates;
 
 public class InheritanceBulkUpdatesSqliteTest : InheritanceBulkUpdatesTestBase<InheritanceBulkUpdatesSqliteFixture>
 {
-    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
@@ -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
diff --git a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/TPTInheritanceBulkUpdatesSqliteTest.cs
