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

OGM-1198 Mapping error with multiple associations on the same entity #786

Closed
wants to merge 5 commits into from

Conversation

@gsmet gsmet self-assigned this Oct 28, 2016
}
}
}

return null;
}

private static boolean isEmpty(String mappedByProperty) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use StringHelper.

if ( isCollectionMatching( mainSideJoinable, inverseCollectionPersister ) ) {
return inverseCollectionPersister.getAssociationKeyMetadata();
String mappedByProperty = inverseCollectionPersister.getMappedByProperty();
if ( isEmpty( mappedByProperty ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

You prefer having a if / else if instead of a or in the first if?

Copy link
Member

Choose a reason for hiding this comment

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

Thinking a bit more about it, if there is no mappedBy, can't we consider it's not the inverse relationship? I'm surprised we returned something in this case.

@gsmet
Copy link
Member

gsmet commented Oct 31, 2016

@DavideD I added a few comments, the important one being about the case when we don't have a mappedBy.

I think this one is bad enough we should consider backporting it to 5.0.x and make a release. WDYT?

@DavideD DavideD force-pushed the OGM-1198-Wrong-Mapping branch 2 times, most recently from d10a163 to 6f9a66a Compare November 1, 2016 14:00
  This fix make sure that when looking for the AssociationKeyMetadata, we consider
  the mappedBy value on the non-owning side of the association.

  Failing to do that causes some errors when there are multiple associations to the same entity
@DavideD
Copy link
Member Author

DavideD commented Nov 1, 2016

I've updated the PR following your remarks.

Thinking a bit more about it, if there is no mappedBy, can't we consider it's not the inverse relationship? I'm surprised we returned something in this case.

Good catch, I've tried to return null in that case and everything seems to work after I've updated a test.
You can check in the last commit
Thanks

@DavideD DavideD mentioned this pull request Nov 1, 2016
@gsmet
Copy link
Member

gsmet commented Nov 2, 2016

@DavideD well, it was a good catch but, in fact, I didn't realize the method was working when we passed an inverse side relationship to it.

The javadoc is very clear it's not supposed to and it's only used to manage the navigational information of the inverse side so I think we're safe and it's better to be strict about this.

I was more thinking about the user declaring a relation that could match but isn't declared as an inverse relationship with a mappedBy and so shouldn't be considered the inverse relationship of the main side.

@gsmet
Copy link
Member

gsmet commented Nov 2, 2016

Rebased and merged!

@gsmet gsmet closed this Nov 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants