clean up the code in SingleIdEntityLoaderStandardImpl

and remove an AtomicInteger that existed only for the benefit of LoadingSmokeTests

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

@@ -111,6 +111,10 @@ public class LoadQueryInfluencers implements Serializable {
 		return enabledCascadingFetchProfile;
 	}
 
+	public boolean hasEnabledCascadingFetchProfile() {
+		return enabledCascadingFetchProfile != null;
+	}
+
 	/**
 	 * Set the effective {@linkplain #getEnabledCascadingFetchProfile() cascading fetch-profile}
 	 */

hibernate-core/src/main/java/org/hibernate/loader/ast/internal/SingleIdEntityLoaderStandardImpl.java

@@ -6,10 +6,7 @@
  */
 package org.hibernate.loader.ast.internal;
 
-import java.util.ArrayList;
 import java.util.EnumMap;
-import java.util.List;
-import java.util.concurrent.atomic.AtomicInteger;
 
 import org.hibernate.Internal;
 import org.hibernate.LockMode;
@@ -19,9 +16,7 @@ import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.engine.spi.SharedSessionContractImplementor;
 import org.hibernate.loader.ast.spi.CascadingFetchProfile;
 import org.hibernate.metamodel.mapping.EntityMappingType;
-import org.hibernate.persister.entity.Loadable;
 import org.hibernate.sql.ast.tree.select.SelectStatement;
-import org.hibernate.sql.ast.tree.expression.JdbcParameter;
 import org.hibernate.sql.exec.spi.JdbcParametersList;
 
 /**
@@ -30,22 +25,17 @@ import org.hibernate.sql.exec.spi.JdbcParametersList;
  * @author Steve Ebersole
  */
 public class SingleIdEntityLoaderStandardImpl<T> extends SingleIdEntityLoaderSupport<T> {
-	private EnumMap<LockMode, SingleIdLoadPlan> selectByLockMode = new EnumMap<>( LockMode.class );
-	private EnumMap<CascadingFetchProfile, SingleIdLoadPlan> selectByInternalCascadeProfile;
 
-	private AtomicInteger nonReusablePlansGenerated = new AtomicInteger();
+	private final EnumMap<LockMode, SingleIdLoadPlan<T>> selectByLockMode = new EnumMap<>( LockMode.class );
-
+	private EnumMap<CascadingFetchProfile, SingleIdLoadPlan<T>> selectByInternalCascadeProfile;
-	public AtomicInteger getNonReusablePlansGenerated() {
-		return nonReusablePlansGenerated;
-	}
 
 	public SingleIdEntityLoaderStandardImpl(
 			EntityMappingType entityDescriptor,
 			SessionFactoryImplementor sessionFactory) {
 		// todo (6.0) : consider creating a base AST and "cloning" it
 		super( entityDescriptor, sessionFactory );
-		// see `org.hibernate.persister.entity.AbstractEntityPersister#createLoaders`
+		// see org.hibernate.persister.entity.AbstractEntityPersister#createLoaders
-		//		we should pre-load a few - maybe LockMode.NONE and LockMode.READ
+		// we should preload a few - maybe LockMode.NONE and LockMode.READ
 		final LockOptions lockOptions = LockOptions.NONE;
 		final LoadQueryInfluencers queryInfluencers = new LoadQueryInfluencers( sessionFactory );
 		final SingleIdLoadPlan<T> plan = createLoadPlan(
@@ -53,7 +43,7 @@ public class SingleIdEntityLoaderStandardImpl<T> extends SingleIdEntityLoaderSup
 				queryInfluencers,
 				sessionFactory
 		);
-		if ( determineIfReusable( lockOptions, queryInfluencers ) ) {
+		if ( isLoadPlanReusable( lockOptions, queryInfluencers ) ) {
 			selectByLockMode.put( lockOptions.getLockMode(), plan );
 		}
 	}
@@ -65,7 +55,6 @@ public class SingleIdEntityLoaderStandardImpl<T> extends SingleIdEntityLoaderSup
 				session.getLoadQueryInfluencers(),
 				session.getFactory()
 		);
-
 		return loadPlan.load( key, readOnly, true, session );
 	}
 
