From 6dedc58767317be9fa5f9e0dc7413623990fbb11 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sat, 25 Mar 2023 12:00:18 +0300 Subject: [PATCH] Fixes around IncludeExpression for ExecuteUpdate/Delete (#30571) * Fully prune IncludeExpression before ExecuteUpdate, not just for the property lambda * Prune also non-owned Includes Fixes #30572 Fixes #30528 (cherry picked from commit 4c6f854b8cbf648550ba484ba7f50c7b7ef337ce) --- ...yableMethodTranslatingExpressionVisitor.cs | 48 ++++++++++++++--- .../NonSharedModelBulkUpdatesTestBase.cs | 52 +++++++++++++++++++ .../NonSharedModelBulkUpdatesSqlServerTest.cs | 24 +++++++++ .../NonSharedModelBulkUpdatesSqliteTest.cs | 14 +++++ 4 files changed, 132 insertions(+), 6 deletions(-) diff --git a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs index ce10009a52c..891971e0c6b 100644 --- a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs @@ -20,6 +20,10 @@ public class RelationalQueryableMethodTranslatingExpressionVisitor : QueryableMe private static readonly bool QuirkEnabled28727 = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue28727", out var enabled) && enabled; + private static readonly bool QuirkEnabled30528 + = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue30528", out var enabled) && enabled; + private static readonly bool QuirkEnabled30572 + = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue30572", out var enabled) && enabled; /// /// Creates a new instance of the class. @@ -1029,7 +1033,14 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp { if (source.ShaperExpression is IncludeExpression includeExpression) { - source = source.UpdateShaperExpression(PruneOwnedIncludes(includeExpression)); + if (QuirkEnabled30572) + { + source = source.UpdateShaperExpression(PruneOwnedIncludes(includeExpression)); + } + else + { + source = source.UpdateShaperExpression(PruneIncludes(includeExpression)); + } } if (source.ShaperExpression is not EntityShaperExpression entityShaperExpression) @@ -1129,6 +1140,7 @@ static bool AreOtherNonOwnedEntityTypesInTheTable(IEntityType rootType, ITableBa return TranslateExecuteDelete((ShapedQueryExpression)Visit(newSource)); + // Old quirked implementation for #30572 static Expression PruneOwnedIncludes(IncludeExpression includeExpression) { if (includeExpression.Navigation is ISkipNavigation @@ -1163,6 +1175,16 @@ static Expression PruneOwnedIncludes(IncludeExpression includeExpression) ShapedQueryExpression source, LambdaExpression setPropertyCalls) { + if (!QuirkEnabled30528) + { + // 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) @@ -1386,7 +1408,6 @@ when methodCallExpression.Method.IsGenericMethod && methodCallExpression.Method.Name == nameof(SetPropertyCalls.SetProperty) && methodCallExpression.Method.DeclaringType!.IsGenericType && methodCallExpression.Method.DeclaringType.GetGenericTypeDefinition() == typeof(SetPropertyCalls<>): - list.Add(((LambdaExpression)methodCallExpression.Arguments[0], methodCallExpression.Arguments[1])); PopulateSetPropertyCalls(methodCallExpression.Object!, list, parameter); @@ -1400,8 +1421,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, @@ -1449,9 +1470,12 @@ static Expression Unwrap(Expression expression) { expression = expression.UnwrapTypeConversion(out _); - while (expression is IncludeExpression includeExpression) + if (QuirkEnabled30528) { - expression = includeExpression.EntityExpression; + while (expression is IncludeExpression includeExpression) + { + expression = includeExpression.EntityExpression; + } } return expression; @@ -1660,6 +1684,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; diff --git a/test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs b/test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs index 10b54dc9b41..65597cc529c 100644 --- a/test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs +++ b/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( + onModelCreating: mb => + { + mb.Entity().OwnsOne(o => o.OwnedReference); + }); + + await AssertUpdate( + async, + contextFactory.CreateContext, + ss => ss.Set(), + 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(); + await AssertDelete(async, contextFactory.CreateContext, ss => ss.Set(), rowsAffectedCount: 0); + } + + protected class Context30572 : DbContext + { + public Context30572(DbContextOptions options) + : base(options) + { + } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + => modelBuilder.Entity().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) diff --git a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs index e800288c5cf..0e2cc91f2f0 100644 --- a/test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs +++ b/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); diff --git a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs index df20f2232fd..c1e08a58be1 100644 --- a/test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs +++ b/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(() => 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);