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

Consider suppressing C# 11 required property warnings in ExecuteUpdate #28557

Closed
roji opened this issue Aug 1, 2022 · 19 comments
Closed

Consider suppressing C# 11 required property warnings in ExecuteUpdate #28557

roji opened this issue Aug 1, 2022 · 19 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@roji
Copy link
Member

roji commented Aug 1, 2022

ExecuteUpdate accepts a NewExpression for expressing the properties to be changed:

context.Customers.Where(...).ExecuteUpdate(c => new Customer { c.Age = c.Age + 1 });

If Customer has any C# 11 required properties on it, this will generate a compiler warning, since the property is uninitialized.

We could write a diagnostics suppressor that suppresses C# 11 required properties if they occur within the ExecuteUpdate lambda.

Raised by @aradalvand in #795 (comment)

@Eli-Black-Work
Copy link

@roji I thought that missing required properties resulted in a compile error, not a warning.

@roji
Copy link
Member Author

roji commented Aug 1, 2022

@Bosch-Eli-Black you may be right - I'll look into this.

@aradalvand
Copy link

aradalvand commented Aug 1, 2022

After reviewing some of the comments in #795 (such as this one) related to the various approaches to designing the ExecuteUpdate method, I've gathered my thoughts on the topic below, which I'm hoping can be helpful:

Problems with ExecuteUpdate(e => new Entity { .. }):

Even if the required issue didn't exist, another subtle drawback of the new { ... } style for ExecuteUpdate would be that if you were to update some regular properties plus some shadow properties at the same time, you would have no choice but to opt for anonymous types, which means you'll get no type-safety for the regular properties inside your expression:

db.Customers.Where(...)
    .ExecuteUpdate(c => new
    {
        Age = c.Age + 1, // Regular property, no type-safety because this is an anonymous type
        IsActive = false, // Regular property, no type-safety because this is an anonymous type
        ShadowProp = "Something",
    });

This is a bummer, of course, but not necessarily a deal-breaker. With the addition of the required keyword and problems that it introduces, however, the new { ... } approach for updates is essentially rendered unfeasible.

There are some other minor downsides, such as the new keyword being somewhat awkward in the context of an update operation — Shay mentioned this here.

Correct me if I'm wrong but given all of this (the main deal-breaker of course being the required issue) I think it's fair to say that the ExecuteUpdate(c => new Customer { ... }) approach most likely has to be ruled out.