@@ -81,7 +70,6 @@ public class SingleIdEntityLoaderStandardImpl<T> extends SingleIdEntityLoaderSup
 				session.getLoadQueryInfluencers(),
 				session.getFactory()
 		);
-
 		return loadPlan.load( key, entityInstance, readOnly, false, session );
 	}
 
@@ -90,81 +78,89 @@ public class SingleIdEntityLoaderStandardImpl<T> extends SingleIdEntityLoaderSup
 			LockOptions lockOptions,
 			LoadQueryInfluencers loadQueryInfluencers,
 			SessionFactoryImplementor sessionFactory) {
+
 		if ( getLoadable().isAffectedByEnabledFilters( loadQueryInfluencers ) ) {
-			// special case of not-cacheable based on enabled filters effecting this load.
-			//
 			// This case is special because the filters need to be applied in order to
-			// 		properly restrict the SQL/JDBC results.  For this reason it has higher
+			// properly restrict the SQL/JDBC results.  For this reason it has higher
-			// 		precedence than even "internal" fetch profiles.
+			// precedence than even "internal" fetch profiles.
-			nonReusablePlansGenerated.incrementAndGet();
 			return createLoadPlan( lockOptions, loadQueryInfluencers, sessionFactory );
 		}
+		else if ( loadQueryInfluencers.hasEnabledCascadingFetchProfile()
+				&& LockMode.WRITE.greaterThan( lockOptions.getLockMode() ) ) {
+			return getInternalCascadeLoadPlan(
+					lockOptions,
+					loadQueryInfluencers,
+					sessionFactory
+			);
+		}
+		else {
+			// otherwise see if the loader for the requested load can be cached,
+			// which also means we should look in the cache for an existing one
+			return getRegularLoadPlan(
+					lockOptions,
+					loadQueryInfluencers,
+					sessionFactory
+			);
+		}
+	}
 
