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

[5.0.1] Avoid stack overflow in negative many-to-many cases #23455

Merged
merged 2 commits into from
Nov 25, 2020

Conversation

AndriySvyryd
Copy link
Member

Fixes #23377
Fixes #23354

Description

Some unsupported models cause StackOverflowException instead of a more useful exception.

Customer Impact

Using the same property for a self-referencing many-to-many or using the same entity type as the join entity type produces a StackOverflowException.

How found

Multiple customers reported on 5.0.

Test coverage

Added tests for affected scenarios

Regression?

No, many-to-many is a new feature in 5.0

Risk

Low. This only affects many-to-many.

t => t.HasOne(a => a.Related).WithMany(b => b.DirectlyRelatedSelfRefs),
t => t.HasOne(a => a.SelfRef1).WithMany(b => b.SelfRef2));

Assert.Equal(CoreStrings.EntityRequiresKey(nameof(SelfRefManyToOne)),
Copy link
Member

Choose a reason for hiding this comment

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

Why is EntityRequiresKey thrown in these cases?

Copy link
Member

Choose a reason for hiding this comment

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

If the join entity is same as one side of skip navigation entities then we don't configure the PK to be composite key of FK properties in join entity.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use the normal rule the PK will include the FKs to the both of the types in many-to-many, and since one of them is the same entity type we are configuring then after setting the PK we will change the FK to target all PK properties and if the FK is changed we'll change the PK to include all of the FK properties and so on.

@ajcvickers
Copy link
Member

Approved by Tactics for 5.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants