Accept Action<SetPropertyCalls> in ExecuteUpdate

[aradalvand]

I was expecting to find an existing issue for this but I couldn't (if there is one please let me know):
ExecuteUpdate currently only accepts an expression, this means you cannot dynamically build an update query, and do things like this:

query.ExecuteUpdate(b =>
{
    if (someCondition)
        b.SetProperty(e => e.Something, "Something");
    else
        b.SetProperty(e => e.Foo, "Foo");
    b.SetProperty(e => e.Bar, "Bar")
});

But this would be extremely useful and is sometimes very much needed.

IIRC I believe somebody brought this up somewhere in one of the associated issues (probably in #795), but I don't remember what the response from the EF team was. It kind of seems surprising that this isn't already supported (there are Stack Overflow questions about it, for example). The expressions passed to .SetProperty() are all that matters anyway, so it seems like the ExecuteUpdate parameter being an expression is unnecessary, it could've just been an Action, but definitely correct me if I'm wrong. Thanks.

Update: Found the comment that had mentioned this: #795 (comment) (see surrounding conversation) — Shay then said this:

Ah, could be... @smitpatel will determine whether this is feasible and reasonable to implement.

I don't think there were any updates afterwards.\

[roji]

@aradalvand it's possible to build the expression tree dynamically; I agree that this is less trivial than if we we had a simple Action parameter, but adding one now would probably be problematic in terms of ambiguous resolution etc.

[aradalvand]

@ajcvickers Why close the issue?! This could still be done by having ExecuteUpdate accept a Func<SetPropertyCalls, SetPropertyCalls>, eliminating the resolution ambiguity problem. No?

[roji]

Given that the current ExecuteUpdate accepts a parameter of type Expression<Func<SetPropertyCalls<TSource>, SetPropertyCalls<TSource>>>, which would it not be ambiguous?

[aradalvand]

@roji I actually meant replacing the current signature with one that accepts Func<SetPropertyCalls<TSource>, SetPropertyCalls<TSource>> instead (stripping Expression<>) and then updating the SetProperty methods accordingly as well (have them receive Expressions), which would avoid a breaking change.

But let me know if I'm missing something.

[roji]

That would break any code already constructing expression trees dynamically and passing them in.

[aradalvand]

@roji But anyone currently doing that is presumably doing it precisely because of this very limitation (e.g. myself, here).

[roji]

It's a breaking change nevertheless for those people already doing it, though.

I'll reopen the issue to see what the team feels about it.

[aradalvand]

It's a breaking change nevertheless for those people already doing it, though.

Sure, but again, rest assured those people would be happy to see what they're currently doing on their own become natively supported. So this would be the best kind of breaking change to experience, if you will.

EF doesn't have a strict no-breaking-changes policy and it does introduce breaking changes where it makes sense (which is a good thing IMO), and I would say this is one of those cases. But of course you guys ultimately know best.

[roji]

Note: this would also allow evaluating the value argument as a regular .NET value, rather than attempting to translate it. For example, the following fails for SQLite since SQLite does not translate DateTimeOffset.UtcNow (#32252):

await ctx.Blogs.ExecuteUpdateAsync(setter => setter.SetProperty(e => e.DateTimeOffset, DateTimeOffset.UtcNow));

If ExecuteUpdateAsync accepted a non-Expression list of setters, DateTimeOffset.UtcNow would be evaluated client-side, just as if it were being set on the entity using change-tracking SaveChanges. This seems to be better than the current way of doing things (the overload accepting a lambda for the value would still be there, and would always be translated to SQL).

[roji]

Another thing that this would enable would be to pass an Expression<Func<Blog, int>> as the property (or value) selector, as requested in #31349 (comment):

Expression<Func<User, int>> p = property => property.ClientId;
DatabaseContext.Entity<User>()
               .Where(w => w.Id == 11)
               .ExecuteUpdateAsync(e => e.SetProperty(p, 30));

That's quite a few legitimate usage types that depend on us making this change...

[aradalvand]

Now that this is being more seriously considered, I'd like to point out that since we're now talking about "replacing" the current signature, replacing it with Action<SetPropertyCalls> would be preferable to Func<SetPropertyCalls, SetPropertyCalls>; both are equally low-impact breaking changes, but given that this is starting to seem like a breaking change well worth making, we should probably go with the ideal Action<SetPropertyCalls> rather than Func<SetPropertyCalls, SetPropertyCalls>. And the reason is pretty obvious — with Func<SetPropertyCalls, SetPropertyCalls>, when your function has a body, you'll have to return:

With Action<SetPropertyCalls>:

db.Products.ExecuteUpdate(b =>
{
    if (someCondition)
        b.SetProperty(p => p.SomeProperty, "NewValue");
});

With Func<SetPropertyCalls, SetPropertyCalls> (notice how the return is necessary, but it feels a bit out of place and redundant, and shouldn't really be needed):

db.Products.ExecuteUpdate(b =>
{
    if (someCondition)
        b.SetProperty(p => p.SomeProperty, "NewValue");
    return b;
});

[roji]

One downside of doing this, is that when one uses the SetProperty overload without the lambda (e.g. SetProperty(x => x.Prop, foo)), we lose the ability to distinguish between parameters/constants - the same problem with have with Skip/Take. We'll have to make an arbitrary decision for that case (we'll most probably parameterize, like with Skip/Take). Of course, the lambda overload is always there in case the user wants to force a constant, so this is quite minor.

[roji]

Another request for dynamic ExecuteUpdate: #33626.

For a sample code sample of dynamic ExecuteUpdate construction with the current mechanism (quite complex!), see #33626 (comment).

[roji]

Design decision: we're OK with making this breaking change. However, we think it's quite late to do it (as a breaking change) for 9.0 - we'll try to prioritize this for the beginning of 10.

[roji]

Another subtle breaking change aspect of this: ExecuteUpdateAsync(s => s.SetProperty(x => x.Foo, 8)) will now send 8 as a parameter and not as a constant, since it's not inside a lambda any more (just like Skip/Take). Switching from constant to parameter in this context seems very unlikely to cause any meaningful regression, since it's simply the value being set (and not e.g. searched); in any case, users will have the option to specify a lambda in any case.