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

[Gail Badner]

I've created a pull request for HHH-5855. [1]

I ran into several bugs related to delayed operations, so this is a
work-in-progess. I wanted to create a pull request of what I have so far. I
will revisit it when I return to work 1/14/16. I plan to address the other
bugs separately.

Regards,
Gail

[1] https://github.com/hibernate/hibernate-orm/pull/1212

On Wed, Dec 23, 2015 at 12:05 PM, Vlad Mihalcea <mihalcea.vlad at gmail.com>
wrote:

> Hi Gail,
>
> I'm glad there is a development plan on this one . I've been following
> this issue for a couple of years and seen some recent comments which
> reminded me of it.
> Someone asked me on my blog if we can get it fixed, as it's causing
> problems when people are trying to merge back detached entities.
>
> If I can help you with anything, just let me know. Now that I've been also
> digging into it, I can at least assist you and test your PR on my fork too.
>
> Vlad
>
> On Wed, Dec 23, 2015 at 9:25 PM, Gail Badner <gbadner at redhat.com> wrote:
>
>> 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
>>>>
>>>
>>>
>>
>