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

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 >