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

[2.1.3] Allow dependents that can match a principal through multiple relationships #12571

Merged
merged 1 commit into from
Jul 10, 2018

Conversation

ajcvickers
Copy link
Contributor

Fixes #12227

This issues happens when the principal side of multiple relationships is using inheritance and the relationships share and FK property and principal key definition. In this case, the FK value may match instances that are not of the correct type for the given relationship. This would normally be an error and we started throwing for it in 2.1. But it is not an error if the principal is correct for some other relationship sharing the same FK property. So the fix here is to check all the FKs to see if any one can be used. If so, then no problem. If not, then still an error to catch the common case. No matches is also okay, since we don't know whether the principal is loaded or not.

Note that technically, GetPrincipal is broken and should be fixed with new API surface to get all possible principals, but not doing this here because it would be new API in a patch.

…ships

Fixes #12227

This issues happens when the principal side of multiple relationships is using inheritance and the relationships share and FK property and principal key definition. In this case, the FK value may match instances that are not of the correct type for the given relationship. This would normally be an error and we started throwing for it in 2.1. But it is not an error if the principal is correct for some other relationship sharing the same FK property. So the fix here is to check all the FKs to see if any one can be used. If so, then no problem. If not, then still an error to catch the common case. No matches is also okay, since we don't know whether the principal is loaded or not.

Note that technically, GetPrincipal is broken and should be fixed with new API surface to get all possible principals, but not doing this here because it would be new API in a patch.
@ajcvickers ajcvickers requested a review from AndriySvyryd July 5, 2018 21:49
@smitpatel smitpatel changed the base branch from master to release/2.1 July 5, 2018 22:38
Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

File an issue to add the GetPrincipal overload that returns all principals, to verify that all principals are valid for required relationship and possibly to allow optional foreign keys on non-nullable properties

@AndriySvyryd AndriySvyryd merged commit 06c6a0a into release/2.1 Jul 10, 2018
@AndriySvyryd AndriySvyryd deleted the StickingToYourPrincipals0705 branch July 10, 2018 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants