Skip to content

Commit

Permalink
Fixes around IncludeExpression for ExecuteUpdate/Delete (dotnet#30571)
Browse files Browse the repository at this point in the history 
* 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)
  • Loading branch information
@roji
roji committed Mar 25, 2023
1 parent 5077465 commit 1ff8573
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 37 deletions.
72 changes: 35 additions & 37 deletions src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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>
Expand All @@ -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)
Expand Down Expand Up @@ -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]));

Expand All @@ -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,
Expand All @@ -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;
Expand All @@ -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)
{
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down
52 changes: 52 additions & 0 deletions test/EFCore.Relational.Specification.Tests/BulkUpdates/NonSharedModelBulkUpdatesTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
24 changes: 24 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
14 changes: 14 additions & 0 deletions test/EFCore.Sqlite.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqliteTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 1ff8573

Please sign in to comment.