HHH-7097 - Entity load event doesn't result in naturalId->pk caching

hibernate-core/src/main/java/org/hibernate/engine/internal/NaturalIdXrefDelegate.java

@@ -39,6 +39,7 @@ import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.engine.spi.SessionImplementor;
 import org.hibernate.event.spi.EventSource;
 import org.hibernate.persister.entity.EntityPersister;
+import org.hibernate.type.Type;
 
 /**
  * Maintains a {@link org.hibernate.engine.spi.PersistenceContext}-level 2-way cross-reference (xref) between the 
@@ -74,11 +75,12 @@ public class NaturalIdXrefDelegate {
 			naturalIdResolutionCacheMap.put( persister, entityNaturalIdResolutionCache );
 		}
 
-		final CachedNaturalId cachedNaturalId = new CachedNaturalId( persister, naturalIdValues );
+		final boolean justAddedToLocalCache = entityNaturalIdResolutionCache.cache( pk, naturalIdValues );
-		entityNaturalIdResolutionCache.pkToNaturalIdMap.put( pk, cachedNaturalId );
-		entityNaturalIdResolutionCache.naturalIdToPkMap.put( cachedNaturalId, pk );
 
-		//If second-level caching is enabled cache the resolution there as well
+		// If second-level caching is enabled cache the resolution there as well
+		//		NOTE : the checks using 'justAddedToLocalCache' below protect only the stat journaling, not actually
+		//		putting into the shared cache.  we still put into the shared cache because that might have locking
+		//		semantics that we need to honor.
 		if ( persister.hasNaturalIdCache() ) {
 			final NaturalIdRegionAccessStrategy naturalIdCacheAccessStrategy = persister.getNaturalIdCacheAccessStrategy();
 			final NaturalIdCacheKey naturalIdCacheKey = new NaturalIdCacheKey( naturalIdValues, persister, session() );
@@ -94,7 +96,7 @@ public class NaturalIdXrefDelegate {
 							null
 					);
 	
-					if ( put && factory.getStatistics().isStatisticsEnabled() ) {
+					if ( put && justAddedToLocalCache && factory.getStatistics().isStatisticsEnabled() ) {
 						factory.getStatisticsImplementor()
 								.naturalIdCachePut( naturalIdCacheAccessStrategy.getRegion().getName() );
 					}
@@ -110,7 +112,7 @@ public class NaturalIdXrefDelegate {
 								public void doAfterTransactionCompletion(boolean success, SessionImplementor session) {
 									final boolean put = naturalIdCacheAccessStrategy.afterInsert( naturalIdCacheKey, pk );
 
-									if ( put && factory.getStatistics().isStatisticsEnabled() ) {
+									if ( put && justAddedToLocalCache && factory.getStatistics().isStatisticsEnabled() ) {
 										factory.getStatisticsImplementor().naturalIdCachePut(
 												naturalIdCacheAccessStrategy.getRegion().getName() );
 									}
@@ -130,7 +132,7 @@ public class NaturalIdXrefDelegate {
 								public void doAfterTransactionCompletion(boolean success, SessionImplementor session) {
 									final boolean put = naturalIdCacheAccessStrategy.afterUpdate( naturalIdCacheKey, pk, lock );
 
-									if ( put && factory.getStatistics().isStatisticsEnabled() ) {
+									if ( put && justAddedToLocalCache && factory.getStatistics().isStatisticsEnabled() ) {
 										factory.getStatisticsImplementor().naturalIdCachePut(
 												naturalIdCacheAccessStrategy.getRegion().getName() );
 									}
@@ -310,16 +312,49 @@ public class NaturalIdXrefDelegate {
 
 	private static class NaturalIdResolutionCache implements Serializable {
 		private final EntityPersister persister;
+		private final Type[] naturalIdTypes;
+
+		private Map<Serializable, CachedNaturalId> pkToNaturalIdMap = new ConcurrentHashMap<Serializable, CachedNaturalId>();
+		private Map<CachedNaturalId, Serializable> naturalIdToPkMap = new ConcurrentHashMap<CachedNaturalId, Serializable>();
 
 		private NaturalIdResolutionCache(EntityPersister persister) {
 			this.persister = persister;
+
+			final int[] naturalIdPropertyIndexes = persister.getNaturalIdentifierProperties();
+			naturalIdTypes = new Type[ naturalIdPropertyIndexes.length ];
+			int i = 0;
+			for ( int naturalIdPropertyIndex : naturalIdPropertyIndexes ) {
+				naturalIdTypes[i++] = persister.getPropertyType( persister.getPropertyNames()[ naturalIdPropertyIndex ] );
+			}
 		}
 
 		public EntityPersister getPersister() {
 			return persister;
 		}
 
-		private Map<Serializable, CachedNaturalId> pkToNaturalIdMap = new ConcurrentHashMap<Serializable, CachedNaturalId>();
+		public boolean cache(Serializable pk, Object[] naturalIdValues) {
-		private Map<CachedNaturalId, Serializable> naturalIdToPkMap = new ConcurrentHashMap<CachedNaturalId, Serializable>();
+			final CachedNaturalId initial = pkToNaturalIdMap.get( pk );
+			if ( initial != null ) {
+				if ( areSame( naturalIdValues, initial.getValues() ) ) {
+					return false;
+				}
+			}
+
+			final CachedNaturalId cachedNaturalId = new CachedNaturalId( persister, naturalIdValues );
+			pkToNaturalIdMap.put( pk, cachedNaturalId );
+			naturalIdToPkMap.put( cachedNaturalId, pk );
+			
+			return true;
+		}
+
+		private boolean areSame(Object[] naturalIdValues, Object[] values) {
+			// lengths have already been verified at this point
+			for ( int i = 0; i < naturalIdTypes.length; i++ ) {
+				if ( naturalIdTypes[i].compare( naturalIdValues[i], values[i] ) != 0 ) {
+					return false;
+				}
+			}
+			return true;
+		}
 	}
 }

hibernate-core/src/main/java/org/hibernate/event/internal/DefaultLoadEventListener.java

@@ -37,13 +37,13 @@ import org.hibernate.cache.spi.access.SoftLock;
 import org.hibernate.cache.spi.entry.CacheEntry;
 import org.hibernate.engine.internal.TwoPhaseLoad;
 import org.hibernate.engine.internal.Versioning;
+import org.hibernate.engine.spi.CachedNaturalIdValueSource;
 import org.hibernate.engine.spi.EntityEntry;
 import org.hibernate.engine.spi.EntityKey;
 import org.hibernate.engine.spi.PersistenceContext;
 import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.engine.spi.SessionImplementor;
 import org.hibernate.engine.spi.Status;
-import org.hibernate.engine.spi.PersistenceContext.CachedNaturalIdValueSource;
 import org.hibernate.event.service.spi.EventListenerRegistry;
 import org.hibernate.event.spi.EventSource;
 import org.hibernate.event.spi.EventType;

hibernate-core/src/matrix/java/org/hibernate/test/annotations/naturalid/NaturalIdOnSingleManyToOneTest.java

@@ -26,6 +26,8 @@ package org.hibernate.test.annotations.naturalid;
 import java.util.List;
 
 import org.jboss.logging.Logger;
+
+import org.junit.After;
 import org.junit.Test;
 
 import org.hibernate.Criteria;
@@ -52,11 +54,16 @@ import static org.junit.Assert.assertTrue;
 public class NaturalIdOnSingleManyToOneTest extends BaseCoreFunctionalTestCase {
 	private static final Logger log = Logger.getLogger( NaturalIdOnSingleManyToOneTest.class );
 
-	@Override
+	@After
-	protected void cleanupTest() throws Exception {
+	public void cleanupData() {
-		this.cleanupCache();
+		super.cleanupCache();
-		
+		Session s = sessionFactory().openSession();
-		this.deleteAllData();
+		s.beginTransaction();
+		s.createQuery( "delete NaturalIdOnManyToOne" ).executeUpdate();
+		s.createQuery( "delete Citizen" ).executeUpdate();
+		s.createQuery( "delete State" ).executeUpdate();
+		s.getTransaction().commit();
+		s.close();
 	}
 
 	@Test

hibernate-core/src/matrix/java/org/hibernate/test/annotations/naturalid/NaturalIdTest.java

@@ -38,6 +38,8 @@ import org.hibernate.criterion.Restrictions;
 import org.hibernate.metadata.ClassMetadata;
 import org.hibernate.stat.Statistics;
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
+
+import org.junit.After;
 import org.junit.Test;
 
 /**
@@ -48,11 +50,16 @@ import org.junit.Test;
  */
 @SuppressWarnings("unchecked")
 public class NaturalIdTest extends BaseCoreFunctionalTestCase {
-	@Override
+	@After
-	protected void cleanupTest() throws Exception {
+	public void cleanupData() {
-		this.cleanupCache();
+		super.cleanupCache();
-		
+		Session s = sessionFactory().openSession();
-		this.deleteAllData();
+		s.beginTransaction();
+		s.createQuery( "delete NaturalIdOnManyToOne" ).executeUpdate();
+		s.createQuery( "delete Citizen" ).executeUpdate();
+		s.createQuery( "delete State" ).executeUpdate();
+		s.getTransaction().commit();
+		s.close();
 	}
 
 	@Test

hibernate-core/src/matrix/java/org/hibernate/test/naturalid/immutableentity/ImmutableEntityNaturalIdTest.java

@@ -133,9 +133,9 @@ public class ImmutableEntityNaturalIdTest extends BaseCoreFunctionalTestCase {
 		building = (Building) naturalIdLoader.load();
 		assertNull( building );
 		assertEquals( "Cache hits should be one after second query", 1, stats.getNaturalIdCacheHitCount() );
-		assertEquals( "Cache misses should be one after second query", 1, stats.getNaturalIdCacheMissCount() );
+		assertEquals( "Cache misses should be two after second query", 2, stats.getNaturalIdCacheMissCount() );
 		assertEquals( "Cache put should be one after second query", 1, stats.getNaturalIdCachePutCount() );
-		assertEquals( "Query count should be one after second query", 1, stats.getNaturalIdQueryExecutionCount() );
+		assertEquals( "Query count should be two after second query", 2, stats.getNaturalIdQueryExecutionCount() );
 
 		// cleanup
 		tx.commit();
@@ -152,17 +152,13 @@ public class ImmutableEntityNaturalIdTest extends BaseCoreFunctionalTestCase {
 		building = (Building) naturalIdLoader.load();
 		assertNull( building );
 		assertEquals( "Cache hits should be one after third query", 1, stats.getNaturalIdCacheHitCount() );
-		assertEquals( "Cache misses should be one after third query", 2, stats.getNaturalIdCacheMissCount() );
+		assertEquals( "Cache misses should be one after third query", 3, stats.getNaturalIdCacheMissCount() );
 		assertEquals( "Cache put should be one after third query", 1, stats.getNaturalIdCachePutCount() );
-		assertEquals( "Query count should be one after third query", 2, stats.getNaturalIdQueryExecutionCount() );
+		assertEquals( "Query count should be one after third query", 3, stats.getNaturalIdQueryExecutionCount() );
 
 		// cleanup
 		tx.rollback();
 		s.close();
-		
-		
-		
-		
 	}
 
 	@Override

hibernate-testing/src/main/java/org/hibernate/testing/junit4/BaseCoreFunctionalTestCase.java

@@ -423,41 +423,6 @@ public abstract class BaseCoreFunctionalTestCase extends BaseUnitTestCase {
 		assertAllDataRemoved();
 	}
 
-	protected void deleteAllData() {
-		// Get all the entities the session factory knows about
-		final Map<String, ClassMetadata> allClassMetadata = this.sessionFactory().getAllClassMetadata();
-		Set<ClassMetadata> entityTypes = new LinkedHashSet<ClassMetadata>(allClassMetadata.values());
-
-        do {
-            final Set<ClassMetadata> failedEntitieTypes = new HashSet<ClassMetadata>();
-            
-            for (final ClassMetadata entityType : entityTypes) {
-                final String entityClassName = entityType.getEntityName();
-                
-                final Session s = openSession();
-                final Transaction tx = s.beginTransaction();
-                try {
-            		final Criteria criteria = s.createCriteria( entityClassName );
-                    final List<?> entities = criteria.list();
-                    for (final Object entity : entities) {
-                        s.delete( entity);
-                    }  
-
-            		tx.commit();
-                }
-                catch (ConstraintViolationException e) {
-                    failedEntitieTypes.add(entityType);
-                    tx.rollback();
-                }
-                finally {
-                	s.close();            
-                }
-            }
-            
-            entityTypes = failedEntitieTypes;
-        } while (!entityTypes.isEmpty());
-	}
-
 	protected void cleanupCache() {
 		if ( sessionFactory != null ) {
 			sessionFactory.getCache().evictCollectionRegions();