[hibernate-dev] HHH-5855 bug report and a possible fix

[Vlad Mihalcea]

Hi guys

I decided to spend some time to investigate the infamous HHH-5855 (
https://hibernate.atlassian.net/browse/HHH-5855 )  bug and this is my
report.

One of the first thing that I noticed is that Sets are fine, while this bug
only replicates with bidirectional Bags.

After some hours of debugging and logging (since debugging triggers
collection initialization), I found the culprit.

In the org.hibernate.type.TypeHelper.replace(Object[] original, Object[]
target, Type[] types, SessionImplementor session, Object owner, Map
copyCache) method, when copying the cached entity state (which contains the
newly added child entity along with its identifier) onto the original
collection:

copied[i] = types[i].replace( original[i], target[i], session, owner,
copyCache );

inside the org.hibernate.type.CollectionType.replace(Object[] original,
Object[] target, Type[] types, SessionImplementor session, Object owner,
Map copyCache)  method there is this check:

if ( !Hibernate.isInitialized( original ) ) {
   return target;
}


For Sets, the collection is always initialized because of this line inside
the add() method of the org.hibernate.collection.internal.PersistentSet:

final Boolean exists = isOperationQueueEnabled() ?
readElementExistence( value ) : null;

Because of the the readElementExistence( value ) call the Set is always
initialized and upon triggering the flush, the newly added Entity being
already managed it will be left alone.

For PersistentList the aforementioned check is false and the replace never
occurs, hence the transient entity lingers in the persistence context and
the flush will trigger another insert, so we get duplicates.

To make the Bag behave like the Set, all we need to do is to change the add
method like this:

public boolean add(Object object) {
   initialize(false);
   if ( !isOperationQueueEnabled() ) {
      write();
      return bag.add( object );
   }
   else {
      queueOperation( new SimpleAdd( object ) );
      return true;
   }
}

but then four tests will fail:

org.hibernate.test.legacy.MasterDetailTest > testQueuedBagAdds FAILED
    java.lang.AssertionError at MasterDetailTest.java:1068

org.hibernate.test.unionsubclass.UnionSubclassTest > testUnionSubclass
FAILED
    org.hibernate.ObjectDeletedException at UnionSubclassTest.java:364

org.hibernate.test.version.VersionTest > testCollectionNoVersion FAILED
    java.lang.AssertionError at VersionTest.java:118

org.hibernate.test.version.VersionTest > testCollectionVersion FAILED
    java.lang.AssertionError at VersionTest.java:79

3 of them fail because we expect the List not to be initialized and
the UnionSubclassTest  fails
for a doubtful reason (we attempt to delete an entity which is still
referenced).

Basically, such a change will finally fix this issue and the Sets and Lists
will behave consistently. Since you know the reasons behind the difference
in how Sets and Lists are initialized, we need to discuss if this change is
appropriate or we should address this issue differently.

I have a branch on my fork with a test that replicates this issue and that
the proposed change manages to fix it:

https://github.com/vladmihalcea/hibernate-orm/tree/feature/hhh5855

Let me know what you think and let's discuss it further.

Vlad