diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/BasicCollectionMapper.java b/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/BasicCollectionMapper.java index 36694e207a..e886d07eef 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/BasicCollectionMapper.java +++ b/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/BasicCollectionMapper.java @@ -25,6 +25,9 @@ import org.hibernate.envers.internal.entities.mapper.relation.lazy.initializor.B import org.hibernate.envers.internal.entities.mapper.relation.lazy.initializor.Initializor; import org.hibernate.envers.internal.reader.AuditReaderImplementor; import org.hibernate.persister.collection.CollectionPersister; +import org.hibernate.persister.entity.EntityPersister; +import org.hibernate.type.EntityType; +import org.hibernate.type.Type; /** * @author Adam Warski (adam at warski dot org) @@ -123,7 +126,7 @@ public class BasicCollectionMapper extends AbstractCollect for ( Object oldEntry : oldCollection ) { for ( Iterator itor = addedElements.iterator(); itor.hasNext(); ) { Object newEntry = itor.next(); - if ( collectionPersister.getElementType().isSame( oldEntry, newEntry ) ) { + if ( isCollectionElementSame( session, collectionPersister, oldEntry, newEntry ) ) { itor.remove(); break; } @@ -136,7 +139,7 @@ public class BasicCollectionMapper extends AbstractCollect for ( Object newEntry : newCollection ) { for ( Iterator itor = deleteElements.iterator(); itor.hasNext(); ) { Object deletedEntry = itor.next(); - if ( collectionPersister.getElementType().isSame( deletedEntry, newEntry ) ) { + if ( isCollectionElementSame( session, collectionPersister, deletedEntry, newEntry ) ) { itor.remove(); break; } @@ -149,4 +152,72 @@ public class BasicCollectionMapper extends AbstractCollect return collectionChanges; } + + private boolean isCollectionElementSame( + SessionImplementor session, + CollectionPersister collectionPersister, + Object lhs, + Object rhs) { + final Type elementType = collectionPersister.getElementType(); + + // If the collection element is an Entity association but the collection does not include the + // REVTYPE column as a part of the primary key, special care must be taken in order to assess + // whether the element actually changed. + // + // Previously we delegated to the element type, which for entity-based collections would be + // EntityType. The EntityType#isSame method results in only a reference equality check. This + // would result in both an ADD/DEL entry trying to be saved for the same entity identifier + // under certain circumstances. While we generally agree with this ORM assessment, this + // leads to HHH-13080 which ultimately is because REVTYPE is not part of the middle entity + // table's primary key. + // + // For 5.x, rather than impose schema changes mid-major release, we're going to explore this + // compromise for now where we're going to treat EntityType-based collections in a slightly + // different way by delegating the equality check to the entity identifier instead. This + // ultimately means that the equality check will leverage both reference and value equality + // since identifiers can be basic or composite types. + // + // In the end for 5.x, this means if an entity is removed from the collection and added + // back with the same identifier, we will treat it as a no-change for now to avoid the + // problem presented in HHH-13080. + // + // todo (6.0) - support REVTYPE as part of the primary key. + // What we actually want to do here is to introduce a legacy compat flag that we check + // when we generate the mapper that influences whether the revisionTypeInId value is + // true or false. When its set to true, we actually will treat all element types, + // regardless if they're entity, embeddables, or basic types equally. + // + // As an example, if a collection is cleared and instances are added back and it just + // so happens that those instances ahve the same entity identifier but aren't reference + // equal to the original collection elements, Envers will then actually treat that as + // a series of DEL followed by ADD operations for those elements, which ultimately is + // the right behavior. But that only works if REVTYPE is part of the primary key so + // that the tuple { owner_id, entity_id, rev, rev_type } differ for the two types of + // revision type operations. + // + // Currently the tuple is { owner_id, entity_id, rev } and so having this special + // treatment is critical to avoid HHH-13080. + // + if ( elementType.isEntityType() && !revisionTypeInId ) { + + // This is a short-circuit to check for reference equality only. + // There is no need to delegate to the identifier if the objects are reference equal. + if ( elementType.isSame( lhs, rhs ) ) { + return true; + } + + final EntityPersister entityPersister = session.getFactory() + .getMetamodel() + .locateEntityPersister( ( (EntityType) elementType ).getAssociatedEntityName() ); + + final Object lhsId = entityPersister.getIdentifier( lhs, session ); + final Object rhsId = entityPersister.getIdentifier( rhs, session ); + + // Since the two instances aren't reference equal, delegate to identifier now. + return entityPersister.getIdentifierType().isSame( lhsId, rhsId ); + } + + // for element types that aren't entities (aka embeddables/basic types), use legacy behavior. + return elementType.isSame( lhs, rhs ); + } }