From 7e24c29b70bd5dfd5facc8a20f247f76a1c4a8fb Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Tue, 18 Mar 2025 10:21:59 +0100 Subject: [PATCH 1/2] HHH-19059 Add test for issue --- .../access/HierarchyPropertyAccessTest.java | 125 +++++++++--------- .../UnsupportedEnhancementStrategyTest.java | 38 +++++- 2 files changed, 97 insertions(+), 66 deletions(-) diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/HierarchyPropertyAccessTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/HierarchyPropertyAccessTest.java index bd5d55c0fe94..b60561d80eb2 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/HierarchyPropertyAccessTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/HierarchyPropertyAccessTest.java @@ -4,110 +4,99 @@ */ package org.hibernate.orm.test.bytecode.enhancement.access; -import org.hibernate.testing.bytecode.enhancement.extension.BytecodeEnhanced; -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.AfterEach; -import org.junit.jupiter.api.Test; - import jakarta.persistence.Access; import jakarta.persistence.AccessType; -import jakarta.persistence.Basic; import jakarta.persistence.DiscriminatorColumn; -import jakarta.persistence.DiscriminatorValue; import jakarta.persistence.Entity; import jakarta.persistence.Id; -import jakarta.persistence.Inheritance; -import jakarta.persistence.Table; +import jakarta.persistence.MappedSuperclass; import jakarta.persistence.Transient; +import org.hibernate.testing.bytecode.enhancement.extension.BytecodeEnhanced; +import org.hibernate.testing.orm.junit.DomainModel; +import org.hibernate.testing.orm.junit.Jira; +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.Test; import static org.assertj.core.api.Assertions.assertThat; -@DomainModel( - annotatedClasses = { - HierarchyPropertyAccessTest.ChildEntity.class, - } -) +@DomainModel(annotatedClasses = { + HierarchyPropertyAccessTest.AbstractSuperclass.class, + HierarchyPropertyAccessTest.ParentEntity.class, + HierarchyPropertyAccessTest.ChildEntity.class, +}) @SessionFactory -@JiraKey("HHH-19140") +@Jira( "https://hibernate.atlassian.net/browse/HHH-19140" ) +@Jira( "https://hibernate.atlassian.net/browse/HHH-19059" ) @BytecodeEnhanced public class HierarchyPropertyAccessTest { - - @Test public void testParent(SessionFactoryScope scope) { - scope.inTransaction( session -> { - session.persist( new ParentEntity( 1L, "field", "transient: property" ) ); - } ); + assertThat( scope.getSessionFactory().getMappingMetamodel().findEntityDescriptor( ParentEntity.class ) + .getBytecodeEnhancementMetadata().isEnhancedForLazyLoading() ).isTrue(); + scope.inTransaction( session -> session.persist( new ParentEntity( 1L, "field", "transient: property" ) ) ); scope.inTransaction( session -> { - ParentEntity entity = session.get( ParentEntity.class, 1L ); - assertThat( entity.persistProperty ).isEqualTo( "property" ); - assertThat( entity.property ).isEqualTo( "transient: property" ); + final ParentEntity entity = session.find( ParentEntity.class, 1L ); + assertThat( entity.getPersistProperty() ).isEqualTo( "property" ); + assertThat( entity.getProperty() ).isEqualTo( "transient: property" ); + assertThat( entity.getSuperProperty() ).isEqualTo( 8 ); entity.setProperty( "transient: updated" ); } ); scope.inTransaction( session -> { - ParentEntity entity = session.get( ParentEntity.class, 1L ); - assertThat( entity.persistProperty ).isEqualTo( "updated" ); - assertThat( entity.property ).isEqualTo( "transient: updated" ); + final ParentEntity entity = session.find( ParentEntity.class, 1L ); + assertThat( entity.getPersistProperty() ).isEqualTo( "updated" ); + assertThat( entity.getProperty() ).isEqualTo( "transient: updated" ); } ); } @Test public void testChild(SessionFactoryScope scope) { - scope.inTransaction( session -> { - session.persist( new ChildEntity( 2L, "field", "transient: property" ) ); - } ); + assertThat( scope.getSessionFactory().getMappingMetamodel().findEntityDescriptor( ChildEntity.class ) + .getBytecodeEnhancementMetadata().isEnhancedForLazyLoading() ).isTrue(); + scope.inTransaction( session -> session.persist( new ChildEntity( 2L, "field", "transient: property" ) ) ); scope.inTransaction( session -> { - ChildEntity entity = session.get( ChildEntity.class, 2L ); - assertThat( entity.persistProperty ).isEqualTo( "property" ); - assertThat( entity.property ).isEqualTo( "transient: property" ); + ChildEntity entity = session.find( ChildEntity.class, 2L ); + assertThat( entity.getPersistProperty() ).isEqualTo( "property" ); + assertThat( entity.getProperty() ).isEqualTo( "transient: property" ); + assertThat( entity.getSuperProperty() ).isEqualTo( 8 ); entity.setProperty( "transient: updated" ); } ); scope.inTransaction( session -> { - ChildEntity entity = session.get( ChildEntity.class, 2L ); - assertThat( entity.persistProperty ).isEqualTo( "updated" ); - assertThat( entity.property ).isEqualTo( "transient: updated" ); + ChildEntity entity = session.find( ChildEntity.class, 2L ); + assertThat( entity.getPersistProperty() ).isEqualTo( "updated" ); + assertThat( entity.getProperty() ).isEqualTo( "transient: updated" ); } ); } - @AfterEach + @AfterAll public void cleanup(SessionFactoryScope scope) { - scope.inTransaction( session -> { - ParentEntity parentEntity = session.get( ParentEntity.class, 1L ); - if (parentEntity != null) { - session.remove( parentEntity ); - } - ChildEntity childEntity = session.get( ChildEntity.class, 2L ); - if (childEntity != null) { - session.remove( childEntity ); - } - } ); + scope.getSessionFactory().getSchemaManager().truncateMappedObjects(); + } + + @MappedSuperclass + static abstract class AbstractSuperclass { + protected Integer superProperty; } - @Entity - @Table(name = "PARENT_ENTITY") - @Inheritance - @DiscriminatorColumn(name = "type") - @DiscriminatorValue("Parent") - static class ParentEntity { + @Entity(name = "ParentEntity") + @DiscriminatorColumn(name = "entity_type") + static class ParentEntity extends AbstractSuperclass { @Id - Long id; + private Long id; - @Basic - String field; + private String field; - String persistProperty; + private String persistProperty; @Transient - String property; + private String property; public ParentEntity() { } @@ -118,7 +107,6 @@ public ParentEntity(Long id, String field, String property) { this.property = property; } - @Basic @Access(AccessType.PROPERTY) public String getPersistProperty() { this.persistProperty = this.property.substring( 11 ); @@ -137,17 +125,24 @@ public String getProperty() { public void setProperty(String property) { this.property = property; } + + @Access(AccessType.PROPERTY) + public Integer getSuperProperty() { + return getPersistProperty().length(); + } + + public void setSuperProperty(Integer superProperty) { + this.superProperty = superProperty; + } } - @Entity - @DiscriminatorValue("Child") + @Entity(name = "ChildEntity") static class ChildEntity extends ParentEntity { - public ChildEntity() { } public ChildEntity(Long id, String field, String property) { - super(id, field, property); + super( id, field, property ); } } } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/UnsupportedEnhancementStrategyTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/UnsupportedEnhancementStrategyTest.java index 0e60fe6647b9..d131692062a9 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/UnsupportedEnhancementStrategyTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/UnsupportedEnhancementStrategyTest.java @@ -9,6 +9,7 @@ import jakarta.persistence.Basic; import jakarta.persistence.Entity; import jakarta.persistence.Id; +import jakarta.persistence.MappedSuperclass; import jakarta.persistence.PostLoad; import jakarta.persistence.PrePersist; import jakarta.persistence.Transient; @@ -103,6 +104,14 @@ public void testAccessTypeFieldEntity() throws IOException { assertThat( doEnhance( ImplicitAccessTypeFieldEntity.class, context ) ).isNotNull(); } + @Test + @Jira( "https://hibernate.atlassian.net/browse/HHH-19059" ) + public void testAccessTypePropertyInherited() throws IOException { + var context = new UnsupportedEnhancerContext(); + // non-null means check passed and enhancement _was_ performed + assertThat( doEnhance( PropertyAccessInheritedEntity.class, context ) ).isNotNull(); + } + private static byte[] doEnhance(Class entityClass, EnhancementContext context) throws IOException { final ByteBuddyState byteBuddyState = new ByteBuddyState(); final Enhancer enhancer = new EnhancerImpl( context, byteBuddyState ); @@ -197,7 +206,7 @@ public String getState() { } @PostLoad - public void setState() { + public void setState(String state) { status = "loaded"; } @@ -251,4 +260,31 @@ public String getAnother() { return "another"; } } + + @MappedSuperclass + static abstract class AbstractSuperclass { + protected String property; + } + + @Entity(name="PropertyAccessInheritedEntity") + static class PropertyAccessInheritedEntity extends AbstractSuperclass { + private Long id; + + @Id + public Long getId() { + return id; + } + + public void setId(Long id) { + this.id = id; + } + + public String getProperty() { + return property; + } + + public void setProperty(String property) { + this.property = property; + } + } } From 9d821a9141c42aee2376647f5e17b39640312051 Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Tue, 18 Mar 2025 10:22:06 +0100 Subject: [PATCH 2/2] HHH-19059 Fix check for property access fields on hierarchies --- .../internal/bytebuddy/EnhancerImpl.java | 106 ++++++++---------- 1 file changed, 49 insertions(+), 57 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java index 349bb49d7657..139de3f1b7e8 100644 --- a/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java @@ -13,6 +13,7 @@ import jakarta.persistence.Entity; import jakarta.persistence.Id; import jakarta.persistence.MappedSuperclass; +import jakarta.persistence.Transient; import jakarta.persistence.metamodel.Type; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.annotation.AnnotationDescription; @@ -74,6 +75,9 @@ import static net.bytebuddy.matcher.ElementMatchers.isDefaultFinalizer; import static net.bytebuddy.matcher.ElementMatchers.isGetter; import static net.bytebuddy.matcher.ElementMatchers.isSetter; +import static net.bytebuddy.matcher.ElementMatchers.isStatic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; public class EnhancerImpl implements Enhancer { @@ -476,21 +480,13 @@ private static boolean hasMappingAnnotation(AnnotationList annotations) { || annotations.isAnnotationPresent( Embeddable.class ); } - private static boolean hasPersistenceAnnotation(AnnotationList annotations) { - boolean found = false; - for ( AnnotationDescription annotation : annotations ) { - final String annotationName = annotation.getAnnotationType().getName(); - if ( annotationName.startsWith( "jakarta.persistence" ) ) { - if ( annotationName.equals( "jakarta.persistence.Transient" ) ) { - // transient property so ignore it - return false; - } - else if ( !found && !IGNORED_PERSISTENCE_ANNOTATIONS.contains( annotationName ) ) { - found = true; - } - } + private static boolean isPersistentMethod(MethodDescription method) { + final AnnotationList annotations = method.getDeclaredAnnotations(); + if ( annotations.isAnnotationPresent( Transient.class ) ) { + return false; } - return found; + + return annotations.stream().noneMatch( a -> IGNORED_PERSISTENCE_ANNOTATIONS.contains( a.getAnnotationType().getName() ) ); } private static final Set IGNORED_PERSISTENCE_ANNOTATIONS = Set.of( @@ -503,6 +499,17 @@ else if ( !found && !IGNORED_PERSISTENCE_ANNOTATIONS.contains( annotationName ) "jakarta.persistence.PreUpdate" ); + private static boolean containsField(Generic type, String fieldName) { + do { + if ( !type.getDeclaredFields().filter( not( isStatic() ).and( named( fieldName ) ) ).isEmpty() ) { + return true; + } + type = type.getSuperClass(); + } + while ( type != null && !type.represents( Object.class ) ); + return false; + } + /** * Check whether an entity class ({@code managedCtClass}) has mismatched names between a persistent field and its * getter/setter when using {@link AccessType#PROPERTY}, which Hibernate does not currently support for enhancement. @@ -545,61 +552,46 @@ private static boolean checkUnsupportedAttributeNaming(TypeDescription managedCt .asMethodList() .filter( isGetter().or( isSetter() ) ); for ( final MethodDescription methodDescription : methods ) { - if ( determineAccessType( methodDescription, defaultAccessType ) != AccessType.PROPERTY ) { + if ( methodDescription.getDeclaringType().represents( Object.class ) + || determineAccessType( methodDescription, defaultAccessType ) != AccessType.PROPERTY ) { // We only need to check this for AccessType.PROPERTY continue; } final String methodName = methodDescription.getActualName(); - String methodFieldName; + String fieldName; if ( methodName.startsWith( "get" ) || methodName.startsWith( "set" ) ) { - methodFieldName = methodName.substring( 3 ); + fieldName = methodName.substring( 3 ); } else { assert methodName.startsWith( "is" ); - methodFieldName = methodName.substring( 2 ); + fieldName = methodName.substring( 2 ); } // convert first field letter to lower case - methodFieldName = getJavaBeansFieldName( methodFieldName ); - if ( methodFieldName != null && hasPersistenceAnnotation( methodDescription.getDeclaredAnnotations() ) ) { - boolean propertyNameMatchesFieldName = false; - for ( final FieldDescription field : methodDescription.getDeclaringType().getDeclaredFields() ) { - if ( !Modifier.isStatic( field.getModifiers() ) ) { - final AnnotatedFieldDescription annotatedField = new AnnotatedFieldDescription( - enhancementContext, - field + fieldName = getJavaBeansFieldName( fieldName ); + if ( fieldName != null && isPersistentMethod( methodDescription ) + && !containsField( managedCtClass.asGenericType(), fieldName ) ) { + // We shouldn't even be in this method if using LEGACY, see top of this method. + switch ( strategy ) { + case SKIP: + log.debugf( + "Skipping enhancement of [%s] because no field named [%s] could be found for property accessor method [%s]." + + " To fix this, make sure all property accessor methods have a matching field.", + managedCtClass.getName(), + fieldName, + methodDescription.getName() ); - if ( enhancementContext.isPersistentField( annotatedField ) ) { - if ( methodFieldName.equals( field.getActualName() ) ) { - propertyNameMatchesFieldName = true; - break; - } - } - } - } - if ( !propertyNameMatchesFieldName ) { - // We shouldn't even be in this method if using LEGACY, see top of this method. - switch ( strategy ) { - case SKIP: - log.debugf( - "Skipping enhancement of [%s] because no field named [%s] could be found for property accessor method [%s]." - + " To fix this, make sure all property accessor methods have a matching field.", - managedCtClass.getName(), - methodFieldName, - methodDescription.getName() - ); - return true; - case FAIL: - throw new EnhancementException( String.format( - "Enhancement of [%s] failed because no field named [%s] could be found for property accessor method [%s]." - + " To fix this, make sure all property accessor methods have a matching field.", - managedCtClass.getName(), - methodFieldName, - methodDescription.getName() - ) ); - default: - throw new AssertionFailure( "Unexpected strategy at this point: " + strategy ); - } + return true; + case FAIL: + throw new EnhancementException( String.format( + "Enhancement of [%s] failed because no field named [%s] could be found for property accessor method [%s]." + + " To fix this, make sure all property accessor methods have a matching field.", + managedCtClass.getName(), + fieldName, + methodDescription.getName() + ) ); + default: + throw new AssertionFailure( "Unexpected strategy at this point: " + strategy ); } } }