HHH-18229 Handle null owner key for collections

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

@@ -215,19 +215,16 @@ public abstract class AbstractEntityInsertAction extends EntityAction {
 			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 );
+			}
 		}
 	}
 

hibernate-core/src/main/java/org/hibernate/collection/spi/AbstractPersistentCollection.java

@@ -40,6 +40,8 @@ import org.hibernate.type.BasicType;
 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 @@ public abstract class AbstractPersistentCollection<E> implements Serializable, P
 	}
 
 	@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 abstract class AbstractPersistentCollection<E> implements Serializable, P
 	}
 
 	@Override
-	public final Serializable getStoredSnapshot() {
+	public final @Nullable Serializable getStoredSnapshot() {
 		return storedSnapshot;
 	}
 
@@ -1354,7 +1356,7 @@ public abstract class AbstractPersistentCollection<E> implements Serializable, P
 	}
 
 	@Override
-	public Object getOwner() {
+	public @Nullable Object getOwner() {
 		return owner;
 	}
 

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 @@ public interface PersistentCollection<E> extends LazyInitializable {
 	 *
 	 * @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 @@ public interface PersistentCollection<E> extends LazyInitializable {
 	 *
 	 * @return The internally stored snapshot state
 	 */
-	Serializable getStoredSnapshot();
+	@Nullable Serializable getStoredSnapshot();
 
 	/**
 	 * Mark the collection as dirty

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

@@ -1050,8 +1050,10 @@ public class StatefulPersistenceContext implements PersistenceContext {
 
 	@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 @@ public class StatefulPersistenceContext implements PersistenceContext {
 	@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 );
 		}

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

@@ -125,8 +125,8 @@ public final class CollectionEntry implements Serializable {
 		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();
 	}

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

@@ -443,7 +443,8 @@ public abstract class AbstractFlushingEventListener implements JpaBootstrapSensi
 		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 @@ public abstract class AbstractFlushingEventListener implements JpaBootstrapSensi
 						//otherwise recreate the mapping between the collection and its key
 						final CollectionKey collectionKey = new CollectionKey(
 								collectionEntry.getLoadedPersister(),
-								collectionEntry.getLoadedKey()
+								key
 						);
 						persistenceContext.addCollectionByKey( collectionKey, persistentCollection );
 					}

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

@@ -123,22 +123,24 @@ public class WrapVisitor extends ProxyVisitor {
 									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 );
+								}
 							}
 						}
 					}

hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java

@@ -1651,7 +1651,7 @@ public interface CoreMessageLogger extends BasicLogger {
 			+ " 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)

hibernate-core/src/main/java/org/hibernate/loader/ast/internal/CollectionLoaderSubSelectFetch.java

@@ -21,6 +21,7 @@ import org.hibernate.engine.spi.PersistenceContext;
 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 class CollectionLoaderSubSelectFetch implements CollectionLoader {
 								persistenceContext,
 								getLoadable().getCollectionDescriptor(),
 								c,
-								c.getKey(),
+								NullnessUtil.castNonNull( c.getKey() ),
 								true
 						);
 					}

hibernate-core/src/main/java/org/hibernate/persister/entity/AbstractEntityPersister.java

@@ -301,6 +301,8 @@ import org.hibernate.type.descriptor.java.MutabilityPlan;
 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 abstract class AbstractEntityPersister
 				// 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 abstract class AbstractEntityPersister
 
 	}
 
-	public Object getCollectionKey(
+	public @Nullable Object getCollectionKey(
 			CollectionPersister persister,
 			Object owner,
 			EntityEntry ownerEntry,

hibernate-core/src/main/java/org/hibernate/type/CollectionType.java

@@ -48,6 +48,8 @@ import org.hibernate.sql.results.graph.collection.LoadingCollectionEntry;
 
 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 abstract class CollectionType extends AbstractType implements Association
 	 * @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 @@ public abstract class CollectionType extends AbstractType implements Association
 			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;
 		}
 	}
 

hibernate-core/src/test/java/org/hibernate/orm/test/collection/NonPkCompositeJoinColumnCollectionTest.java

@@ -0,0 +1,147 @@
+package org.hibernate.orm.test.collection;
+
+import java.util.ArrayList;
+import java.util.Collection;
+
+import org.hibernate.testing.orm.junit.DomainModel;
+import org.hibernate.testing.orm.junit.SessionFactory;
+import org.hibernate.testing.orm.junit.SessionFactoryScope;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+import jakarta.persistence.Column;
+import jakarta.persistence.Entity;
+import jakarta.persistence.GeneratedValue;
+import jakarta.persistence.Id;
+import jakarta.persistence.JoinColumn;
+import jakarta.persistence.OneToMany;
+import jakarta.persistence.Table;
+
+@DomainModel(
+		annotatedClasses = {
+				NonPkCompositeJoinColumnCollectionTest.Order.class,
+				NonPkCompositeJoinColumnCollectionTest.Item.class,
+		}
+)
+@SessionFactory
+public class NonPkCompositeJoinColumnCollectionTest {
+
+	@AfterEach
+	public void setUp(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					session.createMutationQuery( "delete from Item" ).executeUpdate();
+					session.createMutationQuery( "delete from Order" ).executeUpdate();
+				}
+		);
+	}
+
+	@Test
+	public void testCollectionInsertWithNullCollecitonRef(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					session.persist( new Order( null ) );
+				}
+		);
+	}
+
+	@Test
+	public void testCollectionInsertEmptyCollection(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					session.persist( new Order( "O1" ) );
+				}
+		);
+	}
+
+	@Test
+	public void testCollectionInsert(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					Order order = new Order( "O1" );
+					Item item = new Item( "Item 1" );
+					order.addItem( item );
+					session.persist( item );
+					session.persist( order );
+				}
+		);
+	}
+
+	@Entity(name = "Order")
+	@Table(name = "ORDER_TABLE")
+	public static class Order {
+		@Id
+		@GeneratedValue
+		public Integer id;
+
+		@Column(name = "uk1")
+		String uk1;
+		@Column(name = "uk2")
+		String uk2;
+
+		@OneToMany
+		@JoinColumn(name = "fk1", referencedColumnName = "uk1", insertable = false, updatable = false)
+		@JoinColumn(name = "fk2", referencedColumnName = "uk2", insertable = false, updatable = false)
+		Collection<Item> items = new ArrayList<>();
+
+		public Order() {
+		}
+
+		public Order(String uk) {
+			this.uk1 = uk;
+			this.uk2 = uk;
+		}
+
+		public Integer getId() {
+			return id;
+		}
+
+		public String getUk1() {
+			return uk1;
+		}
+
+		public String getUk2() {
+			return uk2;
+		}
+
+		public Collection<Item> getItems() {
+			return items;
+		}
+
+		public void addItem(Item item) {
+			items.add( item );
+			item.fk1 = uk1;
+			item.fk2 = uk2;
+		}
+	}
+
+	@Entity(name = "Item")
+	@Table(name = "ITEM_TABLE")
+	public static class Item {
+		@Id
+		@GeneratedValue
+		public Integer id;
+
+		public String description;
+
+		@Column(name = "fk1")
+		String fk1;
+		@Column(name = "fk2")
+		String fk2;
+
+		public Item() {
+		}
+
+		public Item(String description) {
+			this.description = description;
+		}
+
+		public Integer getId() {
+			return id;
+		}
+
+		public String getDescription() {
+			return description;
+		}
+	}
+}

hibernate-core/src/test/java/org/hibernate/orm/test/collection/NonPkJoinColumnCollectionTest.java

@@ -0,0 +1,195 @@
+package org.hibernate.orm.test.collection;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import org.hibernate.testing.orm.junit.DomainModel;
+import org.hibernate.testing.orm.junit.SessionFactory;
+import org.hibernate.testing.orm.junit.SessionFactoryScope;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+import jakarta.persistence.Column;
+import jakarta.persistence.Entity;
+import jakarta.persistence.GeneratedValue;
+import jakarta.persistence.Id;
+import jakarta.persistence.JoinColumn;
+import jakarta.persistence.OneToMany;
+import jakarta.persistence.Table;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+@DomainModel(
+		annotatedClasses = {
+				NonPkJoinColumnCollectionTest.Order.class,
+				NonPkJoinColumnCollectionTest.Item.class,
+		}
+)
+@SessionFactory
+public class NonPkJoinColumnCollectionTest {
+
+	@AfterEach
+	public void setUp(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					session.createMutationQuery( "delete from Item" ).executeUpdate();
+					session.createMutationQuery( "delete from Order" ).executeUpdate();
+				}
+		);
+	}
+
+	@Test
+	public void testInsertEmptyCollectionWithNullCollecitonRef(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					// Persisting an entity with an empty collection and null owning key
+					Order order = new Order( null );
+					session.persist( order );
+					session.flush();
+					session.clear();
+
+					// Ensure merging a detached object works
+					order.text = "Abc";
+					session.merge( order );
+					session.flush();
+					session.clear();
+
+					Order order1 = session.find( Order.class, order.id );
+					assertThat( order1.text ).isEqualTo( "Abc" );
+					assertThat( order1.items ).isNull();
+				}
+		);
+	}
+
+	@Test
+	public void testInsertCollectionWithNullCollecitonRef(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					// Persisting an entity with a non-empty collection though the owning key is null
+					// It's somewhat debatable whether this should work by simply ignoring the collection
+					// or throw an error that indicates the owning key is missing
+					Order order = new Order( null );
+					Item item = new Item( "Abc" );
+					order.addItem( item );
+					session.persist( item );
+					session.persist( order );
+					session.flush();
+					session.clear();
+
+					// Ensure merging a detached object works
+					order.text = "Abc";
+					session.merge( order );
+					session.flush();
+					session.clear();
+
+					// Also ensure merging a detached object with a new collection works
+					order.items = new ArrayList<>();
+					order.addItem( item );
+					session.merge( order );
+					session.flush();
+					session.clear();
+
+					Order order1 = session.find( Order.class, order.id );
+					assertThat( order1.text ).isEqualTo( "Abc" );
+					assertThat( order1.items ).isNull();
+				}
+		);
+	}
+
+	@Test
+	public void testInsertCollection(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					// Persisting an entity with a collection and non-null owning key
+					Order order = new Order( "some_ref" );
+					Item item = new Item( "Abc" );
+					order.addItem( item );
+					session.persist( item );
+					session.persist( order );
+					session.flush();
+					session.clear();
+
+					// Ensure merging a detached object works
+					order.text = "Abc";
+					session.merge( order );
+					session.flush();
+					session.clear();
+
+					Order order1 = session.find( Order.class, order.id );
+					assertThat( order1.text ).isEqualTo( "Abc" );
+					assertThat( order1.items.size() ).isEqualTo( 1 );
+					assertThat( order1.items.iterator().next().id ).isEqualTo( item.id );
+				}
+		);
+	}
+
+	@Entity(name = "Order")
+	@Table(name = "ORDER_TABLE")
+	public static class Order {
+		@Id
+		@GeneratedValue
+		public Integer id;
+
+		String text;
+
+		@Column(name = "c_ref")
+		String cRef;
+
+		@OneToMany
+		@JoinColumn(name = "p_ref", referencedColumnName = "c_ref", insertable = false, updatable = false)
+		Collection<Item> items = new ArrayList<>();
+
+		public Order() {
+		}
+
+		public Order(String cRef) {
+			this.cRef = cRef;
+		}
+
+		public Integer getId() {
+			return id;
+		}
+
+		public String getcRef() {
+			return cRef;
+		}
+
+		public Collection<Item> getItems() {
+			return items;
+		}
+
+		public void addItem(Item item) {
+			items.add( item );
+			item.pRef = cRef;
+		}
+	}
+
+	@Entity(name = "Item")
+	@Table(name = "ITEM_TABLE")
+	public static class Item {
+		@Id
+		@GeneratedValue
+		public Integer id;
+
+		public String description;
+
+		@Column(name = "p_ref")
+		String pRef;
+
+		public Item() {
+		}
+
+		public Item(String description) {
+			this.description = description;
+		}
+
+		public Integer getId() {
+			return id;
+		}
+
+		public String getDescription() {
+			return description;
+		}
+	}
+}