HHH-6999 check for unsaved refs after processing all cascaded persist operations

Signed-off-by: Gavin King <gavin@hibernate.org>
This commit is contained in:
Gavin King 2024-06-04 13:45:49 +02:00
parent 7b9b495f31
commit 04223f5cb9
11 changed files with 170 additions and 169 deletions

View File

@ -34,6 +34,7 @@ import org.hibernate.type.EntityType;
import org.hibernate.type.Type;
import static org.hibernate.engine.internal.ManagedTypeHelper.isHibernateProxy;
import static org.hibernate.engine.spi.CascadingActions.CHECK_ON_FLUSH;
import static org.hibernate.pretty.MessageHelper.infoString;
import static org.hibernate.type.ForeignKeyDirection.TO_PARENT;
@ -80,15 +81,17 @@ public final class Cascade {
final EntityPersister persister,
final Object parent,
final T anything) throws HibernateException {
if ( persister.hasCascades() || action.requiresNoCascadeChecking() ) { // performance opt
if ( persister.hasCascades() || action == CHECK_ON_FLUSH ) { // performance opt
final boolean traceEnabled = LOG.isTraceEnabled();
if ( traceEnabled ) {
LOG.tracev( "Processing cascade {0} for: {1}", action, persister.getEntityName() );
}
final PersistenceContext persistenceContext = eventSource.getPersistenceContextInternal();
final EntityEntry entry = persistenceContext.getEntry( parent );
if ( entry != null && entry.getLoadedState() == null && entry.getStatus() == Status.MANAGED && persister.getBytecodeEnhancementMetadata()
.isEnhancedForLazyLoading() ) {
if ( entry != null
&& entry.getLoadedState() == null
&& entry.getStatus() == Status.MANAGED
&& persister.getBytecodeEnhancementMetadata().isEnhancedForLazyLoading() ) {
return;
}
final Type[] types = persister.getPropertyTypes();
@ -99,11 +102,11 @@ public final class Cascade {
for ( int i = 0; i < types.length; i++) {
final CascadeStyle style = cascadeStyles[ i ];
final String propertyName = propertyNames[ i ];
final Type type = types[i];
final boolean isUninitializedProperty =
hasUninitializedLazyProperties &&
!persister.getBytecodeEnhancementMetadata().isAttributeLoaded( parent, propertyName );
final Type type = types[i];
if ( style.doCascade( action ) ) {
final Object child;
if ( isUninitializedProperty ) {
@ -169,15 +172,6 @@ public final class Cascade {
);
}
else {
if ( action.requiresNoCascadeChecking() ) {
action.noCascade(
eventSource,
parent,
persister,
type,
i
);
}
// If the property is uninitialized, then there cannot be any orphans.
if ( action.deleteOrphans() && !isUninitializedProperty && isLogicalOneToOne( type ) ) {
cascadeLogicalOneToOneOrphanRemoval(

View File

@ -67,7 +67,8 @@ public final class NonNullableTransientDependencies {
* @return {@code true} indicates there are no path tracked here currently
*/
public boolean isEmpty() {
return propertyPathsByTransientEntity == null || propertyPathsByTransientEntity.isEmpty();
return propertyPathsByTransientEntity == null
|| propertyPathsByTransientEntity.isEmpty();
}
/**

View File

@ -11,7 +11,6 @@ import java.util.Iterator;
import org.hibernate.HibernateException;
import org.hibernate.PropertyValueException;
import org.hibernate.bytecode.enhance.spi.LazyPropertyInitializer;
import org.hibernate.engine.spi.CascadingActions;
import org.hibernate.engine.spi.SharedSessionContractImplementor;
import org.hibernate.persister.entity.EntityPersister;
import org.hibernate.generator.Generator;
@ -19,6 +18,8 @@ import org.hibernate.type.CollectionType;
import org.hibernate.type.CompositeType;
import org.hibernate.type.Type;
import static org.hibernate.engine.spi.CascadingActions.getLoadedElementsIterator;
/**
* Implements the algorithm for validating property values for illegal null values
*
@ -155,7 +156,7 @@ public final class Nullability {
if ( collectionElementType.isComponentType() ) {
// check for all components values in the collection
final CompositeType componentType = (CompositeType) collectionElementType;
final Iterator<?> itr = CascadingActions.getLoadedElementsIterator( session, collectionType, value );
final Iterator<?> itr = getLoadedElementsIterator( session, collectionType, value );
while ( itr.hasNext() ) {
final Object compositeElement = itr.next();
if ( compositeElement != null ) {

View File

@ -25,6 +25,7 @@ import java.util.concurrent.ConcurrentLinkedQueue;
import org.hibernate.AssertionFailure;
import org.hibernate.HibernateException;
import org.hibernate.PropertyValueException;
import org.hibernate.TransientObjectException;
import org.hibernate.action.internal.AbstractEntityInsertAction;
import org.hibernate.action.internal.BulkOperationCleanupAction;
import org.hibernate.action.internal.CollectionRecreateAction;
@ -492,7 +493,17 @@ public class ActionQueue {
*/
public void executeActions() throws HibernateException {
if ( hasUnresolvedEntityInsertActions() ) {
throw new IllegalStateException( "About to execute actions, but there are unresolved entity insert actions." );
final AbstractEntityInsertAction insertAction =
unresolvedInsertions.getDependentEntityInsertActions()
.iterator().next();
final NonNullableTransientDependencies transientEntities = insertAction.findNonNullableTransientEntities();
final Object transientEntity = transientEntities.getNonNullableTransientEntities().iterator().next();
final String path = transientEntities.getNonNullableTransientPropertyPaths(transientEntity).iterator().next();
//TODO: should be TransientPropertyValueException
throw new TransientObjectException( "Persistent instance of '" + insertAction.getEntityName()
+ "' with id '" + insertAction.getId()
+ "' references an unsaved transient instance via attribute '" + path
+ "' (save the transient instance before flushing)" );
}
for ( OrderedActions action : ORDERED_OPERATIONS ) {

View File

@ -68,7 +68,8 @@ public final class CascadeStyles {
public static final CascadeStyle UPDATE = new BaseCascadeStyle() {
@Override
public boolean doCascade(CascadingAction action) {
return action == CascadingActions.SAVE_UPDATE;
return action == CascadingActions.SAVE_UPDATE
|| action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
@ -83,7 +84,8 @@ public final class CascadeStyles {
public static final CascadeStyle LOCK = new BaseCascadeStyle() {
@Override
public boolean doCascade(CascadingAction action) {
return action == CascadingActions.LOCK;
return action == CascadingActions.LOCK
|| action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
@ -98,7 +100,8 @@ public final class CascadeStyles {
public static final CascadeStyle REFRESH = new BaseCascadeStyle() {
@Override
public boolean doCascade(CascadingAction action) {
return action == CascadingActions.REFRESH;
return action == CascadingActions.REFRESH
|| action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
@ -113,7 +116,8 @@ public final class CascadeStyles {
public static final CascadeStyle EVICT = new BaseCascadeStyle() {
@Override
public boolean doCascade(CascadingAction action) {
return action == CascadingActions.EVICT;
return action == CascadingActions.EVICT
|| action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
@ -128,7 +132,8 @@ public final class CascadeStyles {
public static final CascadeStyle REPLICATE = new BaseCascadeStyle() {
@Override
public boolean doCascade(CascadingAction action) {
return action == CascadingActions.REPLICATE;
return action == CascadingActions.REPLICATE
|| action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
@ -143,7 +148,8 @@ public final class CascadeStyles {
public static final CascadeStyle MERGE = new BaseCascadeStyle() {
@Override
public boolean doCascade(CascadingAction action) {
return action == CascadingActions.MERGE;
return action == CascadingActions.MERGE
|| action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
@ -159,7 +165,7 @@ public final class CascadeStyles {
@Override
public boolean doCascade(CascadingAction action) {
return action == CascadingActions.PERSIST
|| action == CascadingActions.PERSIST_ON_FLUSH;
|| action == CascadingActions.PERSIST_ON_FLUSH;
}
@Override
@ -174,7 +180,8 @@ public final class CascadeStyles {
public static final CascadeStyle DELETE = new BaseCascadeStyle() {
@Override
public boolean doCascade(CascadingAction action) {
return action == CascadingActions.DELETE;
return action == CascadingActions.DELETE
|| action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
@ -189,12 +196,15 @@ public final class CascadeStyles {
public static final CascadeStyle DELETE_ORPHAN = new BaseCascadeStyle() {
@Override
public boolean doCascade(CascadingAction action) {
return action == CascadingActions.DELETE || action == CascadingActions.SAVE_UPDATE;
return action == CascadingActions.DELETE
|| action == CascadingActions.SAVE_UPDATE //WHAT??
|| action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
public boolean reallyDoCascade(CascadingAction action) {
return action == CascadingActions.DELETE;
return action == CascadingActions.DELETE
|| action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
@ -214,7 +224,7 @@ public final class CascadeStyles {
public static final CascadeStyle NONE = new BaseCascadeStyle() {
@Override
public boolean doCascade(CascadingAction action) {
return false;
return action == CascadingActions.CHECK_ON_FLUSH;
}
@Override
@ -296,6 +306,9 @@ public final class CascadeStyles {
@Override
public boolean doCascade(CascadingAction action) {
if ( action == CascadingActions.CHECK_ON_FLUSH ) {
return !doCascade( CascadingActions.PERSIST_ON_FLUSH );
}
for ( CascadeStyle style : styles ) {
if ( style.doCascade( action ) ) {
return true;

View File

@ -65,8 +65,13 @@ public interface CascadingAction<T> {
* Does the specified cascading action require verification of no cascade validity?
*
* @return True if this action requires no-cascade verification; false otherwise.
*
* @deprecated No longer used
*/
boolean requiresNoCascadeChecking();
@Deprecated(since = "6.6", forRemoval = true)
default boolean requiresNoCascadeChecking() {
return false;
}
/**
* Called (in the case of {@link #requiresNoCascadeChecking} returning true) to validate
@ -77,8 +82,11 @@ public interface CascadingAction<T> {
* @param persister The entity persister for the owner
* @param propertyType The property type
* @param propertyIndex The index of the property within the owner.
*
* @deprecated No longer used
*/
void noCascade(EventSource session, Object parent, EntityPersister persister, Type propertyType, int propertyIndex);
@Deprecated(since = "6.6", forRemoval = true)
default void noCascade(EventSource session, Object parent, EntityPersister persister, Type propertyType, int propertyIndex) {}
/**
* Should this action be performed (or noCascade consulted) in the case of lazy properties.

View File

@ -6,28 +6,26 @@
*/
package org.hibernate.engine.spi;
import java.util.Iterator;
import org.hibernate.HibernateException;
import org.hibernate.Internal;
import org.hibernate.LockMode;
import org.hibernate.LockOptions;
import org.hibernate.ReplicationMode;
import org.hibernate.TransientPropertyValueException;
import org.hibernate.TransientObjectException;
import org.hibernate.collection.spi.PersistentCollection;
import org.hibernate.engine.internal.ForeignKeys;
import org.hibernate.event.spi.DeleteContext;
import org.hibernate.event.spi.MergeContext;
import org.hibernate.event.spi.EventSource;
import org.hibernate.event.spi.MergeContext;
import org.hibernate.event.spi.PersistContext;
import org.hibernate.event.spi.RefreshContext;
import org.hibernate.internal.CoreMessageLogger;
import org.hibernate.persister.entity.EntityPersister;
import org.hibernate.type.CollectionType;
import org.hibernate.type.EntityType;
import org.hibernate.type.Type;
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;
/**
@ -346,67 +344,90 @@ public class CascadingActions {
return true;
}
@Override
public boolean requiresNoCascadeChecking() {
return true;
}
@Override
public void noCascade(
EventSource session,
Object parent,
EntityPersister persister,
Type propertyType,
int propertyIndex) {
if ( propertyType.isEntityType() ) {
final Object child = persister.getValue( parent, propertyIndex );
if ( child != null
&& !isInManagedState( child, session )
&& !isHibernateProxy( child ) ) { //a proxy cannot be transient and it breaks ForeignKeys.isTransient
final String childEntityName =
((EntityType) propertyType).getAssociatedEntityName( session.getFactory() );
if ( ForeignKeys.isTransient( childEntityName, child, null, session ) ) {
String parentEntityName = persister.getEntityName();
String propertyName = persister.getPropertyNames()[propertyIndex];
throw new TransientPropertyValueException(
"object references an unsaved transient instance - save the transient instance before flushing",
childEntityName,
parentEntityName,
propertyName
);
}
}
}
}
@Override
public boolean performOnLazyProperty() {
return false;
}
private boolean isInManagedState(Object child, EventSource session) {
EntityEntry entry = session.getPersistenceContextInternal().getEntry( child );
if ( entry == null ) {
return false;
}
else {
switch ( entry.getStatus() ) {
case MANAGED:
case READ_ONLY:
case SAVING:
return true;
default:
return false;
}
}
}
@Override
public String toString() {
return "ACTION_PERSIST_ON_FLUSH";
}
};
@Internal
// this is not a real type of cascade, but it's a check that
// is at least a bit sensitive to the cascade style, and it's
// convenient to be able to use the graph-walking logic that
// is already implemented in the Cascade class (though perhaps
// we could reuse the logic in Nullability instead)
public static final CascadingAction<Void> CHECK_ON_FLUSH = new BaseCascadingAction<>() {
@Override
public void cascade(
EventSource session,
Object child,
String entityName,
Void context,
boolean isCascadeDeleteEnabled)
throws HibernateException {
if ( child != null
&& !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
throw new TransientObjectException( "persistent instance references an unsaved transient instance of '"
+ entityName + "' (save the transient instance before flushing)" );
// throw new TransientPropertyValueException(
// "object references an unsaved transient instance - save the transient instance before flushing",
// entityName,
// persister.getEntityName(),
// persister.getPropertyNames()[propertyIndex]
// );
}
}
}
@Override
public Iterator<?> getCascadableChildrenIterator(
EventSource session,
CollectionType collectionType,
Object collection) {
return emptySet().iterator();
}
@Override
public boolean deleteOrphans() {
return false;
}
@Override
public boolean performOnLazyProperty() {
return false;
}
@Override
public String toString() {
return "ACTION_CHECK_ON_FLUSH";
}
};
private static boolean isInManagedState(Object child, EventSource session) {
final EntityEntry entry = session.getPersistenceContextInternal().getEntry( child );
if ( entry == null ) {
return false;
}
else {
switch ( entry.getStatus() ) {
case MANAGED:
case READ_ONLY:
case SAVING:
return true;
default:
return false;
}
}
}
/**
* @see org.hibernate.Session#replicate
*/
@ -444,15 +465,6 @@ public class CascadingActions {
};
public abstract static class BaseCascadingAction<T> implements CascadingAction<T> {
@Override
public boolean requiresNoCascadeChecking() {
return false;
}
@Override
public void noCascade(EventSource session, Object parent, EntityPersister persister, Type propertyType, int propertyIndex) {
}
@Override
public boolean performOnLazyProperty() {
return true;

View File

@ -155,6 +155,14 @@ public abstract class AbstractFlushingEventListener implements JpaBootstrapSensi
cascadeOnFlush( session, entry.getPersister(), me.getKey(), context );
}
}
// perform these checks after all cascade persist events have been
// processed, so that all entities which will be persisted are
// persistent when we do the check (I wonder if we could move this
// into Nullability, instead of abusing the Cascade infrastructure)
persistenceContext.getEntitiesByKey().forEach( (entry, entity) -> {
Cascade.cascade( CascadingActions.CHECK_ON_FLUSH, CascadePoint.BEFORE_FLUSH, session, entry.getPersister(), entity, null );
} );
}
private static boolean flushable(EntityEntry entry) {

View File

@ -48,7 +48,6 @@ import org.hibernate.query.sqm.ComparisonOperator;
import org.hibernate.spi.EntityIdentifierNavigablePath;
import org.hibernate.spi.NavigablePath;
import org.hibernate.sql.ast.SqlAstJoinType;
import org.hibernate.sql.ast.internal.TableGroupJoinHelper;
import org.hibernate.sql.ast.spi.AliasCollector;
import org.hibernate.sql.ast.spi.FromClauseAccess;
import org.hibernate.sql.ast.spi.SimpleFromClauseAccessImpl;

View File

@ -8,10 +8,11 @@ package org.hibernate.orm.test.jpa.cascade.multicircle;
import jakarta.persistence.RollbackException;
import org.hibernate.SessionFactory;
import org.hibernate.TransientObjectException;
import org.hibernate.TransientPropertyValueException;
import org.hibernate.testing.orm.junit.EntityManagerFactoryScope;
import org.hibernate.testing.orm.junit.FailureExpected;
import org.hibernate.testing.orm.junit.Jpa;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
@ -88,7 +89,6 @@ public class MultiCircleJpaCascadeTest {
private E e;
private F f;
private G g;
private boolean skipCleanup;
@BeforeEach
public void setup() {
@ -121,51 +121,12 @@ public class MultiCircleJpaCascadeTest {
g.setB( b );
g.getFCollection().add( f );
skipCleanup = false;
}
@AfterEach
public void cleanup(EntityManagerFactoryScope scope) {
if ( !skipCleanup ) {
b.setC( null );
b.setD( null );
b.getGCollection().remove( g );
c.getBCollection().remove( b );
c.getDCollection().remove( d );
d.getBCollection().remove( b );
d.setC( null );
d.setE( null );
d.getFCollection().remove( f );
e.getDCollection().remove( d );
e.setF( null );
f.setD( null );
f.getECollection().remove( e );
f.setG( null );
g.setB( null );
g.getFCollection().remove( f );
scope.inTransaction(
entityManager -> {
b = entityManager.merge( b );
c = entityManager.merge( c );
d = entityManager.merge( d );
e = entityManager.merge( e );
f = entityManager.merge( f );
g = entityManager.merge( g );
entityManager.remove( f );
entityManager.remove( g );
entityManager.remove( b );
entityManager.remove( d );
entityManager.remove( e );
entityManager.remove( c );
}
);
}
scope.getEntityManagerFactory().unwrap(SessionFactory.class)
.getSchemaManager().truncateMappedObjects();
}
@Test
@ -181,7 +142,6 @@ public class MultiCircleJpaCascadeTest {
@Test
public void testPersistNoCascadeToTransient(EntityManagerFactoryScope scope) {
skipCleanup = true;
scope.inEntityManager(
entityManager -> {
entityManager.getTransaction().begin();
@ -204,10 +164,7 @@ public class MultiCircleJpaCascadeTest {
}
@Test
@FailureExpected(jiraKey = "HHH-6999") // remove skipCleanup below when this annotation is removed, added it to avoid failure in the cleanup
// fails on d.e; should pass
public void testPersistThenUpdate(EntityManagerFactoryScope scope) {
skipCleanup = true;
scope.inTransaction(
entityManager -> {
entityManager.persist( b );
@ -230,9 +187,6 @@ public class MultiCircleJpaCascadeTest {
@Test
public void testPersistThenUpdateNoCascadeToTransient(EntityManagerFactoryScope scope) {
// expected to fail, so nothing will be persisted.
skipCleanup = true;
scope.inEntityManager(
entityManager -> {
// remove elements from collections and persist
@ -251,19 +205,20 @@ public class MultiCircleJpaCascadeTest {
catch (RollbackException ex) {
assertTrue( ex.getCause() instanceof IllegalStateException );
IllegalStateException ise = (IllegalStateException) ex.getCause();
// should fail on entity g (due to no cascade to f.g);
// instead it fails on entity e ( due to no cascade to d.e)
// because e is not in the process of being saved yet.
// when HHH-6999 is fixed, this test should be changed to
// check for g and f.g
//noinspection ThrowableResultOfMethodCallIgnored
TransientPropertyValueException tpve = assertTyping(
TransientPropertyValueException.class,
assertTyping(
TransientObjectException.class,
ise.getCause()
);
assertEquals( E.class.getName(), tpve.getTransientEntityName() );
assertEquals( D.class.getName(), tpve.getPropertyOwnerEntityName() );
assertEquals( "e", tpve.getPropertyName() );
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 {
entityManager.getTransaction().rollback();
entityManager.close();

View File

@ -9,7 +9,6 @@ package org.hibernate.orm.test.jpa.cascade2;
import org.hibernate.Session;
import org.hibernate.TransientObjectException;
import org.hibernate.TransientPropertyValueException;
import org.hibernate.orm.test.jpa.model.AbstractJPATest;
import org.junit.jupiter.api.Test;
@ -87,7 +86,7 @@ public class CascadeTest extends AbstractJPATest {
}
catch (IllegalStateException e) {
// expected result
assertInstanceOf( TransientPropertyValueException.class, e.getCause() );
assertInstanceOf( TransientObjectException.class, e.getCause() );
s.getTransaction().rollback();
}
finally {
@ -119,7 +118,7 @@ public class CascadeTest extends AbstractJPATest {
}
catch (IllegalStateException e) {
// expected result
assertInstanceOf( TransientPropertyValueException.class, e.getCause() );
assertInstanceOf( TransientObjectException.class, e.getCause() );
s.getTransaction().rollback();
}
finally {
@ -149,7 +148,7 @@ public class CascadeTest extends AbstractJPATest {
}
catch (IllegalStateException e) {
// expected result
assertInstanceOf( TransientPropertyValueException.class, e.getCause() );
assertInstanceOf( TransientObjectException.class, e.getCause() );
s.getTransaction().rollback();
}
finally {
@ -179,7 +178,7 @@ public class CascadeTest extends AbstractJPATest {
}
catch (IllegalStateException e) {
// expected result
assertInstanceOf( TransientPropertyValueException.class, e.getCause() );
assertInstanceOf( TransientObjectException.class, e.getCause() );
s.getTransaction().rollback();
}
finally {
@ -207,7 +206,7 @@ public class CascadeTest extends AbstractJPATest {
}
catch (IllegalStateException e) {
// expected result
assertInstanceOf( TransientPropertyValueException.class, e.getCause() );
assertInstanceOf( TransientObjectException.class, e.getCause() );
s.getTransaction().rollback();
}
finally {
@ -235,7 +234,7 @@ public class CascadeTest extends AbstractJPATest {
}
catch (IllegalStateException e) {
// expected result
assertInstanceOf( TransientPropertyValueException.class, e.getCause() );
assertInstanceOf( TransientObjectException.class, e.getCause() );
s.getTransaction().rollback();
}
finally {
@ -264,7 +263,7 @@ public class CascadeTest extends AbstractJPATest {
}
catch (IllegalStateException e) {
// expected result
assertInstanceOf( TransientPropertyValueException.class, e.getCause() );
assertInstanceOf( TransientObjectException.class, e.getCause() );
s.getTransaction().rollback();
}
finally {
@ -293,7 +292,7 @@ public class CascadeTest extends AbstractJPATest {
}
catch (IllegalStateException e) {
// expected result
assertInstanceOf( TransientPropertyValueException.class, e.getCause() );
assertInstanceOf( TransientObjectException.class, e.getCause() );
s.getTransaction().rollback();
}
finally {