HHH-4301 eliminate unnecessary DELETE for collections with @OnDelete(CASCADE)

This change works for @ElementCollection and @ManyToMany

In future we could do a similar thing for @OneToMany and SET_NULL

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

hibernate-core/src/main/java/org/hibernate/action/internal/QueuedOperationCollectionAction.java

@@ -48,7 +48,8 @@ public final class QueuedOperationCollectionAction extends CollectionAction {
 
 		// TODO: It would be nice if this could be done safely by CollectionPersister#processQueuedOps;
 		//       Can't change the SPI to do this though.
-		((AbstractPersistentCollection<?>) getCollection() ).clearOperationQueue();
+		AbstractPersistentCollection<?> collection = (AbstractPersistentCollection<?>) getCollection();
+		collection.clearOperationQueue();
 
 		// The other CollectionAction types call CollectionEntry#afterAction, which
 		// clears the dirty flag. We don't want to call CollectionEntry#afterAction unless

hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionBinder.java

@@ -65,7 +65,6 @@ import org.hibernate.annotations.OptimisticLock;
 import org.hibernate.annotations.Parameter;
 import org.hibernate.annotations.Persister;
 import org.hibernate.annotations.QueryCacheLayout;
-import org.hibernate.annotations.ResultCheckStyle;
 import org.hibernate.annotations.SQLDelete;
 import org.hibernate.annotations.SQLDeleteAll;
 import org.hibernate.annotations.SQLInsert;
@@ -97,7 +96,6 @@ import org.hibernate.engine.spi.FilterDefinition;
 import org.hibernate.internal.CoreMessageLogger;
 import org.hibernate.internal.util.collections.CollectionHelper;
 import org.hibernate.jdbc.Expectation;
-import org.hibernate.jdbc.Expectations;
 import org.hibernate.mapping.Any;
 import org.hibernate.mapping.Backref;
 import org.hibernate.mapping.CheckConstraint;
@@ -520,7 +518,6 @@ public abstract class CollectionBinder {
 			final Class<?> targetElement = elementCollectionAnn.targetClass();
 			collectionBinder.setTargetEntity( reflectionManager.toXClass( targetElement ) );
 			//collectionBinder.setCascadeStrategy( getCascadeStrategy( embeddedCollectionAnn.cascade(), hibernateCascade ) );
-			//While this is a collection-valued property, its mapping significantly differs from those of one-to-many.
 			collectionBinder.setOneToMany( false );
 		}
 		else if ( manyToManyAnn != null ) {
@@ -1294,8 +1291,7 @@ public abstract class CollectionBinder {
 				&& !property.isAnnotationPresent( JoinColumns.class )) {
 			throw new AnnotationException( "Unidirectional '@OneToMany' association '"
 					+ qualify( propertyHolder.getPath(), propertyName )
-					+ "' is annotated '@OnDelete' and must explicitly specify a '@JoinColumn'"
+					+ "' is annotated '@OnDelete' and must explicitly specify a '@JoinColumn'" );
-					+ " (so that Join Table mechanic is not used)" );
 		}
 	}
 

hibernate-core/src/main/java/org/hibernate/engine/internal/Collections.java

@@ -16,7 +16,7 @@ import org.hibernate.engine.spi.EntityKey;
 import org.hibernate.engine.spi.PersistenceContext;
 import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.engine.spi.SessionImplementor;
-import org.hibernate.engine.spi.Status;
+import org.hibernate.event.spi.EventSource;
 import org.hibernate.internal.CoreMessageLogger;
 import org.hibernate.persister.collection.CollectionPersister;
 import org.hibernate.pretty.MessageHelper;
@@ -293,9 +293,28 @@ public final class Collections {
 		}
 	}
 
