Fixes around IncludeExpression for ExecuteUpdate/Delete (dotnet#30571)

* Fully prune IncludeExpression before ExecuteUpdate, not just for the
  property lambda
* Prune also non-owned Includes

Fixes dotnet#30572
Fixes dotnet#30528

(cherry picked from commit 4c6f854)

Author: roji

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

@@ -1029,7 +1029,7 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
     {
         if (source.ShaperExpression is IncludeExpression includeExpression)
         {
-            source = source.UpdateShaperExpression(PruneOwnedIncludes(includeExpression));
+            source = source.UpdateShaperExpression(PruneIncludes(includeExpression));
         }
 
         if (source.ShaperExpression is not EntityShaperExpression entityShaperExpression)
@@ -1128,20 +1128,6 @@ static bool AreOtherNonOwnedEntityTypesInTheTable(IEntityType rootType, ITableBa
             Expression.Quote(Expression.Lambda(predicateBody, entityParameter)));
 
         return TranslateExecuteDelete((ShapedQueryExpression)Visit(newSource));
-
-        static Expression PruneOwnedIncludes(IncludeExpression includeExpression)
-        {
-            if (includeExpression.Navigation is ISkipNavigation
-                || includeExpression.Navigation is not INavigation navigation
-                || !navigation.ForeignKey.IsOwnership)
-            {
-                return includeExpression;
-            }
-
-            return includeExpression.EntityExpression is IncludeExpression innerIncludeExpression
-                ? PruneOwnedIncludes(innerIncludeExpression)
-                : includeExpression.EntityExpression;
-        }
     }
 
     /// <summary>
@@ -1163,6 +1149,13 @@ static Expression PruneOwnedIncludes(IncludeExpression includeExpression)
         ShapedQueryExpression source,
         LambdaExpression setPropertyCalls)
     {
+        // Our source may have IncludeExpressions because of owned entities or auto-include; unwrap these, as they're meaningless for
+        // ExecuteUpdate's lambdas. Note that we don't currently support updates across tables.
+        if (source.ShaperExpression is IncludeExpression includeExpression)
+        {
+            source = source.UpdateShaperExpression(PruneIncludes(includeExpression));
+        }
+
         var propertyValueLambdaExpressions = new List<(LambdaExpression, Expression)>();
         PopulateSetPropertyCalls(setPropertyCalls.Body, propertyValueLambdaExpressions, setPropertyCalls.Parameters[0]);
         if (TranslationErrorDetails != null)
@@ -1381,11 +1374,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]));
 
@@ -1400,8 +1398,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 owned IncludeExpressions have already been pruned from the
+        // source before remapping the lambda (#28727).
         static bool TryProcessPropertyAccess(
             IModel model,
             ref Expression expression,
@@ -1410,7 +1408,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;
@@ -1420,7 +1418,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)
                     {
@@ -1434,7 +1432,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;
@@ -1444,18 +1442,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;
-            }
         }
 
         // Old quirked implementation only
@@ -1660,6 +1646,18 @@ private Expression RemapLambdaBody(ShapedQueryExpression shapedQueryExpression,
     private Expression ExpandSharedTypeEntities(SelectExpression selectExpression, Expression lambdaBody)
         => _sharedTypeEntityExpandingExpressionVisitor.Expand(selectExpression, lambdaBody);
 
+    private static Expression PruneIncludes(IncludeExpression includeExpression)
+    {
+        if (includeExpression.Navigation is ISkipNavigation or not INavigation)
+        {
+            return includeExpression;
+        }
+
+        return includeExpression.EntityExpression is IncludeExpression innerIncludeExpression
+            ? PruneIncludes(innerIncludeExpression)
+            : includeExpression.EntityExpression;
+    }
+
     private sealed class SharedTypeEntityExpandingExpressionVisitor : ExpressionVisitor
     {
         private readonly RelationalSqlTranslatingExpressionVisitor _sqlTranslator;

test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs

@@ -109,6 +109,58 @@ 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_entity_with_auto_include(bool async)
+    {
+        var contextFactory = await InitializeAsync<Context30572>();
+        await AssertDelete(async, contextFactory.CreateContext, ss => ss.Set<Context30572_Principal>(), rowsAffectedCount: 0);
+    }
+
+    protected class Context30572 : DbContext
+    {
+        public Context30572(DbContextOptions options)
+            : base(options)
+        {
+        }
+
+        protected override void OnModelCreating(ModelBuilder modelBuilder)
+            => modelBuilder.Entity<Context30572_Principal>().Navigation(o => o.Dependent).AutoInclude();
+    }
+
+    public class Context30572_Principal
+    {
+        public int Id { get; set; }
+        public string? Title { get; set; }
+
+        public Context30572_Dependent? Dependent { get; set; }
+    }
+
+    public class Context30572_Dependent
+    {
+        public int Id { get; set; }
+
+        public int Number { get; set; }
+    }
+
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual async Task Delete_predicate_based_on_optional_navigation(bool async)

test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs

@@ -53,6 +53,30 @@ 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_entity_with_auto_include(bool async)
+    {
+        await base.Delete_entity_with_auto_include(async);
+
+        AssertSql(
+"""
+DELETE FROM [c]
+FROM [Context30572_Principal] AS [c]
+LEFT JOIN [Context30572_Dependent] AS [c0] ON [c].[DependentId] = [c0].[Id]
+""");
+    }
+
     public override async Task Delete_predicate_based_on_optional_navigation(bool async)
     {
         await base.Delete_predicate_based_on_optional_navigation(async);

test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs

@@ -50,6 +50,20 @@ 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 Task Delete_entity_with_auto_include(bool async)
+        => Assert.ThrowsAsync<ArgumentOutOfRangeException>(() => base.Delete_entity_with_auto_include(async));
+
     public override async Task Delete_predicate_based_on_optional_navigation(bool async)
     {
         await base.Delete_predicate_based_on_optional_navigation(async);