HHH-18229 Handle null owner key for collections

beikov · beikov · commit 1f08501d8092 · 2024-07-26T16:42:59.000+02:00
diff --git a/hibernate-core/src/main/java/org/hibernate/action/internal/AbstractEntityInsertAction.java b/hibernate-core/src/main/java/org/hibernate/action/internal/AbstractEntityInsertAction.java
@@ -215,19 +215,16 @@ private void addCollectionKey(
 			PersistenceContext persistenceContext) {
 		if ( o instanceof PersistentCollection ) {
 			final CollectionPersister collectionPersister = pluralAttributeMapping.getCollectionDescriptor();
-			final CollectionKey collectionKey = new CollectionKey(
+			final Object key = ( (AbstractEntityPersister) getPersister() ).getCollectionKey(
 					collectionPersister,
-					( (AbstractEntityPersister) getPersister() ).getCollectionKey(
-							collectionPersister,
-							getInstance(),
-							persistenceContext.getEntry( getInstance() ),
-							getSession()
-					)
-			);
-			persistenceContext.addCollectionByKey(
-					collectionKey,
-					(PersistentCollection<?>) o
+					getInstance(),
+					persistenceContext.getEntry( getInstance() ),
+					getSession()
 			);
+			if ( key != null ) {
+				final CollectionKey collectionKey = new CollectionKey( collectionPersister, key );
+				persistenceContext.addCollectionByKey( collectionKey, (PersistentCollection<?>) o );
+			}
 		}
 	}
 
diff --git a/hibernate-core/src/main/java/org/hibernate/collection/spi/AbstractPersistentCollection.java b/hibernate-core/src/main/java/org/hibernate/collection/spi/AbstractPersistentCollection.java
@@ -40,6 +40,8 @@
 import org.hibernate.type.CompositeType;
 import org.hibernate.type.Type;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import static org.hibernate.pretty.MessageHelper.collectionInfoString;
 
 /**
@@ -58,16 +60,16 @@ public abstract class AbstractPersistentCollection<E> implements Serializable, P
 
 	private transient List<DelayedOperation<E>> operationQueue;
 	private transient boolean directlyAccessible;
-	private Object owner;
+	private @Nullable Object owner;
 	private int cachedSize = -1;
 
-	private String role;
-	private Object key;
+	private @Nullable String role;
+	private @Nullable Object key;
 	// collections detect changes made via their public interface and mark
 	// themselves as dirty as a performance optimization
 	private boolean dirty;
 	protected boolean elementRemoved;
-	private Serializable storedSnapshot;
+	private @Nullable Serializable storedSnapshot;
 
 	private String sessionFactoryUuid;
 	private boolean allowLoadOutsideTransaction;
@@ -84,12 +86,12 @@ protected AbstractPersistentCollection(SharedSessionContractImplementor session)
 	}
 
 	@Override
-	public final String getRole() {
+	public final @Nullable String getRole() {
 		return role;
 	}
 
 	@Override
-	public final Object getKey() {
+	public final @Nullable Object getKey() {
 		return key;
 	}
 
@@ -120,7 +122,7 @@ public final void dirty() {
 	}
 
 	@Override
-	public final Serializable getStoredSnapshot() {
+	public final @Nullable Serializable getStoredSnapshot() {
 		return storedSnapshot;
 	}
 
@@ -1354,7 +1356,7 @@ public Object getIdentifier(Object entry, int i) {
 	}
 
 	@Override
-	public Object getOwner() {
+	public @Nullable Object getOwner() {
 		return owner;
 	}
 
diff --git a/hibernate-core/src/main/java/org/hibernate/collection/spi/PersistentCollection.java b/hibernate-core/src/main/java/org/hibernate/collection/spi/PersistentCollection.java
@@ -62,7 +62,7 @@ public interface PersistentCollection<E> extends LazyInitializable {
 	 *
 	 * @return The owner
 	 */
-	Object getOwner();
+	@Nullable Object getOwner();
 
 	/**
 	 * Set the reference to the owning entity
@@ -405,14 +405,14 @@ default boolean needsUpdating(
 	 *
 	 * @return the current collection key value
 	 */
-	Object getKey();
+	@Nullable Object getKey();
 
 	/**
 	 * Get the current role name
 	 *
 	 * @return the collection role name
 	 */
-	String getRole();
+	@Nullable String getRole();
 
 	/**
 	 * Is the collection unreferenced?
@@ -461,7 +461,7 @@ default boolean isDirectlyProvidedCollection(Object collection) {
 	 *
 	 * @return The internally stored snapshot state
 	 */
-	Serializable getStoredSnapshot();
+	@Nullable Serializable getStoredSnapshot();
 
 	/**
 	 * Mark the collection as dirty
diff --git a/hibernate-core/src/main/java/org/hibernate/engine/internal/StatefulPersistenceContext.java b/hibernate-core/src/main/java/org/hibernate/engine/internal/StatefulPersistenceContext.java
@@ -1050,8 +1050,10 @@ public void addUninitializedCollection(CollectionPersister persister, Persistent
 
 	@Override
 	public void addUninitializedDetachedCollection(CollectionPersister persister, PersistentCollection<?> collection) {
-		final CollectionEntry ce = new CollectionEntry( persister, collection.getKey() );
-		addCollection( collection, ce, collection.getKey() );
+		final Object key = collection.getKey();
+		assert key != null;
+		final CollectionEntry ce = new CollectionEntry( persister, key );
+		addCollection( collection, ce, key );
 		if ( session.getLoadQueryInfluencers().effectivelyBatchLoadable( persister ) ) {
 			getBatchFetchQueue().addBatchLoadableCollection( collection, ce );
 		}
@@ -1109,7 +1111,7 @@ private void addCollection(PersistentCollection<?> collection, CollectionPersist
 	@Override
 	public void addInitializedDetachedCollection(CollectionPersister collectionPersister, PersistentCollection<?> collection)
 			throws HibernateException {
-		if ( collection.isUnreferenced() ) {
+		if ( collection.isUnreferenced() || collection.getKey() == null ) {
 			//treat it just like a new collection
 			addCollection( collection, collectionPersister );
 		}
diff --git a/hibernate-core/src/main/java/org/hibernate/engine/spi/CollectionEntry.java b/hibernate-core/src/main/java/org/hibernate/engine/spi/CollectionEntry.java
@@ -125,8 +125,8 @@ public CollectionEntry(PersistentCollection<?> collection, SessionFactoryImpleme
 		ignore = false;
 
 		loadedKey = collection.getKey();
-		loadedPersister = factory.getRuntimeMetamodels().getMappingMetamodel().getCollectionDescriptor( collection.getRole() );
-		this.role = ( loadedPersister == null ? null : loadedPersister.getRole() );
+		role = collection.getRole();
+		loadedPersister = factory.getRuntimeMetamodels().getMappingMetamodel().getCollectionDescriptor( NullnessUtil.castNonNull( role ) );
 
 		snapshot = collection.getStoredSnapshot();
 	}
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
@@ -443,7 +443,8 @@ protected void postFlush(SessionImplementor session) throws HibernateException {
 		persistenceContext.forEachCollectionEntry(
 				(persistentCollection, collectionEntry) -> {
 					collectionEntry.postFlush( persistentCollection );
-					if ( collectionEntry.getLoadedPersister() == null ) {
+					final Object key;
+					if ( collectionEntry.getLoadedPersister() == null || ( key = collectionEntry.getLoadedKey() ) == null ) {
 						//if the collection is dereferenced, unset its session reference and remove from the session cache
 						//iter.remove(); //does not work, since the entrySet is not backed by the set
 						persistentCollection.unsetSession( session );
@@ -453,7 +454,7 @@ protected void postFlush(SessionImplementor session) throws HibernateException {
 						//otherwise recreate the mapping between the collection and its key
 						final CollectionKey collectionKey = new CollectionKey(
 								collectionEntry.getLoadedPersister(),
-								collectionEntry.getLoadedKey()
+								key
 						);
 						persistenceContext.addCollectionByKey( collectionKey, persistentCollection );
 					}
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
@@ -123,22 +123,24 @@ else if ( attributeInterceptor != null
 									entry,
 									session
 							);
-							PersistentCollection<?> collectionInstance = persistenceContext.getCollection(
-									new CollectionKey( persister, key )
-							);
-
-							if ( collectionInstance == null ) {
-								// the collection has not been initialized and new collection values have been assigned,
-								// we need to be sure to delete all the collection elements before inserting the new ones
-								collectionInstance = persister.getCollectionSemantics().instantiateWrapper(
-										key,
-										persister,
-										session
+							if ( key != null ) {
+								PersistentCollection<?> collectionInstance = persistenceContext.getCollection(
+										new CollectionKey( persister, key )
 								);
-								persistenceContext.addUninitializedCollection( persister, collectionInstance, key );
-								final CollectionEntry collectionEntry =
-										persistenceContext.getCollectionEntry( collectionInstance );
-								collectionEntry.setDoremove( true );
+
+								if ( collectionInstance == null ) {
+									// the collection has not been initialized and new collection values have been assigned,
+									// we need to be sure to delete all the collection elements before inserting the new ones
+									collectionInstance = persister.getCollectionSemantics().instantiateWrapper(
+											key,
+											persister,
+											session
+									);
+									persistenceContext.addUninitializedCollection( persister, collectionInstance, key );
+									final CollectionEntry collectionEntry =
+											persistenceContext.getCollectionEntry( collectionInstance );
+									collectionEntry.setDoremove( true );
+								}
 							}
 						}
 					}
diff --git a/hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java b/hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java
@@ -1651,7 +1651,7 @@ void cannotResolveNonNullableTransientDependencies(
 			+ " This is likely due to unsafe use of the session (e.g. used in multiple threads concurrently, updates during entity lifecycle hooks).",
 			id = 479
 	)
-	String collectionNotProcessedByFlush(String role);
+	String collectionNotProcessedByFlush(@Nullable String role);
 
 	@LogMessage(level = WARN)
 	@Message(value = "A ManagedEntity was associated with a stale PersistenceContext. A ManagedEntity may only be associated with one PersistenceContext at a time; %s", id = 480)
diff --git a/hibernate-core/src/main/java/org/hibernate/loader/ast/internal/CollectionLoaderSubSelectFetch.java b/hibernate-core/src/main/java/org/hibernate/loader/ast/internal/CollectionLoaderSubSelectFetch.java
@@ -21,6 +21,7 @@
 import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.engine.spi.SharedSessionContractImplementor;
 import org.hibernate.engine.spi.SubselectFetch;
+import org.hibernate.internal.util.NullnessUtil;
 import org.hibernate.internal.util.collections.CollectionHelper;
 import org.hibernate.loader.ast.spi.CollectionLoader;
 import org.hibernate.metamodel.mapping.PluralAttributeMapping;
@@ -153,7 +154,7 @@ public PersistentCollection<?> load(Object triggerKey, SharedSessionContractImpl
 								persistenceContext,
 								getLoadable().getCollectionDescriptor(),
 								c,
-								c.getKey(),
+								NullnessUtil.castNonNull( c.getKey() ),
 								true
 						);
 					}
diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/AbstractEntityPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/AbstractEntityPersister.java
@@ -301,6 +301,8 @@
 import org.hibernate.type.descriptor.java.spi.JavaTypeRegistry;
 import org.hibernate.type.spi.TypeConfiguration;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import static java.util.Collections.emptyList;
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.emptySet;
@@ -1555,6 +1557,7 @@ public Object initializeLazyProperty(String fieldName, Object entity, SharedSess
 				// see if there is already a collection instance associated with the session
 				// 		NOTE : can this ever happen?
 				final Object key = getCollectionKey( persister, entity, entry, session );
+				assert key != null;
 				PersistentCollection<?> collection = persistenceContext.getCollection( new CollectionKey( persister, key ) );
 				if ( collection == null ) {
 					collection = collectionType.instantiate( session, persister, key );
@@ -1623,7 +1626,7 @@ public Object initializeLazyProperty(String fieldName, Object entity, SharedSess
 
 	}
 
-	public Object getCollectionKey(
+	public @Nullable Object getCollectionKey(
 			CollectionPersister persister,
 			Object owner,
 			EntityEntry ownerEntry,
diff --git a/hibernate-core/src/main/java/org/hibernate/type/CollectionType.java b/hibernate-core/src/main/java/org/hibernate/type/CollectionType.java
@@ -48,6 +48,8 @@
 
 import org.jboss.logging.Logger;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import static org.hibernate.bytecode.enhance.spi.LazyPropertyInitializer.UNFETCHED_PROPERTY;
 import static org.hibernate.proxy.HibernateProxy.extractLazyInitializer;
 
@@ -376,7 +378,7 @@ public ForeignKeyDirection getForeignKeyDirection() {
 	 * @param session The session from which the request is originating.
 	 * @return The collection owner's key
 	 */
-	public Object getKeyOfOwner(Object owner, SharedSessionContractImplementor session) {
+	public @Nullable Object getKeyOfOwner(Object owner, SharedSessionContractImplementor session) {
 		final EntityEntry entityEntry = session.getPersistenceContextInternal().getEntry( owner );
 		if ( entityEntry == null ) {
 			// This just handles a particular case of component
@@ -387,28 +389,10 @@ else if ( foreignKeyPropertyName == null ) {
 			return entityEntry.getId();
 		}
 		else {
-			// TODO: at the point where we are resolving collection references, we don't
-			// know if the uk value has been resolved (depends if it was earlier or
-			// later in the mapping document) - now, we could try and use e.getStatus()
-			// to decide to semiResolve(), trouble is that initializeEntity() reuses
-			// the same array for resolved and hydrated values
 			final Object loadedValue = entityEntry.getLoadedValue( foreignKeyPropertyName );
-			final Object id = loadedValue == null
+			return loadedValue == null
 					? entityEntry.getPersister().getPropertyValue( owner, foreignKeyPropertyName )
 					: loadedValue;
-
-			// NOTE VERY HACKISH WORKAROUND!!
-			// TODO: Fix this so it will work for non-POJO entity mode
-			if ( !keyClass( session ).isInstance( id ) ) {
-				throw new UnsupportedOperationException( "Re-work support for semi-resolve" );
-//				id = keyType.semiResolve(
-//						entityEntry.getLoadedValue( foreignKeyPropertyName ),
-//						session,
-//						owner
-//				);
-			}
-
-			return id;
 		}
 	}
 
diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/collection/NonPkCompositeJoinColumnCollectionTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/collection/NonPkCompositeJoinColumnCollectionTest.java
diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/collection/NonPkJoinColumnCollectionTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/collection/NonPkJoinColumnCollectionTest.java

Original file line number	Diff line number	Diff line change
`@@ -125,8 +125,8 @@ public CollectionEntry(PersistentCollection<?> collection, SessionFactoryImpleme`
`125`	`125`	`ignore = false;`
`126`	`126`
`127`	`127`	`loadedKey = collection.getKey();`
`128`		`- loadedPersister = factory.getRuntimeMetamodels().getMappingMetamodel().getCollectionDescriptor( collection.getRole() );`
`129`		`- this.role = ( loadedPersister == null ? null : loadedPersister.getRole() );`
	`128`	`+ role = collection.getRole();`
	`129`	`+ loadedPersister = factory.getRuntimeMetamodels().getMappingMetamodel().getCollectionDescriptor( NullnessUtil.castNonNull( role ) );`
`130`	`130`
`131`	`131`	`snapshot = collection.getStoredSnapshot();`
`132`	`132`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1651,7 +1651,7 @@ void cannotResolveNonNullableTransientDependencies(`
`1651`	`1651`	`+ " This is likely due to unsafe use of the session (e.g. used in multiple threads concurrently, updates during entity lifecycle hooks).",`
`1652`	`1652`	`id = 479`
`1653`	`1653`	`)`
`1654`		`- String collectionNotProcessedByFlush(String role);`
	`1654`	`+ String collectionNotProcessedByFlush(@Nullable String role);`
`1655`	`1655`
`1656`	`1656`	`@LogMessage(level = WARN)`
`1657`	`1657`	`@Message(value = "A ManagedEntity was associated with a stale PersistenceContext. A ManagedEntity may only be associated with one PersistenceContext at a time; %s", id = 480)`