Throw early when user mistakenly uses OwnsOne() but points to a collection

[lajones]

If the user mistakenly uses OwnsOne() on a collection or OwnsMany() on a reference we should point this out with an error message.

E.g. see

efcore/test/EFCore.Tests/ModelBuilding/OwnedTypesTestBase.cs

Line 1394 in 112b606

.OwnsMany(

If you change that line to an OwnsOne() call instead, the NonGenericString version of the test eventually fails but with an unhelpful error message deep in the code. Some of the other (Generic/NonGeneric etc) OwnedTypes Navigation tests actually pass.

We should throw in that situation providing a helpful error message as early as possible.

[ajcvickers]

@lajones

OwnsOne() on a collection

How do we test if a property represents a collection? We can know it isn't a collection if it doesn't implement IEnumerable<T>, but the converse is not true. That is, its valid for the object on the end of a reference navigation property to implement IEnumerable<T> without being conceptually a collection to EF.

[lajones]

This is more of a placeholder for me. It's meant to cover both the OwnsOne() and OwnsMany() scenarios but the title was getting very long. I don't know exactly what we can do yet. But I do remember that the error message I saw was not helpful at all in helping me track down that that was the problem. I hope we can find something that will help customers.

[lajones]

@AndriySvyryd Re the remaining part - throwing for OwnsOne pointing towards a collection. It's more difficult to throw directly as @ajcvickers pointed out above. I have a potential approach but I'd like your feedback.

This line in Navigation.IsCompatible() is where we are returning false (rather than, in good circumstances where we are calling OwnsMany on a collection, returning true); this causes the Navigation not to be created - which then causes us to throw later when we say "no such navigation" if you try to configure that Navigation.

However I could at this point add in some lines which check whether we are only failing this because of multiplicity i.e. whether navigation.GetMemberType() represents an IEnumerable<targetClrType>. If that is true then I could add the name of the navigation property to a new annotation on the sourceEntityType, say CoreAnnotationNames.MismatchedMultiplicityNavigationProperties.

Then when we try to build the Navigation and throw the current exception at InternalEntityTypeBuilder.cs#L762 I could check this annotation and if a matching navPropName is there then throw a more helpful error message saying that we earlier failed to set up a Navigation for this property because of the mismatch in multiplicity and that that is probably the problem.

But to do that I would need to update a few of the IsCompatible() methods on Navigation and InternalForeignKeyBuilder to pass in the entity types themselves rather than their ClrTypes so that I could attach the annotation there if needed. This looks possible, so does that sound like a good approach?

[AndriySvyryd]

@lajones Why can't you call Navigation.IsCompatible() with shouldThrow: true ?

[lajones]

It's coming through this line which only sets shouldThrow to true if the configurationSource is Explicit. Here this is all being done by the RelationshipDiscoveryConvention which, of course, sets configurationSource to Convention.

[AndriySvyryd]

Here this is all being done by the RelationshipDiscoveryConvention which, of course, sets configurationSource to Convention.

I thought we were talking about the case where user calls .OwnsOne() explicitly

[lajones]

Sorry - let me be more clear. The test case looks like this where note we are calling OwnsOne() on what is really a collection NavProp:

    modelBuilder.Entity<OneToManyNavPrincipalOwner>()
        .OwnsOne<OwnedOneToManyNavDependent>(
        "OwnedDependents");

    modelBuilder.Entity<OneToManyNavPrincipalOwner>()
        .Navigation("OwnedDependents");

Ideally we'd throw in OwnsOne() saying that what you're doing is wrong - but I don't think we can because of @ajcvickers comment above - it's perfectly valid for TargetType to implement IEnumerable<TargetType> if it wants to.

Currently the Navigation call throws but just saying that there should be an existing Navigation to configure - which is not helpful as it looks as though the Navigation should be there. But it's not because of the line I mentioned above.

While this line is called through OwnsOne(), it is also called earlier by the Entity() call. OwnsOne() is just picking up NavProps which were already configured by the Entity() call, which is triggering AddEntity() which triggers the RelationshipDiscoveryConvention which initially sets up the NavProps - successfully if the multiplicities are correct, unsuccessfully (but with no error) if not.

So I'd like to record, as we go through this code "I tried to set up a Navigation here but couldn't because the multiplicity was wrong" and then later on when we throw during the Navigation() call, check whether that was the problem and throw a more helpful error message at that point.

[AndriySvyryd]

So the problem in this case is that

    modelBuilder.Entity<OneToManyNavPrincipalOwner>()
        .OwnsOne<OwnedOneToManyNavDependent>(
        "OwnedDependents");

Is not creating a navigation. This code should either create a navigation or throw.

So I'd like to record, as we go through this code "I tried to set up a Navigation here but couldn't because the multiplicity was wrong" and then later on when we throw during the Navigation() call, check whether that was the problem and throw a more helpful error message at that point.

This would produce a lot of noise in the model. There's nothing RelationshipDiscoveryConvention does that can't be done in Navigation() if needed. Since this is a negative scenario the perf impact should be minimal if we do some things again.

[lajones]

OK. Will implement that. Thanks.