Skip to content

Commit

Permalink
Support interfaces and entities with owned types in ExecuteUpdate set…
Browse files Browse the repository at this point in the history
…ter property lambda (dotnet#29672)

Fixes dotnet#29618
Fixes dotnet#28727
  • Loading branch information
roji authored Nov 30, 2022
1 parent 315be6c commit d4be66b
Show file tree
Hide file tree
Showing 14 changed files with 260 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,7 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio
return null;
}

if (!(expression is NewExpression
|| expression is MemberInitExpression
|| expression is EntityShaperExpression
|| expression is IncludeExpression))
if (expression is not NewExpression and not MemberInitExpression and not EntityShaperExpression and not IncludeExpression)
{
if (_indexBasedBinding)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1195,8 +1195,8 @@ static Expression PruneOwnedIncludes(IncludeExpression includeExpression)
foreach (var (propertyExpression, _) in propertyValueLambdaExpressions)
{
var left = RemapLambdaBody(source, propertyExpression);
left = left.UnwrapTypeConversion(out _);
if (!IsValidPropertyAccess(RelationalDependencies.Model, left, out var ese))

if (!TryProcessPropertyAccess(RelationalDependencies.Model, ref left, out var ese))
{
AddTranslationErrorDetails(RelationalStrings.InvalidPropertyInSetProperty(propertyExpression.Print()));
return null;
Expand Down Expand Up @@ -1399,36 +1399,63 @@ when methodCallExpression.Method.IsGenericMethod
}
}

static bool IsValidPropertyAccess(
// 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).
static bool TryProcessPropertyAccess(
IModel model,
Expression expression,
ref Expression expression,
[NotNullWhen(true)] out EntityShaperExpression? entityShaperExpression)
{
if (expression is MemberExpression { Expression: EntityShaperExpression ese })
expression = expression.UnwrapTypeConversion(out _);

if (expression is MemberExpression { Expression : not null } memberExpression
&& Unwrap(memberExpression.Expression) is EntityShaperExpression ese)
{
expression = memberExpression.Update(ese);
entityShaperExpression = ese;
return true;
}

if (expression is MethodCallExpression mce)
{
if (mce.TryGetEFPropertyArguments(out var source, out _)
&& source is EntityShaperExpression ese1)
&& Unwrap(source) is EntityShaperExpression ese1)
{
if (source != ese1)
{
var rewrittenArguments = mce.Arguments.ToArray();
rewrittenArguments[0] = ese1;
expression = mce.Update(mce.Object, rewrittenArguments);
}

entityShaperExpression = ese1;
return true;
}

if (mce.TryGetIndexerArguments(model, out var source2, out _)
&& source2 is EntityShaperExpression ese2)
&& Unwrap(source2) is EntityShaperExpression ese2)
{
expression = mce.Update(ese2, mce.Arguments);
entityShaperExpression = ese2;
return true;
}
}

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 @@ -1645,11 +1672,9 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
}

protected override Expression VisitExtension(Expression extensionExpression)
=> extensionExpression is EntityShaperExpression
|| extensionExpression is ShapedQueryExpression
|| extensionExpression is GroupByShaperExpression
? extensionExpression
: base.VisitExtension(extensionExpression);
=> extensionExpression is EntityShaperExpression or ShapedQueryExpression or GroupByShaperExpression
? extensionExpression
: base.VisitExtension(extensionExpression);

