diff --git a/hibernate-core/src/main/java/org/hibernate/action/internal/QueuedOperationCollectionAction.java b/hibernate-core/src/main/java/org/hibernate/action/internal/QueuedOperationCollectionAction.java index a592b582df..cfafa8efd3 100644 --- a/hibernate-core/src/main/java/org/hibernate/action/internal/QueuedOperationCollectionAction.java +++ b/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 diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionBinder.java b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionBinder.java index f42561f13c..dd4a09b6d9 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/CollectionBinder.java +++ b/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'" - + " (so that Join Table mechanic is not used)" ); + + "' is annotated '@OnDelete' and must explicitly specify a '@JoinColumn'" ); } } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/internal/Collections.java b/hibernate-core/src/main/java/org/hibernate/engine/internal/Collections.java index e69fbc52e4..84cc70798b 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/internal/Collections.java +++ b/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() { } + } diff --git a/hibernate-core/src/main/java/org/hibernate/event/internal/AbstractFlushingEventListener.java b/hibernate-core/src/main/java/org/hibernate/event/internal/AbstractFlushingEventListener.java index ed332e027c..274b6897ae 100644 --- a/hibernate-core/src/main/java/org/hibernate/event/internal/AbstractFlushingEventListener.java +++ b/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( - new CollectionRemoveAction( - coll, - ce.getLoadedPersister(), - ce.getLoadedKey(), - ce.isSnapshotEmpty( coll ), - session - ) - ); + if ( !skipRemoval( session, ce.getLoadedPersister(), ce.getLoadedKey() ) ) { + actionQueue.addAction( + new CollectionRemoveAction( + coll, + ce.getLoadedPersister(), + ce.getLoadedKey(), + ce.isSnapshotEmpty( coll ), + session + ) + ); + } } if ( ce.isDoupdate() ) { interceptor.onCollectionUpdate( coll, ce.getLoadedKey() ); diff --git a/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultDeleteEventListener.java b/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultDeleteEventListener.java index 23b1446ec0..e871a806e2 100644 --- a/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultDeleteEventListener.java +++ b/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 ) ); } } diff --git a/hibernate-core/src/main/java/org/hibernate/event/internal/WrapVisitor.java b/hibernate-core/src/main/java/org/hibernate/event/internal/WrapVisitor.java index 2d7d2314e2..7898e0a265 100644 --- a/hibernate-core/src/main/java/org/hibernate/event/internal/WrapVisitor.java +++ b/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( - collectionInstance ); + final CollectionEntry collectionEntry = + persistenceContext.getCollectionEntry( collectionInstance ); collectionEntry.setDoremove( true ); } } diff --git a/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java index 5ac7f0ed8e..a12dcb731e 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java +++ b/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; diff --git a/hibernate-core/src/main/java/org/hibernate/persister/collection/BasicCollectionPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/collection/BasicCollectionPersister.java index 1f51160f56..e948281080 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/collection/BasicCollectionPersister.java +++ b/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, diff --git a/hibernate-core/src/main/java/org/hibernate/persister/collection/OneToManyPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/collection/OneToManyPersister.java index 042f0ee0c6..7c6d6d9a00 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/collection/OneToManyPersister.java +++ b/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 { diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/collectionelement/OnDeleteCascadeToElementCollectionTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/collectionelement/OnDeleteCascadeToElementCollectionTest.java index f244dc8aa6..897aaa098e 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/collectionelement/OnDeleteCascadeToElementCollectionTest.java +++ b/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) {