HHH-16195 Restore logic for declared non-identifier Component properties that use generics

Also remove some duplicate logic for setting declared properties on superclass and add some test cases with embeddables and generics

hibernate-core/src/main/java/org/hibernate/boot/model/internal/ClassPropertyHolder.java

@@ -7,10 +7,13 @@
 package org.hibernate.boot.model.internal;
 
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.Map;
+import java.util.function.Consumer;
 
 import org.hibernate.AssertionFailure;
 import org.hibernate.MappingException;
+import org.hibernate.PropertyNotFoundException;
 import org.hibernate.annotations.common.reflection.XClass;
 import org.hibernate.annotations.common.reflection.XProperty;
 import org.hibernate.boot.spi.MetadataBuildingContext;
@@ -249,46 +252,54 @@ public class ClassPropertyHolder extends AbstractPropertyHolder {
 
 	private void addPropertyToMappedSuperclass(Property prop, XClass declaringClass) {
 		final Class<?> type = getContext().getBootstrapContext().getReflectionManager().toClass( declaringClass );
-		MappedSuperclass superclass = getContext().getMetadataCollector().getMappedSuperclass( type );
+		final MappedSuperclass superclass = getContext().getMetadataCollector().getMappedSuperclass( type );
+		prepareActualPropertyForSuperclass( prop, type, true, getContext(), superclass::addDeclaredProperty );
+	}
+
+	static void prepareActualPropertyForSuperclass(
+			Property prop,
+			Class<?> type,
+			boolean allowCollections,
+			MetadataBuildingContext context,
+			Consumer<Property> propertyConsumer) {
 		if ( type.getTypeParameters().length == 0 ) {
-			superclass.addDeclaredProperty( prop );
+			propertyConsumer.accept( prop );
 		}
 		else {
 			// If the type has type parameters, we have to look up the XClass and actual property again
 			// because the given XClass has a TypeEnvironment based on the type variable assignments of a subclass
 			// and that might result in a wrong property type being used for a property which uses a type variable
-			final XClass actualDeclaringClass = getContext().getBootstrapContext().getReflectionManager().toXClass( type );
+			final XClass actualDeclaringClass = context.getBootstrapContext().getReflectionManager().toXClass( type );
 			for ( XProperty declaredProperty : actualDeclaringClass.getDeclaredProperties( prop.getPropertyAccessorName() ) ) {
 				if ( prop.getName().equals( declaredProperty.getName() ) ) {
 					final PropertyData inferredData = new PropertyInferredData(
 							actualDeclaringClass,
 							declaredProperty,
 							null,
-							getContext().getBootstrapContext().getReflectionManager()
+							context.getBootstrapContext().getReflectionManager()
 					);
 					final Value originalValue = prop.getValue();
 					if ( originalValue instanceof SimpleValue ) {
 						// Avoid copying when the property doesn't depend on a type variable
 						if ( inferredData.getTypeName().equals( getTypeName( prop ) ) ) {
-							superclass.addDeclaredProperty( prop );
+							propertyConsumer.accept( prop );
 							return;
 						}
 					}
-					if ( originalValue instanceof Component ) {
-						superclass.addDeclaredProperty( prop );
-						return;
-					}
 					// If the property depends on a type variable, we have to copy it and the Value
 					final Property actualProperty = prop.copy();
 					actualProperty.setReturnedClassName( inferredData.getTypeName() );
 					final Value value = actualProperty.getValue().copy();
 					if ( value instanceof Collection ) {
+						if ( !allowCollections ) {
+							throw new AssertionFailure( "Collections are not allowed as identifier properties" );
+						}
 						final Collection collection = (Collection) value;
 						// The owner is a MappedSuperclass which is not a PersistentClass, so set it to null
 //						collection.setOwner( null );
 						collection.setRole( type.getName() + "." + prop.getName() );
 						// To copy the element and key values, we need to defer setting the type name until the CollectionBinder ran
-						getContext().getMetadataCollector().addSecondPass(
+						context.getMetadataCollector().addSecondPass(
 								new SecondPass() {
 									@Override
 									public void doSecondPass(Map persistentClasses) throws MappingException {
@@ -308,33 +319,33 @@ public class ClassPropertyHolder extends AbstractPropertyHolder {
 					else {
 						setTypeName( value, inferredData.getTypeName() );
 					}
-//					if ( value instanceof Component ) {
+					if ( value instanceof Component ) {
-//						Component component = ( (Component) value );
+						final Component component = ( (Component) value );
-//						Iterator<Property> propertyIterator = component.getPropertyIterator();
+						final Iterator<Property> propertyIterator = component.getPropertyIterator();
-//						while ( propertyIterator.hasNext() ) {
+						while ( propertyIterator.hasNext() ) {
-//							Property property = propertyIterator.next();
+							Property property = propertyIterator.next();
-//							try {
+							try {
-//								property.getGetter( component.getComponentClass() );
+								property.getGetter( component.getComponentClass() );
-//							}
+							}
-//							catch (PropertyNotFoundException e) {
+							catch (PropertyNotFoundException e) {
-//								propertyIterator.remove();
+								propertyIterator.remove();
-//							}
+							}
-//						}
+						}
-//					}
+					}
 					actualProperty.setValue( value );
-					superclass.addDeclaredProperty( actualProperty );
+					propertyConsumer.accept( actualProperty );
 					break;
 				}
 			}
 		}
 	}
 
-	static String getTypeName(Property property) {
+	private static String getTypeName(Property property) {
 		final String typeName = getTypeName( property.getValue() );
 		return typeName != null ? typeName : property.getReturnedClassName();
 	}
 
-	static String getTypeName(Value value) {
+	private static String getTypeName(Value value) {
 		if ( value instanceof Component ) {
 			final Component component = (Component) value;
 			final String typeName = component.getTypeName();
@@ -346,7 +357,7 @@ public class ClassPropertyHolder extends AbstractPropertyHolder {
 		return ( (SimpleValue) value ).getTypeName();
 	}
 
-	static void setTypeName(Value value, String typeName) {
+	private static void setTypeName(Value value, String typeName) {
 		if ( value instanceof ToOne ) {
 			final ToOne toOne = (ToOne) value;
 			toOne.setReferencedEntityName( typeName );
@@ -354,7 +365,11 @@ public class ClassPropertyHolder extends AbstractPropertyHolder {
 		}
 		else if ( value instanceof Component ) {
 			final Component component = (Component) value;
+			// Avoid setting component class name to java.lang.Object
+			// for embeddable types with generic type parameters
+			if ( !typeName.equals( Object.class.getName() ) ) {
 				component.setComponentClassName( typeName );
+			}
 			if ( component.getTypeName() != null ) {
 				component.setTypeName( typeName );
 			}

hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java

@@ -343,58 +343,14 @@ public class PropertyBinder {
 			rootClass.setDeclaredIdentifierProperty( prop );
 			return;
 		}
-		// If the type has type parameters, we have to set the declared identifier property on the rootClass
-		// to be able to retrieve it with the correct type based on type variable assignment in the subclass
 		final Class<?> type = buildingContext.getBootstrapContext().getReflectionManager().toClass( declaringClass );
-		if ( type.getTypeParameters().length == 0 ) {
+		ClassPropertyHolder.prepareActualPropertyForSuperclass(
-			superclass.setDeclaredIdentifierProperty( prop );
+				prop,
-		}
+				type,
-		else {
+				false,
-			// If the type has type parameters, we have to look up the XClass and actual property again
+				buildingContext,
-			// because the given XClass has a TypeEnvironment based on the type variable assignments of a subclass
+				superclass::setDeclaredIdentifierProperty
-			// and that might result in a wrong property type being used for a property which uses a type variable
-			final XClass actualDeclaringClass = buildingContext.getBootstrapContext().getReflectionManager().toXClass( type );
-			for ( XProperty declaredProperty : actualDeclaringClass.getDeclaredProperties( prop.getPropertyAccessorName() ) ) {
-				if ( prop.getName().equals( declaredProperty.getName() ) ) {
-					final PropertyData inferredData = new PropertyInferredData(
-							actualDeclaringClass,
-							declaredProperty,
-							null,
-							buildingContext.getBootstrapContext().getReflectionManager()
 		);
-					final Value originalValue = prop.getValue();
-					if ( originalValue instanceof SimpleValue ) {
-						// Avoid copying when the property doesn't depend on a type variable
-						if ( inferredData.getTypeName().equals( ClassPropertyHolder.getTypeName( prop ) ) ) {
-							superclass.setDeclaredIdentifierProperty( prop );
-							return;
-						}
-					}
-					// If the property depends on a type variable, we have to copy it and the Value
-					final Property actualProperty = prop.copy();
-					actualProperty.setReturnedClassName( inferredData.getTypeName() );
-					final Value value = actualProperty.getValue().copy();
-					assert !(value instanceof Collection);
-					ClassPropertyHolder.setTypeName( value, inferredData.getTypeName() );
-					if ( value instanceof Component ) {
-						Component component = ( (Component) value );
-						Iterator<Property> propertyIterator = component.getPropertyIterator();
-						while ( propertyIterator.hasNext() ) {
-							Property property = propertyIterator.next();
-							try {
-								property.getGetter( component.getComponentClass() );
-							}
-							catch (PropertyNotFoundException e) {
-								propertyIterator.remove();
-							}
-						}
-					}
-					actualProperty.setValue( value );
-					superclass.setDeclaredIdentifierProperty( actualProperty );
-					break;
-				}
-			}
-		}
 	}
 
 	private Component getOrCreateCompositeId(RootClass rootClass) {

hibernate-core/src/test/java/org/hibernate/orm/test/annotations/embeddables/EmbeddableAndMappedSuperClassWithGenericsTest.java

@@ -12,7 +12,8 @@ import org.hibernate.testing.TestForIssue;
 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.BeforeEach;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
 import jakarta.persistence.Column;
@@ -34,17 +35,19 @@ import static org.assertj.core.api.Assertions.assertThat;
 public class EmbeddableAndMappedSuperClassWithGenericsTest {
 
 	private final static long POPULAR_BOOK_ID = 1l;
+	private final static String POPULAR_BOOK_CODE = "POP";
 	private final static long RARE_BOOK_ID = 2l;
+	private final static Integer RARE_BOOK_CODE = 123;
 
-	@BeforeEach
+	@BeforeAll
 	public void setUp(SessionFactoryScope scope) {
 		scope.inTransaction(
 				session -> {
 					Edition popularEdition = new Edition( "Popular" );
-					PopularBook popularBook = new PopularBook( POPULAR_BOOK_ID, popularEdition, "POP" );
+					PopularBook popularBook = new PopularBook( POPULAR_BOOK_ID, popularEdition, POPULAR_BOOK_CODE );
 
 					Edition rareEdition = new Edition( "Rare" );
-					RareBook rareBook = new RareBook( RARE_BOOK_ID, rareEdition, 123 );
+					RareBook rareBook = new RareBook( RARE_BOOK_ID, rareEdition, RARE_BOOK_CODE );
 
 					session.persist( popularBook );
 					session.persist( rareBook );
@@ -52,6 +55,14 @@ public class EmbeddableAndMappedSuperClassWithGenericsTest {
 		);
 	}
 
+	@AfterAll
+	public void tearDown(SessionFactoryScope scope) {
+		scope.inTransaction( session -> {
+			session.createMutationQuery( "delete from PopularBook" ).executeUpdate();
+			session.createMutationQuery( "delete from RareBook" ).executeUpdate();
+		} );
+	}
+
 	@Test
 	public void test(SessionFactoryScope scope) {
 		scope.inTransaction(
@@ -64,7 +75,7 @@ public class EmbeddableAndMappedSuperClassWithGenericsTest {
 					assertThat( rareBookCodes.size() ).isEqualTo( 1 );
 
 					Integer code = rareBookCodes.get( 0 );
-					assertThat( code ).isEqualTo( 123 );
+					assertThat( code ).isEqualTo( RARE_BOOK_CODE );
 				}
 		);
 
@@ -78,7 +89,38 @@ public class EmbeddableAndMappedSuperClassWithGenericsTest {
 					assertThat( populareBookCodes.size() ).isEqualTo( 1 );
 
 					String code = populareBookCodes.get( 0 );
-					assertThat( code ).isEqualTo( "POP" );
+					assertThat( code ).isEqualTo( POPULAR_BOOK_CODE );
+				}
+		);
+	}
+
+	@Test
+	public void testQueryParam(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					List<Long> rareBookIds = session.createQuery(
+							"select id from RareBook b where b.code = :code",
+							Long.class
+					).setParameter( "code", RARE_BOOK_CODE ).list();
+
+					assertThat( rareBookIds ).hasSize( 1 );
+
+					Long id = rareBookIds.get( 0 );
+					assertThat( id ).isEqualTo( RARE_BOOK_ID );
+				}
+		);
+
+		scope.inTransaction(
+				session -> {
+					List<Long> populareBookIds = session.createQuery(
+							"select id from PopularBook b where b.code = :code",
+							Long.class
+					).setParameter( "code", POPULAR_BOOK_CODE ).list();
+
+					assertThat( populareBookIds ).hasSize( 1 );
+
+					Long id = populareBookIds.get( 0 );
+					assertThat( id ).isEqualTo( POPULAR_BOOK_ID );
 				}
 		);
 	}

hibernate-core/src/test/java/org/hibernate/orm/test/annotations/embeddables/EmbeddableWithGenericAndMappedSuperClassTest.java

@@ -125,6 +125,65 @@ public class EmbeddableWithGenericAndMappedSuperClassTest {
 		);
 	}
 
+	@Test
+	public void testQueryParam(SessionFactoryScope scope) {
+		scope.inTransaction(
+				session -> {
+					List<Long> rareBookIds = session.createQuery(
+							"select id from RareBook b where b.edition.code = :code",
+							Long.class
+					).setParameter( "code", RARE_BOOK_CODE ).list();
+
+					assertThat( rareBookIds ).hasSize( 1 );
+
+					Long id = rareBookIds.get( 0 );
+					assertThat( id ).isEqualTo( RARE_BOOK_ID );
+				}
+		);
+
+		scope.inTransaction(
+				session -> {
+					List<Long> rareBookIds = session.createQuery(
+							"select id from RareBook b where b.edition = :edition",
+							Long.class
+					).setParameter( "edition", new Edition<>( "Rare", RARE_BOOK_CODE ) ).list();
+
+					assertThat( rareBookIds ).hasSize( 1 );
+
+					Long id = rareBookIds.get( 0 );
+					assertThat( id ).isEqualTo( RARE_BOOK_ID );
+				}
+		);
+
+		scope.inTransaction(
+				session -> {
+					List<Long> popularBookIds = session.createQuery(
+							"select id from PopularBook b where b.edition.code = :code",
+							Long.class
+					).setParameter( "code", POPULAR_BOOK_CODE ).list();
+
+					assertThat( popularBookIds ).hasSize( 1 );
+
+					Long id = popularBookIds.get( 0 );
+					assertThat( id ).isEqualTo( POPULAR_BOOK_ID );
+				}
+		);
+
+		scope.inTransaction(
+				session -> {
+					List<Long> popularBookIds = session.createQuery(
+							"select id from PopularBook b where b.edition = :edition",
+							Long.class
+					).setParameter( "edition", new Edition<>( "Popular", POPULAR_BOOK_CODE ) ).list();
+
+					assertThat( popularBookIds ).hasSize( 1 );
+
+					Long id = popularBookIds.get( 0 );
+					assertThat( id ).isEqualTo( POPULAR_BOOK_ID );
+				}
+		);
+	}
+
 	@Test
 	@TestForIssue(jiraKey = "HHH-4299")
 	public void testGenericEmbeddedAttribute(SessionFactoryScope scope) {

hibernate-core/src/test/java/org/hibernate/orm/test/compositefk/OneToOneEmbeddedIdWithGenericAttributeTest.java

@@ -0,0 +1,246 @@
+/*
+ * Hibernate, Relational Persistence for Idiomatic Java
+ *
+ * License: GNU Lesser General Public License (LGPL), version 2.1 or later
+ * See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
+ */
+package org.hibernate.orm.test.compositefk;
+
+import java.io.Serializable;
+
+import org.hibernate.testing.orm.junit.DomainModel;
+import org.hibernate.testing.orm.junit.JiraKey;
+import org.hibernate.testing.orm.junit.SessionFactory;
+import org.hibernate.testing.orm.junit.SessionFactoryScope;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import jakarta.persistence.Column;
+import jakarta.persistence.Embeddable;
+import jakarta.persistence.EmbeddedId;
+import jakarta.persistence.Entity;
+import jakarta.persistence.MappedSuperclass;
+import jakarta.persistence.OneToOne;
+import jakarta.persistence.Table;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+/**
+ * @author Marco Belladelli
+ */
+@DomainModel(annotatedClasses = {
+		OneToOneEmbeddedIdWithGenericAttributeTest.Customer.class,
+		OneToOneEmbeddedIdWithGenericAttributeTest.Invoice.class
+})
+@SessionFactory
+@JiraKey("HHH-16070")
+@JiraKey("HHH-16195")
+public class OneToOneEmbeddedIdWithGenericAttributeTest {
+	@BeforeAll
+	public void setUp(SessionFactoryScope scope) {
+		scope.inTransaction( session -> {
+			final Customer customer = new Customer( 1, "Francisco" );
+			final Invoice invoice = new Invoice( 1, customer );
+			customer.setInvoice( invoice );
+			session.persist( customer );
+			session.persist( invoice );
+		} );
+	}
+
+	@AfterAll
+	public void tearDown(SessionFactoryScope scope) {
+		scope.inTransaction( session -> {
+			session.createMutationQuery( "delete from Invoice" ).executeUpdate();
+			session.createMutationQuery( "delete from Customer" ).executeUpdate();
+		} );
+	}
+
+	@Test
+	public void test(SessionFactoryScope scope) {
+		final Customer customer = scope.fromTransaction( session -> session.createQuery(
+				"from Customer",
+				Customer.class
+		).getSingleResult() );
+		assertNotNull( customer );
+
+		scope.inTransaction( session -> {
+			final Invoice invoice = session.createQuery(
+							"from Invoice i where i.customer.id = :customerId",
+							Invoice.class
+					)
+					.setParameter( "customerId", customer.getId() )
+					.getSingleResult();
+			assertNotNull( invoice );
+		} );
+
+		scope.inTransaction( session -> {
+			final Invoice invoice = session.createQuery(
+							"from Invoice i where i.customer.id.value = :idValue",
+							Invoice.class
+					)
+					.setParameter( "idValue", customer.getId().getValue() )
+					.getSingleResult();
+			assertNotNull( invoice );
+		} );
+	}
+
+	@Test
+	public void testInverse(SessionFactoryScope scope) {
+		final Invoice invoice = scope.fromTransaction( session -> session.createQuery(
+				"from Invoice",
+				Invoice.class
+		).getSingleResult() );
+		assertNotNull( invoice );
+
+		scope.inTransaction( session -> {
+			final Customer customer = session.createQuery(
+							"from Customer c where c.invoice.id = :invoiceId",
+							Customer.class
+					)
+					.setParameter( "invoiceId", invoice.getId() )
+					.getSingleResult();
+			assertNotNull( customer );
+		} );
+
+		scope.inTransaction( session -> {
+			final Customer customer = session.createQuery(
+							"from Customer c where c.invoice.id.value = :idValue",
+							Customer.class
+					)
+					.setParameter( "idValue", invoice.getId().getValue() )
+					.getSingleResult();
+			assertNotNull( customer );
+		} );
+	}
+
+	@Embeddable
+	public static class DomainEntityId<T> implements Serializable {
+		@Column(name = "id_value")
+		private T value;
+
+		public DomainEntityId() {
+		}
+
+		public DomainEntityId(T value) {
+			this.value = value;
+		}
+
+		public T getValue() {
+			return value;
+		}
+
+		public void setValue(T value) {
+			this.value = value;
+		}
+	}
+
+	@MappedSuperclass
+	public abstract static class DomainEntityModel<T> {
+		@EmbeddedId
+		private DomainEntityId<T> id;
+
+		public DomainEntityModel() {
+		}
+
+		protected DomainEntityModel(DomainEntityId<T> id) {
+			this.id = id;
+		}
+
+		public DomainEntityId<T> getId() {
+			return id;
+		}
+
+		public void setId(DomainEntityId<T> id) {
+			this.id = id;
+		}
+	}
+
+	@Embeddable
+	public static class CustomerId extends DomainEntityId<String> {
+		public CustomerId() {
+		}
+
+		public CustomerId(String value) {
+			super( value );
+		}
+	}
+
+	@Entity(name = "Customer")
+	@Table(name = "Customer")
+	public static class Customer extends DomainEntityModel<String> {
+		private Integer code;
+		private String name;
+
+		@OneToOne(mappedBy = "customer")
+		private Invoice invoice;
+
+		public Customer(Integer code, String name) {
+			this();
+			this.code = code;
+			this.name = name;
+		}
+
+		protected Customer() {
+			super( new CustomerId( "customer" ) );
+		}
+
+		public Integer getCode() {
+			return code;
+		}
+
+		public void setCode(Integer code) {
+			this.code = code;
+		}
+
+		public String getName() {
+			return name;
+		}
+
+		public void setName(String name) {
+			this.name = name;
+		}
+
+		public Invoice getInvoice() {
+			return invoice;
+		}
+
+		public void setInvoice(Invoice invoice) {
+			this.invoice = invoice;
+		}
+	}
+
+	@Embeddable
+	public static class InvoiceId extends DomainEntityId<Integer> {
+		public InvoiceId() {
+		}
+
+		public InvoiceId(Integer value) {
+			super( value );
+		}
+	}
+
+	@Entity(name = "Invoice")
+	@Table(name = "Invoice")
+	public static class Invoice extends DomainEntityModel<Integer> {
+		@OneToOne
+		private Customer customer;
+
+		public Invoice() {
+			super();
+		}
+
+		public Invoice(Integer serial, Customer customer) {
+			super( new InvoiceId( serial ) );
+			this.customer = customer;
+		}
+
+		public Customer getCustomer() {
+			return customer;
+		}
+
+		public void setCustomer(Customer customer) {
+			this.customer = customer;
+		}
+	}
+}

hibernate-core/src/test/java/org/hibernate/orm/test/compositefk/OneToOneEmbeddedIdWithGenericsTest.java

@@ -82,7 +82,7 @@ public class OneToOneEmbeddedIdWithGenericsTest {
 					)
 					.setParameter( "invoiceId", invoice.getId() )
 					.getSingleResult();
-			assertNotNull( invoice );
+			assertNotNull( customer );
 		} );
 	}