HHH-15848 completely new impl of DefaultDirtyCheckEventListener

this is definitely not perfect yet, but it's certainly a much
better foundation than the ancient implementation which was bad
and side-effecty in all sorts of ways

Author: gavinking

hibernate-core/src/main/java/org/hibernate/Session.java

@@ -382,8 +382,8 @@ public interface Session extends SharedSessionContract, EntityManager {
 	 * the database?  In other words, would any DML operations be executed if
 	 * we flushed this session?
 	 *
-	 * @return {@code true} if the session contains pending changes; {@code false} otherwise.
-	 * @throws HibernateException could not perform dirtying checking
+	 * @return {@code true} if the session contains pending changes;
+	 *         {@code false} otherwise.
 	 */
 	boolean isDirty();
 

hibernate-core/src/main/java/org/hibernate/engine/spi/CollectionEntry.java

@@ -9,7 +9,6 @@
 import org.hibernate.collection.spi.PersistentCollection;
 import org.hibernate.internal.CoreLogging;
 import org.hibernate.internal.CoreMessageLogger;
-import org.hibernate.internal.util.NullnessUtil;
 import org.hibernate.persister.collection.CollectionPersister;
 
 import java.io.IOException;
@@ -20,6 +19,7 @@
 
 import org.checkerframework.checker.nullness.qual.Nullable;
 
+import static org.hibernate.internal.util.NullnessUtil.castNonNull;
 import static org.hibernate.pretty.MessageHelper.collectionInfoString;
 
 /**
@@ -68,11 +68,11 @@ public CollectionEntry(CollectionPersister persister, PersistentCollection<?> co
 		// during flush shouldn't be ignored
 		ignore = false;
 
-		collection.clearDirty(); //a newly wrapped collection is NOT dirty (or we get unnecessary version updates)
+		// a newly wrapped collection is NOT dirty
+		// (or we get unnecessary version updates)
+		collection.clearDirty();
 
-		snapshot = persister.isMutable() ?
-				collection.getSnapshot( persister ) :
-				null;
+		snapshot = persister.isMutable() ? collection.getSnapshot( persister ) : null;
 		collection.setSnapshot( loadedKey, role, snapshot );
 	}
 
@@ -123,7 +123,7 @@ public CollectionEntry(PersistentCollection<?> collection, SessionFactoryImpleme
 
 		loadedKey = collection.getKey();
 		role = collection.getRole();
-		loadedPersister = factory.getRuntimeMetamodels().getMappingMetamodel().getCollectionDescriptor( NullnessUtil.castNonNull( role ) );
+		loadedPersister = factory.getMappingMetamodel().getCollectionDescriptor( castNonNull( role ) );
 
 		snapshot = collection.getStoredSnapshot();
 	}
@@ -156,12 +156,11 @@ private void dirty(PersistentCollection<?> collection) throws HibernateException
 		final CollectionPersister loadedPersister = getLoadedPersister();
 		final boolean forceDirty =
 				collection.wasInitialized()
-			&& !collection.isDirty() //optimization
-			&& loadedPersister != null
-			&& loadedPersister.isMutable() //optimization
-			&& ( collection.isDirectlyAccessible() || loadedPersister.getElementType().isMutable() ) //optimization
-			&& !collection.equalsSnapshot( loadedPersister );
-
+						&& !collection.isDirty() //optimization
+						&& loadedPersister != null
+						&& loadedPersister.isMutable() //optimization
+						&& ( collection.isDirectlyAccessible() || loadedPersister.getElementType().isMutable() ) //optimization
+						&& !collection.equalsSnapshot( loadedPersister );
 		if ( forceDirty ) {
 			collection.dirty();
 		}
@@ -174,23 +173,19 @@ public void preFlush(PersistentCollection<?> collection) throws HibernateExcepti
 		}
 
 		final CollectionPersister loadedPersister = getLoadedPersister();
-		boolean nonMutableChange = collection.isDirty()
-				&& loadedPersister != null
-				&& !loadedPersister.isMutable();
+		final boolean nonMutableChange =
+				collection.isDirty()
+						&& loadedPersister != null
+						&& !loadedPersister.isMutable();
 		if ( nonMutableChange ) {
-			throw new HibernateException(
-					"changed an immutable collection instance: " +
-					collectionInfoString( NullnessUtil.castNonNull( loadedPersister ).getRole(), getLoadedKey() )
-			);
+			throw new HibernateException( "changed an immutable collection instance: " +
+					collectionInfoString( castNonNull( loadedPersister ).getRole(), getLoadedKey() ) );
 		}
 
 		dirty( collection );
 
 		if ( LOG.isDebugEnabled() && collection.isDirty() && loadedPersister != null ) {
-			LOG.debugf(
-					"Collection dirty: %s",
-					collectionInfoString( loadedPersister.getRole(), getLoadedKey() )
-			);
+			LOG.debug( "Collection dirty: " + collectionInfoString( loadedPersister.getRole(), getLoadedKey() ) );
 		}
 
 		setReached( false );
@@ -208,9 +203,9 @@ public void postInitialize(PersistentCollection<?> collection, SharedSessionCont
 				? collection.getSnapshot( loadedPersister )
 				: null;
 		collection.setSnapshot( loadedKey, role, snapshot );
-		if ( loadedPersister != null && session.getLoadQueryInfluencers().effectivelyBatchLoadable( loadedPersister ) ) {
-			session.getPersistenceContextInternal()
-					.getBatchFetchQueue()
+		if ( loadedPersister != null
+				&& session.getLoadQueryInfluencers().effectivelyBatchLoadable( loadedPersister ) ) {
+			session.getPersistenceContextInternal().getBatchFetchQueue()
 					.removeBatchLoadableCollection( this );
 		}
 	}
@@ -236,12 +231,12 @@ public void afterAction(PersistentCollection<?> collection) {
 		loadedKey = getCurrentKey();
 		setLoadedPersister( getCurrentPersister() );
 
-		boolean resnapshot = collection.wasInitialized()
+		final boolean resnapshot = collection.wasInitialized()
 				&&  ( isDoremove() || isDorecreate() || isDoupdate() );
 		if ( resnapshot ) {
-			snapshot = loadedPersister == null || !loadedPersister.isMutable() ?
-					null :
-					collection.getSnapshot( NullnessUtil.castNonNull( loadedPersister ) ); //re-snapshot
+			snapshot = loadedPersister != null && loadedPersister.isMutable()
+					? collection.getSnapshot( castNonNull( loadedPersister ) )
+					: null; //re-snapshot
 		}
 
 		collection.postAction();
@@ -271,13 +266,11 @@ public void afterAction(PersistentCollection<?> collection) {
 	public void resetStoredSnapshot(PersistentCollection<?> collection, Serializable storedSnapshot) {
 		LOG.debugf("Reset storedSnapshot to %s for %s", storedSnapshot, this);
 
-		if ( fromMerge ) {
-			return; // EARLY EXIT!
+		if ( !fromMerge ) {
+			snapshot = storedSnapshot;
+			collection.setSnapshot( loadedKey, role, snapshot );
+			fromMerge = true;
 		}
-
-		snapshot = storedSnapshot;
-		collection.setSnapshot( loadedKey, role, snapshot );
-		fromMerge = true;
 	}
 
 	private void setLoadedPersister(@Nullable CollectionPersister persister) {
@@ -286,12 +279,9 @@ private void setLoadedPersister(@Nullable CollectionPersister persister) {
 	}
 
 	void afterDeserialize(@Nullable SessionFactoryImplementor factory) {
-		if ( factory == null ) {
-			loadedPersister = null;
-		}
-		else {
-			loadedPersister = factory.getRuntimeMetamodels().getMappingMetamodel().getCollectionDescriptor( NullnessUtil.castNonNull( role ) );
-		}
+		loadedPersister = factory == null ? null
+				: factory.getRuntimeMetamodels().getMappingMetamodel()
+						.getCollectionDescriptor( castNonNull( role ) );
 	}
 
 	public boolean wasDereferenced() {
@@ -379,13 +369,15 @@ public void setRole(@Nullable String role) {
 
 	@Override
 	public String toString() {
-		String result = "CollectionEntry" +
-				collectionInfoString( role, loadedKey );
-		if ( currentPersister != null ) {
-			result += "->" +
-					collectionInfoString( currentPersister.getRole(), currentKey );
+		final StringBuilder result =
+				new StringBuilder( "CollectionEntry" )
+						.append( collectionInfoString( role, loadedKey ) );
+		final CollectionPersister persister = currentPersister;
+		if ( persister != null ) {
+			result.append( "->" )
+					.append( collectionInfoString( persister.getRole(), currentKey ) );
 		}
-		return result;
+		return result.toString();
 	}
 
 	/**
@@ -432,9 +424,8 @@ public void serialize(ObjectOutputStream oos) throws IOException {
 	 *
 	 * @return The deserialized CollectionEntry
 	 */
-	public static CollectionEntry deserialize(
-			ObjectInputStream ois,
-			SessionImplementor session) throws IOException, ClassNotFoundException {
+	public static CollectionEntry deserialize(ObjectInputStream ois, SessionImplementor session)
+			throws IOException, ClassNotFoundException {
 		return new CollectionEntry(
 				(String) ois.readObject(),
 				(Serializable) ois.readObject(),

hibernate-core/src/main/java/org/hibernate/engine/spi/Status.java

@@ -7,8 +7,9 @@
 /**
  * Represents the status of an entity with respect to
  * this session. These statuses are for internal
- * book-keeping only and are not intended to represent
- * any notion that is visible to the _application_.
+ * bookkeeping only and are not intended to represent
+ * any notion that is visible to the application
+ * program.
  */
 public enum Status {
 	MANAGED,

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

@@ -63,37 +63,30 @@ public abstract class AbstractFlushingEventListener {
 	 * @throws HibernateException Error flushing caches to execution queues.
 	 */
 	protected void flushEverythingToExecutions(FlushEvent event) throws HibernateException {
-
 		LOG.trace( "Flushing session" );
-
 		final EventSource session = event.getSession();
-
 		final PersistenceContext persistenceContext = session.getPersistenceContextInternal();
 		preFlush( session, persistenceContext );
-
 		flushEverythingToExecutions( event, persistenceContext, session );
 	}
 
 	protected void flushEverythingToExecutions(FlushEvent event, PersistenceContext persistenceContext, EventSource session) {
 		persistenceContext.setFlushing( true );
 		try {
-			int entityCount = flushEntities( event, persistenceContext );
-			int collectionCount = flushCollections( session, persistenceContext );
-
+			final int entityCount = flushEntities( event, persistenceContext );
+			final int collectionCount = flushCollections( session, persistenceContext );
 			event.setNumberOfEntitiesProcessed( entityCount );
 			event.setNumberOfCollectionsProcessed( collectionCount );
 		}
 		finally {
 			persistenceContext.setFlushing( false);
 		}
-
 		//some statistics
 		logFlushResults( event );
 	}
 
 	protected void preFlush(EventSource session, PersistenceContext persistenceContext) {
 		session.getInterceptor().preFlush( persistenceContext.managedEntitiesIterator() );
-
 		prepareEntityFlushes( session, persistenceContext );
 		// we could move this inside if we wanted to
 		// tolerate collection initializations during
@@ -134,9 +127,7 @@ protected void logFlushResults(FlushEvent event) {
 	 * and also apply orphan delete
 	 */
 	private void prepareEntityFlushes(EventSource session, PersistenceContext persistenceContext) throws HibernateException {
-
 		LOG.debug( "Processing flush-time cascades" );
-
 		final PersistContext context = PersistContext.create();
 		// safe from concurrent modification because of how concurrentEntries() is implemented on IdentityMap
 		for ( Map.Entry<Object,EntityEntry> me : persistenceContext.reentrantSafeEntityEntries() ) {
@@ -146,7 +137,6 @@ private void prepareEntityFlushes(EventSource session, PersistenceContext persis
 				cascadeOnFlush( session, entry.getPersister(), me.getKey(), context );
 			}
 		}
-
 		checkForTransientReferences( session, persistenceContext );
 	}
 
@@ -194,14 +184,14 @@ private void cascadeOnFlush(EventSource session, EntityPersister persister, Obje
 	 * dirty check.
 	 */
 	private void prepareCollectionFlushes(PersistenceContext persistenceContext) throws HibernateException {
-
 		// Initialize dirty flags for arrays + collections with composite elements
 		// and reset reached, doupdate, etc.
-
 		LOG.debug( "Dirty checking collections" );
-		final Map<PersistentCollection<?>, CollectionEntry> collectionEntries = persistenceContext.getCollectionEntries();
+		final Map<PersistentCollection<?>, CollectionEntry> collectionEntries =
+				persistenceContext.getCollectionEntries();
 		if ( collectionEntries != null ) {
-			for ( Map.Entry<PersistentCollection<?>, CollectionEntry> entry : ( (IdentityMap<PersistentCollection<?>, CollectionEntry>) collectionEntries ).entryArray() ) {
+			for ( Map.Entry<PersistentCollection<?>, CollectionEntry> entry :
+					( (IdentityMap<PersistentCollection<?>, CollectionEntry>) collectionEntries ).entryArray() ) {
 				entry.getValue().preFlush( entry.getKey() );
 			}
 		}
@@ -214,36 +204,28 @@ private void prepareCollectionFlushes(PersistenceContext persistenceContext) thr
 	 */
 	private int flushEntities(final FlushEvent event, final PersistenceContext persistenceContext)
 			throws HibernateException {
-
 		LOG.trace( "Flushing entities and processing referenced collections" );
 
 		final EventSource source = event.getSession();
 		final EventListenerGroup<FlushEntityEventListener> flushListeners =
 				event.getFactory().getFastSessionServices().eventListenerGroup_FLUSH_ENTITY;
 
-		// Among other things, updateReachables() will recursively load all
-		// collections that are moving roles. This might cause entities to
-		// be loaded.
-
+		// Among other things, updateReachables() recursively loads all
+		// collections that are changing roles. This might cause entities
+		// to be loaded.
 		// So this needs to be safe from concurrent modification problems.
-
 		final Map.Entry<Object,EntityEntry>[] entityEntries = persistenceContext.reentrantSafeEntityEntries();
 		final int count = entityEntries.length;
 
 		FlushEntityEvent entityEvent = null; //allow reuse of the event as it's heavily allocated in certain use cases
 		int eventGenerationId = 0; //Used to double-check the instance reuse won't cause problems
-
 		for ( Map.Entry<Object,EntityEntry> me : entityEntries ) {
 			// Update the status of the object and if necessary, schedule an update
-
 			final EntityEntry entry = me.getValue();
 			final Status status = entry.getStatus();
-
 			if ( status != Status.LOADING && status != Status.GONE ) {
 				entityEvent = createOrReuseEventInstance( entityEvent, source, me.getKey(), entry );
-
 				entityEvent.setInstanceGenerationId( ++eventGenerationId );
-
 				flushListeners.fireEventOnEachListener( entityEvent, FlushEntityEventListener::onFlushEntity );
 				entityEvent.setAllowedToReuse( true );
 				assert entityEvent.getInstanceGenerationId() == eventGenerationId;
@@ -281,7 +263,6 @@ private FlushEntityEvent createOrReuseEventInstance(
 	private int flushCollections(final EventSource session, final PersistenceContext persistenceContext)
 			throws HibernateException {
 		LOG.trace( "Processing unreferenced collections" );
-
 		final Map<PersistentCollection<?>, CollectionEntry> collectionEntries = persistenceContext.getCollectionEntries();
 		final int count;
 		if ( collectionEntries == null ) {
@@ -300,7 +281,6 @@ private int flushCollections(final EventSource session, final PersistenceContext
 		// Schedule updates to collections:
 
 		LOG.trace( "Scheduling collection removes/(re)creates/updates" );
-
 		final ActionQueue actionQueue = session.getActionQueue();
 		final Interceptor interceptor = session.getInterceptor();
 		persistenceContext.forEachCollectionEntry(
@@ -407,7 +387,6 @@ protected void performExecutions(EventSource session) {
 	 * 3. call Interceptor.postFlush()
 	 */
 	protected void postFlush(SessionImplementor session) throws HibernateException {
-
 		LOG.trace( "Post flush" );
 
 		final PersistenceContext persistenceContext = session.getPersistenceContextInternal();
@@ -429,13 +408,12 @@ protected void postFlush(SessionImplementor session) throws HibernateException {
 					}
 					else {
 						//otherwise recreate the mapping between the collection and its key
-						final CollectionKey collectionKey = new CollectionKey(
-								collectionEntry.getLoadedPersister(),
-								key
-						);
+						final CollectionKey collectionKey =
+								new CollectionKey( collectionEntry.getLoadedPersister(), key );
 						persistenceContext.addCollectionByKey( collectionKey, persistentCollection );
 					}
-				}, true
+				},
+				true
 		);
 	}
 

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

@@ -60,19 +60,19 @@ void processValue(int i, Object[] values, Type[] types) {
 	}
 
 	boolean includeEntityProperty(Object[] values, int i) {
-		return includeProperty(values, i);
+		return includeProperty( values, i );
 	}
 
 	boolean includeProperty(Object[] values, int i) {
-		return values[i]!= LazyPropertyInitializer.UNFETCHED_PROPERTY;
+		return values[i] != LazyPropertyInitializer.UNFETCHED_PROPERTY;
 	}
 
 	/**
 	 * Visit a component. Dispatch each property
 	 * to processValue().
 	 */
 	Object processComponent(Object component, CompositeType componentType) throws HibernateException {
-		if ( component!=null ) {
+		if ( component != null ) {
 			processValues( componentType.getPropertyValues(component, session), componentType.getSubtypes() );
 		}
 		return null;