Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 22, 2024

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.

@@ -138,6 +138,32 @@ protected override Expression VisitExtension(Expression extensionExpression)
protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
{
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)

test/EFCore.Tests/Query/QueryProviderTest.cs Show resolved Hide resolved
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