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

Allow passing complex type instances to ExecuteUpdate #32058

Closed
Tracked by #31238
xamir82 opened this issue Oct 16, 2023 · 19 comments · Fixed by #32499
Closed
Tracked by #31238

Allow passing complex type instances to ExecuteUpdate #32058

xamir82 opened this issue Oct 16, 2023 · 19 comments · Fixed by #32499
Assignees
Labels
area-bulkcud closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@xamir82
Copy link

xamir82 commented Oct 16, 2023

Imagine this model:

public class User
{
    public int Id { get; set; }
    public required FullName FullName { get; set; }
}

[Owned]
public class FullName
{
    public required FirstName { get; set; }
    public required LastName { get; set; }
}

Inserting a User with by assigning a FullName object to the FullName property works:

db.Add(new User
{
    FullName = someFullNameObj,
});
db.SaveChanges();

But doing the same with ExecuteUpdate doesn't work:

db.Users.Where(u => u.Id == userId)
    .ExecuteUpdate(b => b
        .SetProperty(u => u.FullName, someFullNameObj)
    );

It throws an exception along these lines:

The LINQ expression ... could not be translated. The following lambda argument to 'SetProperty' does not represent a valid property to be set: 'u => u.FullName'.

This is an unexpected asymmetry between how insert and update work, I think most people would intuitively expect the ExecuteUpdate example to work just fine, it's a bit surprising, a weird limitation.

Let me know if there's an existing issue for this.

@roji
Copy link
Member

roji commented Oct 16, 2023

ExecuteUpdate doesn't (currently) support passing an entire owned object; it requires you reference properties that map to columns in the database. Simply decompose the owned instance as follows:

db.Users
    .Where(u => u.Id == userId)
    .ExecuteUpdate(b => b
        .SetProperty(u => u.FullName.FirstName, someFullNameObj.FirstName)
        .SetProperty(u => u.FullName.LastName, someFullNameObj.LastName)
    );

@xamir82
Copy link
Author

xamir82 commented Oct 16, 2023

@roji Thanks for the quick response!
You're right, and that's the workaround I'm currently using. But don't you plan to support passing an entire owned object? Is there an issue tracking that? It'd be much more convenient when your method is actually receiving an owned type instance to simply pass it directly, as opposed to tediously call SetProperty on all its individual properties.

And also: Would EF8's complex types support this? Or do they have the same problem?

@roji
Copy link
Member

roji commented Oct 16, 2023

@xamir82 I do think we should support this, but given that you can decompose to individual property setters this would simply be a bit of sugar, nothing more. So I'd say this is quite low-priority compared to our other issues.

Complex types will have the same limitation, yes.

@ajcvickers ajcvickers changed the title ExecuteUpdate not working with owned type property Allow passing entire entity instances to ExecuteUpdate Oct 27, 2023
@ajcvickers ajcvickers added this to the Backlog milestone Oct 27, 2023
@aradalvand
Copy link

aradalvand commented Nov 20, 2023

Turns out this is not merely a nice-to-have as has been implied here, there would actually be no way to use ExecuteUpdate to update a collection backed by a JSON column without this very feature.

@roji Don't you think it would make sense to add the consider-for-current-release tag to this issue, given that?

@roji
Copy link
Member

roji commented Nov 21, 2023

I don't think there's anything special regarding collections here: just like you can't replace a standalone object within JSON (that's not in a collection), you can't replace an object that's within a collection. And once #28766 is implemented, you could modify all the properties within an object in a collection, in the same way you would for an object not within a collection.

So unless I'm mistaken, passing in an entire entity instance still effectively be a nicer alternative to listing out the properties one-by-one - or am I missing something?

@aradalvand
Copy link

aradalvand commented Nov 21, 2023

So unless I'm mistaken, passing in an entire entity instance still effectively be a nicer alternative to listing out the properties one-by-one - or am I missing something?

@roji Well, how would you do the equivalent of the following, even if #28766 was implemented?

db.Products.ExecuteUpdate(b => b.SetProperty(p => p.Traits, newTraits));

(where newTraits is e.g. a List<Trait> that is coming from somewhere else (an API client, for example), and should simply replace the current traits.)

@roji
Copy link
Member

roji commented Nov 21, 2023

That's not an entity instance - it's a list of instances. We can indeed track that here as part of this issue, though it would likely require it's own specific work... In any case, I've put the consider-for-current-release label on this.

@aradalvand
Copy link

aradalvand commented Nov 21, 2023

That's not an entity instance - it's a list of instances. We can indeed track that here as part of this issue

