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

Owned types with composite primary keys that include the foreign key fail on searches #30205

Closed
Blackclaws opened this issue Feb 3, 2023 · 8 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@Blackclaws
Copy link

Blackclaws commented Feb 3, 2023

File a bug

When defining an owned type where you can forego a key column because your business logic guarantees that for a given foreign key all fields have to be unique it makes sense to define a composite key like this:

        modelBuilder.Entity<TestEntity>().OwnsMany<TestOwned>(x => x.OwnedInternal, navigationBuilder =>
        {
            navigationBuilder.ToTable("OwnedTable");
            navigationBuilder.Property(x => x.Value).HasColumnName("Value");
            navigationBuilder.Property(x => x.ValueTwo).HasColumnName("ValueTwo");
            navigationBuilder.WithOwner().HasForeignKey("OwnerKey");
            navigationBuilder.HasKey("Value", "ValueTwo", "OwnerKey");
        });

While this works fine and produces the desired table layout, EfCore can not handle Where clauses that do equality comparisons on the Owned type such as:

var testOwnedOne = new TestOwned("TestValueOne", "TestValueTwo");
var firstOrDefault = context.Set<TestEntity>().FirstOrDefault(x => x.OwnedInternal.Any(y => y == testOwnedOne));

I expected those to work because the foreign key constraint makes it clear what the expression should be matching on. Instead I get an invalid operation error because of the ForeignKey not having a property to back it.

Include your code

Repro is here: https://github.com/Blackclaws/EFCoreOwnedKeyPropertyErrorRepro

Include stack traces

