HHH-9195 - Adding an entity at a given index in a list annotated with OrderColumn adds the entity at the end

hibernate-core/src/main/java/org/hibernate/collection/internal/AbstractPersistentCollection.java

@@ -371,12 +371,16 @@ public abstract class AbstractPersistentCollection implements Serializable, Pers
 		return cachedSize;
 	}
 
-	private boolean isConnectedToSession() {
+	protected boolean isConnectedToSession() {
 		return session != null
 				&& session.isOpen()
 				&& session.getPersistenceContext().containsCollection( this );
 	}
 
+	protected boolean isInitialized() {
+		return initialized;
+	}
+
 	/**
 	 * Called by any writer method of the collection interface
 	 */
@@ -424,7 +428,7 @@ public abstract class AbstractPersistentCollection implements Serializable, Pers
 	 * Is this the "inverse" end of a bidirectional association?
 	 */
 	@SuppressWarnings({"JavaDoc"})
-	private boolean isInverseCollection() {
+	protected boolean isInverseCollection() {
 		final CollectionEntry ce = session.getPersistenceContext().getCollectionEntry( this );
 		return ce != null && ce.getLoadedPersister().isInverse();
 	}
@@ -434,7 +438,7 @@ public abstract class AbstractPersistentCollection implements Serializable, Pers
 	 * no orphan delete enabled?
 	 */
 	@SuppressWarnings({"JavaDoc"})
-	private boolean isInverseCollectionNoOrphanDelete() {
+	protected boolean isInverseCollectionNoOrphanDelete() {
 		final CollectionEntry ce = session.getPersistenceContext().getCollectionEntry( this );
 		return ce != null
 				&&
@@ -447,7 +451,7 @@ public abstract class AbstractPersistentCollection implements Serializable, Pers
 	 * of a collection with no orphan delete?
 	 */
 	@SuppressWarnings({"JavaDoc"})
-	private boolean isInverseOneToManyOrNoOrphanDelete() {
+	protected boolean isInverseOneToManyOrNoOrphanDelete() {
 		final CollectionEntry ce = session.getPersistenceContext().getCollectionEntry( this );
 		return ce != null
 				&& ce.getLoadedPersister().isInverse()

hibernate-core/src/main/java/org/hibernate/collection/internal/PersistentList.java

@@ -14,6 +14,7 @@ import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
 import java.util.ListIterator;
+import java.util.Map;
 
 import org.hibernate.HibernateException;
 import org.hibernate.engine.spi.SessionImplementor;
@@ -314,7 +315,12 @@ public class PersistentList extends AbstractPersistentCollection implements List
 		if ( index < 0 ) {
 			throw new ArrayIndexOutOfBoundsException( "negative index" );
 		}
-		if ( !isOperationQueueEnabled() ) {
+		if ( !isInitialized() && isConnectedToSession() ) {
+			// NOTE : we don't care about the inverse part here because
+			// even if the collection is inverse, this side is driving the
+			// writing of the indexes.  And because this is a positioned-add
+			// we need to load the underlying elements to know how that
+			// affects overall re-ordering
 			write();
 			list.add( index, value );
 		}

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

@@ -289,6 +289,8 @@ public abstract class AbstractFlushingEventListener implements Serializable {
 							)
 					);
 			}
+
+			// todo : I'm not sure the !wasInitialized part should really be part of this check
 			if ( !coll.wasInitialized() && coll.hasQueuedOperations() ) {
 				actionQueue.addAction(
 						new QueuedOperationCollectionAction(

hibernate-core/src/test/java/org/hibernate/test/collection/delayedOperation/ListAddTest.java

@@ -18,16 +18,19 @@ import javax.persistence.OrderColumn;
 import javax.persistence.Table;
 
 import org.hibernate.Hibernate;
+import org.hibernate.LazyInitializationException;
 import org.hibernate.Session;
 import org.hibernate.Transaction;
+import org.hibernate.collection.spi.PersistentCollection;
 
-import org.hibernate.testing.FailureExpected;
 import org.hibernate.testing.junit4.BaseNonConfigCoreFunctionalTestCase;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.fail;
 
 /**
  * @author Steve Ebersole
@@ -70,31 +73,24 @@ public class ListAddTest extends BaseNonConfigCoreFunctionalTestCase {
 	 * This test fails, but shouldn't
 	 */
 	@Test
-	@FailureExpected( jiraKey = "HHH-9195" )
 	public void addQuestionWithIndexShouldAddQuestionAtSpecifiedIndex() {
 		Session session = openSession();
 		Transaction transaction = session.beginTransaction();
-
 		Quizz quizz = session.get( Quizz.class, 1 );
 		quizz.addQuestion( 1, new Question( 4, "question that should be at index 1" ) );
-
 		transaction.commit();
 		session.close();
 
 		session = openSession();
 		transaction = session.beginTransaction();
-
 		quizz = session.get( Quizz.class,  1);
-
 		assertEquals( 4, quizz.getQuestions().size() );
 		assertEquals( 4, quizz.getQuestions().get( 1 ).getId().longValue() );
-
 		transaction.commit();
 		session.close();
 	}
 
 	@Test
-	@FailureExpected( jiraKey = "HHH-9195" )
 	public void addQuestionToDetachedQuizz() {
 		Session session = openSession();
 		session.beginTransaction();
@@ -102,21 +98,34 @@ public class ListAddTest extends BaseNonConfigCoreFunctionalTestCase {
 		session.getTransaction().commit();
 		session.close();
 
-		quizz.addQuestion( 1, new Question( 4, "question that should be at index 1" ) );
+		assertFalse( ( (PersistentCollection) quizz.getQuestions() ).wasInitialized() );
 
-		session = openSession();
+		try {
-		session.beginTransaction();
+			// this is the crux of the comment on HHH-9195 in regard to uninitialized, detached collections and
-		session.merge( quizz );
+			// not allowing additions
-		session.getTransaction().commit();
+			quizz.addQuestion( new Question( 4, "question 4" ) );
-		session.close();
 
-		session = openSession();
+			// indexed-addition should fail
-		session.getTransaction().begin();
+			quizz.addQuestion( 1, new Question( 5, "question that should be at index 1" ) );
-		quizz = session.get( Quizz.class,  1);
+			fail( "Expecting a failure" );
-		assertEquals( 4, quizz.getQuestions().size() );
+		}
-		assertEquals( 4, quizz.getQuestions().get( 1 ).getId().longValue() );
+		catch (LazyInitializationException ignore) {
-		session.getTransaction().commit();
+			// expected
-		session.close();
+		}
+
+//		session = openSession();
+//		session.beginTransaction();
+//		session.merge( quizz );
+//		session.getTransaction().commit();
+//		session.close();
+//
+//		session = openSession();
+//		session.getTransaction().begin();
+//		quizz = session.get( Quizz.class,  1);
+//		assertEquals( 5, quizz.getQuestions().size() );
+//		assertEquals( 5, quizz.getQuestions().get( 1 ).getId().longValue() );
+//		session.getTransaction().commit();
+//		session.close();
 	}
 
 	/**