-		final CascadingFetchProfile enabledCascadingFetchProfile = loadQueryInfluencers.getEnabledCascadingFetchProfile();
+	private SingleIdLoadPlan<T> getRegularLoadPlan(
-		if ( enabledCascadingFetchProfile != null ) {
+			LockOptions lockOptions,
-			if ( LockMode.WRITE.greaterThan( lockOptions.getLockMode() ) ) {
+			LoadQueryInfluencers loadQueryInfluencers,
-				if ( selectByInternalCascadeProfile == null ) {
+			SessionFactoryImplementor sessionFactory) {
-					selectByInternalCascadeProfile = new EnumMap<>( CascadingFetchProfile.class );
-				}
-				else {
-					final SingleIdLoadPlan existing = selectByInternalCascadeProfile.get( enabledCascadingFetchProfile );
-					if ( existing != null ) {
-						//noinspection unchecked
-						return existing;
-					}
-				}
 
+		if ( isLoadPlanReusable( lockOptions, loadQueryInfluencers )  ) {
+			final SingleIdLoadPlan<T> existing = selectByLockMode.get( lockOptions.getLockMode() );
+			if ( existing != null ) {
+				return existing;
+			}
+			else {
 				final SingleIdLoadPlan<T> plan = createLoadPlan(
 						lockOptions,
 						loadQueryInfluencers,
 						sessionFactory
 				);
-				selectByInternalCascadeProfile.put( enabledCascadingFetchProfile, plan );
+				selectByLockMode.put( lockOptions.getLockMode(), plan );
 				return plan;
 			}
 		}
-
+		else {
-		// otherwise see if the loader for the requested load can be cached - which
+			return createLoadPlan(lockOptions, loadQueryInfluencers, sessionFactory);
-		// 		also means we should look in the cache for an existing one
-
-		final boolean reusable = determineIfReusable( lockOptions, loadQueryInfluencers );
-
-		if ( reusable ) {
-			final SingleIdLoadPlan existing = selectByLockMode.get( lockOptions.getLockMode() );
-			if ( existing != null ) {
-				//noinspection unchecked
-				return existing;
-			}
-
-			final SingleIdLoadPlan<T> plan = createLoadPlan(
-					lockOptions,
-					loadQueryInfluencers,
-					sessionFactory
-			);
-			selectByLockMode.put( lockOptions.getLockMode(), plan );
-
-			return plan;
 		}
-
-		nonReusablePlansGenerated.incrementAndGet();
-		return createLoadPlan( lockOptions, loadQueryInfluencers, sessionFactory );
 	}
 
-	private boolean determineIfReusable(LockOptions lockOptions, LoadQueryInfluencers loadQueryInfluencers) {
+	private SingleIdLoadPlan<T> getInternalCascadeLoadPlan(
-		if ( getLoadable().isAffectedByEntityGraph( loadQueryInfluencers ) ) {
+			LockOptions lockOptions,
-			return false;
+			LoadQueryInfluencers loadQueryInfluencers,
+			SessionFactoryImplementor sessionFactory) {
+
+		final CascadingFetchProfile fetchProfile =
+				loadQueryInfluencers.getEnabledCascadingFetchProfile();
+
+		if ( selectByInternalCascadeProfile == null ) {
+			selectByInternalCascadeProfile = new EnumMap<>( CascadingFetchProfile.class );
+		}
+		else {
+			final SingleIdLoadPlan<T> existing =
+					selectByInternalCascadeProfile.get( fetchProfile );
+			if ( existing != null ) {
+				return existing;
+			}
 		}
 
-		if ( getLoadable().isAffectedByEnabledFetchProfiles( loadQueryInfluencers ) ) {
+		final SingleIdLoadPlan<T> plan = createLoadPlan(
-			return false;
+				lockOptions,
-		}
+				loadQueryInfluencers,
+				sessionFactory
+		);
+		selectByInternalCascadeProfile.put( fetchProfile, plan );
+		return plan;
+	}
 
-		//noinspection RedundantIfStatement
+	private boolean isLoadPlanReusable(LockOptions lockOptions, LoadQueryInfluencers loadQueryInfluencers) {
-		if ( lockOptions.getTimeOut() != LockOptions.WAIT_FOREVER ) {
+		return lockOptions.getTimeOut() == LockOptions.WAIT_FOREVER
-			return false;
+			&& !getLoadable().isAffectedByEntityGraph( loadQueryInfluencers )
-		}
+			&& !getLoadable().isAffectedByEnabledFetchProfiles( loadQueryInfluencers );
-
-		return true;
 	}
 
 	private SingleIdLoadPlan<T> createLoadPlan(
@@ -173,7 +169,6 @@ public class SingleIdEntityLoaderStandardImpl<T> extends SingleIdEntityLoaderSup
 			SessionFactoryImplementor sessionFactory) {
 
 		final JdbcParametersList.Builder jdbcParametersBuilder = JdbcParametersList.newBuilder();
-
 		final SelectStatement sqlAst = LoaderSelectBuilder.createSelect(
 				getLoadable(),
 				// null here means to select everything
@@ -186,13 +181,11 @@ public class SingleIdEntityLoaderStandardImpl<T> extends SingleIdEntityLoaderSup
 				jdbcParametersBuilder::add,
 				sessionFactory
 		);
-
-		final JdbcParametersList jdbcParameters = jdbcParametersBuilder.build();
 		return new SingleIdLoadPlan<>(
-				(Loadable) getLoadable(),
+				getLoadable(),
 				getLoadable().getIdentifierMapping(),
 				sqlAst,
-				jdbcParameters,
+				jdbcParametersBuilder.build(),
 				lockOptions,
 				sessionFactory
 		);

hibernate-core/src/test/java/org/hibernate/orm/test/loading/LoadingSmokeTests.java

@@ -62,10 +62,6 @@ public class LoadingSmokeTests {
 
 					final SingleIdEntityLoader singleIdEntityLoader = entityDescriptor.getSingleIdLoader();
 					assertThat( singleIdEntityLoader, instanceOf( SingleIdEntityLoaderStandardImpl.class ) );
-					assertThat(
-							( (SingleIdEntityLoaderStandardImpl) singleIdEntityLoader ).getNonReusablePlansGenerated().get(),
-							is( 0 )
-					);
 				}
 		);
 	}