The only other alternative that was proposed (in #795) involved a separate .Set() method:

db.Customers.Where(...)
    .Set(c => c.Age, c => c.Age + 1)
    .Set(c => c.IsActive, false)
    .Set("ShadowProp", true)
    .ExecuteUpdate();

This approach suffers from none of the aforementioned problems. Namely, the required problem is no longer an issue, the addition of shadow properties into the mix won't turn off type-safety for the regular properties, and there's no (slightly) awkward new keyword either.

Another benefit this approach provides is composability:

var query = db.Customers.Where(...);

query.Set(c => c.Age, c => c.Age + 1)
if (shouldBeDeactivated)
    query.Set(c => c.IsActive, false);
query.ExecuteUpdate();

This is also something to take note of.

Unless I'm missing something, it's easy to see how .Set() is the winner here, all things considered.


Additional proposals (variations of the .Set approach):

Although most of the suggestions that involved .Set() in #795 basically had .Set as an extension method on IQueryable, an alternative would be for ExecuteUpdate to have its own "builder":

db.Customers.Where(...)
    .ExecuteUpdate(b => b
        .Set(c => c.Age, c => c.Age + 1)
        .Set(c => c.IsActive, false)
        .Set("ShadowProp", true)
    );

This has two main advantages over the IQueryable.Set() approach:

  1. It prevents people from mistakenly assuming that just calling db.Customers.Set(...) is enough or somehow "does" something (to the database).
  2. A more stylistic advantage is that all the .Set calls are "nested" within ExecuteUpdate(), as opposed to preceding it; yielding a more visually similar style to .ExecuteUpdate(c => new Customer { ... }).

I'm perfectly fine with any disagreements with this last style, I'm not religious about it by any means, I wouldn't really mind the .Set().ExecuteUpdate() approach either.

A slightly more exotic variation would be:

db.Customers.Where(...)
    .ExecuteUpdate(
        c => EF.Set(c.Age).To(c.Age + 1),
        c => EF.Set(c.IsActive).To(false),
        c => EF.Set("ShadowProp").To(true)
    );

This wouldn't require a builder parameter, and would also remove the need for having two expressions in the first .Set call — although admittedly it looks weird, so again, feel free to disagree, I'm just spitting out ideas.
This one is basically an explicit workaround around C#'s lack of support for assignments in expressions. Once those are supported (hopefully, it's been god knows how many years...), this API can change accordingly with minimal modification.


@roji Would love to know your thoughts on all of this.

@roji
Copy link
Member Author

roji commented Aug 1, 2022

Thanks for your thoughts @aradalvand.

Even if the required issue didn't exist, another subtle drawback of the new { ... } style for ExecuteUpdate would be that if you were to update some regular properties plus some shadow properties at the same time, you would have no choice but to opt for anonymous types, which means you'll get no type-safety for the regular properties inside your expression

I personally think this argument isn't very strong. Updating shadow properties is generally a somewhat niche scenario, and losing the strong typing for non-shadow properties in the same update seems like a very small problem. After all, you've already opted into weakly-typed because of the shadow properties.

However, I do agree that required properties pose a problem here. Let us discuss this together in the team and see what we think.

@aradalvand
Copy link

aradalvand commented Aug 1, 2022

I personally think this argument isn't very strong. Updating shadow properties is generally a somewhat niche scenario, and losing the strong typing for non-shadow properties in the same update seems like a very small problem. After all, you've already opted into weakly-typed because of the shadow properties.

Fair enough, I just wanted to point out some of the other subtle quirks that this approach has and .Set() doesn't, this minor problem on its own isn't significant of course, but in light of the required issue, and also given that .Set() basically solves all of these issues (major AND minor), and provides better composability — with the tradeoff being that it's slightly more verbose — I was just trying to make the case for it :)

However, I do agree that required properties pose a problem here. Let us discuss this together in the team and see what we think.

Sure! Thank you all.

@bachratyg
Copy link

Unless I'm missing something, it's easy to see how .Set() is the winner here, all things considered.

See #795 (comment) #795 (comment)

The version 3 syntax (#795 (comment)) avoids the requiredness check by splitting the expression at the assignment:

db.Customers
    .Where(...)
    .ExecuteUpdate(
        c => new { c.Age, c.IsActive, Shadow = EF.Property(c, "Shadow") }, // shadows work too
        c => new { Age = c.Age + 1, IsActive = true, Shadow = 123 }
    )

This is somewhat similar to a join's key selectors

from a in ...
join b in ... on new { a.Prop1, a.Prop2 } equals new { b.Prop1, b.Prop2 }

though it's still rather verbose.

@aradalvand
Copy link

aradalvand commented Aug 1, 2022

@bachratyg I'm not sure I quite understand what you're saying but that looks way too overly verbose and complicated.
I don't see the point? How's that superior to .Set(...)?

@bachratyg
Copy link

@aradalvand the approach you propose (one Set per column to be updated) cannot be superior if it cannot express some more complex commands efficiently. I have linked some examples above, most use data from a different table/query. The currently favored (?) new {...} approach can handle these (but not required properties). How would I write such a query with your proposal?

What I wrote above can handle both the requiredness and the complexity e.g. both of these are correct:

.ExecuteUpdate(c => c.Age, c => c.Age + 1);
.ExecuteUpdate(c => new { c.Age, c.IsActive }, c => new { Age = c.Age + 1, IsActive = true });

but yes, the verbosity is unpleasant.

@aradalvand
Copy link

aradalvand commented Aug 1, 2022

In this example:

.ExecuteUpdate(c => new { c.Age, c.IsActive }, c => new { Age = c.Age + 1, IsActive = true });

The second argument (lambda expression) is going to lack strong typing because it's an anonymous type. Am I right? So how is this any better than the following, which is, again, not type safe, but achieves the same result?

.ExecuteUpdate(c => new { Age = c.Age + 1, IsActive = true });

@bachratyg
Copy link

bachratyg commented Aug 1, 2022

Not necessarily if the signature is

ExecuteUpdate<TEntity, TExpr>(
    this IQueryable<TEntity> source,
    Expression<Func<TEntity, TExpr>> columnSelector,
    Expression<Func<TEntity, TExpr>> expressionSelector)

Then the result type of both arguments must match otherwise the compiler will complain. This is the same as the key selectors on Queryable.Join.

@aradalvand
Copy link

aradalvand commented Aug 1, 2022

@bachratyg Oh okay I get it now. Interesting, but there seems to be several problems with that:

  1. Auto-completion doesn't seem to work well with this approach:
    image
  2. If you misspell a property name, for example, the red squiggly will appear under the method name and not the property name, and the error message isn't particularly helpful either:
    image
  3. You'll have to mention every property twice.
  4. It looks complicated and is hard to parse in your head.

Correct me if I'm wrong but I don't think there's anything this could express that the .Set(...) approach couldn't. They're functionally almost identical except the .Set(...) approach is far easier to grasp and wrap your head around.

@bachratyg
Copy link

  1. True, c#/roslyn may remedy this one day
  2. True, same as above
  3. True, though I would also be mentioning Set(c => (method name, lambda arg and arrow) once per column for single-column Set()
  4. it's somewhat subjective, but I concur

Note that for single column updates, there's no need for an anonymous type, this "just works":

courses.ExecuteUpdate(e => e.Deleted, e => false);

Here's a query I don't see how could be done efficiently with single property .Set(...):

db.Customers.ExecuteUpdate(
    c => new { c.BestOrder, c.AverageOrder, ... },
    c => (from o in c.Orders
          group o by 1 into g  // imagine a query with arbitrary complexity here, this is just a basic example
          select new
          {
              BestOrder = g.Max(o => o.TotalValue),
              AverageOrder = g.Average(o => o.TotalValue), ...
          }).First() // Always a single row, this could be omitted with some signature tweaking
)

I only want to scan the Orders table once, but the result of that scan is needed in more than one column.

@smitpatel
Copy link
Contributor

smitpatel commented Aug 1, 2022

Set has severe limitation in generic types when it comes to API definition in order to express what database can support in Update statement. Very unlikely it will work-out. We will try to find another solution for required.

Proposal by @bachratyg may work better though auto-completion issue remains. See #6560 for our past experience in trying to get intellisense to work better.

@wsq003
Copy link

wsq003 commented Aug 2, 2022

context.Customers.Where(...).ExecuteUpdate(c => c.Age = c.Age + 1 && c.IsActive = true );

How about this ?

@John0King
Copy link

@

context.Customers.Where(...).ExecuteUpdate(c => c.Age = c.Age + 1 && c.IsActive = true );

How about this ?

I don't think this is a valid lamda expression

@wsq003
Copy link

wsq003 commented Aug 2, 2022

@

context.Customers.Where(...).ExecuteUpdate(c => c.Age = c.Age + 1 && c.IsActive = true );

How about this ?

I don't think this is a valid lamda expression

Is it possible to make this syntax valid?

https://stackoverflow.com/questions/807797/linq-select-an-object-and-change-some-properties-without-creating-a-new-object

Such condition is quite common.

@Eli-Black-Work
Copy link

Eli-Black-Work commented Aug 2, 2022

I think the long form would work fine:

context.Customers.Where(...).ExecuteUpdate(c => {
   c.Age++;
   c.IsActive = true;
});

@roji
Copy link
Member Author

roji commented Aug 2, 2022

@Bosch-Eli-Black that's not a valid lambda expression either.

Note that almost all of the above have already been proposed and discussed in #795 (several times, in fact), though I understand that it's a very long issue and hard to read through. In any case, we're actively looking into the ExecuteUpdate API shape.

@roji
Copy link
Member Author

roji commented Aug 2, 2022

I am closing this issue as its original purpose - suppressing the diagnostic for C# 11 require properties - is impossible. We'll continue discussing in #795.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Aug 2, 2022
@roji roji added closed-no-further-action The issue is closed and no further action is planned. and removed type-enhancement area-bulkcud labels Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

7 participants