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

[Gail Badner]

We really don't want to initialize a List when merging. Instead, we want to
do the same sort of replace on the values stored in the DelayedOperation
objects. That way, the collection will be initialized only when necessary.
The DelayedOperations are executed on flush. I'll should get a pull request
out for this today or tomorrow.

Vlad, If you are interested in digging into an issue that is assigned to
me, I'm happy to tell you my status and share what I know about it. I would
certainly welcome your help.

Thanks,
Gail

On Wed, Dec 23, 2015 at 10:51 AM, Gail Badner <gbadner at redhat.com> wrote:

> 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
>>
>
>