[hibernate-dev] HHH-5855 bug report and a possible fix
Vlad Mihalcea
mihalcea.vlad at gmail.com
Thu Jan 7 04:45:17 EST 2016
Thanks Gail,
I'll try it on my fork after I push the migrated User Guide.
Vlad
On Thu, Jan 7, 2016 at 11:36 AM, Gail Badner <gbadner at redhat.com> wrote:
> 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
>>>>>
>>>>
>>>>
>>>
>>
>
More information about the hibernate-dev
mailing list