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.

Sign up for GitHub

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

Jump to bottom

Fix for 10508. Create Association Entity automatically by convention. #21484

Merged
merged 1 commit into from
Jul 8, 2020
Merged

Fix for 10508. Create Association Entity automatically by convention. #21484

merged 1 commit into from
Jul 8, 2020

Conversation

lajones
Copy link
Contributor

@lajones lajones commented Jul 1, 2020

Fixes #10508

Create Association Entity automatically by convention. Note: this is only the model building part - also need #19003 for the ChangeTracking part.

@lajones lajones changed the title Fix for 10508 Fix for 10508. Create Association Entity automatically by convention. Jul 1, 2020
@lajones lajones requested a review from AndriySvyryd July 1, 2020 17:07
@lajones lajones self-assigned this Jul 1, 2020
@lajones lajones mentioned this pull request Jul 1, 2020
Closed
@smitpatel smitpatel closed this Jul 1, 2020
@smitpatel smitpatel reopened this Jul 1, 2020
@smitpatel
Copy link
Contributor

@lajones - It passed CLA check now. Probably intermittent error in the server.

AndriySvyryd
AndriySvyryd reviewed Jul 1, 2020
View reviewed changes
src/EFCore/Metadata/Conventions/ManyToManyConvention.cs Outdated
// do not create an automatic many-to-many association entity type
// for a self-referencing skip navigation, or for one that
// is already "in use" (i.e. has its Foreign Key assigned).
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not doing it for self-ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I have 2 collection navigations pointing to an Entity Type from that same Entity Type, my expectation would be two 1:N navigations to self rather than one N:N. I guess I thought that it was ambiguous, and in ambiguous cases we do nothing and let the user decide.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should discuss with the team

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any m2m navigation pair is potentially 2 different 1:N relationships.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree we should discuss with the team.

AndriySvyryd
AndriySvyryd reviewed Jul 2, 2020
View reviewed changes
src/EFCore/Metadata/Internal/InternalModelBuilder.cs Outdated
@@ -211,6 +212,191 @@ public InternalModelBuilder([NotNull] Model metadata)
return entityTypeWithDefiningNavigation?.Builder;
}

private const string AssociationEntityTypeNameTemplate = "Join_{0}_{1}";
private const string AssociationPropertyNameTemplate = "{0}_{1}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should discuss both of these with the team

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the team decision WRT to FK property naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #21565 and #21567

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndriySvyryd Is there any reason we need special rules for FKs? We decided in the design meeting to just use the current rules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. That would be my preference as well.

@lajones lajones force-pushed the 20200630_Issue10508_15 branch from fe3f06d to 31d8567 Compare July 2, 2020 23:58
@lajones lajones force-pushed the 20200630_Issue10508_15 branch from 565e044 to eb03576 Compare July 7, 2020 23:53
@lajones
Create Join Entity automatically by RelationshipDiscoveryConvention a…
61f6cb9 
…t convention-time and through HasMany().WithMany() for fluent API.
@lajones lajones force-pushed the 20200630_Issue10508_15 branch from eb03576 to 61f6cb9 Compare July 8, 2020 01:20
@lajones lajones merged commit 5515d7e into master Jul 8, 2020
@lajones lajones deleted the 20200630_Issue10508_15 branch July 8, 2020 02:39
@ajcvickers ajcvickers mentioned this pull request Jul 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndriySvyryd AndriySvyryd AndriySvyryd approved these changes

@smitpatel smitpatel smitpatel left review comments

@ajcvickers ajcvickers ajcvickers left review comments

Assignees

@lajones lajones

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement many-to-many relationships without mapping join table
4 participants
@lajones @smitpatel @ajcvickers @AndriySvyryd