private Expression? TryExpand(Expression? source, MemberIdentity member)
{
Expand Down
8 changes: 4 additions & 4 deletions src/Shared/ExpressionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ public static LambdaExpression UnwrapLambdaFromQuote(this Expression expression)
public static Expression? UnwrapTypeConversion(this Expression? expression, out Type? convertedType)
{
convertedType = null;
while (expression is UnaryExpression unaryExpression
&& (unaryExpression.NodeType == ExpressionType.Convert
|| unaryExpression.NodeType == ExpressionType.ConvertChecked
|| unaryExpression.NodeType == ExpressionType.TypeAs))
while (expression is UnaryExpression
{
NodeType: ExpressionType.Convert or ExpressionType.ConvertChecked or ExpressionType.TypeAs
} unaryExpression)
{
expression = unaryExpression.Operand;
if (unaryExpression.Type != typeof(object) // Ignore object conversion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,5 +158,26 @@ public virtual Task Update_where_keyless_entity_mapped_to_sql_query(bool async)
s => s.SetProperty(e => e.Name, "Eagle"),
rowsAffectedCount: 1));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Update_with_interface_in_property_expression(bool async)
=> AssertUpdate(
async,
ss => ss.Set<Coke>(),
e => e,
s => s.SetProperty(c => ((ISugary)c).SugarGrams, 0),
rowsAffectedCount: 1);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Update_with_interface_in_EF_Property_in_property_expression(bool async)
=> AssertUpdate(
async,
ss => ss.Set<Coke>(),
e => e,
// ReSharper disable once RedundantCast
s => s.SetProperty(c => EF.Property<int>((ISugary)c, nameof(ISugary.SugarGrams)), 0),
rowsAffectedCount: 1);

protected abstract void ClearLog();
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,25 @@ public class OtherReference
}

#nullable enable

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Update_non_owned_property_on_entity_with_owned(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, "SomeValue"),
rowsAffectedCount: 0);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Delete_predicate_based_on_optional_navigation(bool async)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,14 @@ public override Task Update_where_hierarchy_derived(bool async)
=> AssertTranslationFailed(
RelationalStrings.ExecuteOperationOnTPT("ExecuteUpdate", "Kiwi"),
() => base.Update_where_hierarchy_derived(async));

public override Task Update_with_interface_in_property_expression(bool async)
=> AssertTranslationFailed(
RelationalStrings.ExecuteOperationOnTPT("ExecuteUpdate", "Coke"),
() => base.Update_with_interface_in_property_expression(async));

public override Task Update_with_interface_in_EF_Property_in_property_expression(bool async)
=> AssertTranslationFailed(
RelationalStrings.ExecuteOperationOnTPT("ExecuteUpdate", "Coke"),
() => base.Update_with_interface_in_EF_Property_in_property_expression(async));
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates;

public class InheritanceBulkUpdatesSqlServerTest : InheritanceBulkUpdatesTestBase<InheritanceBulkUpdatesSqlServerFixture>
{
public InheritanceBulkUpdatesSqlServerTest(InheritanceBulkUpdatesSqlServerFixture fixture)
public InheritanceBulkUpdatesSqlServerTest(
InheritanceBulkUpdatesSqlServerFixture fixture,
ITestOutputHelper testOutputHelper)

: base(fixture)
{
ClearLog();
// Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
}

[ConditionalFact]
Expand Down Expand Up @@ -205,6 +209,32 @@ public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool
AssertExecuteUpdateSql();
}

public override async Task Update_with_interface_in_property_expression(bool async)
{
await base.Update_with_interface_in_property_expression(async);

AssertExecuteUpdateSql(
"""
UPDATE [d]
SET [d].[SugarGrams] = 0
FROM [Drinks] AS [d]
WHERE [d].[Discriminator] = N'Coke'
""");
}

public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async)
{
await base.Update_with_interface_in_EF_Property_in_property_expression(async);

AssertExecuteUpdateSql(
"""
UPDATE [d]
SET [d].[SugarGrams] = 0
FROM [Drinks] AS [d]
WHERE [d].[Discriminator] = N'Coke'
""");
}

protected override void ClearLog()
=> Fixture.TestSqlLoggerFactory.Clear();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,18 @@ public override async Task Delete_aggregate_root_when_table_sharing_with_non_own
AssertSql();
}

