Partial fix for 6674. #20271
Partial fix for 6674. #20271
Conversation
src/EFCore/Metadata/Internal/InternalNavigationIdentityBuilder.cs
Outdated
Show resolved
Hide resolved
|
@AndriySvyryd See also #20276 for being able to configure the dependent side navigation of an Owned-type. |
|
I pushed a commit for what I was talking about |
|
@AndriySvyryd Ah - understand now. Thanks. |
Not in this PR. Where are those changes? |
|
@smitpatel I'm not removing the old way until we're agreed this is the way forward. |
|
@smitpatel The tests are renamed in the other PR (#20276) - where we allow you to specify the dependent end as well. |
|
Squash the commits. Might as well include #20276 too |
|
@Andriy Need to change the names in the tests to get rid of NavigationIdentityBuilder there too. Then will do. |
…ntityBuilder. Added OnetoMany, ManyToOne, ManyToMany, OneToOne and principal side of the Owned Type tests.
lajones
force-pushed
the
20200312_Issue6674_Nav_12
branch
from
d820f99
to
caa5582
Compare
| Builder = ((Navigation)navigation).Builder; | ||
| NavBuilder = (navigationOrSkipNavigation as Navigation)?.Builder; | ||
| SkipNavBuilder = (navigationOrSkipNavigation as SkipNavigation)?.Builder; | ||
| Metadata = (IMutableNavigationBase)NavBuilder?.Metadata ?? SkipNavBuilder?.Metadata; |
There was a problem hiding this comment.
The Metadata property should be computed, it's rarely accessed.
If you are going to store it, then assign from navigationOrSkipNavigation
There was a problem hiding this comment.
You mean (navigationOrSkipNavigation as Navigation)?.Builder.Metadata ?? (navigationOrSkipNavigation as SkipNavigation)?.Builder.Metadata? That seems worse?
There was a problem hiding this comment.
Metadata = navigationOrSkipNavigation
There was a problem hiding this comment.
Just FMI why don't we do this on all of the builders?
There was a problem hiding this comment.
It uses more memory and is completely unnecessary
There was a problem hiding this comment.
Do you want to just go back to doing it your way then? It looked good to me but I'm happy to revert.
There was a problem hiding this comment.
I'd prefer that, but I don't feel strongly
|
|
||
| modelBuilder.Entity<ManyToManyNavPrincipal>() | ||
| .HasMany<NavDependent>("Dependents") | ||
| .WithMany("ManyToManyPrincipals"); |
There was a problem hiding this comment.
You should use names that are different from the CLR properties to get better coverage.
There was a problem hiding this comment.
I tried what I thought you meant and I get The navigation property 'NonClrDependents' cannot be added to the entity type 'ManyToManyNavPrincipal' because there is no corresponding CLR property on the underlying type and navigations properties cannot be added to shadow state.
Can you explain more?
There was a problem hiding this comment.
Oh, you also need to use shadow entity types
There was a problem hiding this comment.
Let's add that separately. It sounds likes that going to be tricky given the error above "cannot be added to shadow state". I'll add it to the issue to make sure I don't forget. I want to get feedback that the rest of the team likes this new approach before I flesh out the tests.
Partial fix for #6674
Created new Navigation() calls directly on EntityBuilder. Added tests for OneToMany, ManyToOne, ManyToMany, OneToOne and principal side of the Owned Type.
Things remaining:
WithOwner()to allow configuring a navigation on the dependent side of an Owned Type.