HHH-10984 - Have multiLoad not return (unflushed) DELETED entities;

HHH-10617 - multiLoad behavior

(cherry picked from commit 72e948514e)

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

@@ -10,7 +10,8 @@ import java.io.Serializable;
 import java.util.List;
 
 /**
- * Loads multiple entities at once by identifiers
+ * Loads multiple entities at once by identifiers, ultimately via one of the
+ * {@link #multiLoad} methods, using the various options specified (if any)
  *
  * @author Steve Ebersole
  */
@@ -49,18 +50,53 @@ public interface MultiIdentifierLoadAccess<T> {
 	MultiIdentifierLoadAccess<T> withBatchSize(int batchSize);
 
 	/**
-	 * Should we check the Session to see whether it already contains any of the
+	 * Specify whether we should check the Session to see whether it already contains any of the
 	 * entities to be loaded in a managed state <b>for the purpose of not including those
-	 * ids to the batch-load SQL</b>
+	 * ids to the batch-load SQL</b>.
 	 *
-	 * @param enabled {@code true} enables this checking; {@code false} disables it.
+	 * @param enabled {@code true} enables this checking; {@code false} (the default) disables it.
 	 *
 	 * @return {@code this}, for method chaining
 	 */
 	MultiIdentifierLoadAccess<T> enableSessionCheck(boolean enabled);
 
 	/**
-	 * Perform a load of multiple entities by identifiers
+	 * Should the multi-load operation be allowed to return entities that are locally
+	 * deleted?  A locally deleted entity is one which has been passed to this
+	 * Session's {@link Session#delete} / {@link Session#remove} method, but not
+	 * yet flushed.  The default behavior is to handle them as null in the return
+	 * (see {@link #enableOrderedReturn}).
+	 *
+	 * @param enabled {@code true} enables returning the deleted entities;
+	 * {@code false} (the default) disables it.
+	 *
+	 * @return {@code this}, for method chaining
+	 */
+	MultiIdentifierLoadAccess<T> enableReturnOfDeletedEntities(boolean enabled);
+
+	/**
+	 * Should the return List be ordered and positional in relation to the
+	 * incoming ids?  If enabled (the default), the return List is ordered and
+	 * positional relative to the incoming ids.  In other words, a request to
+	 * {@code multiLoad([2,1,3])} will return {@code [Entity#2, Entity#1, Entity#3]}.
+	 * <p/>
+	 * An important distinction is made here in regards to the handling of
+	 * unknown entities depending on this "ordered return" setting.  If enabled
+	 * a null is inserted into the List at the proper position(s).  If disabled,
+	 * the nulls are not put into the return List.  In other words, consumers of
+	 * the returned ordered List would need to be able to handle null elements.
+	 *
+	 * @param enabled {@code true} (the default) enables ordering;
+	 * {@code false} disables it.
+	 *
+	 * @return {@code this}, for method chaining
+	 */
+	MultiIdentifierLoadAccess<T> enableOrderedReturn(boolean enabled);
+
+	/**
+	 * Perform a load of multiple entities by identifiers.  See {@link #enableOrderedReturn}
+	 * and {@link #enableReturnOfDeletedEntities} for options which effect
+	 * the size and "shape" of the return list.
 	 *
 	 * @param ids The ids to load
 	 * @param <K> The identifier type
@@ -70,7 +106,9 @@ public interface MultiIdentifierLoadAccess<T> {
 	<K extends Serializable> List<T> multiLoad(K... ids);
 
 	/**
-	 * Perform a load of multiple entities by identifiers
+	 * Perform a load of multiple entities by identifiers.  See {@link #enableOrderedReturn}
+	 * and {@link #enableReturnOfDeletedEntities} for options which effect
+	 * the size and "shape" of the return list.
 	 *
 	 * @param ids The ids to load
 	 * @param <K> The identifier type

hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java

@@ -2778,6 +2778,8 @@ public final class SessionImpl extends AbstractSessionImpl implements EventSourc
 		private CacheMode cacheMode;
 		private Integer batchSize;
 		private boolean sessionCheckingEnabled;
+		private boolean returnOfDeletedEntitiesEnabled;
+		private boolean orderedReturnEnabled = true;
 
 		public MultiIdentifierLoadAccessImpl(EntityPersister entityPersister) {
 			this.entityPersister = entityPersister;
@@ -2827,6 +2829,28 @@ public final class SessionImpl extends AbstractSessionImpl implements EventSourc
 			return this;
 		}
 
+		@Override
+		public boolean isReturnOfDeletedEntitiesEnabled() {
+			return returnOfDeletedEntitiesEnabled;
+		}
+
+		@Override
+		public MultiIdentifierLoadAccess<T> enableReturnOfDeletedEntities(boolean enabled) {
+			this.returnOfDeletedEntitiesEnabled = enabled;
+			return this;
+		}
+
+		@Override
+		public boolean isOrderReturnEnabled() {
+			return orderedReturnEnabled;
+		}
+
+		@Override
+		public MultiIdentifierLoadAccess<T> enableOrderedReturn(boolean enabled) {
+			this.orderedReturnEnabled = enabled;
+			return this;
+		}
+
 		@Override
 		@SuppressWarnings("unchecked")
 		public <K extends Serializable> List<T> multiLoad(K... ids) {

hibernate-core/src/main/java/org/hibernate/loader/entity/DynamicBatchingEntityLoaderBuilder.java

@@ -18,6 +18,7 @@ import java.util.List;
 import org.hibernate.LockMode;
 import org.hibernate.LockOptions;
 import org.hibernate.dialect.pagination.LimitHelper;
+import org.hibernate.engine.spi.EntityEntry;
 import org.hibernate.engine.spi.EntityKey;
 import org.hibernate.engine.spi.LoadQueryInfluencers;
 import org.hibernate.engine.spi.PersistenceContext;
@@ -25,6 +26,7 @@ import org.hibernate.engine.spi.QueryParameters;
 import org.hibernate.engine.spi.RowSelection;
 import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.engine.spi.SessionImplementor;
+import org.hibernate.engine.spi.Status;
 import org.hibernate.internal.util.StringHelper;
 import org.hibernate.internal.util.collections.ArrayHelper;
 import org.hibernate.internal.util.collections.CollectionHelper;
@@ -47,12 +49,136 @@ public class DynamicBatchingEntityLoaderBuilder extends BatchingEntityLoaderBuil
 
 	public static final DynamicBatchingEntityLoaderBuilder INSTANCE = new DynamicBatchingEntityLoaderBuilder();
 
-	@SuppressWarnings("unchecked")
 	public List multiLoad(
 			OuterJoinLoadable persister,
 			Serializable[] ids, SessionImplementor session,
 			MultiLoadOptions loadOptions) {
-		List result = CollectionHelper.arrayList( ids.length );
+		if ( loadOptions.isOrderReturnEnabled() ) {
+			return performOrderedMultiLoad( persister, ids, session, loadOptions );
+		}
+		else {
+			return performUnorderedMultiLoad( persister, ids, session, loadOptions );
+		}
+	}
+
+	@SuppressWarnings("unchecked")
+	private List performOrderedMultiLoad(
+			OuterJoinLoadable persister,
+			Serializable[] ids,
+			SessionImplementor session,
+			MultiLoadOptions loadOptions) {
+		assert loadOptions.isOrderReturnEnabled();
+
+		final List result = CollectionHelper.arrayList( ids.length );
+
+		final LockOptions lockOptions = (loadOptions.getLockOptions() == null)
+				? new LockOptions( LockMode.NONE )
+				: loadOptions.getLockOptions();
+
+		final int maxBatchSize;
+		if ( loadOptions.getBatchSize() != null && loadOptions.getBatchSize() > 0 ) {
+			maxBatchSize = loadOptions.getBatchSize();
+		}
+		else {
+			maxBatchSize = session.getFactory().getJdbcServices().getJdbcEnvironment().getDialect().getDefaultBatchLoadSizingStrategy().determineOptimalBatchLoadSize(
+					persister.getIdentifierType().getColumnSpan( session.getFactory() ),
+					ids.length
+			);
+		}
+
+		final List<Serializable> idsInBatch = new ArrayList<>();
+		final List<Integer> elementPositionsLoadedByBatch = new ArrayList<>();
+
+		for ( int i = 0; i < ids.length; i++ ) {
+			final Serializable id = ids[i];
+			final EntityKey entityKey = new EntityKey( id, persister );
+
+			if ( loadOptions.isSessionCheckingEnabled() ) {
+				// look for it in the Session first
+				final Object managedEntity = session.getPersistenceContext().getEntity( entityKey );
+				if ( managedEntity != null ) {
+					if ( !loadOptions.isReturnOfDeletedEntitiesEnabled() ) {
+						final EntityEntry entry = session.getPersistenceContext().getEntry( managedEntity );
+						if ( entry.getStatus() == Status.DELETED || entry.getStatus() == Status.GONE ) {
+							// put a null in the result
+							result.add( i, null );
+							continue;
+						}
+					}
+					// if we did not hit the continue above, there is already an
+					// entry in the PC for that entity, so use it...
+					result.add( i, managedEntity );
+					continue;
+				}
+			}
+
+			// if we did not hit any of the continues above, then we need to batch
+			// load the entity state.
+			idsInBatch.add( ids[i] );
+
+			if ( idsInBatch.size() >= maxBatchSize ) {
+				performOrderedBatchLoad( idsInBatch, lockOptions, persister, session );
+			}
+
+			// Save the EntityKey instance for use later!
+			result.add( i, entityKey );
+			elementPositionsLoadedByBatch.add( i );
+		}
+
+		if ( !idsInBatch.isEmpty() ) {
+			performOrderedBatchLoad( idsInBatch, lockOptions, persister, session );
+		}
+
+		for ( Integer position : elementPositionsLoadedByBatch ) {
+			// the element value at this position in the result List should be
+			// the EntityKey for that entity; reuse it!
+			final EntityKey entityKey = (EntityKey) result.get( position );
+			Object entity = session.getPersistenceContext().getEntity( entityKey );
+			if ( entity != null && !loadOptions.isReturnOfDeletedEntitiesEnabled() ) {
+				// make sure it is not DELETED
+				final EntityEntry entry = session.getPersistenceContext().getEntry( entity );
+				if ( entry.getStatus() == Status.DELETED || entry.getStatus() == Status.GONE ) {
+					// the entity is locally deleted, and the options ask that we not return such entities...
+					entity = null;
+				}
+			}
+			result.set( position, entity );
+		}
+
+		return result;
+	}
+
+	private void performOrderedBatchLoad(
+			List<Serializable> idsInBatch,
+			LockOptions lockOptions,
+			OuterJoinLoadable persister,
+			SessionImplementor session) {
+		final int batchSize =  idsInBatch.size();
+		final DynamicEntityLoader batchingLoader = new DynamicEntityLoader(
+				persister,
+				batchSize,
+				lockOptions,
+				session.getFactory(),
+				session.getLoadQueryInfluencers()
+		);
+
+		final Serializable[] idsInBatchArray = idsInBatch.toArray( new Serializable[ idsInBatch.size() ] );
+
+		QueryParameters qp = buildMultiLoadQueryParameters( persister, idsInBatchArray, lockOptions );
+		batchingLoader.doEntityBatchFetch( session, qp, idsInBatchArray );
+
+		idsInBatch.clear();
+	}
+
+	@SuppressWarnings("unchecked")
+	protected List performUnorderedMultiLoad(
+			OuterJoinLoadable persister,
+			Serializable[] ids,
+			SessionImplementor session,
+			MultiLoadOptions loadOptions) {
+		assert !loadOptions.isOrderReturnEnabled();
+
+		final List result = CollectionHelper.arrayList( ids.length );
 
 		if ( loadOptions.isSessionCheckingEnabled() ) {
 			// the user requested that we exclude ids corresponding to already managed
@@ -66,6 +192,12 @@ public class DynamicBatchingEntityLoaderBuilder extends BatchingEntityLoaderBuil
 				final EntityKey entityKey = new EntityKey( id, persister );
 				final Object managedEntity = session.getPersistenceContext().getEntity( entityKey );
 				if ( managedEntity != null ) {
+					if ( !loadOptions.isReturnOfDeletedEntitiesEnabled() ) {
+						final EntityEntry entry = session.getPersistenceContext().getEntry( managedEntity );
+						if ( entry.getStatus() == Status.DELETED || entry.getStatus() == Status.GONE ) {
+							continue;
+						}
+					}
 					foundAnyManagedEntities = true;
 					result.add( managedEntity );
 				}
@@ -257,8 +389,7 @@ public class DynamicBatchingEntityLoaderBuilder extends BatchingEntityLoaderBuil
 					-1,
 					lockMode,
 					factory,
-					loadQueryInfluencers
+					loadQueryInfluencers) {
-			) {
 				@Override
 				protected StringBuilder whereString(String alias, String[] columnNames, int batchSize) {
 					return StringHelper.buildBatchFetchRestrictionFragment( alias, columnNames, getFactory().getDialect() );

hibernate-core/src/main/java/org/hibernate/persister/entity/MultiLoadOptions.java

@@ -15,6 +15,8 @@ import org.hibernate.LockOptions;
  */
 public interface MultiLoadOptions {
 	boolean isSessionCheckingEnabled();
+	boolean isReturnOfDeletedEntitiesEnabled();
+	boolean isOrderReturnEnabled();
 
 	LockOptions getLockOptions();
 

hibernate-core/src/test/java/org/hibernate/test/ops/multiLoad/MultiLoadTest.java

@@ -23,13 +23,17 @@ import org.hibernate.cfg.AvailableSettings;
 import org.hibernate.engine.spi.EntityKey;
 import org.hibernate.engine.spi.SessionImplementor;
 
+import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseNonConfigCoreFunctionalTestCase;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import static org.hibernate.testing.transaction.TransactionUtil.doInHibernate;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 
@@ -79,12 +83,70 @@ public class MultiLoadTest extends BaseNonConfigCoreFunctionalTestCase {
 
 	@Test
 	public void testBasicMultiLoad() {
-		Session session = openSession();
+		doInHibernate(
-		session.getTransaction().begin();
+				this::sessionFactory, session -> {
 					List<SimpleEntity> list = session.byMultipleIds( SimpleEntity.class ).multiLoad( ids(56) );
 					assertEquals( 56, list.size() );
-		session.getTransaction().commit();
+				}
-		session.close();
+		);
+	}
+
+	@Test
+	@TestForIssue( jiraKey = "HHH-10984" )
+	public void testUnflushedDeleteAndThenMultiLoad() {
+		doInHibernate(
+				this::sessionFactory, session -> {
+					// delete one of them (but do not flush)...
+					session.delete( session.load( SimpleEntity.class, 5 ) );
+
+					// as a baseline, assert based on how load() handles it
+					SimpleEntity s5 = session.load( SimpleEntity.class, 5 );
+					assertNotNull( s5 );
+
+					// and then, assert how get() handles it
+					s5 = session.get( SimpleEntity.class, 5 );
+					assertNull( s5 );
+
+					// finally assert how multiLoad handles it
+					List<SimpleEntity> list = session.byMultipleIds( SimpleEntity.class ).multiLoad( ids(56) );
+					assertEquals( 56, list.size() );
+				}
+		);
+	}
+
+	@Test
+	@TestForIssue( jiraKey = "HHH-10617" )
+	public void testDuplicatedRequestedIds() {
+		doInHibernate(
+				this::sessionFactory, session -> {
+					// ordered multiLoad
+					List<SimpleEntity> list = session.byMultipleIds( SimpleEntity.class ).multiLoad( 1, 2, 3, 2, 2 );
+					assertEquals( 5, list.size() );
+					assertSame( list.get( 1 ), list.get( 3 ) );
+					assertSame( list.get( 1 ), list.get( 4 ) );
+
+					// un-ordered multiLoad
+					list = session.byMultipleIds( SimpleEntity.class ).enableOrderedReturn( false ).multiLoad( 1, 2, 3, 2, 2 );
+					assertEquals( 3, list.size() );
+				}
+		);
+	}
+
+	@Test
+	@TestForIssue( jiraKey = "HHH-10617" )
+	public void testNonExistentIdRequest() {
+		doInHibernate(
+				this::sessionFactory, session -> {
+					// ordered multiLoad
+					List<SimpleEntity> list = session.byMultipleIds( SimpleEntity.class ).multiLoad( 1, 699, 2 );
+					assertEquals( 3, list.size() );
+					assertNull( list.get( 1 ) );
+
+					// un-ordered multiLoad
+					list = session.byMultipleIds( SimpleEntity.class ).enableOrderedReturn( false ).multiLoad( 1, 699, 2 );
+					assertEquals( 2, list.size() );
+				}
+		);
 	}
 
 	@Test