Fully unwrap IncludeExpression before ExecuteUpdate

Fixes dotnet#30528
roji · Mar 24, 2023 · 8153f74 · 8153f74
1 parent e5b8fb4
commit 8153f74
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 22 deletions.
diff --git a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
@@ -1191,6 +1191,13 @@ static Expression PruneOwnedIncludes(IncludeExpression includeExpression)
             return null;
         }
 
+        // Our source may have IncludeExpressions because of owned entities or auto-include; unwrap these, as they're meaningless for
+        // ExecuteUpdate.
+        while (source.ShaperExpression is IncludeExpression includeExpression)
+        {
+            source = source.UpdateShaperExpression(includeExpression.EntityExpression);
+        }
+
         EntityShaperExpression? entityShaperExpression = null;
         var remappedUnwrappedLeftExpressions = new List<Expression>();
         foreach (var (propertyExpression, _) in propertyValueLambdaExpressions)
@@ -1382,11 +1389,16 @@ void PopulateSetPropertyCalls(
                     when parameter == p:
                     break;
 
-                case MethodCallExpression methodCallExpression
-                    when methodCallExpression.Method.IsGenericMethod
-                    && methodCallExpression.Method.Name == nameof(SetPropertyCalls<int>.SetProperty)
-                    && methodCallExpression.Method.DeclaringType!.IsGenericType
-                    && methodCallExpression.Method.DeclaringType.GetGenericTypeDefinition() == typeof(SetPropertyCalls<>):
+                case MethodCallExpression
+                    {
+                        Method:
+                        {
+                            IsGenericMethod: true,
+                            Name: nameof(SetPropertyCalls<int>.SetProperty),
+                            DeclaringType.IsGenericType: true
+                        }
+                    } methodCallExpression
+                    when methodCallExpression.Method.DeclaringType.GetGenericTypeDefinition() == typeof(SetPropertyCalls<>):
 
                     list.Add(((LambdaExpression)methodCallExpression.Arguments[0], methodCallExpression.Arguments[1]));
 
@@ -1401,8 +1413,8 @@ 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).
+        // We also unwrap casts to interface/base class (#29618). Note that IncludeExpressions (which occur when the target entity has
+        // owned entities, #28727) have already been removed from the source before remapping the lambda.
         static bool TryProcessPropertyAccess(
             IModel model,
             ref Expression expression,
@@ -1411,7 +1423,7 @@ static bool TryProcessPropertyAccess(
             expression = expression.UnwrapTypeConversion(out _);
 
             if (expression is MemberExpression { Expression : not null } memberExpression
-                && Unwrap(memberExpression.Expression) is EntityShaperExpression ese)
+                && memberExpression.Expression.UnwrapTypeConversion(out _) is EntityShaperExpression ese)
             {
                 expression = memberExpression.Update(ese);
                 entityShaperExpression = ese;
@@ -1421,7 +1433,7 @@ static bool TryProcessPropertyAccess(
             if (expression is MethodCallExpression mce)
             {
                 if (mce.TryGetEFPropertyArguments(out var source, out _)
-                    && Unwrap(source) is EntityShaperExpression ese1)
+                    && source.UnwrapTypeConversion(out _) is EntityShaperExpression ese1)
                 {
                     if (source != ese1)
                     {
@@ -1435,7 +1447,7 @@ static bool TryProcessPropertyAccess(
                 }
 
                 if (mce.TryGetIndexerArguments(model, out var source2, out _)
-                    && Unwrap(source2) is EntityShaperExpression ese2)
+                    && source2.UnwrapTypeConversion(out _) is EntityShaperExpression ese2)
                 {
                     expression = mce.Update(ese2, mce.Arguments);
                     entityShaperExpression = ese2;
@@ -1445,18 +1457,6 @@ static bool TryProcessPropertyAccess(
 
             entityShaperExpression = null;
             return false;
-
-            static Expression Unwrap(Expression expression)
-            {
-                expression = expression.UnwrapTypeConversion(out _);
-
-                while (expression is IncludeExpression includeExpression)
-                {
-                    expression = includeExpression.EntityExpression;
-                }
-
-                return expression;
-            }
         }
 
         static Expression GetEntitySource(IModel model, Expression propertyAccessExpression)

diff --git a/test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs b/test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs
@@ -109,6 +109,24 @@ await AssertUpdate(
             rowsAffectedCount: 0);
     }
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual async Task Update_non_owned_property_on_entity_with_owned2(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, o => o.Title + "_Suffix"),
+            rowsAffectedCount: 0);
+    }
+
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual async Task Delete_predicate_based_on_optional_navigation(bool async)

diff --git a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs
@@ -53,6 +53,18 @@ FROM [Owner] AS [o]
 """);
     }
 
+    public override async Task Update_non_owned_property_on_entity_with_owned2(bool async)
+    {
+        await base.Update_non_owned_property_on_entity_with_owned2(async);
+
+        AssertSql(
+"""
+UPDATE [o]
+SET [o].[Title] = COALESCE([o].[Title], N'') + N'_Suffix'
+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.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs
@@ -50,6 +50,17 @@ public override async Task Update_non_owned_property_on_entity_with_owned(bool a
 """);
     }
 
+    public override async Task Update_non_owned_property_on_entity_with_owned2(bool async)
+    {
+        await base.Update_non_owned_property_on_entity_with_owned2(async);
+
+        AssertSql(
+"""
+UPDATE "Owner" AS "o"
+SET "Title" = COALESCE("o"."Title", '') || '_Suffix'
+""");
+    }
+
     public override async Task Delete_predicate_based_on_optional_navigation(bool async)
     {
         await base.Delete_predicate_based_on_optional_navigation(async);