Skip to content

Move ExecuteUpdate/ExecuteDelete to core so non-relational providers can implement#34257

Merged
ajcvickers merged 2 commits intomainfrom
GetItDone]
Jul 22, 2024
Merged

Move ExecuteUpdate/ExecuteDelete to core so non-relational providers can implement#34257
ajcvickers merged 2 commits intomainfrom
GetItDone]

Conversation

@ajcvickers
Copy link
Contributor

Fixes #31052

@ajcvickers ajcvickers requested a review from a team July 20, 2024 15:29
@ajcvickers
Copy link
Contributor Author

/cc @damieng

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, see the notes about moving up the dispatch for update/delete, and also not sure we should implement the test suites for InMemory.

case nameof(EntityFrameworkQueryableExtensions.ExecuteDeleteAsync):
case nameof(EntityFrameworkQueryableExtensions.ExecuteUpdate):
case nameof(EntityFrameworkQueryableExtensions.ExecuteUpdateAsync):
return ProcessUnknownMethod(methodCallExpression);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should implement proper nav expansion for ExecuteUpdate (see #32493), but this can be done separately in another PR (this doesn't make things any worse than the current situation, even if it doesn't make them better).

@roji
Copy link
Member

roji commented Jul 20, 2024

One more thought... We should follow the pattern of adding TranslateExecuteUpdate/Delete methods to QueryableMethodTranslatingExpresionVisitor, and dispatching to them from the big method dispatch like with the others. Rather than making them abtract like the others, we should just provide a default implementation that adds a specific error message "your provider does not support ExecuteUpdate/Delete" and returns null. Relational would simply override these methods rather than implementing its own special dispatch in VisitMethodCall (which should be removed).

@ajcvickers
Copy link
Contributor Author

@roji I moved the dispatch to core.

@ajcvickers
Copy link
Contributor Author

@roji Helps if I actually push the commit.

{
var method = methodCallExpression.Method;

if (method.DeclaringType == typeof(EntityFrameworkQueryableExtensions))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just merge this into the block below, this would deduplicate the visitation, check for SQE, etc. (we check on the generic methods for each case anyway)

{
var queryable = context.Set<OrderDetail>().FromSqlRaw(
NormalizeDelimitersInRawString(
@"SELECT [OrderID], [ProductID], [UnitPrice], [Quantity], [Discount]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: raw literal string (and below)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Promote ExecuteUpdate/Delete from relational to core

2 participants