From 6c4335287113b25fbd814c7cfc876f7647e770ff Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Thu, 23 Feb 2023 15:01:21 +0100 Subject: [PATCH] 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 --- .../model/internal/ClassPropertyHolder.java | 71 +++-- .../boot/model/internal/PropertyBinder.java | 58 +---- ...leAndMappedSuperClassWithGenericsTest.java | 54 +++- ...bleWithGenericAndMappedSuperClassTest.java | 101 +++++++ ...OneEmbeddedIdWithGenericAttributeTest.java | 246 ++++++++++++++++++ .../OneToOneEmbeddedIdWithGenericsTest.java | 2 +- 6 files changed, 446 insertions(+), 86 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/orm/test/compositefk/OneToOneEmbeddedIdWithGenericAttributeTest.java diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/ClassPropertyHolder.java b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/ClassPropertyHolder.java index 5be0e8c1d6..f2e8a47cd6 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/ClassPropertyHolder.java +++ b/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 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 ) { -// Component component = ( (Component) value ); -// Iterator propertyIterator = component.getPropertyIterator(); -// while ( propertyIterator.hasNext() ) { -// Property property = propertyIterator.next(); -// try { -// property.getGetter( component.getComponentClass() ); -// } -// catch (PropertyNotFoundException e) { -// propertyIterator.remove(); -// } -// } -// } + if ( value instanceof Component ) { + final Component component = ( (Component) value ); + final Iterator propertyIterator = component.getPropertyIterator(); + while ( propertyIterator.hasNext() ) { + Property property = propertyIterator.next(); + try { + property.getGetter( component.getComponentClass() ); + } + catch (PropertyNotFoundException e) { + 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; - component.setComponentClassName( typeName ); + // 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 ); } diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java index 43bd70b1f6..c728051354 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java +++ b/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 ) { - superclass.setDeclaredIdentifierProperty( 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 = 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 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; - } - } - } + ClassPropertyHolder.prepareActualPropertyForSuperclass( + prop, + type, + false, + buildingContext, + superclass::setDeclaredIdentifierProperty + ); } private Component getOrCreateCompositeId(RootClass rootClass) { diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/embeddables/EmbeddableAndMappedSuperClassWithGenericsTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/embeddables/EmbeddableAndMappedSuperClassWithGenericsTest.java index 18e00b9cf9..bb36cb32d2 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/embeddables/EmbeddableAndMappedSuperClassWithGenericsTest.java +++ b/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 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 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 ); } ); } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/embeddables/EmbeddableWithGenericAndMappedSuperClassTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/embeddables/EmbeddableWithGenericAndMappedSuperClassTest.java index 8eed560657..87a7a11f71 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/embeddables/EmbeddableWithGenericAndMappedSuperClassTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/embeddables/EmbeddableWithGenericAndMappedSuperClassTest.java @@ -108,6 +108,107 @@ public class EmbeddableWithGenericAndMappedSuperClassTest { ); } + @Test + public void testQueryParam(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + List 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 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 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 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) { + scope.inTransaction( + session -> { + GenericExample ge = session.get( GenericExample.class, 1 ); + assertThat( ge ).isNotNull(); + assertThat( ge ).extracting( "bounds" ).isNotNull(); + } + ); + } + + @Entity(name = "GenericExample") + public static class GenericExample { + @Id + private int id; + @Embedded + private Range bounds; + + public GenericExample() { + } + + public GenericExample(int id, Range bounds) { + this.id = id; + this.bounds = bounds; + } + } + + public static class Range { + + private T minimum; + private T maximum; + + public Range() { + } + + public Range(T minimum, T maximum) { + this.minimum = minimum; + this.maximum = maximum; + } + } + @Embeddable public static class Edition { private String editorName; diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/compositefk/OneToOneEmbeddedIdWithGenericAttributeTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/compositefk/OneToOneEmbeddedIdWithGenericAttributeTest.java new file mode 100644 index 0000000000..d90cac75d5 --- /dev/null +++ b/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 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 { + @EmbeddedId + private DomainEntityId id; + + public DomainEntityModel() { + } + + protected DomainEntityModel(DomainEntityId id) { + this.id = id; + } + + public DomainEntityId getId() { + return id; + } + + public void setId(DomainEntityId id) { + this.id = id; + } + } + + @Embeddable + public static class CustomerId extends DomainEntityId { + public CustomerId() { + } + + public CustomerId(String value) { + super( value ); + } + } + + @Entity(name = "Customer") + @Table(name = "Customer") + public static class Customer extends DomainEntityModel { + 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 { + public InvoiceId() { + } + + public InvoiceId(Integer value) { + super( value ); + } + } + + @Entity(name = "Invoice") + @Table(name = "Invoice") + public static class Invoice extends DomainEntityModel { + @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; + } + } +} diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/compositefk/OneToOneEmbeddedIdWithGenericsTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/compositefk/OneToOneEmbeddedIdWithGenericsTest.java index cbfd8baa69..7f8328e275 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/compositefk/OneToOneEmbeddedIdWithGenericsTest.java +++ b/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 ); } ); }