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

[Gail Badner]

Hi Vlad,

I've spend quite a bit of time on this one and already have a fix. I just
have some tests to add to confirm. I will look into what you suggest, but
please check with me first if you see that an issue is assigned to me.

Thanks,
Gail

On Wed, Dec 23, 2015 at 4:13 AM, Vlad Mihalcea <mihalcea.vlad at gmail.com>
wrote:

> 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
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>