ExecuteUpdate: Allow using other tables in the query to generate result set #28731
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,15 +80,14 @@ protected override Expression VisitUpdate(UpdateExpression updateExpression) | |
| var selectExpression = updateExpression.SelectExpression; | ||
|
|
||
| if (selectExpression.Offset == null | ||
| && selectExpression.Having == null | ||
| && selectExpression.Orderings.Count == 0 | ||
| && selectExpression.GroupBy.Count == 0 | ||
| && selectExpression.Projection.Count == 0) | ||
| { | ||
| Sql.Append("UPDATE "); | ||
| GenerateTop(selectExpression); | ||
|
There was a problem hiding this comment. This is so weird... especially since it doesn't respect any ordering... So what, change only 10 random rows and leave the others??
There was a problem hiding this comment. I want you to change destiny of people but only x number of them. Rest can suffer their entire life! There was a problem hiding this comment. Wait, actually.... just to confirm... If the user does OrderBy and then Take, will it translate correctly? I mean, if you push both operators in the the subquery it's fine, but if the TOP is at the top-level and the OrderBy is in the query, then the translation is bad... There was a problem hiding this comment. (removing auto-merge just to be on the safe side) There was a problem hiding this comment. existing order by causes subquery and top goes inside the subquery. |
||
|
|
||
| Sql.AppendLine($"{Dependencies.SqlGenerationHelper.DelimitIdentifier(updateExpression.Table.Alias)}"); | ||
| using (Sql.Indent()) | ||
| { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,6 +146,52 @@ protected override bool IsValidSelectExpressionForExecuteDelete( | |
| return false; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to | ||
| /// the same compatibility standards as public APIs. It may be changed or removed without notice in | ||
| /// any release. You should only use it directly in your code with extreme caution and knowing that | ||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| protected override bool IsValidSelectExpressionForExecuteUpdate( | ||
| SelectExpression selectExpression, | ||
| EntityShaperExpression entityShaperExpression, | ||
| [NotNullWhen(true)] out TableExpression? tableExpression) | ||
| { | ||
| if (selectExpression.Offset == null | ||
| // If entity type has primary key then Distinct is no-op | ||
| && (!selectExpression.IsDistinct || entityShaperExpression.EntityType.FindPrimaryKey() != null) | ||
| && selectExpression.GroupBy.Count == 0 | ||
| && selectExpression.Having == null | ||
| && selectExpression.Orderings.Count == 0) | ||
| { | ||
| TableExpressionBase table; | ||
| if (selectExpression.Tables.Count == 1) | ||
| { | ||
| table = selectExpression.Tables[0]; | ||
| } | ||
| else | ||
| { | ||
| var projectionBindingExpression = (ProjectionBindingExpression)entityShaperExpression.ValueBufferExpression; | ||
| var entityProjectionExpression = (EntityProjectionExpression)selectExpression.GetProjection(projectionBindingExpression); | ||
| var column = entityProjectionExpression.BindProperty(entityShaperExpression.EntityType.GetProperties().First()); | ||
| table = column.Table; | ||
| if (table is JoinExpressionBase joinExpressionBase) | ||
| { | ||
| table = joinExpressionBase.Table; | ||
| } | ||
| } | ||
|
|
||
| if (table is TableExpression te) | ||
| { | ||
| tableExpression = te; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| tableExpression = null; | ||
| return false; | ||
| } | ||
|
|
||
| private sealed class TemporalAnnotationApplyingExpressionVisitor : ExpressionVisitor | ||
| { | ||
| private readonly Func<TableExpression, TableExpressionBase> _annotationApplyingFunc; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -343,6 +343,29 @@ public virtual Task Update_where_constant(bool async) | |
| rowsAffectedCount: 8, | ||
| (b, a) => a.ForEach(c => Assert.Equal("Updated", c.ContactName))); | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual async Task Update_where_parameter_in_predicate(bool async) | ||
| { | ||
| var customer = "ALFKI"; | ||
| await AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Customer>().Where(c => c.CustomerID == customer), | ||
| e => e, | ||
| s => s.SetProperty(c => c.ContactName, c => "Updated"), | ||
| rowsAffectedCount: 1, | ||
| (b, a) => a.ForEach(c => Assert.Equal("Updated", c.ContactName))); | ||
|
|
||
| customer = null; | ||
| await AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Customer>().Where(c => c.CustomerID == customer), | ||
| e => e, | ||
| s => s.SetProperty(c => c.ContactName, c => "Updated"), | ||
| rowsAffectedCount: 0, | ||
| (b, a) => a.ForEach(c => Assert.Equal("Updated", c.ContactName))); | ||
| } | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_parameter(bool async) | ||
|
|
@@ -357,6 +380,113 @@ public virtual Task Update_where_parameter(bool async) | |
| (b, a) => a.ForEach(c => Assert.Equal("Abc", c.ContactName))); | ||
| } | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_take_constant(bool async) | ||
| => AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Customer>().Where(c => c.CustomerID.StartsWith("F")).Take(4), | ||
| e => e, | ||
| s => s.SetProperty(c => c.ContactName, c => "Updated"), | ||
| rowsAffectedCount: 4, | ||
| (b, a) => a.ForEach(c => Assert.Equal("Updated", c.ContactName))); | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_group_by_aggregate_constant(bool async) | ||
| => AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Customer>() | ||
| .Where(c => c.CustomerID == ss.Set<Order>() | ||
| .GroupBy(e => e.CustomerID).Where(g => g.Count() > 11).Select(e => e.Key).FirstOrDefault()), | ||
| e => e, | ||
| s => s.SetProperty(c => c.ContactName, c => "Updated"), | ||
| rowsAffectedCount: 1, | ||
| (b, a) => a.ForEach(c => Assert.Equal("Updated", c.ContactName))); | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_group_by_first_constant(bool async) | ||
| => AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Customer>() | ||
| .Where(c => c.CustomerID == ss.Set<Order>() | ||
| .GroupBy(e => e.CustomerID).Where(g => g.Count() > 11).Select(e => e.First().CustomerID).FirstOrDefault()), | ||
| e => e, | ||
| s => s.SetProperty(c => c.ContactName, c => "Updated"), | ||
| rowsAffectedCount: 1, | ||
| (b, a) => a.ForEach(c => Assert.Equal("Updated", c.ContactName))); | ||
|
|
||
| [ConditionalTheory(Skip = "Issue#26753")] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_group_by_first_constant_2(bool async) | ||
| => AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Customer>() | ||
| .Where(c => c == ss.Set<Order>() | ||
| .GroupBy(e => e.CustomerID).Where(g => g.Count() > 11).Select(e => e.First().Customer).FirstOrDefault()), | ||
| e => e, | ||
| s => s.SetProperty(c => c.ContactName, c => "Updated"), | ||
| rowsAffectedCount: 1, | ||
| (b, a) => a.ForEach(c => Assert.Equal("Updated", c.ContactName))); | ||
|
|
||
| [ConditionalTheory(Skip = "Issue#28524")] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_group_by_first_constant_3(bool async) | ||
| => AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Customer>() | ||
| .Where(c => ss.Set<Order>() | ||
| .GroupBy(e => e.CustomerID).Where(g => g.Count() > 11).Select(e => e.First().Customer).Contains(c)), | ||
| e => e, | ||
| s => s.SetProperty(c => c.ContactName, c => "Updated"), | ||
| rowsAffectedCount: 1, | ||
| (b, a) => a.ForEach(c => Assert.Equal("Updated", c.ContactName))); | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_distinct_constant(bool async) | ||
| => AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Customer>().Where(c => c.CustomerID.StartsWith("F")).Distinct(), | ||
| e => e, | ||
| s => s.SetProperty(c => c.ContactName, c => "Updated"), | ||
| rowsAffectedCount: 8, | ||
| (b, a) => a.ForEach(c => Assert.Equal("Updated", c.ContactName))); | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_using_navigation(bool async) | ||
| => AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Order>().Where(o => o.Customer.City == "Seattle"), | ||
| e => e, | ||
| s => s.SetProperty(c => c.OrderDate, c => null), | ||
| rowsAffectedCount: 14, | ||
| (b, a) => a.ForEach(c => Assert.Null(c.OrderDate))); | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_using_navigation_2(bool async) | ||
| => AssertUpdate( | ||
| async, | ||
| ss => ss.Set<OrderDetail>().Where(od => od.Order.Customer.City == "Seattle"), | ||
| e => e, | ||
| s => s.SetProperty(c => c.Quantity, c => 1), | ||
| rowsAffectedCount: 40, | ||
| (b, a) => a.ForEach(c => Assert.Equal(1, c.Quantity))); | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_select_many(bool async) | ||
| => AssertUpdate( | ||
| async, | ||
| ss => ss.Set<Customer>().Where(c => c.CustomerID.StartsWith("F")).SelectMany(c => c.Orders), | ||
| e => e, | ||
| s => s.SetProperty(c => c.OrderDate, c => null), | ||
| rowsAffectedCount: 63, | ||
| (b, a) => a.ForEach(c => Assert.Null(c.OrderDate))); | ||
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Update_where_using_property_plus_constant(bool async) | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this kind of SQL manipulation, remember I moved the logic for PG DELETE to a separate visitor at the end of the pipeline, to keep SQL generation simple/clean. We could do something similar here, though of course it's not strictly necessary (it's just a pretty unique case where we're doing that in SQL generation).
I'll probably just do use the same visitor for update in PG 😈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered it but there were few differences in that case. Using doesn't allow using any joins so you would need to move everything. Here it is just 1 table which needs to changed that way so I just inlined here. Also this is relational which SqlServer overrides, changing expression here means providers other than Sqliite/Postgre need to block the conversion. Which makes task more overall.