Skip to content

Commit

Permalink
Fully unwrap IncludeExpression before ExecuteUpdate
Browse files Browse the repository at this point in the history 
Fixes dotnet#30528
  • Loading branch information
@roji
roji committed Mar 24, 2023
1 parent e5b8fb4 commit 945be53
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 36 deletions.
72 changes: 36 additions & 36 deletions src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1143,20 +1143,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 @@ -1178,6 +1164,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(PruneOwnedIncludes(includeExpression));
}

var propertyValueLambdaExpressions = new List<(LambdaExpression, Expression)>();
PopulateSetPropertyCalls(setPropertyCalls.Body, propertyValueLambdaExpressions, setPropertyCalls.Parameters[0]);
if (TranslationErrorDetails != null)
Expand Down Expand Up @@ -1382,11 +1375,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 @@ -1401,8 +1399,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 @@ -1411,7 +1409,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 @@ -1421,7 +1419,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 @@ -1435,7 +1433,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 @@ -1445,18 +1443,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)
Expand Down Expand Up @@ -1628,6 +1614,20 @@ private Expression RemapLambdaBody(ShapedQueryExpression shapedQueryExpression,
private Expression ExpandSharedTypeEntities(SelectExpression selectExpression, Expression lambdaBody)
=> _sharedTypeEntityExpandingExpressionVisitor.Expand(selectExpression, lambdaBody);

private 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;
}

private sealed class SharedTypeEntityExpandingExpressionVisitor : ExpressionVisitor
{
private readonly RelationalSqlTranslatingExpressionVisitor _sqlTranslator;
Expand Down
18 changes: 18 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,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)
Expand Down
12 changes: 12 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,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);
Expand Down
11 changes: 11 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,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);
Expand Down

0 comments on commit 945be53

Please sign in to comment.