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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

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(a)redhat.com&gt; 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(a)gmail.com&gt; > 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(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > >

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(a)gmail.com&gt; 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(a)redhat.com&gt; 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(a)redhat.com&gt; >> 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(a)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(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev >>>> >>> >>> >> >