public override async Task Update_non_owned_property_on_entity_with_owned(bool async)
{
await base.Update_non_owned_property_on_entity_with_owned(async);

AssertSql(
"""
UPDATE [o]
SET [o].[Title] = N'SomeValue'
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates;

public class TPCInheritanceBulkUpdatesSqlServerTest : TPCInheritanceBulkUpdatesTestBase<TPCInheritanceBulkUpdatesSqlServerFixture>
{
public TPCInheritanceBulkUpdatesSqlServerTest(TPCInheritanceBulkUpdatesSqlServerFixture fixture)
public TPCInheritanceBulkUpdatesSqlServerTest(
TPCInheritanceBulkUpdatesSqlServerFixture fixture,
ITestOutputHelper testOutputHelper)
: base(fixture)
{
ClearLog();
// Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
}

[ConditionalFact]
Expand Down Expand Up @@ -176,6 +179,30 @@ FROM [Kiwi] AS [k]
""");
}

public override async Task Update_with_interface_in_property_expression(bool async)
{
await base.Update_with_interface_in_property_expression(async);

AssertExecuteUpdateSql(
"""
UPDATE [c]
SET [c].[SugarGrams] = 0
FROM [Coke] AS [c]
""");
}

public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async)
{
await base.Update_with_interface_in_EF_Property_in_property_expression(async);

AssertExecuteUpdateSql(
"""
UPDATE [c]
SET [c].[SugarGrams] = 0
FROM [Coke] AS [c]
""");
}

public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool async)
{
await base.Update_where_keyless_entity_mapped_to_sql_query(async);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,20 @@ public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool
AssertExecuteUpdateSql();
}

public override async Task Update_with_interface_in_property_expression(bool async)
{
await base.Update_with_interface_in_property_expression(async);

AssertExecuteUpdateSql();
}

public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async)
{
await base.Update_with_interface_in_EF_Property_in_property_expression(async);

AssertExecuteUpdateSql();
}

protected override void ClearLog()
=> Fixture.TestSqlLoggerFactory.Clear();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@ namespace Microsoft.EntityFrameworkCore.BulkUpdates;

public class InheritanceBulkUpdatesSqliteTest : InheritanceBulkUpdatesTestBase<InheritanceBulkUpdatesSqliteFixture>
{
public InheritanceBulkUpdatesSqliteTest(InheritanceBulkUpdatesSqliteFixture fixture)
public InheritanceBulkUpdatesSqliteTest(
InheritanceBulkUpdatesSqliteFixture fixture,
ITestOutputHelper testOutputHelper)
: base(fixture)
{
ClearLog();
// Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
}

[ConditionalFact]
Expand Down Expand Up @@ -196,6 +199,30 @@ public override async Task Update_where_keyless_entity_mapped_to_sql_query(bool
AssertExecuteUpdateSql();
}

public override async Task Update_with_interface_in_property_expression(bool async)
{
await base.Update_with_interface_in_property_expression(async);

AssertExecuteUpdateSql(
"""
UPDATE "Drinks" AS "d"
SET "SugarGrams" = 0
WHERE "d"."Discriminator" = 'Coke'
""");
}

public override async Task Update_with_interface_in_EF_Property_in_property_expression(bool async)
{
await base.Update_with_interface_in_EF_Property_in_property_expression(async);

AssertExecuteUpdateSql(
"""
UPDATE "Drinks" AS "d"
SET "SugarGrams" = 0
WHERE "d"."Discriminator" = 'Coke'
""");
}

protected override void ClearLog()
=> Fixture.TestSqlLoggerFactory.Clear();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ public override async Task Delete_aggregate_root_when_table_sharing_with_non_own
AssertSql();
}

public override async Task Update_non_owned_property_on_entity_with_owned(bool async)
{
await base.Update_non_owned_property_on_entity_with_owned(async);

AssertSql(
"""
UPDATE "Owner" AS "o"
SET "Title" = 'SomeValue'
""");
}

public override async Task Delete_predicate_based_on_optional_navigation(bool async)
{
await base.Delete_predicate_based_on_optional_navigation(async);
Expand Down
Loading

0 comments on commit d4be66b

Please sign in to comment.