diff --git a/hibernate-core/src/main/java/org/hibernate/event/internal/EventCache.java b/hibernate-core/src/main/java/org/hibernate/event/internal/EventCache.java index 8205cc3eaa..94263b3aad 100644 --- a/hibernate-core/src/main/java/org/hibernate/event/internal/EventCache.java +++ b/hibernate-core/src/main/java/org/hibernate/event/internal/EventCache.java @@ -20,47 +20,12 @@ * Free Software Foundation, Inc. * 51 Franklin Street, Fifth Floor * Boston, MA 02110-1301 USA - * - NOTICE: File changed by Wim Ockerman @ CISCO on 2011/10/20 - * Reasons of change: - * 1. The EventCache is used during the merging of object graphs - * to persisted objects. During this process the EventCache - * builds up a map of entities (persistable objects) to - * copies (original detached objects or loaded entities). - * At certain places in the merge algorithm the inverse relation - * is needed, see - * ...def.AbstractSaveEventListener.performSaveOrReplicate - * -> calling persister.getPropertyValuesToInsert(.., getMergeMap(),..) - * The getMergeMap() call calls through this this class' invertMap method. - * - * In the original implementation of invertMap() a map was created on the spot - * and all the elements in the entityToCopyMap where put in, now as copy - * to entity pair. - * Because this action can happen a lot in a big detached graph wile merging, the size - * of the entityToCopyMap can become substantial and thus also the creation of the - * inverted map. - * Tests with large graphs thus showed quadratic loss of performance. From a certain - * graph size this became substantial. - * - * - * As solution an inverseEntitiyToCopyMap (so a copyToEntityMap) is maintained now - * along with the changes to the entityToCopyMap. - * - * This made the merge action more linear in terms of performance. - * - * The changes are covered by a new UnitTest. - * - * - * Changes in the code have preceding comments of format - * CHANGE Wim Ockerman [:] - * And end with: - * END OF CHANGE */ package org.hibernate.event.internal; import java.util.Collection; +import java.util.Collections; import java.util.IdentityHashMap; -import java.util.Iterator; import java.util.Map; import java.util.Set; @@ -86,15 +51,14 @@ import org.hibernate.AssertionFailure; * @author Gail Badner */ class EventCache implements Map { - private Map entityToCopyMap = new IdentityHashMap(10); + private Map entityToCopyMap = new IdentityHashMap(10); // key is an entity involved with the operation performed by the listener; // value can be either a copy of the entity or the entity itself - // CHANGE Wim Ockerman 2011/10/20: maintains the inverse of the entityToCopyMap for performance reasons. - private Map copyToEntityMap = new IdentityHashMap( 10 ); - // END OF CHANGE + private Map copyToEntityMap = new IdentityHashMap( 10 ); + // maintains the inverse of the entityToCopyMap for performance reasons. - private Map entityToOperatedOnFlagMap = new IdentityHashMap( 10 ); + private Map entityToOperatedOnFlagMap = new IdentityHashMap( 10 ); // key is an entity involved with the operation performed by the listener; // value is a flag indicating if the listener explicitly operates on the entity @@ -103,11 +67,7 @@ class EventCache implements Map { */ public void clear() { entityToCopyMap.clear(); - - // CHANGE Wim Ockerman 2011/10/20 copyToEntityMap.clear(); - // END OF CHANGE - entityToOperatedOnFlagMap.clear(); } @@ -134,15 +94,17 @@ class EventCache implements Map { if ( copy == null ) { throw new NullPointerException( "null copies are not supported by " + getClass().getName() ); } - return entityToCopyMap.containsValue( copy ); + return copyToEntityMap.containsKey( copy ); } /** - * Returns a set view of the entity-to-copy mappings contained in this EventCache. - * @return set view of the entity-to-copy mappings contained in this EventCache + * Returns an unmodifiable set view of the entity-to-copy mappings contained in this EventCache. + * @return an unmodifiable set view of the entity-to-copy mappings contained in this EventCache + * + * @see {@link Collections#unmodifiableSet(java.util.Set)} */ public Set entrySet() { - return entityToCopyMap.entrySet(); + return Collections.unmodifiableSet( entityToCopyMap.entrySet() ); } /** @@ -167,11 +129,13 @@ class EventCache implements Map { } /** - * Returns a set view of the entities contained in this EventCache - * @return a set view of the entities contained in this EventCache + * Returns an unmodifiable set view of the entities contained in this EventCache + * @return an unmodifiable set view of the entities contained in this EventCache + * + * @see {@link Collections#unmodifiableSet(java.util.Set)} */ public Set keySet() { - return entityToCopyMap.keySet(); + return Collections.unmodifiableSet( entityToCopyMap.keySet() ); } /** @@ -183,14 +147,7 @@ class EventCache implements Map { * @throws NullPointerException if entity or copy is null */ public Object put(Object entity, Object copy) { - if ( entity == null || copy == null ) { - throw new NullPointerException( "null entities and copies are not supported by " + getClass().getName() ); - } - entityToOperatedOnFlagMap.put( entity, Boolean.FALSE ); - // CHANGE Wim Ockerman 2011/10/20 - copyToEntityMap.put(copy, entity); - // END OF CHANGE - return entityToCopyMap.put( entity, copy ); + return put( entity, copy, Boolean.FALSE ); } /** @@ -207,11 +164,35 @@ class EventCache implements Map { if ( entity == null || copy == null ) { throw new NullPointerException( "null entities and copies are not supported by " + getClass().getName() ); } - entityToOperatedOnFlagMap.put( entity, Boolean.valueOf( isOperatedOn ) ); - // CHANGE Wim Ockerman 2011/10/20 - copyToEntityMap.put(copy, entity); - // END OF CHANGE - return entityToCopyMap.put( entity, copy ); + + Object oldCopy = entityToCopyMap.put( entity, copy ); + Boolean oldOperatedOn = entityToOperatedOnFlagMap.put( entity, isOperatedOn ); + Object oldEntity = copyToEntityMap.put( copy, entity ); + + if ( oldCopy == null ) { + if ( oldEntity != null ) { + throw new IllegalStateException( "An entity copy is already assigned to a different entity." ); + } + if ( oldOperatedOn != null ) { + throw new IllegalStateException( "entityToOperatedOnFlagMap contains an entity, but entityToCopyMap does not." ); + } + } + else { + if ( oldEntity == null ) { + throw new IllegalStateException( "An entity already had a copy in entityToCopyMap, but the specified copy was not in copyToEntityMap." ); + } + if ( oldOperatedOn == null ) { + throw new IllegalStateException( "entityToCopyMap contained an entity, but entityToOperatedOnFlagMap did not." ); + } + if ( oldEntity != entity ) { + throw new IllegalStateException( "An entity copy was associated with a different entity than provided." ); + } + if ( oldCopy != copy ) { + throw new IllegalStateException( "An entity was already associated with a copy that is different from the copy provided." ); + } + } + + return oldCopy; } /** @@ -220,16 +201,9 @@ class EventCache implements Map { * @throws NullPointerException if any map keys or values are null */ public void putAll(Map map) { - for ( Iterator it=map.entrySet().iterator(); it.hasNext(); ) { - Map.Entry entry = ( Map.Entry ) it.next(); - if ( entry.getKey() == null || entry.getValue() == null ) { - throw new NullPointerException( "null entities and copies are not supported by " + getClass().getName() ); - } - entityToCopyMap.put( entry.getKey(), entry.getValue() ); - // CHANGE Wim Ockerman 2011/10/20 - copyToEntityMap.put(entry.getValue(), entry.getKey()); - // END OF CHANGE - entityToOperatedOnFlagMap.put( entry.getKey(), Boolean.FALSE ); + for ( Object o : map.entrySet() ) { + Entry entry = (Entry) o; + put( entry.getKey(), entry.getValue() ); } } @@ -243,12 +217,28 @@ class EventCache implements Map { if ( entity == null ) { throw new NullPointerException( "null entities are not supported by " + getClass().getName() ); } - entityToOperatedOnFlagMap.remove( entity ); - // CHANGE Wim Ockerman 2011/10/20 - Object result = entityToCopyMap.remove( entity ); - copyToEntityMap.remove(result); - // END OF CHANGE - return result; + Boolean oldOperatedOn = entityToOperatedOnFlagMap.remove( entity ); + Object oldCopy = entityToCopyMap.remove( entity ); + Object oldEntity = oldCopy != null ? copyToEntityMap.remove( oldCopy ) : null; + + if ( oldCopy == null ) { + if ( oldOperatedOn != null ) { + throw new IllegalStateException( "Removed entity from entityToOperatedOnFlagMap, but entityToCopyMap did not contain the entity." ); + } + } + else { + if ( oldEntity == null ) { + throw new IllegalStateException( "Removed entity from entityToCopyMap, but copyToEntityMap did not contain the entity." ); + } + if ( oldOperatedOn == null ) { + throw new IllegalStateException( "entityToCopyMap contained an entity, but entityToOperatedOnFlagMap did not." ); + } + if ( oldEntity != entity ) { + throw new IllegalStateException( "An entity copy was associated with a different entity than provided." ); + } + } + + return oldCopy; } /** @@ -260,11 +250,13 @@ class EventCache implements Map { } /** - * Returns a collection view of the entity copies contained in this EventCache. - * @return a collection view of the entity copies contained in this EventCache + * Returns an unmodifiable set view of the entity copies contained in this EventCache. + * @return an unmodifiable set view of the entity copies contained in this EventCache + * + * @see {@link Collections#unmodifiableSet(java.util.Set)} */ public Collection values() { - return entityToCopyMap.values(); + return Collections.unmodifiableCollection( entityToCopyMap.values() ); } /** @@ -277,13 +269,12 @@ class EventCache implements Map { if ( entity == null ) { throw new NullPointerException( "null entities are not supported by " + getClass().getName() ); } - return ( ( Boolean ) entityToOperatedOnFlagMap.get( entity ) ).booleanValue(); + return entityToOperatedOnFlagMap.get( entity ); } /** * Set flag to indicate if the listener is performing the operation on the specified entity. * @param entity must be non-null and this EventCache must contain a mapping for this entity - * @return true if the listener is performing the operation on the specified entity * @throws NullPointerException if entity is null * @throws AssertionFailure if this EventCache does not contain a mapping for the specified entity */ @@ -295,16 +286,16 @@ class EventCache implements Map { ! entityToCopyMap.containsKey( entity ) ) { throw new AssertionFailure( "called EventCache.setOperatedOn() for entity not found in EventCache" ); } - entityToOperatedOnFlagMap.put( entity, Boolean.valueOf( isOperatedOn ) ); + entityToOperatedOnFlagMap.put( entity, isOperatedOn ); } /** - * Returns the copy-entity mappings - * @return the copy-entity mappings + * Returns an unmodifiable map view of the copy-entity mappings + * @return an unmodifiable map view of the copy-entity mappings + * + * @see {@link Collections#unmodifiableMap(java.util.Map)} */ public Map invertMap() { - // CHANGE Wim Ockerman 2011/10/20 - return copyToEntityMap; - // END OF CHANGE + return Collections.unmodifiableMap( copyToEntityMap ); } } \ No newline at end of file diff --git a/hibernate-core/src/test/java/org/hibernate/event/internal/EventCacheTest.java b/hibernate-core/src/test/java/org/hibernate/event/internal/EventCacheTest.java index 32ab342745..b3186a43c5 100644 --- a/hibernate-core/src/test/java/org/hibernate/event/internal/EventCacheTest.java +++ b/hibernate-core/src/test/java/org/hibernate/event/internal/EventCacheTest.java @@ -24,8 +24,11 @@ */ package org.hibernate.event.internal; +import java.util.Collection; import java.util.HashMap; +import java.util.Iterator; import java.util.Map; +import java.util.Set; import junit.framework.TestCase; @@ -40,50 +43,67 @@ public class EventCacheTest extends TestCase { public void testEntityToCopyFillFollowedByCopyToEntityMapping() { EventCache cache = new EventCache(); - Object entity = new Integer(1); - Object copy = new Integer(2); + Object entity = new Simple( 1 ); + Object copy = new Simple( 2 ); cache.put(entity, copy); - assertTrue(cache.containsKey(entity)); - assertFalse(cache.containsKey(copy)); - - assertTrue(cache.invertMap().containsKey(copy)); - assertFalse(cache.invertMap().containsKey(entity)); - - cache.clear(); - assertFalse(cache.containsKey(entity)); - assertFalse(cache.invertMap().containsKey(copy)); - } + + checkCacheConsistency( cache, 1 ); + + assertTrue( cache.containsKey( entity ) ); + assertFalse( cache.containsKey( copy ) ); + assertTrue( cache.containsValue( copy ) ); + + assertTrue( cache.invertMap().containsKey( copy ) ); + assertFalse( cache.invertMap().containsKey( entity ) ); + assertTrue( cache.invertMap().containsValue( entity ) ); + + cache.clear(); + + checkCacheConsistency( cache, 0 ); + + assertFalse(cache.containsKey(entity)); + assertFalse(cache.invertMap().containsKey(copy)); + } public void testEntityToCopyFillFollowedByCopyToEntityMappingOnRemove() { EventCache cache = new EventCache(); - Object entity = new Integer(1); - Object copy = new Integer(2); + Object entity = new Simple( 1 ); + Object copy = new Simple( 2 ); cache.put(entity, copy); - assertTrue(cache.containsKey(entity)); - assertFalse(cache.containsKey(copy)); + + checkCacheConsistency( cache, 1 ); + + assertTrue(cache.containsKey(entity)); + assertFalse( cache.containsKey( copy ) ); - assertTrue(cache.invertMap().containsKey(copy)); - assertFalse(cache.invertMap().containsKey(entity)); + assertTrue( cache.invertMap().containsKey( copy ) ); + assertFalse( cache.invertMap().containsKey( entity ) ); - cache.remove(entity); + cache.remove( entity ); + + checkCacheConsistency( cache, 0 ); + assertFalse(cache.containsKey(entity)); assertFalse(cache.invertMap().containsKey(copy)); } public void testEntityToCopyFillFollowedByCopyToEntityUsingPutAll() { EventCache cache = new EventCache(); - Map input = new HashMap(); - Object entity1 = new Integer(1); - Object copy1 = new Integer(2); + Map input = new HashMap(); + Object entity1 = new Simple( 1 ); + // + Object copy1 = new Integer( 2 ); input.put(entity1, copy1); - Object entity2 = new Integer(3); - Object copy2 = new Integer(2); + Object entity2 = new Simple( 3 ); + Object copy2 = new Integer( 2 ); input.put(entity2, copy2); cache.putAll(input); - - assertTrue(cache.containsKey(entity1)); + + checkCacheConsistency( cache, 2 ); + + assertTrue(cache.containsKey(entity1)); assertFalse(cache.containsKey(copy1)); assertTrue(cache.containsKey(entity2)); assertFalse(cache.containsKey(copy2)); @@ -97,21 +117,321 @@ public class EventCacheTest extends TestCase { public void testEntityToCopyFillFollowedByCopyToEntityMappingUsingPutWithSetOperatedOnArg() { EventCache cache = new EventCache(); - Object entity = new Integer(1); - Object copy = new Integer(2); + Object entity = new Simple( 1 ); + Object copy = new Simple( 2 ); cache.put(entity, copy, true); - assertTrue(cache.containsKey(entity)); - assertFalse(cache.containsKey(copy)); + + checkCacheConsistency( cache, 1 ); + + assertTrue(cache.containsKey(entity)); + assertFalse( cache.containsKey( copy ) ); + + assertTrue( cache.invertMap().containsKey( copy ) ); + assertFalse( cache.invertMap().containsKey( entity ) ); - assertTrue(cache.invertMap().containsKey(copy)); - assertFalse(cache.invertMap().containsKey(entity)); - - cache.clear(); - cache.put(entity, copy, false); - assertTrue(cache.containsKey(entity)); + cache.clear(); + + checkCacheConsistency( cache, 0 ); + + cache.put(entity, copy, false); + + checkCacheConsistency( cache, 1 ); + + assertTrue(cache.containsKey(entity)); assertFalse(cache.containsKey(copy)); } - + public void testEntityToCopyFillFollowedByIterateEntrySet() { + EventCache cache = new EventCache(); + Object entity = new Simple( 1 ); + Object copy = new Simple( 2 ); + + cache.put( entity, copy, true ); + + checkCacheConsistency( cache, 1 ); + + Iterator it = cache.entrySet().iterator(); + assertTrue( it.hasNext() ); + Map.Entry entry = ( Map.Entry ) it.next(); + assertSame( entity, entry.getKey() ); + assertSame( copy, entry.getValue() ); + assertFalse( it.hasNext() ); + + } + + public void testEntityToCopyFillFollowedByModifyEntrySet() { + EventCache cache = new EventCache(); + Object entity = new Simple( 1 ); + Object copy = new Simple( 2 ); + + cache.put( entity, copy, true ); + + Iterator it = cache.entrySet().iterator(); + try { + it.remove(); + fail( "should have thrown UnsupportedOperationException" ); + } + catch ( UnsupportedOperationException ex ) { + // expected + } + + Map.Entry entry = (Map.Entry) cache.entrySet().iterator().next(); + try { + cache.entrySet().remove( entry ); + fail( "should have thrown UnsupportedOperationException" ); + } + catch ( UnsupportedOperationException ex ) { + // expected + } + + Map.Entry anotherEntry = new Map.Entry() { + private Object key = new Simple( 3 ); + private Object value = 4; + @Override + public Object getKey() { + return key; + } + + @Override + public Object getValue() { + return value; + } + + @Override + public Object setValue(Object value) { + Object oldValue = this.value; + this.value = value; + return oldValue; + } + }; + try { + cache.entrySet().add( anotherEntry ); + fail( "should have thrown UnsupportedOperationException" ); + } + catch ( UnsupportedOperationException ex ) { + // expected + } + + } + + public void testEntityToCopyFillFollowedByModifyKeys() { + EventCache cache = new EventCache(); + Object entity = new Simple( 1 ); + Object copy = new Simple( 2 ); + + cache.put( entity, copy, true ); + + Iterator it = cache.keySet().iterator(); + try { + it.remove(); + fail( "should have thrown UnsupportedOperationException" ); + } + catch ( UnsupportedOperationException ex ) { + // expected + } + + try { + cache.keySet().remove( entity ); + fail( "should have thrown UnsupportedOperationException" ); + } + catch ( UnsupportedOperationException ex ) { + // expected + } + + Object newCopy = new Simple( 3 ); + try { + cache.keySet().add( newCopy ); + fail( "should have thrown UnsupportedOperationException" ); + } + catch ( UnsupportedOperationException ex ) { + // expected + } + } + + public void testEntityToCopyFillFollowedByModifyValues() { + EventCache cache = new EventCache(); + Object entity = new Simple( 1 ); + Object copy = new Simple( 2 ); + + cache.put( entity, copy, true ); + + Iterator it = cache.values().iterator(); + try { + it.remove(); + fail( "should have thrown UnsupportedOperationException" ); + } + catch ( UnsupportedOperationException ex ) { + // expected + } + + try { + cache.values().remove( copy ); + fail( "should have thrown UnsupportedOperationException" ); + } + catch ( UnsupportedOperationException ex ) { + // expected + } + + Object newCopy = new Simple( 3 ); + try { + cache.values().add( newCopy ); + fail( "should have thrown UnsupportedOperationException" ); + } + catch ( UnsupportedOperationException ex ) { + // expected + } + } + + public void testEntityToCopyFillFollowedByModifyKeyOfEntrySetElement() { + + EventCache cache = new EventCache(); + Simple entity = new Simple( 1 ); + Simple copy = new Simple( 0 ); + cache.put(entity, copy, true); + + Map.Entry entry = (Map.Entry) cache.entrySet().iterator().next(); + ( ( Simple ) entry.getKey() ).setValue( 2 ); + assertEquals( 2, entity.getValue() ); + + checkCacheConsistency( cache, 1 ); + + entry = (Map.Entry) cache.entrySet().iterator().next(); + assertSame( entity, entry.getKey() ); + assertSame( copy, entry.getValue() ); + } + + public void testEntityToCopyFillFollowedByModifyValueOfEntrySetElement() { + + EventCache cache = new EventCache(); + Simple entity = new Simple( 1 ); + Simple copy = new Simple( 0 ); + cache.put(entity, copy, true); + + Map.Entry entry = (Map.Entry) cache.entrySet().iterator().next(); + ( ( Simple ) entry.getValue() ).setValue( 2 ); + assertEquals( 2, copy.getValue() ); + + checkCacheConsistency( cache, 1 ); + + entry = (Map.Entry) cache.entrySet().iterator().next(); + assertSame( entity, entry.getKey() ); + assertSame( copy, entry.getValue() ); + } + + public void testReplaceCopyForEntity() { + + EventCache cache = new EventCache(); + Simple entity = new Simple( 1 ); + Simple copy = new Simple( 0 ); + cache.put(entity, copy); + + try { + cache.put( entity, new Simple( 0 ) ); + fail( "should have thrown IllegalStateException"); + } + catch( IllegalStateException ex ) { + // expected + } + + } + + public void testReplaceEntityForCopy() { + + EventCache cache = new EventCache(); + Simple entity = new Simple( 1 ); + Simple copy = new Simple( 0 ); + cache.put(entity, copy); + + try { + cache.put( new Simple( 1 ), copy ); + fail( "should have thrown IllegalStateException"); + } + catch( IllegalStateException ex ) { + // expected + } + + } + + public void testReplaceEntityForExistingCopy() { + + EventCache cache = new EventCache(); + Simple entity1 = new Simple( 1 ); + Simple copy1 = new Simple( 0 ); + cache.put(entity1, copy1); + Simple entity2 = new Simple( 2 ); + Simple copy2 = new Simple( 0 ); + cache.put( entity2, copy2 ); + + try { + cache.put( entity1, copy2 ); + fail( "should have thrown IllegalStateException"); + } + catch( IllegalStateException ex ) { + // expected + } + } + + public void testReplaceCopyForExistingEntity() { + + EventCache cache = new EventCache(); + Simple entity1 = new Simple( 1 ); + Simple copy1 = new Simple( 0 ); + cache.put(entity1, copy1); + Simple entity2 = new Simple( 2 ); + Simple copy2 = new Simple( 0 ); + cache.put( entity2, copy2 ); + + try { + cache.put( entity1, copy2 ); + fail( "should have thrown IllegalStateException"); + } + catch( IllegalStateException ex ) { + // expected + } + } + + public void testRemoveNonExistingEntity() { + + EventCache cache = new EventCache(); + assertNull( cache.remove( new Simple( 1 ) ) ); + } + + private void checkCacheConsistency(EventCache cache, int expectedSize) { + + Set entrySet = cache.entrySet(); + Set cacheKeys = cache.keySet(); + Collection cacheValues = cache.values(); + Map invertedMap = cache.invertMap(); + + assertEquals( expectedSize, entrySet.size() ); + assertEquals( expectedSize, cache.size() ); + assertEquals( expectedSize, cacheKeys.size() ); + assertEquals( expectedSize, cacheValues.size() ); + assertEquals( expectedSize, invertedMap.size() ); + + for ( Object entry : cache.entrySet() ) { + Map.Entry mapEntry = ( Map.Entry ) entry; + assertSame( cache.get( mapEntry.getKey() ), mapEntry.getValue() ); + assertTrue( cacheKeys.contains( mapEntry.getKey() ) ); + assertTrue( cacheValues.contains( mapEntry.getValue() ) ); + assertSame( mapEntry.getKey(), invertedMap.get( mapEntry.getValue() ) ); + } + } + + private static class Simple { + private int value; + + public Simple(int value) { + this.value = value; + } + + public int getValue() { + return value; + } + + public void setValue(int value) { + this.value = value; + } + } }