HHH-18230 throw when collection contains an unsaved instance

Signed-off-by: Gavin King <gavin@hibernate.org>

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

@@ -24,7 +24,6 @@ import org.jboss.logging.Logger;
 
 import java.util.Iterator;
 
-import static java.util.Collections.emptySet;
 import static org.hibernate.engine.internal.ForeignKeys.isTransient;
 import static org.hibernate.engine.internal.ManagedTypeHelper.isHibernateProxy;
 
@@ -371,12 +370,25 @@ public class CascadingActions {
 				boolean isCascadeDeleteEnabled)
 				throws HibernateException {
 			if ( child != null
+					// a proxy is always non-transient
+					// and ForeignKeys.isTransient()
+					// is not written to expect a proxy
+					&& !isHibernateProxy( child )
+					// if it's associated with the session
+					// we are good, even if it's not yet
+					// inserted, since ordering problems
+					// are detected and handled elsewhere
 					&& !isInManagedState( child, session )
-					&& !isHibernateProxy( child ) ) { //a proxy cannot be transient and it breaks ForeignKeys.isTransient
-				if ( isTransient( entityName, child, null, session ) ) {
-					//TODO: should be TransientPropertyValueException
+					// TODO: check if it is a merged entity which has not yet been flushed
+					// Currently this throws if you directly reference a new transient
+					// instance after a call to merge() that results in its managed copy
+					// being scheduled for insertion, if the insert has not yet occurred.
+					// This is not terrible: it's more correct to "swap" the reference to
+					// point to the managed instance, but it's probably too heavy-handed.
+					&& isTransient( entityName, child, null, session ) ) {
 				throw new TransientObjectException( "persistent instance references an unsaved transient instance of '"
 						+ entityName + "' (save the transient instance before flushing)" );
+				//TODO: should be TransientPropertyValueException
 //				throw new TransientPropertyValueException(
 //						"object references an unsaved transient instance - save the transient instance before flushing",
 //						entityName,
@@ -385,14 +397,13 @@ public class CascadingActions {
 //				);
 			}
 		}
-		}
 
 		@Override
 		public Iterator<?> getCascadableChildrenIterator(
 				EventSource session,
 				CollectionType collectionType,
 				Object collection) {
-			return emptySet().iterator();
+			return getLoadedElementsIterator( session, collectionType, collection );
 		}
 
 		@Override
@@ -501,7 +512,7 @@ public class CascadingActions {
 			return collectionType.getElementsIterator( collection );
 		}
 		else {
-			// does not handle arrays (thats ok, cos they can't be lazy)
+			// does not handle arrays (that's ok, cos they can't be lazy)
 			// or newly instantiated collections, so we can do the cast
 			return ((PersistentCollection<?>) collection).queuedAdditionIterator();
 		}

hibernate-core/src/test/java/org/hibernate/orm/test/entitygraph/FindWithEntityGraphTest.java

@@ -40,7 +40,7 @@ public class FindWithEntityGraphTest {
 					Person person = new Person( 1L, "test" );
 
 					Person child1 = new Person( 2L, "child1" );
-					Person child2 = new Person( 2L, "child2" );
+					Person child2 = new Person( 3L, "child2" );
 					child1.addParent( person );
 					child2.addParent( person );
 					entityManager.persist( person );
@@ -81,7 +81,7 @@ public class FindWithEntityGraphTest {
 		@OneToOne(mappedBy = "person", orphanRemoval = true, cascade = CascadeType.ALL)
 		private PersonContact personContact;
 
-		@OneToMany(mappedBy = "parent")
+		@OneToMany(mappedBy = "parent", orphanRemoval = true, cascade = CascadeType.ALL)
 		private Set<Person> children = new HashSet<>( 0 );
 
 		public Person() {

hibernate-core/src/test/java/org/hibernate/orm/test/iterate/ForwardOnlyScrollNoTransactionTest.java

@@ -3,6 +3,7 @@ package org.hibernate.orm.test.iterate;
 import java.util.ArrayList;
 import java.util.List;
 
+import jakarta.persistence.CascadeType;
 import org.hibernate.ScrollMode;
 import org.hibernate.ScrollableResults;
 import org.hibernate.query.spi.QueryImplementor;
@@ -71,7 +72,7 @@ public class ForwardOnlyScrollNoTransactionTest {
 
 		private String name;
 
-		@OneToMany(mappedBy = "father", fetch = FetchType.EAGER)
+		@OneToMany(mappedBy = "father", fetch = FetchType.EAGER, cascade = CascadeType.PERSIST)
 		public List<Son> sons = new ArrayList<>();
 
 		public Father() {

hibernate-core/src/test/java/org/hibernate/orm/test/jpa/cascade/multicircle/MultiCircleJpaCascadeTest.java

@@ -30,7 +30,7 @@ import static org.junit.jupiter.api.Assertions.fail;
  * <p>
  * All IDs are generated from a sequence.
  * <p>
- * JPA cascade types are used (jakarta.persistence.CascadeType)..
+ * JPA cascade types are used (jakarta.persistence.CascadeType).
  * <p>
  * This test uses the following model:
  *
@@ -186,7 +186,7 @@ public class MultiCircleJpaCascadeTest {
 	}
 
 	@Test
-	public void testPersistThenUpdateNoCascadeToTransient(EntityManagerFactoryScope scope) {
+	public void testPersistThenUpdateNoCascadeToTransient1(EntityManagerFactoryScope scope) {
 		scope.inEntityManager(
 				entityManager -> {
 					// remove elements from collections and persist
@@ -205,21 +205,63 @@ public class MultiCircleJpaCascadeTest {
 					catch (RollbackException ex) {
 						assertTrue( ex.getCause() instanceof IllegalStateException );
 						IllegalStateException ise = (IllegalStateException) ex.getCause();
-						assertTyping(
-								TransientObjectException.class,
-								ise.getCause()
+						assertTyping( TransientObjectException.class, ise.getCause() );
+						String message = ise.getCause().getMessage();
+						assertTrue( message.contains("'org.hibernate.orm.test.jpa.cascade.multicircle.B'") );
+					}
+					finally {
+						entityManager.getTransaction().rollback();
+						entityManager.close();
+					}
+				}
 		);
+	}
+
+	@Test
+	public void testPersistThenUpdateNoCascadeToTransient2(EntityManagerFactoryScope scope) {
+		scope.inEntityManager(
+				entityManager -> {
+					// remove elements from collections and persist
+					c.getBCollection().clear();
+					c.getDCollection().clear();
+
+					entityManager.getTransaction().begin();
+					entityManager.persist( c );
+					entityManager.persist( b );
+					// now add the elements back
+					c.getBCollection().add( b );
+					c.getDCollection().add( d );
+					entityManager.getTransaction().commit();
+				}
+		);
+	}
+
+	@Test
+	public void testPersistThenUpdateNoCascadeToTransient3(EntityManagerFactoryScope scope) {
+		scope.inEntityManager(
+				entityManager -> {
+					// remove elements from collections and persist
+					c.getBCollection().clear();
+					c.getDCollection().clear();
+					d.getBCollection().clear();
+
+					entityManager.getTransaction().begin();
+					entityManager.persist( c );
+					// now add the elements back
+					c.getDCollection().add( d );
+					try {
+						entityManager.getTransaction().commit();
+						fail( "should have thrown IllegalStateException" );
+					}
+					catch (RollbackException ex) {
+						assertTrue( ex.getCause() instanceof IllegalStateException );
+						IllegalStateException ise = (IllegalStateException) ex.getCause();
+						assertTyping( TransientObjectException.class, ise.getCause() );
 						String message = ise.getCause().getMessage();
 						assertTrue( message.contains("'org.hibernate.orm.test.jpa.cascade.multicircle.F'") );
 						assertTrue( message.contains("'g'") );
-//						TransientPropertyValueException tpve = assertTyping(
-//								TransientPropertyValueException.class,
-//								ise.getCause()
-//						);
-//						assertEquals( G.class.getName(), tpve.getTransientEntityName() );
-//						assertEquals( F.class.getName(), tpve.getPropertyOwnerEntityName() );
-//						assertEquals( "g", tpve.getPropertyName() );
-					} finally {
+					}
+					finally {
 						entityManager.getTransaction().rollback();
 						entityManager.close();
 					}

hibernate-core/src/test/java/org/hibernate/orm/test/jpa/compliance/JoinTest.java

@@ -77,7 +77,7 @@ public class JoinTest {
 
 					entityManager.persist( order1 );
 					entityManager.persist( order2 );
-					entityManager.persist( order2 );
+					entityManager.persist( order3 );
 
 				}
 		);

hibernate-core/src/test/java/org/hibernate/orm/test/jpa/graphs/EntityGraphTest.java

@@ -322,6 +322,7 @@ public class EntityGraphTest extends BaseEntityManagerFunctionalTestCase {
 		Book book = new Book();
 		author.books.put(1, book);
 		em.persist(author);
+		em.persist(book);
 
 		em.getTransaction().commit();
 		em.clear();

hibernate-core/src/test/java/org/hibernate/orm/test/orderupdates/OrderUpdatesTest.java

@@ -59,14 +59,16 @@ public class OrderUpdatesTest {
 					Child child1 = new Child();
 					child1.setName( "name1" );
 					child1.setValue( "value" );
-					parent1.addChild( child1 );
+					child1.setParent( parent1 );
 					child1 = session.merge( child1 );
+					parent1.addChild( child1 );
 
 					Child child2 = new Child();
 					child2.setName( "name1" );
 					child2.setValue( "value" );
-					parent2.addChild( child2 );
+					child2.setParent( parent2 );
 					child2 = session.merge( child2 );
+					parent2.addChild( child2 );
 
 					session.flush();
 

hibernate-envers/src/test/java/org/hibernate/orm/test/envers/integration/proxy/RemovedObjectQueryTest.java

@@ -156,6 +156,7 @@ public class RemovedObjectQueryTest extends BaseEnversJPAFunctionalTestCase {
 		// Revision 9 - removing first object
 		em.getTransaction().begin();
 		setOwningEntity2 = em.find( SetOwningEntity.class, 7 );
+		setOwnedEntity2.getReferencing().remove( setOwningEntity2 );
 		em.remove( setOwningEntity2 );
 		em.getTransaction().commit();