I guess I assumed that this represents basically the same underlying problem (or at least that they're related). So yes, I think it does make sense to track it here. @xamir82 perhaps you could adjust the title of the issue a little bit to reflect this more clearly? Thanks.

In any case, I've put the consider-for-current-release label on this.

Thank you very much!

@roji roji changed the title Allow passing entire entity instances to ExecuteUpdate Allow passing entity instances (or lists thereof) to ExecuteUpdate Nov 21, 2023
@xamir82
Copy link
Author

xamir82 commented Nov 21, 2023

roji beat me to it :P

@xamir82 xamir82 changed the title Allow passing entity instances (or lists thereof) to ExecuteUpdate Allow passing entity & complex type instances (or lists thereof) to ExecuteUpdate Nov 28, 2023
@roji roji changed the title Allow passing entity & complex type instances (or lists thereof) to ExecuteUpdate Allow passing complex type instances to ExecuteUpdate Dec 3, 2023
@roji roji self-assigned this Dec 3, 2023
roji added a commit to roji/efcore that referenced this issue Dec 3, 2023
@roji
Copy link
Member

roji commented Dec 3, 2023

I implemented updating complex types in #32499 (that's the "sugar" mentioned above).

The EF 8.0 support for complex types doesn't support collections; we hope to make this possible for 9.0 via JSON mapping of complex types. When this happens, using ExecuteUpdate (and possibly ExecuteDelete) to make changes inside JSON documented is tracked by #28766.

@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Dec 3, 2023
@roji roji removed this from the Backlog milestone Dec 3, 2023
@xamir82
Copy link
Author

xamir82 commented Dec 3, 2023

Thank you @roji
Does #32499 also add the same ability for owned types? If not, then this issue shouldn't be closed since it's about both owned types and complex types. Right?

When this happens, using ExecuteUpdate (and possibly ExecuteDelete) to make changes inside JSON documented is tracked by #28766

When that happens, will support for using ExecuteUpdate on the JSON column itself be supported automatically? You said "make changes inside JSON documents", but this issue is about making changes to the JSON column as a whole via ExecuteUpdate.

roji added a commit to roji/efcore that referenced this issue Dec 4, 2023
roji added a commit to roji/efcore that referenced this issue Dec 4, 2023
@SukharevAndrey

This comment was marked as off-topic.

@roji

This comment was marked as off-topic.

@roji
Copy link
Member

roji commented Dec 5, 2023

Does #32499 also add the same ability for owned types? If not, then this issue shouldn't be closed since it's about both owned types and complex types. Right?

No, it does not. Just like you cannot assign a customer's owned shipping address to be their billing address (that would mean the same entity instance is referenced twice), I don't think it makes sense to allow this via ExecuteUpdate. This is exactly the kind of mismatch that makes the new complex types better for modeling containment/document structures than the current owned entity support. Hopefully everything works out for complex types in 9.0 and you'll be able to use them to model JSON etc.

When that happens, will support for using ExecuteUpdate on the JSON column itself be supported automatically?

That's indeed something that should be supported as well - I'll add a note on #28766.

@xamir82
Copy link
Author

xamir82 commented Dec 5, 2023

Makes sense. Thanks!

@ajcvickers
Copy link
Member

Note from triage: updating owned entity type instances should be covered by #29654, unless that turns out to be prohibitively expensive.

@ajcvickers ajcvickers added this to the 9.0.0 milestone Jan 4, 2024
@roji
Copy link
Member

roji commented Jan 4, 2024

Reopening as #32499 hasn't been merged yet (waiting to be reviewed).

Regardless, @ajcvickers I don't think #29654 is quite the right issue here... I think we're discussing two possibly different capabilities:

  1. A shortcut/syntactic sugar to overwrite all properties of a type in the database, given a .NET object. This is what this issue specifically tracks for complex types, and I agree it probably makes sense for entity types (and possibly also owned).
  2. Higher-level API over ExecuteUpdate for variable property sets #29654 is about not having to overwrite all properties of the type, but rather only the properties that have been changed (e.g. as delivered from a client via an HTTP PATCH request or similar). So it's more about some way of constructing "dynamic" ExecuteUpdate statements. Note that Accept Action<SetPropertyCalls> in ExecuteUpdate #32018 goes in this direction, as it makes it easier to compose setters together without messing around with expression trees.

If that all makes sense, then I think we're missing an issue for option (1) applied to entity types.

@roji roji reopened this Jan 4, 2024
@ajcvickers
Copy link
Member

@roji Yeah, makes sense. I was on the fence as to whether #29654 really covered this case too.

@roji
Copy link
Member

roji commented Jan 4, 2024

@ajcvickers opened #32719 with some design thoughts... it's a bit more complicated than it looks :) (surprise!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-bulkcud closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants