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

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