+	/**
+	 * Determines if we can skip the explicit SQL delete statement, since
+	 * the rows will be deleted by {@code on delete cascade}.
+	 */
+	public static boolean skipRemoval(EventSource session, CollectionPersister persister, Object key) {
+		if ( persister != null
+				// TODO: same optimization for @OneToMany @OnDelete(action=SET_NULL)
+				&& !persister.isOneToMany() && persister.isCascadeDeleteEnabled() ) {
+			final EntityKey entityKey = session.generateEntityKey( key, persister.getOwnerEntityPersister() );
+			final PersistenceContext persistenceContext = session.getPersistenceContextInternal();
+			final EntityEntry entry = persistenceContext.getEntry( persistenceContext.getEntity( entityKey ) );
+			return entry == null || entry.getStatus().isDeletedOrGone();
+		}
+		else {
+			return false;
+		}
+	}
+
 	/**
 	 * Disallow instantiation
 	 */
 	private Collections() {
 	}
+
 }

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

@@ -39,6 +39,8 @@ import org.hibernate.persister.entity.EntityPersister;
 
 import org.jboss.logging.Logger;
 
+import static org.hibernate.engine.internal.Collections.skipRemoval;
+
 /**
  * A convenience base class for listeners whose functionality results in flushing.
  *
@@ -301,15 +303,17 @@ public abstract class AbstractFlushingEventListener implements JpaBootstrapSensi
 					}
 					if ( ce.isDoremove() ) {
 						interceptor.onCollectionRemove( coll, ce.getLoadedKey() );
-						actionQueue.addAction(
+						if ( !skipRemoval( session, ce.getLoadedPersister(), ce.getLoadedKey() ) ) {
-								new CollectionRemoveAction(
+							actionQueue.addAction(
-										coll,
+									new CollectionRemoveAction(
-										ce.getLoadedPersister(),
+											coll,
-										ce.getLoadedKey(),
+											ce.getLoadedPersister(),
-										ce.isSnapshotEmpty( coll ),
+											ce.getLoadedKey(),
-										session
+											ce.isSnapshotEmpty( coll ),
-								)
+											session
-						);
+									)
+							);
+						}
 					}
 					if ( ce.isDoupdate() ) {
 						interceptor.onCollectionUpdate( coll, ce.getLoadedKey() );

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

@@ -51,6 +51,8 @@ import org.hibernate.type.CompositeType;
 import org.hibernate.type.Type;
 import org.hibernate.type.TypeHelper;
 
+import static org.hibernate.engine.internal.Collections.skipRemoval;
+
 /**
  * Defines the default delete event listener used by hibernate for deleting entities
  * from the datastore in response to generated delete events.
@@ -138,7 +140,7 @@ public class DefaultDeleteEventListener implements DeleteEventListener,	Callback
 		if ( type.isCollectionType() ) {
 			final String role = ( (CollectionType) type ).getRole();
 			final CollectionPersister persister = mappingMetamodel.getCollectionDescriptor(role);
-			if ( !persister.isInverse() ) {
+			if ( !persister.isInverse() && !skipRemoval( session, persister, key ) ) {
 				actionQueue.addAction( new CollectionRemoveAction( persister, key, session ) );
 			}
 		}

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

@@ -136,8 +136,8 @@ public class WrapVisitor extends ProxyVisitor {
 										session
 								);
 								persistenceContext.addUninitializedCollection( persister, collectionInstance, key );
-								final CollectionEntry collectionEntry = persistenceContext.getCollectionEntry(
+								final CollectionEntry collectionEntry =
-										collectionInstance );
+										persistenceContext.getCollectionEntry( collectionInstance );
 								collectionEntry.setDoremove( true );
 							}
 						}

hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java

@@ -206,6 +206,8 @@ public abstract class AbstractCollectionPersister
 	private final boolean hasOrphanDelete;
 	private final boolean subselectLoadable;
 
+	private final boolean cascadeDeleteEnabled;
+
 	// extra information about the element type
 	private final Class<?> elementClass;
 
@@ -589,6 +591,9 @@ public abstract class AbstractCollectionPersister
 		}
 
 		tableMapping = buildCollectionTableMapping( collectionBootDescriptor, getTableName(), getCollectionSpaces() );
+
+		cascadeDeleteEnabled = collectionBootDescriptor.getKey().isCascadeDeleteEnabled()
+				&& creationContext.getDialect().supportsCascadeDelete();
 	}
 
 	private BeforeExecutionGenerator createGenerator(RuntimeModelCreationContext context, IdentifierCollection collection) {
@@ -1115,6 +1120,10 @@ public abstract class AbstractCollectionPersister
 		return isInverse;
 	}
 
+	public boolean isCascadeDeleteEnabled() {
+		return cascadeDeleteEnabled;
+	}
+
 	@Override
 	public String getTableName() {
 		return qualifiedTableName;

hibernate-core/src/main/java/org/hibernate/persister/collection/BasicCollectionPersister.java

@@ -81,10 +81,6 @@ public class BasicCollectionPersister extends AbstractCollectionPersister {
 	private final DeleteRowsCoordinator deleteRowsCoordinator;
 	private final RemoveCoordinator removeCoordinator;
 
-	public boolean isCascadeDeleteEnabled() {
-		return false;
-	}
-
 	@Deprecated(since = "6.0")
 	public BasicCollectionPersister(
 			Collection collectionBinding,

hibernate-core/src/main/java/org/hibernate/persister/collection/OneToManyPersister.java

@@ -100,7 +100,6 @@ public class OneToManyPersister extends AbstractCollectionPersister {
 	private final DeleteRowsCoordinator deleteRowsCoordinator;
 	private final RemoveCoordinator removeCoordinator;
 
-	private final boolean cascadeDeleteEnabled;
 	private final boolean keyIsNullable;
 	private final MutationExecutorService mutationExecutorService;
 
@@ -117,8 +116,6 @@ public class OneToManyPersister extends AbstractCollectionPersister {
 			CollectionDataAccess cacheAccessStrategy,
 			RuntimeModelCreationContext creationContext) throws MappingException, CacheException {
 		super( collectionBinding, cacheAccessStrategy, creationContext );
-		cascadeDeleteEnabled = collectionBinding.getKey().isCascadeDeleteEnabled()
-				&& creationContext.getDialect().supportsCascadeDelete();
 		keyIsNullable = collectionBinding.getKey().isNullable();
 
 		this.rowMutationOperations = buildRowMutationOperations();
@@ -157,10 +154,6 @@ public class OneToManyPersister extends AbstractCollectionPersister {
 		return super.isRowDeleteEnabled() && keyIsNullable;
 	}
 
-	public boolean isCascadeDeleteEnabled() {
-		return cascadeDeleteEnabled;
-	}
-
 	@Override
 	public void recreate(PersistentCollection<?> collection, Object id, SharedSessionContractImplementor session)
 			throws HibernateException {

hibernate-core/src/test/java/org/hibernate/orm/test/annotations/collectionelement/OnDeleteCascadeToElementCollectionTest.java

@@ -10,6 +10,7 @@ import org.hibernate.annotations.OnDelete;
 import org.hibernate.annotations.OnDeleteAction;
 import org.hibernate.exception.ConstraintViolationException;
 
+import org.hibernate.stat.spi.StatisticsImplementor;
 import org.hibernate.testing.orm.junit.DialectFeatureChecks;
 import org.hibernate.testing.orm.junit.DomainModel;
 import org.hibernate.testing.orm.junit.ExpectedException;
@@ -28,6 +29,7 @@ import jakarta.persistence.GeneratedValue;
 import jakarta.persistence.Id;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 
 @DomainModel(
 		annotatedClasses = {
@@ -36,7 +38,7 @@ import static org.assertj.core.api.Assertions.assertThat;
 				OnDeleteCascadeToElementCollectionTest.NonCascading.class
 		}
 )
-@SessionFactory
+@SessionFactory(generateStatistics = true)
 @ExtendWith(ExpectedExceptionExtension.class)
 @JiraKey("HHH-4301")
 public class OnDeleteCascadeToElementCollectionTest {
@@ -76,6 +78,99 @@ public class OnDeleteCascadeToElementCollectionTest {
 		);
 	}
 
+	@Test
+	@RequiresDialectFeature(feature = DialectFeatureChecks.SupportsCascadeDeleteCheck.class)
+	public void testCascadingDeleteUnloaded(SessionFactoryScope scope) {
+		var instance = new Cascading();
+
+		scope.inTransaction(
+				session -> {
+					instance.labels = new HashSet<>( Set.of( "one", "two" ) );
+					instance.tickets = new HashMap<>( Map.of(
+							"t1", new Ticket( "t1-2398", LocalDate.of( 2023, 8, 26 ) ),
+							"t2", new Ticket( "t2-23132", LocalDate.of( 2007, 9, 26 ) )
+					) );
+					session.persist( instance );
+				}
+		);
+
+		StatisticsImplementor statistics = scope.getSessionFactory().getStatistics();
+		statistics.clear();
+
+		scope.inTransaction(
+				session -> {
+					session.remove(session.getReference(instance));
+				}
+		);
+
+		assertEquals( 0, statistics.getEntityLoadCount() );
+		assertEquals( 1, statistics.getEntityDeleteCount() );
+		assertEquals( 0, statistics.getCollectionLoadCount() );
+		assertEquals( 0, statistics.getCollectionRemoveCount() );
+
+		scope.inTransaction(
+				session -> {
+					assertEquals( 0L,
+							session.createSelectionQuery( "from Cascading", Long.class )
+									.getResultCount() );
+					assertEquals( 0L,
+							session.createNativeQuery( "select count(*) from Cascading_tickets", Long.class )
+									.getSingleResult() );
+					assertEquals( 0L,
+							session.createNativeQuery( "select count(*) from Cascading_labels", Long.class )
+									.getSingleResult() );
+				}
+		);
+	}
+
+	@Test
+	@RequiresDialectFeature(feature = DialectFeatureChecks.SupportsCascadeDeleteCheck.class)
+	public void testCascadingDeleteLoaded(SessionFactoryScope scope) {
+		var instance = new Cascading();
+
+		scope.inTransaction(
+				session -> {
+					instance.labels = new HashSet<>( Set.of( "one", "two" ) );
+					instance.tickets = new HashMap<>( Map.of(
+							"t1", new Ticket( "t1-2398", LocalDate.of( 2023, 8, 26 ) ),
+							"t2", new Ticket( "t2-23132", LocalDate.of( 2007, 9, 26 ) )
+					) );
+					session.persist( instance );
+				}
+		);
+
+		StatisticsImplementor statistics = scope.getSessionFactory().getStatistics();
+		statistics.clear();
+
+		scope.inTransaction(
+				session -> {
+					Cascading entity = session.find(Cascading.class, instance.id);
+					entity.labels.size();
+					entity.tickets.size();
+					session.remove( entity );
+				}
+		);
+
+		assertEquals( 1, statistics.getEntityLoadCount() );
+		assertEquals( 1, statistics.getEntityDeleteCount() );
+		assertEquals( 2, statistics.getCollectionLoadCount() );
+		assertEquals( 0, statistics.getCollectionRemoveCount() );
+
+		scope.inTransaction(
+				session -> {
+					assertEquals( 0L,
+							session.createSelectionQuery( "from Cascading", Long.class )
+									.getResultCount() );
+					assertEquals( 0L,
+							session.createNativeQuery( "select count(*) from Cascading_tickets", Long.class )
+									.getSingleResult() );
+					assertEquals( 0L,
+							session.createNativeQuery( "select count(*) from Cascading_labels", Long.class )
+									.getSingleResult() );
+				}
+		);
+	}
+
 	@Test
 	@ExpectedException(ConstraintViolationException.class)
 	public void testNonCascading(SessionFactoryScope scope) {