HHH-10476 : Getting an entity with composite ID containing a detached entity fails if PersistenceContext contains a managed copy

hibernate-core/src/main/java/org/hibernate/loader/plan/build/internal/FetchStyleLoadPlanBuildingAssociationVisitationStrategy.java

@@ -19,6 +19,10 @@ import org.hibernate.loader.plan.spi.EntityReturn;
 import org.hibernate.loader.plan.spi.LoadPlan;
 import org.hibernate.loader.plan.spi.Return;
 import org.hibernate.persister.walking.spi.AssociationAttributeDefinition;
+import org.hibernate.persister.walking.spi.EncapsulatedEntityIdentifierDefinition;
+import org.hibernate.persister.walking.spi.EntityIdentifierDefinition;
+import org.hibernate.persister.walking.spi.NonEncapsulatedEntityIdentifierDefinition;
+import org.hibernate.persister.walking.spi.WalkingException;
 
 import org.jboss.logging.Logger;
 
@@ -37,6 +41,9 @@ public class FetchStyleLoadPlanBuildingAssociationVisitationStrategy
 
 	private Return rootReturn;
 
+	// flag that indicates whether executing handleAssociationAttribute() should be vetoed;
+	private boolean vetoHandleAssociationAttribute;
+
 	/**
 	 * Constructs a FetchStyleLoadPlanBuildingAssociationVisitationStrategy.
 	 *
@@ -71,6 +78,73 @@ public class FetchStyleLoadPlanBuildingAssociationVisitationStrategy
 		this.rootReturn = rootReturn;
 	}
 
+	@Override
+	public void startingEntityIdentifier(EntityIdentifierDefinition identifierDefinition ) {
+		if ( vetoHandleAssociationAttribute ) {
+			throw new WalkingException( "vetoHandleAssociationAttribute is true when starting startingEntityIdentifier()" );
+		}
+		vetoHandleAssociationAttribute = shouldVetoHandleAssociationAttributeInId(
+				rootReturn,
+				identifierDefinition
+		);
+		super.startingEntityIdentifier( identifierDefinition );
+	}
+
+	@Override
+	public void finishingEntityIdentifier(EntityIdentifierDefinition identifierDefinition) {
+		super.finishingEntityIdentifier( identifierDefinition );
+		if ( vetoHandleAssociationAttribute !=
+				shouldVetoHandleAssociationAttributeInId( rootReturn, identifierDefinition ) ) {
+			throw new WalkingException(
+					"vetoHandleAssociationAttribute has unexpected value: " + vetoHandleAssociationAttribute
+			);
+		}
+		vetoHandleAssociationAttribute = false;
+	}
+
+	private static boolean shouldVetoHandleAssociationAttributeInId(
+			Return rootReturn,
+			EntityIdentifierDefinition identifierDefinition) {
+		// only check the identifierDefinition for a root EntityReturn.
+		if ( EntityReturn.class.isInstance( rootReturn ) ) {
+			final EntityIdentifierDefinition rootEntityIdentifierDefinition =
+					( (EntityReturn) rootReturn ).getEntityPersister().getEntityKeyDefinition();
+			if ( rootEntityIdentifierDefinition == identifierDefinition ) {
+				// There are 2 cases where an association in an ID should not be "handled":
+				// 1) a composite, encapsulated ID (e.g., @EmbeddedId). In this case, the ID is provided
+				//    by the application by Session#get or EntityManager#find. Hibernate uses the
+				//    provided ID "as is".
+				// 2) a non-encapsulated ID without an @IdClass. In this case, the application provides
+				//    an instance of the entity with the ID properties initialized. Hibernate uses
+				//    the provided ID properties "as is".
+				// In these two cases, it is important that associations in the ID not be "handled"
+				// (i.e, joined); doing so can result in unexpected results.
+				if ( rootEntityIdentifierDefinition.isEncapsulated() ) {
+					final EncapsulatedEntityIdentifierDefinition encapsulated =
+							(EncapsulatedEntityIdentifierDefinition ) rootEntityIdentifierDefinition;
+					if ( encapsulated.getAttributeDefinition().getType().isComponentType() ) {
+						// This is 1) (@EmbeddedId).
+						return true;
+					}
+				}
+				else {
+					final NonEncapsulatedEntityIdentifierDefinition nonEncapsulated =
+							(NonEncapsulatedEntityIdentifierDefinition) rootEntityIdentifierDefinition;
+					if ( nonEncapsulated.getSeparateIdentifierMappingClass() == null ) {
+						// This is 2) (a non-encapsulated ID without an @IdClass)
+						return true;
+					}
+				}
+			}
+		}
+		return false;
+	}
+
+	@Override
+	protected boolean handleAssociationAttribute(AssociationAttributeDefinition attributeDefinition) {
+		return !vetoHandleAssociationAttribute && super.handleAssociationAttribute( attributeDefinition );
+	}
+
 	@Override
 	public LoadPlan buildLoadPlan() {
 		log.debug( "Building LoadPlan..." );

hibernate-core/src/main/java/org/hibernate/loader/plan/exec/internal/EntityLoadQueryDetails.java

@@ -219,40 +219,16 @@ public class EntityLoadQueryDetails extends AbstractLoadQueryDetails {
 			// if the entity reference we are hydrating is a Return, it is possible that its EntityKey is
 			// supplied by the QueryParameter optional entity information
 			if ( context.shouldUseOptionalEntityInformation() && context.getQueryParameters().getOptionalId() != null ) {
-				EntityKey entityKey = ResultSetProcessorHelper.getOptionalObjectKey(
+				final EntityKey entityKey = context.getSession().generateEntityKey(
-						context.getQueryParameters(),
+						context.getQueryParameters().getOptionalId(),
-						context.getSession()
+						processingState.getEntityReference().getEntityPersister()
 				);
 				processingState.registerIdentifierHydratedForm( entityKey.getIdentifier() );
 				processingState.registerEntityKey( entityKey );
-				final EntityPersister entityPersister = processingState.getEntityReference().getEntityPersister();
-				if ( entityPersister.getIdentifierType().isComponentType()  ) {
-					final CompositeType identifierType = (CompositeType) entityPersister.getIdentifierType();
-					if ( !identifierType.isEmbedded() ) {
-						addKeyManyToOnesToSession(
-								context,
-								identifierType,
-								entityKey.getIdentifier()
-						);
-					}
-				}
 			}
 			return super.readRow( resultSet, context );
 		}
 
-		private void addKeyManyToOnesToSession(ResultSetProcessingContextImpl context, CompositeType componentType, Object component ) {
-			for ( int i = 0 ; i < componentType.getSubtypes().length ; i++ ) {
-				final Type subType = componentType.getSubtypes()[ i ];
-				final Object subValue = componentType.getPropertyValue( component, i, context.getSession() );
-				if ( subType.isEntityType() ) {
-					( (Session) context.getSession() ).buildLockRequest( LockOptions.NONE ).lock( subValue );
-				}
-				else if ( subType.isComponentType() ) {
-					addKeyManyToOnesToSession( context, (CompositeType) subType, subValue  );
-				}
-			}
-		}
-
 		@Override
 		protected Object readLogicalRow(ResultSet resultSet, ResultSetProcessingContextImpl context) throws SQLException {
 			return rootReturnReader.read( resultSet, context );

hibernate-core/src/main/java/org/hibernate/loader/plan/exec/process/internal/EntityReferenceInitializerImpl.java

@@ -201,25 +201,23 @@ public class EntityReferenceInitializerImpl implements EntityReferenceInitialize
 		// Otherwise, we need to load it from the ResultSet...
 
 		// determine which entity instance to use.  Either the supplied one, or instantiate one
-		Object optionalEntityInstance = null;
+		Object entityInstance = null;
-		if ( isReturn && context.shouldUseOptionalEntityInformation() ) {
+		if ( isReturn &&
+				context.shouldUseOptionalEntityInformation() &&
+				context.getQueryParameters().getOptionalObject() != null ) {
 			final EntityKey optionalEntityKey = ResultSetProcessorHelper.getOptionalObjectKey(
 					context.getQueryParameters(),
 					context.getSession()
 			);
-			if ( optionalEntityKey != null ) {
+			if ( optionalEntityKey != null && optionalEntityKey.equals( entityKey ) ) {
-				if ( optionalEntityKey.equals( entityKey ) ) {
+				entityInstance = context.getQueryParameters().getOptionalObject();
-					optionalEntityInstance = context.getQueryParameters().getOptionalObject();
-				}
 			}
 		}
 
 		final String concreteEntityTypeName = getConcreteEntityTypeName( resultSet, context, entityKey );
-
+		if ( entityInstance == null ) {
-		final Object entityInstance = optionalEntityInstance != null
+			entityInstance = context.getSession().instantiate( concreteEntityTypeName, entityKey.getIdentifier() );
-				? optionalEntityInstance
+		}
-				: context.getSession().instantiate( concreteEntityTypeName, entityKey.getIdentifier() );
-
 		processingState.registerEntityInstance( entityInstance );
 
 		// need to hydrate it.

hibernate-core/src/main/java/org/hibernate/persister/walking/internal/EntityIdentifierDefinitionHelper.java

@@ -79,7 +79,9 @@ public final class EntityIdentifierDefinitionHelper {
 
 			@Override
 			public Class getSeparateIdentifierMappingClass() {
-				return entityPersister.getEntityMetamodel().getIdentifierProperty().getType().getReturnedClass();
+				return entityPersister.getEntityMetamodel().getIdentifierProperty().hasIdentifierMapper() ?
+						entityPersister.getEntityMetamodel().getIdentifierProperty().getType().getReturnedClass() :
+						null;
 			}
 
 			@Override

hibernate-core/src/test/java/org/hibernate/test/annotations/cid/Channel.java

@@ -19,4 +19,6 @@ public class Channel {
 	@Id
 	@GeneratedValue
 	public Integer id;
+
+	public String name;
 }

hibernate-core/src/test/java/org/hibernate/test/annotations/cid/CompositeIdTest.java

@@ -9,6 +9,7 @@ package org.hibernate.test.annotations.cid;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
+import java.util.Map;
 
 import org.junit.Test;
 
@@ -18,10 +19,16 @@ import org.hibernate.Session;
 import org.hibernate.Transaction;
 import org.hibernate.criterion.Disjunction;
 import org.hibernate.criterion.Restrictions;
+import org.hibernate.engine.spi.SessionImplementor;
+import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 
 /**
  * test some composite id functionalities
@@ -98,6 +105,49 @@ public class CompositeIdTest extends BaseCoreFunctionalTestCase {
 		s.close();
 	}
 
+	@Test
+	@TestForIssue( jiraKey = "HHH-10476")
+	public void testManyToOneInCompositePkInPC() throws Exception {
+		Session s;
+		Transaction tx;
+		s = openSession();
+		tx = s.beginTransaction();
+		ParentPk ppk = new ParentPk();
+		ppk.setFirstName( "Emmanuel" );
+		ppk.setLastName( "Bernard" );
+		Parent p = new Parent();
+		p.id = ppk;
+		s.persist( p );
+		ChildPk cpk = new ChildPk();
+		cpk.parent = p;
+		cpk.nthChild = 1;
+		Child c = new Child();
+		c.id = cpk;
+		s.persist( c );
+		tx.commit();
+		s.close();
+
+		s = openSession();
+		tx = s.beginTransaction();
+		p = (Parent) s.get( Parent.class, ppk);
+		// p.id should be ppk.
+		assertSame( ppk, p.id );
+		tx.commit();
+		s.close();
+
+		s = openSession();
+		tx = s.beginTransaction();
+		c = (Child) s.get( Child.class, cpk );
+		// c.id should be cpk
+		assertSame( cpk, c.id );
+		// only Child should be in PC (c.id.parent should not be in PC)
+		SessionImplementor sessionImplementor = (SessionImplementor) s;
+		assertTrue( sessionImplementor.getPersistenceContext().isEntryFor( c ) );
+		assertFalse( sessionImplementor.getPersistenceContext().isEntryFor( c.id.parent ) );
+		tx.commit();
+		s.close();
+	}
+
 	/**
 	 * This feature is not supported by the EJB3
 	 * this is an hibernate extension
@@ -200,6 +250,109 @@ public class CompositeIdTest extends BaseCoreFunctionalTestCase {
 		s.close();
 	}
 
+	@Test
+	@TestForIssue(jiraKey = "HHH-10476")
+	public void testManyToOneInCompositeIdClassInPC() throws Exception {
+		Session s = openSession();
+		Transaction tx = s.beginTransaction();
+		Order order = new Order();
+		s.persist( order );
+		Product product = new Product();
+		product.name = "small car";
+		s.persist( product );
+		OrderLine orderLine = new OrderLine();
+		orderLine.order = order;
+		orderLine.product = product;
+		s.persist( orderLine );
+		s.flush();
+		s.clear();
+
+		s.clear();
+		OrderLinePk orderLinePK = new OrderLinePk();
+		orderLinePK.order = orderLine.order;
+		orderLinePK.product = orderLine.product;
+		orderLine = (OrderLine) s.get( OrderLine.class, orderLinePK );
+		assertTrue( orderLine.order != orderLinePK.order );
+		assertTrue( orderLine.product != orderLinePK.product );
+		SessionImplementor sessionImplementor = (SessionImplementor) s;
+		assertTrue( sessionImplementor.getPersistenceContext().isEntryFor( orderLine ) );
+		assertTrue( sessionImplementor.getPersistenceContext().isEntryFor( orderLine.order ) );
+		assertTrue( sessionImplementor.getPersistenceContext().isEntryFor( orderLine.product ) );
+		assertFalse( sessionImplementor.getPersistenceContext().isEntryFor( orderLinePK.order ) );
+		assertFalse( sessionImplementor.getPersistenceContext().isEntryFor( orderLinePK.product ) );
+		tx.rollback();
+		s.close();
+	}
+
+	@Test
+	@TestForIssue(jiraKey = "HHH-10476")
+	public void testGetWithUpdatedDetachedEntityInCompositeID() throws Exception {
+		Session s = openSession();
+		Transaction tx = s.beginTransaction();
+		Channel channel = new Channel();
+		Presenter presenter = new Presenter();
+		presenter.name = "Jane";
+		TvMagazin tvMagazin = new TvMagazin();
+		tvMagazin.id = new TvMagazinPk();
+		tvMagazin.id.channel = channel;
+		tvMagazin.id.presenter = presenter;
+		s.persist( channel );
+		s.persist( presenter );
+		s.persist( tvMagazin );
+		s.flush();
+
+		s.clear();
+		// update channel
+		channel.name = "chnl";
+		TvMagazinPk pkNew = new TvMagazinPk();
+		// set pkNew.channel to the unmerged copy.
+		pkNew.channel = channel;
+		pkNew.presenter = presenter;
+		// the following fails because there is already a managed channel
+		tvMagazin = s.get( TvMagazin.class, pkNew );
+		channel = s.get( Channel.class, channel.id );
+		assertNull( channel.name );
+		s.flush();
+		s.clear();
+
+		// make sure that channel.name is still null
+		channel = s.get( Channel.class, channel.id );
+		assertNull( channel.name );
+
+		tx.rollback();
+		s.close();
+	}
+
+	@Test
+	@TestForIssue(jiraKey = "HHH-10476")
+	public void testGetWithDetachedEntityInCompositeIDWithManagedCopy() throws Exception {
+		Session s = openSession();
+		Transaction tx = s.beginTransaction();
+		Channel channel = new Channel();
+		Presenter presenter = new Presenter();
+		presenter.name = "Jane";
+		TvMagazin tvMagazin = new TvMagazin();
+		tvMagazin.id = new TvMagazinPk();
+		tvMagazin.id.channel = channel;
+		tvMagazin.id.presenter = presenter;
+		s.persist( channel );
+		s.persist( presenter );
+		s.persist( tvMagazin );
+		s.flush();
+
+		s.clear();
+		// merge channel to put channel back in PersistenceContext
+		s.merge( channel );
+		TvMagazinPk pkNew = new TvMagazinPk();
+		// set pkNew.channel to the unmerged copy.
+		pkNew.channel = channel;
+		pkNew.presenter = presenter;
+		// the following fails because there is already a managed channel
+		tvMagazin = s.get( TvMagazin.class, pkNew );
+		tx.rollback();
+		s.close();
+	}
+
 	@Test
 	public void testSecondaryTableWithCompositeId() throws Exception {
 		Session s = openSession();

hibernate-core/src/test/java/org/hibernate/test/annotations/derivedidentities/bidirectional/CompositeDerivedIdentityTest.java

@@ -9,9 +9,15 @@ package org.hibernate.test.annotations.derivedidentities.bidirectional;
 import org.junit.Test;
 
 import org.hibernate.Session;
+import org.hibernate.engine.spi.SessionImplementor;
+import org.hibernate.internal.SessionImpl;
+import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 
 public class CompositeDerivedIdentityTest extends BaseCoreFunctionalTestCase {
 	@Override
@@ -54,4 +60,41 @@ public class CompositeDerivedIdentityTest extends BaseCoreFunctionalTestCase {
 		session.getTransaction().commit();
 		session.close();
 	}
+
+	@Test
+	@TestForIssue( jiraKey = "HHH-10476")
+	public void testBidirectonalKeyManyToOneId() {
+		Product product = new Product();
+		product.setName( "Product 1" );
+
+		Session session = openSession();
+		session.beginTransaction();
+		session.save( product );
+		session.getTransaction().commit();
+		session.close();
+
+		Order order = new Order();
+		order.setName( "Order 1" );
+		order.addLineItem( product, 2 );
+
+		session = openSession();
+		session.beginTransaction();
+		session.save( order );
+		session.getTransaction().commit();
+		session.close();
+
+		session = openSession();
+		session.beginTransaction();
+		OrderLine orderLine = order.getLineItems().iterator().next();
+		orderLine.setAmount( 5 );
+		OrderLine orderLineGotten = session.get( OrderLine.class, orderLine );
+		assertSame( orderLineGotten, orderLine );
+		assertEquals( Integer.valueOf( 2 ), orderLineGotten.getAmount() );
+		SessionImplementor si = (SessionImplementor) session;
+		assertTrue( si.getPersistenceContext().isEntryFor( orderLineGotten ) );
+		assertFalse( si.getPersistenceContext().isEntryFor( orderLineGotten.getOrder() ) );
+		assertFalse( si.getPersistenceContext().isEntryFor( orderLineGotten.getProduct() ) );
+		session.getTransaction().commit();
+		session.close();
+	}
 }

hibernate-core/src/test/java/org/hibernate/test/annotations/derivedidentities/bidirectional/OneToOneWithDerivedIdentityTest.java

@@ -7,12 +7,16 @@
 package org.hibernate.test.annotations.derivedidentities.bidirectional;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 
 import java.util.List;
 
 import org.hibernate.Query;
 import org.hibernate.Session;
+import org.hibernate.engine.spi.SessionImplementor;
 import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
 import org.junit.Test;
@@ -43,6 +47,36 @@ public class OneToOneWithDerivedIdentityTest extends BaseCoreFunctionalTestCase
 		s.close();
 	}
 
+	@Test
+	@TestForIssue( jiraKey = "HHH-10476")
+	public void testInsertFooAndBarWithDerivedIdPC() {
+		Session s = openSession();
+		s.beginTransaction();
+		Bar bar = new Bar();
+		bar.setDetails( "Some details" );
+		Foo foo = new Foo();
+		foo.setBar( bar );
+		bar.setFoo( foo );
+		s.persist( foo );
+		s.flush();
+		assertNotNull( foo.getId() );
+		assertEquals( foo.getId(), bar.getFoo().getId() );
+
+		s.clear();
+		Bar barWithFoo = new Bar();
+		barWithFoo.setFoo( foo );
+		barWithFoo.setDetails( "wrong details" );
+		bar = (Bar) s.get( Bar.class, barWithFoo );
+		assertSame( bar, barWithFoo );
+		assertEquals( "Some details", bar.getDetails() );
+		SessionImplementor si = (SessionImplementor) s;
+		assertTrue( si.getPersistenceContext().isEntryFor( bar ) );
+		assertFalse( si.getPersistenceContext().isEntryFor( bar.getFoo() ) );
+
+		s.getTransaction().rollback();
+		s.close();
+	}
+
 	@Test
 	@TestForIssue(jiraKey = "HHH-6813")
 	public void testSelectWithDerivedId() {

hibernate-core/src/test/java/org/hibernate/test/loadplans/plans/LoadPlanStructureAssertionTest.java

@@ -131,30 +131,10 @@ public class LoadPlanStructureAssertionTest extends BaseUnitTestCase {
 			final EntityReturn cardFieldReturn = assertTyping( EntityReturn.class, loadPlan.getReturns().get( 0 ) );
 			assertEquals( 0, cardFieldReturn.getFetches().length );
 
-			// CardField defines a composite pk with 2 fetches : Card and Key (the id description acts as the composite)
+			// CardField defines a composite pk with 2 many-to-ones : Card and Key (the id description acts as the composite);
-			assertTrue( cardFieldReturn.getIdentifierDescription().hasFetches() );
+			// because it is an @EmbeddedId, the ID provided by the application is used "as is"
-			final FetchSource cardFieldIdAsFetchSource = assertTyping( FetchSource.class, cardFieldReturn.getIdentifierDescription() );
+			// and fetches are not included in the load plan.
-			assertEquals( 2, cardFieldIdAsFetchSource.getFetches().length );
+			assertFalse( cardFieldReturn.getIdentifierDescription().hasFetches() );
-
-			// First the key-many-to-one to Card...
-			final EntityFetch cardFieldIdCardFetch = assertTyping(
-					EntityFetch.class,
-					cardFieldIdAsFetchSource.getFetches()[0]
-			);
-			assertFalse( cardFieldIdCardFetch.getIdentifierDescription().hasFetches() );
-			// i think this one might be a mistake; i think the collection reader still needs to be registered.  Its zero
-			// because the inverse of the key-many-to-one already had a registered AssociationKey and so saw the
-			// CollectionFetch as a circularity (I think)
-			assertEquals( 0, cardFieldIdCardFetch.getFetches().length );
-
-			// then the Key..
-			final EntityFetch cardFieldIdKeyFetch = assertTyping(
-					EntityFetch.class,
-					cardFieldIdAsFetchSource.getFetches()[1]
-			);
-			assertFalse( cardFieldIdKeyFetch.getIdentifierDescription().hasFetches() );
-			assertEquals( 0, cardFieldIdKeyFetch.getFetches().length );
-
 
 			// we need the readers ordered in a certain manner.  Here specifically: Fetch(Card), Fetch(Key), Return(CardField)
 			//