Author: steve.ebersole(a)jboss.com Date: 2006-11-06 17:14:32 -0500 (Mon, 06 Nov 2006) New Revision: 10741 Modified: branches/Branch_3_2/Hibernate3/src/org/hibernate/type/CollectionType.java branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/MergeTest.java branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/OptLockEntity.hbm.xml branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/VersionedEntity.java Log: HHH-1401 : merge operation casuing unecessary updates Modified: branches/Branch_3_2/Hibernate3/src/org/hibernate/type/CollectionType.java =================================================================== --- branches/Branch_3_2/Hibernate3/src/org/hibernate/type/CollectionType.java 2006-11-06 22:13:39 UTC (rev 10740) +++ branches/Branch_3_2/Hibernate3/src/org/hibernate/type/CollectionType.java 2006-11-06 22:14:32 UTC (rev 10741) @@ -397,15 +397,25 @@ } /** - * Replace the elements of a collection with the elements of another collection + * Replace the elements of a collection with the elements of another collection. + * + * @param original The 'source' of the replacement elements (where we copy from) + * @param target The target of the replacement elements (where we copy to) + * @param owner The owner of the collection being merged + * @param copyCache The map of elements already replaced. + * @param session The session from which the merge event originated. + * @return The merged collection. */ - public Object replaceElements(Object original, Object target, Object owner, Map copyCache, - SessionImplementor session) throws HibernateException { - + public Object replaceElements( + Object original, + Object target, + Object owner, + Map copyCache, + SessionImplementor session) { // TODO: does not work for EntityMode.DOM4J yet! - - java.util.Collection result = (java.util.Collection) target; - + java.util.Collection result = ( java.util.Collection ) target; + final boolean isPC = ( result instanceof PersistentCollection ); + final boolean wasOriginalDirty = ( original instanceof PersistentCollection && ( ( PersistentCollection ) original ).isDirty() ); result.clear(); // copy elements into newly empty target collection @@ -414,9 +424,12 @@ while ( iter.hasNext() ) { result.add( elemType.replace( iter.next(), null, session, owner, copyCache ) ); } - + + if ( result instanceof PersistentCollection && !wasOriginalDirty ) { + ( ( PersistentCollection ) result ).clearDirty(); + } + return result; - } /** @@ -439,24 +452,25 @@ * * @param anticipatedSize The anticipated size of the instaniated collection * after we are done populating it. + * @return A newly instantiated collection to be wrapped. */ public abstract Object instantiate(int anticipatedSize); + /** + * {@inheritDoc} + */ public Object replace( final Object original, final Object target, final SessionImplementor session, final Object owner, - final Map copyCache) - throws HibernateException { - + final Map copyCache) throws HibernateException { if ( original == null ) { return null; } if ( !Hibernate.isInitialized( original ) ) { return target; } - //if ( original == target ) return target; // can't do this, since need to merge element references // for a null target, or a target which is the same as the original, we // need to put the merged elements in a new collection @@ -475,7 +489,6 @@ } return result; - } /** Modified: branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/MergeTest.java =================================================================== --- branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/MergeTest.java 2006-11-06 22:13:39 UTC (rev 10740) +++ branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/MergeTest.java 2006-11-06 22:14:32 UTC (rev 10741) @@ -2,6 +2,7 @@ package org.hibernate.test.ops; import java.util.ArrayList; +import java.util.Iterator; import junit.framework.Test; import junit.framework.TestSuite; @@ -24,6 +25,167 @@ super(str); } + public void testNoExtraUpdatesOnMerge() throws Exception { + Session s = openSession(); + s.beginTransaction(); + Node node = new Node( "test" ); + s.persist( node ); + s.getTransaction().commit(); + s.close(); + + clearCounts(); + + // node is now detached, but we have made no changes. so attempt to merge it + // into this new session; this should cause no updates... + s = openSession(); + s.beginTransaction(); + node = ( Node ) s.merge( node ); + s.getTransaction().commit(); + s.close(); + + assertUpdateCount( 0 ); + assertInsertCount( 0 ); + + /////////////////////////////////////////////////////////////////////// + // as a control measure, now update the node while it is detached and + // make sure we get an update as a result... + node.setDescription( "new description" ); + s = openSession(); + s.beginTransaction(); + node = ( Node ) s.merge( node ); + s.getTransaction().commit(); + s.close(); + assertUpdateCount( 1 ); + assertInsertCount( 0 ); + /////////////////////////////////////////////////////////////////////// + + cleanup(); + } + + public void testNoExtraUpdatesOnMergeWithCollection() throws Exception { + Session s = openSession(); + s.beginTransaction(); + Node parent = new Node( "parent" ); + Node child = new Node( "child" ); + parent.getChildren().add( child ); + child.setParent( parent ); + s.persist( parent ); + s.getTransaction().commit(); + s.close(); + + clearCounts(); + + // parent is now detached, but we have made no changes. so attempt to merge it + // into this new session; this should cause no updates... + s = openSession(); + s.beginTransaction(); + parent = ( Node ) s.merge( parent ); + s.getTransaction().commit(); + s.close(); + + assertUpdateCount( 0 ); + assertInsertCount( 0 ); + + /////////////////////////////////////////////////////////////////////// + // as a control measure, now update the node while it is detached and + // make sure we get an update as a result... + ( ( Node ) parent.getChildren().iterator().next() ).setDescription( "child's new description" ); + parent.getChildren().add( new Node( "second child" ) ); + s = openSession(); + s.beginTransaction(); + parent = ( Node ) s.merge( parent ); + s.getTransaction().commit(); + s.close(); + assertUpdateCount( 1 ); + assertInsertCount( 1 ); + /////////////////////////////////////////////////////////////////////// + + cleanup(); + } + + public void testNoExtraUpdatesOnMergeVersioned() throws Exception { + Session s = openSession(); + s.beginTransaction(); + VersionedEntity entity = new VersionedEntity( "entity", "entity" ); + s.persist( entity ); + s.getTransaction().commit(); + s.close(); + + clearCounts(); + + // entity is now detached, but we have made no changes. so attempt to merge it + // into this new session; this should cause no updates... + s = openSession(); + s.beginTransaction(); + VersionedEntity mergedEntity = ( VersionedEntity ) s.merge( entity ); + s.getTransaction().commit(); + s.close(); + + assertUpdateCount( 0 ); + assertInsertCount( 0 ); + assertEquals( "unexpected version increment", entity.getVersion(), mergedEntity.getVersion() ); + + + /////////////////////////////////////////////////////////////////////// + // as a control measure, now update the node while it is detached and + // make sure we get an update as a result... + entity.setName( "new name" ); + s = openSession(); + s.beginTransaction(); + entity = ( VersionedEntity ) s.merge( entity ); + s.getTransaction().commit(); + s.close(); + assertUpdateCount( 1 ); + assertInsertCount( 0 ); + /////////////////////////////////////////////////////////////////////// + + cleanup(); + } + + public void testNoExtraUpdatesOnMergeVersionedWithCollection() throws Exception { + Session s = openSession(); + s.beginTransaction(); + VersionedEntity parent = new VersionedEntity( "parent", "parent" ); + VersionedEntity child = new VersionedEntity( "child", "child" ); + parent.getChildren().add( child ); + child.setParent( parent ); + s.persist( parent ); + s.getTransaction().commit(); + s.close(); + + clearCounts(); + + // parent is now detached, but we have made no changes. so attempt to merge it + // into this new session; this should cause no updates... + s = openSession(); + s.beginTransaction(); + VersionedEntity mergedParent = ( VersionedEntity ) s.merge( parent ); + s.getTransaction().commit(); + s.close(); + + assertUpdateCount( 0 ); + assertInsertCount( 0 ); + assertEquals( "unexpected parent version increment", parent.getVersion(), mergedParent.getVersion() ); + VersionedEntity mergedChild = ( VersionedEntity ) mergedParent.getChildren().iterator().next(); + assertEquals( "unexpected child version increment", child.getVersion(), mergedChild.getVersion() ); + + /////////////////////////////////////////////////////////////////////// + // as a control measure, now update the node while it is detached and + // make sure we get an update as a result... + mergedParent.setName( "new name" ); + mergedParent.getChildren().add( new VersionedEntity( "child2", "new child" ) ); + s = openSession(); + s.beginTransaction(); + parent = ( VersionedEntity ) s.merge( mergedParent ); + s.getTransaction().commit(); + s.close(); + assertUpdateCount( 1 ); + assertInsertCount( 1 ); + /////////////////////////////////////////////////////////////////////// + + cleanup(); + } + public void testPersistThenMergeInSameTxnWithVersion() { Session s = openSession(); Transaction tx = s.beginTransaction(); @@ -43,11 +205,7 @@ tx.commit(); s.close(); - s = openSession(); - tx = s.beginTransaction(); - s.delete( entity ); - tx.commit(); - s.close(); + cleanup(); } public void testPersistThenMergeInSameTxnWithTimestamp() { @@ -69,11 +227,7 @@ tx.commit(); s.close(); - s = openSession(); - tx = s.beginTransaction(); - s.delete( entity ); - tx.commit(); - s.close(); + cleanup(); } public void testMergeDeepTree() { @@ -123,7 +277,7 @@ assertInsertCount(2); assertUpdateCount(0); clearCounts(); - + s = openSession(); tx = s.beginTransaction(); s.delete(grandchild); @@ -134,7 +288,7 @@ s.delete(root); tx.commit(); s.close(); - + } public void testMergeDeepTreeWithGeneratedId() { @@ -188,7 +342,7 @@ assertInsertCount(2); assertUpdateCount(0); clearCounts(); - + s = openSession(); tx = s.beginTransaction(); s.createQuery("delete from NumberedNode where name like 'grand%'").executeUpdate(); @@ -196,7 +350,7 @@ s.createQuery("delete from NumberedNode").executeUpdate(); tx.commit(); s.close(); - + } public void testMergeTree() { @@ -230,14 +384,8 @@ assertInsertCount(1); assertUpdateCount(2); - - s = openSession(); - tx = s.beginTransaction(); - s.createQuery("delete from Node where parent is not null").executeUpdate(); - s.createQuery("delete from Node").executeUpdate(); - tx.commit(); - s.close(); - + + cleanup(); } public void testMergeTreeWithGeneratedId() { @@ -271,14 +419,8 @@ assertInsertCount(1); assertUpdateCount(2); - - s = openSession(); - tx = s.beginTransaction(); - s.createQuery("delete from NumberedNode where parent is not null").executeUpdate(); - s.createQuery("delete from NumberedNode").executeUpdate(); - tx.commit(); - s.close(); + cleanup(); } public void testMergeManaged() { @@ -317,11 +459,8 @@ .uniqueResult(), new Integer(2) ); - s.delete(root); - s.delete(mergedChild); - tx.commit(); - s.close(); - + + cleanup(); } public void testRecursiveMergeTransient() { @@ -339,7 +478,9 @@ s.clear(); s.merge( jboss.getEmployees().iterator().next() ); tx.commit(); - s.close(); + s.close(); + + cleanup(); } public void testDeleteAndMerge() throws Exception { @@ -361,20 +502,22 @@ s.merge( jboss ); s.getTransaction().commit(); s.close(); + + cleanup(); } private void clearCounts() { getSessions().getStatistics().clear(); } - private void assertInsertCount(int count) { + private void assertInsertCount(int expected) { int inserts = (int) getSessions().getStatistics().getEntityInsertCount(); - assertEquals(count, inserts); + assertEquals( "unexpected insert count", expected, inserts ); } - private void assertUpdateCount(int count) { + private void assertUpdateCount(int expected) { int updates = (int) getSessions().getStatistics().getEntityUpdateCount(); - assertEquals(count, updates); + assertEquals( "unexpected update counts", expected, updates ); } protected void configure(Configuration cfg) { @@ -390,5 +533,25 @@ return new TestSuite(MergeTest.class); } + private void cleanup() { + Session s = openSession(); + s.beginTransaction(); + s.createQuery("delete from NumberedNode where parent is not null").executeUpdate(); + s.createQuery("delete from NumberedNode").executeUpdate(); + s.createQuery("delete from Node where parent is not null").executeUpdate(); + s.createQuery("delete from Node").executeUpdate(); + s.createQuery("delete from VersionedEntity where parent is not null").executeUpdate(); + s.createQuery("delete from VersionedEntity").executeUpdate(); + s.createQuery("delete from TimestampedEntity").executeUpdate(); + + Iterator itr = s.createQuery( "from Employer" ).list().iterator(); + while ( itr.hasNext() ) { + final Employer employer = ( Employer ) itr.next(); + s.delete( employer ); + } + + s.getTransaction().commit(); + s.close(); + } } Modified: branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/OptLockEntity.hbm.xml =================================================================== --- branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/OptLockEntity.hbm.xml 2006-11-06 22:13:39 UTC (rev 10740) +++ branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/OptLockEntity.hbm.xml 2006-11-06 22:14:32 UTC (rev 10741) @@ -15,6 +15,13 @@ </id> <version name="version" column="VERS" type="long" /> <property name="name" column="NAME" type="string" /> + <many-to-one name="parent" class="VersionedEntity"/> + <set name="children" + inverse="true" + cascade="persist,merge,save-update,evict"> + <key column="parent"/> + <one-to-many class="VersionedEntity"/> + </set> </class> <class name="TimestampedEntity" table="T_ENTITY"> Modified: branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/VersionedEntity.java =================================================================== --- branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/VersionedEntity.java 2006-11-06 22:13:39 UTC (rev 10740) +++ branches/Branch_3_2/Hibernate3/test/org/hibernate/test/ops/VersionedEntity.java 2006-11-06 22:14:32 UTC (rev 10741) @@ -1,7 +1,10 @@ package org.hibernate.test.ops; +import java.util.Set; +import java.util.HashSet; + /** - * todo: describe VersionedEntity + * VersionedEntity * * @author Steve Ebersole */ @@ -10,6 +13,9 @@ private String name; private long version; + private VersionedEntity parent; + private Set children = new HashSet(); + public VersionedEntity() { } @@ -41,4 +47,20 @@ public void setVersion(long version) { this.version = version; } + + public VersionedEntity getParent() { + return parent; + } + + public void setParent(VersionedEntity parent) { + this.parent = parent; + } + + public Set getChildren() { + return children; + } + + public void setChildren(Set children) { + this.children = children; + } }