Unhandled exception. System.InvalidOperationException: No backing field could be found for property 'TestOwned.OwnerKey' and the property does not have a getter.
   at Microsoft.EntityFrameworkCore.Metadata.IPropertyBase.GetMemberInfo(Boolean forMaterialization, Boolean forSet)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.ClrPropertyGetterFactory.Create(IPropertyBase property)
   at Microsoft.EntityFrameworkCore.Metadata.RuntimePropertyBase.<>c.<Microsoft.EntityFrameworkCore.Metadata.IPropertyBase.GetGetter>b__35_0(RuntimePropertyBase property)
   at Microsoft.EntityFrameworkCore.Internal.NonCapturingLazyInitializer.EnsureInitialized[TParam,TValue](TValue& target, TParam param, Func`2 valueFactory)
   at Microsoft.EntityFrameworkCore.Metadata.RuntimePropertyBase.Microsoft.EntityFrameworkCore.Metadata.IPropertyBase.GetGetter()
   at lambda_method67(Closure, QueryContext)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.Execute[TResult](Expression query)
   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.Execute[TResult](Expression expression)
   at Program.<Main>$(String[] args) in /home/felix/spielwiese/efcoreRepro/EFCoreTest/Program.cs:line 51
   at Program.<Main>(String[] args)

Include provider and version information

EF Core version: 7.0.2
Database provider: Sqlite (though any other also shows the same issue)
Target framework: 7.0
Operating system: Linux
IDE: Rider 2022.3

@Blackclaws
Copy link
Author

Another interesting hint:

If you drop the default equality:

var firstOrDefault = context.Set<TestEntity>().FirstOrDefault(x => x.OwnedInternal.Any(y => y == testOwnedOne));

and instead match on specific fields:

var firstOrDefault = context.Set<TestEntity>().FirstOrDefault(x => x.OwnedInternal.Any(y => y.Value == testOwnedOne.Value && y.ValueTwo == testOwnedOne.ValueTwo));

Then it works fine.

I think the issue might boil down to shadow properties being used in equality operations where they simply don't belong because they are an implementation detail in this case.

@ajcvickers
Copy link
Member

@Blackclaws This is a duplicate of #29442. However, you appear to be saying that the values of "Value" and "ValueTwo" uniquely identify the entity instance. If this is the case, then you shouldn't need to include "OwnerValue" in the key.

@Blackclaws
Copy link
Author

However, you appear to be saying that the values of "Value" and "ValueTwo" uniquely identify the entity instance

@ajcvickers They do, but only when combined with the parent. Basically the parent contains a list of value objects. The list is distinct, therefore there are no duplicates for each parent. However they can be duplicated between different parents. Because they are in their own table I cannot simply define a key over the two values because the combination will not be unique. However it will be unique when combined with the parent key. Hence why I wanted to do exactly that.

I'd also contest that this is a duplicate of #29442 . That issue deals with the exception being not helpful, and while I understand the implication raised in: #29405 (comment) I wonder whether having three separate issues all running into the issue of EfCore using a key in a comparison operation where there simply is none that would take part in this comparison on the .net side is an issue/feature-request in and off itself.

Wouldn't a solution be that if building the comparison query shadow properties are ignored? We are comparing value objects (in this case specifically records) and at least for records you can be sure that the comparison should only be for the properties that are actually defined on the record and not for any shadow properties.

That EfCore magically introduces the key into the comparison operation isn't really what was intended by the one writing the query and EfCore should translate as faithfully as possible the intention of the query author. If it would be more helpful that I rephrase this as a feature request let me know and I'll do so.

@ajcvickers
Copy link
Member

@Blackclaws Don't forget that owned types are not a good representation for value objects. Owned types are entity types; they have keys. The key properties are hidden by default, which was perhaps a mistake, but nevertheless, they are not value objects and should not be treated as such.

@Blackclaws
Copy link
Author

Blackclaws commented Feb 4, 2023

Don't forget that owned types are not a good representation for value objects.

@ajcvickers They are the closest/only thing EfCore offers for them though. And it works pretty well for 1:1 relationships. 1:N is where things start to become a bit problematic. Also while they might not be a perfect fit its what everyone uses because its the only thing available aside from serializing them down a single field and being restricted to a full equals comparison (you can't do HasConversion to multiple columns after all).

Owned types are entity types; they have keys.

Only in the OwnsMany case though, and even then they don't need to be individually identifiable, only with respect to their owner. As shown you can happily define a primary key for them by including the foreign key of their owner as long as they are guaranteed to be distinct for each owner. That way you don't need a key column for them and also no key property.

In any case, if I create a query that involves an owned type I don't expect that any parts of the object I am comparing that aren't part of my .net definition get compared. Whether that expectation is correct is a different story. That's why I asked whether this might better be formulated as a feature request.

For record types I would certainly expect that equality comparisons are not by reference but by equality of all properties instead as that is what they are all about. So for that specific case it might make sense to change the way that queries are built up to begin with.

For standard entity types comparison by key is logical (its still very confusing when your custom Equals method gets completely ignored by EfCore but at least its very understandable why it is). But for owned types I do not expect a key to play any role in such comparisons.

You also can't define a ValueComparer via Metadata for 1:N owned types, so I don't even see a way this could be configured centrally instead of having to list all properties I want to compare at each callsite (or with an extension method).

@ajcvickers
Copy link
Member

And it works pretty well for 1:1 relationships.

Disagree. It doesn't work well for many cases with 1:1 relationships either.

Only thing available

Yes, this is unfortunate, but doesn't change that it doesn't work well.

Only in the OwnsMany case though

All owned types have a key, regardless if they are used with OwnsOne or OwnsMany

Whether that expectation is correct is a different story.

It's not correct.

That's why I asked whether this might better be formulated as a feature request.

The feature request is tracked by #9906. Namely, stop making owned types the way to do value objects.

I agree that the behavior for owned types does not match the behavior that would be expected for value objects. But that's fundamentally because owned types are entity types and don't have the semantics of value objects. The way to fix this is to not use owned types as value objects.

@Blackclaws
Copy link
Author

All owned types have a key, regardless if they are used with OwnsOne or OwnsMany

Interesting. I just tested it and indeed this issue also happens for OwnsOne types. I was not aware that a key was even needed in such cases and therefore assumed one wouldn't be present as the key would just be the one on the entity itself and change tracking would happen via the main entity. That explains why you can't transfer an Owned type from one entity to another.

I guess there is a lot of "this is most likely how its implemented" thinking on my part here that just doesn't apply due to the history of how this came about and how EfCore in general needs to interface with databases to pull these types together from multiple tables.

The feature request is tracked by #9906. Namely, stop making owned types the way to do value objects.

I see. So basically this is kind of supposed to work using whatever new mechanic arises for proper value objects in (hopefully) EF 8.0.

I agree that the behavior for owned types does not match the behavior that would be expected for value objects. But that's fundamentally because owned types are entity types and don't have the semantics of value objects. The way to fix this is to not use owned types as value objects.

Well, I guess right now that would be the same as saying: "Don't use value objects with EfCore Entities because there is no way to do this properly."

One proposal aside from #29442 would be to amend the documentation on owned types here:
https://learn.microsoft.com/en-us/ef/core/modeling/owned-entities#current-shortcomings

Explicitly stating that equality comparisons for owned types by default involve the key property (as they are treated as entities) and therefore will not show value object semantics.

Is there any way to configure how these equality comparisons are translated/transformed to SQL for owned types, or is that simply not accessible right now? I already found out that there seems to be no way to define a ValueComparer for them.

@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Feb 16, 2023
@ajcvickers
Copy link
Member

Is there any way to configure how these equality comparisons are translated/transformed to SQL for owned types

This is no something that is supported.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Feb 16, 2023
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

2 participants