- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3.7k
Fix HHH-19076 #10281
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
Fix HHH-19076 #10281
Conversation
        
          
                ...rnate-core/src/main/java/org/hibernate/metamodel/model/domain/internal/JpaMetamodelImpl.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | AbstractIdentifiableType<?> ownerType, | ||
| MetadataContext metadataContext) { | ||
| final java.util.List<EntityPersister> resultList = getDeclarerEntityPersister( ownerType, metadataContext ); | ||
| return resultList.isEmpty() ? null : resultList.getFirst(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there are multiple entities with different composite id types? Is it still possible that the wrong one is selected? I see that this method is still used in the identifierMemberResolver and versionMemberResolver, so I guess this situation could be triggered when e.g. a version is declared in the mapped superclass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there are multiple entities with different composite id types?
It looks like it is working correctly. I have updated the test case to with a second Entity with a different composite id class.
Is it still possible that the wrong one is selected?
I am not sure if this even matters. It was probably / hopefully not a problem before to just grab a (de facto) random Entity for a given @MappedSuperclass for some operations. I cannot rule out that a similar fix to resolveVirtualIdentifierMember might be necessary in other situations.
so I guess this situation could be triggered when e.g. a version is declared in the mapped superclass?
I have updated the other test to also have a @Version and it seems to be working just fine. I have also added a @Version to only one @Entity in the other test and this also works.
I am still very new to the hibernate code base, so I am not sure if the same fix is needed for other code paths. However, I am (currently) unable to find other / similar problems. If there are other areas that require a specific @Entity for a @MappedSuperclass, then this fix will in the worst case scenario just select a different random entity than before.
IMHO, trying to find other, non obvious cases where a similar fix is required is outside the scope of this PR (or at least outside the scope of my capabilities 😃)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution and for giving this a try. Actually, your PR got me thinking that something is not right and I played around with simply resolving the boot property member directly: #10468
This commit fixes HHH-19076. This bug is not trivial to reproduce since it requires a very specific entity setup: - There must be at least 2 entities - Both entities must extend the same @MappedSuperclass - The @MappedSuperclass must have an @id - One (and only one) of the entities must additionally use @IdClass - The entities must be named / loaded / etc. so that the entity WITHOUT the @IdClass is processed first Specifically the last step is impossible to reproduce reliably, since it depends on the [order hibernate processes the entities][1], which in turn depends on the order of the values of [a map `entityBindingMap`][2]. Depending on the order the entites are processed, the [`MetadataContext.mappedSuperClassTypeToPersistentClass`][3] map stores for the shared @MappedSuperclass a different entity. However, to correctly process the attributes of the @MappedSuperclass, the entity with the @IdClass must be used in [`resolveVirtualIdentifierMember`][4]. To fix this, this commit changes the simple [`mappedSuperClassTypeToPersistentClass`][3] map to store a set of entities instead of only the first encountered entity. Next, the [`resolveVirtualIdentifierMember`][4] function uses the first matching entity of this set. [1]: https://github.com/hibernate/hibernate-orm/blob/3cfeb8fa29769258a0c0615b7e14b469798f0f3a/hibernate-core/src/main/java/org/hibernate/metamodel/model/domain/internal/JpaMetamodelImpl.java#L627-L629 [2]: https://github.com/hibernate/hibernate-orm/blob/3cfeb8fa29769258a0c0615b7e14b469798f0f3a/hibernate-core/src/main/java/org/hibernate/boot/internal/MetadataImpl.java#L86 [3]: https://github.com/hibernate/hibernate-orm/blob/3cfeb8fa29769258a0c0615b7e14b469798f0f3a/hibernate-core/src/main/java/org/hibernate/metamodel/internal/MetadataContext.java#L96 [4]: https://github.com/hibernate/hibernate-orm/blob/3cfeb8fa29769258a0c0615b7e14b469798f0f3a/hibernate-core/src/main/java/org/hibernate/metamodel/internal/AttributeFactory.java#L687
d60d394    to
    ffb7ba6      
    Compare
  
    | Thanks for your contribution. Superseded by #10468 | 
This commit fixes HHH-19076. This bug is not trivial to reproduce since it requires a very specific entity setup:
@MappedSuperclass@MappedSuperclassmust have an@Id@IdClass@IdClassis processed firstSpecifically the last step is impossible to reproduce reliably, since it depends on the order hibernate processes the entities, which in turn depends on the order of the values of a map
entityBindingMap.Depending on the order the entities are processed, the
MetadataContext.mappedSuperClassTypeToPersistentClassmap stores for the shared@MappedSuperclassa different entity.However, to correctly process the attributes of the
@MappedSuperclass, the entity with the@IdClassmust be used inresolveVirtualIdentifierMember.To fix this, this commit changes the simple
mappedSuperClassTypeToPersistentClassmap to store a setof entities instead of only the first encountered entity.
Next, the
resolveVirtualIdentifierMemberfunction uses the first matching entity of this set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
https://hibernate.atlassian.net/browse/HHH-19076