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 c2630ab3bb90..2d81e19eb541 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 @@ -6,23 +6,31 @@ import jakarta.persistence.Access; import jakarta.persistence.AccessType; +import jakarta.persistence.Embeddable; +import jakarta.persistence.EmbeddedId; +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import jakarta.persistence.MappedSuperclass; import jakarta.persistence.metamodel.Type; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.annotation.AnnotationDescription; import net.bytebuddy.description.annotation.AnnotationList; +import net.bytebuddy.description.annotation.AnnotationSource; import net.bytebuddy.description.field.FieldDescription; import net.bytebuddy.description.field.FieldDescription.InDefinedShape; import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.method.MethodList; import net.bytebuddy.description.type.TypeDefinition; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.description.type.TypeDescription.Generic; -import net.bytebuddy.description.type.TypeList; import net.bytebuddy.dynamic.DynamicType; import net.bytebuddy.dynamic.scaffold.MethodGraph; import net.bytebuddy.implementation.FieldAccessor; import net.bytebuddy.implementation.FixedValue; import net.bytebuddy.implementation.Implementation; import net.bytebuddy.implementation.StubMethod; + +import org.checkerframework.checker.nullness.qual.Nullable; import org.hibernate.AssertionFailure; import org.hibernate.Version; import org.hibernate.bytecode.enhance.VersionMismatchException; @@ -58,9 +66,12 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.function.Supplier; import static net.bytebuddy.matcher.ElementMatchers.isDefaultFinalizer; +import static net.bytebuddy.matcher.ElementMatchers.isGetter; +import static net.bytebuddy.matcher.ElementMatchers.isSetter; public class EnhancerImpl implements Enhancer { @@ -173,7 +184,7 @@ private DynamicType.Builder doEnhance(Supplier> builde } if ( enhancementContext.isEntityClass( managedCtClass ) ) { - if ( checkUnsupportedAttributeNaming( managedCtClass ) ) { + if ( checkUnsupportedAttributeNaming( managedCtClass, enhancementContext ) ) { // do not enhance classes with mismatched names for PROPERTY-access persistent attributes return null; } @@ -337,7 +348,7 @@ private DynamicType.Builder doEnhance(Supplier> builde return createTransformer( managedCtClass ).applyTo( builder ); } else if ( enhancementContext.isCompositeClass( managedCtClass ) ) { - if ( checkUnsupportedAttributeNaming( managedCtClass ) ) { + if ( checkUnsupportedAttributeNaming( managedCtClass, enhancementContext ) ) { // do not enhance classes with mismatched names for PROPERTY-access persistent attributes return null; } @@ -375,9 +386,8 @@ else if ( enhancementContext.isCompositeClass( managedCtClass ) ) { return createTransformer( managedCtClass ).applyTo( builder ); } else if ( enhancementContext.isMappedSuperclassClass( managedCtClass ) ) { - // Check for HHH-16572 (PROPERTY attributes with mismatched field and method names) - if ( checkUnsupportedAttributeNaming( managedCtClass ) ) { + if ( checkUnsupportedAttributeNaming( managedCtClass, enhancementContext ) ) { return null; } @@ -397,19 +407,101 @@ else if ( enhancementContext.doExtendedEnhancement( managedCtClass ) ) { } } + /** + * Utility that determines the access-type of a mapped class based on an explicit annotation + * or guessing it from the placement of its identifier property. Implementation should be + * aligned with {@code InheritanceState#determineDefaultAccessType()}. + * + * @return the {@link AccessType} used by the mapped class + * + * @implNote this does not fully account for embeddables, as they should inherit the access-type + * from the entities they're used in - defaulting to PROPERTY to always run the accessor check + */ + private static AccessType determineDefaultAccessType(TypeDefinition typeDefinition) { + for ( TypeDefinition candidate = typeDefinition; candidate != null && !candidate.represents( Object.class ); candidate = candidate.getSuperClass() ) { + final AnnotationList annotations = candidate.asErasure().getDeclaredAnnotations(); + if ( hasMappingAnnotation( annotations ) ) { + final AnnotationDescription.Loadable access = annotations.ofType( Access.class ); + if ( access != null ) { + return access.load().value(); + } + } + } + // Guess from identifier. + // FIX: Shouldn't this be determined by the first attribute (i.e., field or property) with annotations, + // but without an explicit Access annotation, according to JPA 2.0 spec 2.3.1: Default Access Type? + for ( TypeDefinition candidate = typeDefinition; candidate != null && !candidate.represents( Object.class ); candidate = candidate.getSuperClass() ) { + final AnnotationList annotations = candidate.asErasure().getDeclaredAnnotations(); + if ( hasMappingAnnotation( annotations ) ) { + for ( FieldDescription ctField : candidate.getDeclaredFields() ) { + if ( !Modifier.isStatic( ctField.getModifiers() ) ) { + final AnnotationList annotationList = ctField.getDeclaredAnnotations(); + if ( annotationList.isAnnotationPresent( Id.class ) || annotationList.isAnnotationPresent( EmbeddedId.class ) ) { + return AccessType.FIELD; + } + } + } + } + } + // We can assume AccessType.PROPERTY here + return AccessType.PROPERTY; + } + + /** + * Determines the access-type of the given annotation source if an explicit {@link Access} annotation + * is present, otherwise defaults to the provided {@code defaultAccessType} + */ + private static AccessType determineAccessType(AnnotationSource annotationSource, AccessType defaultAccessType) { + final AnnotationDescription.Loadable access = annotationSource.getDeclaredAnnotations().ofType( Access.class ); + return access != null ? access.load().value() : defaultAccessType; + } + + private static boolean hasMappingAnnotation(AnnotationList annotations) { + return annotations.isAnnotationPresent( Entity.class ) + || annotations.isAnnotationPresent( MappedSuperclass.class ) + || 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; + } + } + } + return found; + } + + private static final Set IGNORED_PERSISTENCE_ANNOTATIONS = Set.of( + "jakarta.persistence.PostLoad", + "jakarta.persistence.PostPersist", + "jakarta.persistence.PostRemove", + "jakarta.persistence.PostUpdate", + "jakarta.persistence.PrePersist", + "jakarta.persistence.PreRemove", + "jakarta.persistence.PreUpdate" + ); + /** * 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. - * See https://hibernate.atlassian.net/browse/HHH-16572 + * See HHH-16572 * - * @return {@code true} if enhancement of the class must be {@link org.hibernate.bytecode.enhance.spi.UnsupportedEnhancementStrategy#SKIP skipped} + * @return {@code true} if enhancement of the class must be {@link UnsupportedEnhancementStrategy#SKIP skipped} * because it has mismatched names. * {@code false} if enhancement of the class must proceed, either because it doesn't have any mismatched names, - * or because {@link org.hibernate.bytecode.enhance.spi.UnsupportedEnhancementStrategy#LEGACY legacy mode} was opted into. - * @throws EnhancementException if enhancement of the class must {@link org.hibernate.bytecode.enhance.spi.UnsupportedEnhancementStrategy#FAIL abort} because it has mismatched names. + * or because {@link UnsupportedEnhancementStrategy#LEGACY legacy mode} was opted into. + * @throws EnhancementException if enhancement of the class must {@link UnsupportedEnhancementStrategy#FAIL abort} because it has mismatched names. */ @SuppressWarnings("deprecation") - private boolean checkUnsupportedAttributeNaming(TypeDescription managedCtClass) { + private static boolean checkUnsupportedAttributeNaming(TypeDescription managedCtClass, ByteBuddyEnhancementContext enhancementContext) { var strategy = enhancementContext.getUnsupportedEnhancementStrategy(); if ( UnsupportedEnhancementStrategy.LEGACY.equals( strategy ) ) { // Don't check anything and act as if there was nothing unsupported in the class. @@ -433,71 +525,66 @@ private boolean checkUnsupportedAttributeNaming(TypeDescription managedCtClass) // // Check name of the getter/setter method with persistence annotation and getter/setter method name that doesn't refer to an entity field // and will return false. If the property accessor method(s) are named to match the field name(s), return true. - boolean propertyHasAnnotation = false; - MethodGraph.Linked methodGraph = MethodGraph.Compiler.Default.forJavaHierarchy().compile((TypeDefinition) managedCtClass); - for (MethodGraph.Node node : methodGraph.listNodes()) { - MethodDescription methodDescription = node.getRepresentative(); - if (methodDescription.getDeclaringType().represents(Object.class)) { // skip class java.lang.Object methods + final AccessType defaultAccessType = determineDefaultAccessType( managedCtClass ); + final MethodList methods = MethodGraph.Compiler.DEFAULT.compile( (TypeDefinition) managedCtClass ) + .listNodes() + .asMethodList() + .filter( isGetter().or( isSetter() ) ); + for ( final MethodDescription methodDescription : methods ) { + if ( determineAccessType( methodDescription, defaultAccessType ) != AccessType.PROPERTY ) { + // We only need to check this for AccessType.PROPERTY continue; } - String methodName = methodDescription.getActualName(); - if (methodName.equals("") || - (!methodName.startsWith("get") && !methodName.startsWith("set") && !methodName.startsWith("is"))) { - continue; - } + final String methodName = methodDescription.getActualName(); String methodFieldName; - if (methodName.startsWith("is")) { // skip past "is" - methodFieldName = methodName.substring(2); - } - else if (methodName.startsWith("get") || - methodName.startsWith("set")) { // skip past "get" or "set" - methodFieldName = methodName.substring(3); + if ( methodName.startsWith( "get" ) || methodName.startsWith( "set" ) ) { + methodFieldName = methodName.substring( 3 ); } else { - // not a property accessor method so ignore it - continue; + assert methodName.startsWith( "is" ); + methodFieldName = methodName.substring( 2 ); } - boolean propertyNameMatchesFieldName = false; - // extract the property name from method name - methodFieldName = getJavaBeansFieldName(methodFieldName); - TypeList typeList = methodDescription.getDeclaredAnnotations().asTypeList(); - if (typeList.stream().anyMatch(typeDefinitions -> - (typeDefinitions.getName().equals("jakarta.persistence.Transient")))) { - // transient property so ignore it - continue; - } - if (typeList.stream().anyMatch(typeDefinitions -> - (typeDefinitions.getName().contains("jakarta.persistence")))) { - propertyHasAnnotation = true; - } - for (FieldDescription ctField : methodDescription.getDeclaringType().getDeclaredFields()) { - if (!Modifier.isStatic(ctField.getModifiers())) { - AnnotatedFieldDescription annotatedField = new AnnotatedFieldDescription(enhancementContext, ctField); - if (enhancementContext.isPersistentField(annotatedField)) { - if (methodFieldName.equals(ctField.getActualName())) { - propertyNameMatchesFieldName = true; - break; + // 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 + ); + if ( enhancementContext.isPersistentField( annotatedField ) ) { + if ( methodFieldName.equals( field.getActualName() ) ) { + propertyNameMatchesFieldName = true; + break; + } } } } - } - if ( propertyHasAnnotation && !propertyNameMatchesFieldName ) { - 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( + if ( !propertyNameMatchesFieldName ) { + // We shouldn't even be in this method if using LEGACY, see top of this method. + return 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() + ); + yield 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: - // We shouldn't even be in this method if using LEGACY, see top of this method. - throw new AssertionFailure( "Unexpected strategy at this point: " + strategy ); + managedCtClass.getName(), + methodFieldName, + methodDescription.getName() + ) ); + default -> throw new AssertionFailure( "Unexpected strategy at this point: " + strategy ); + }; } } } @@ -507,17 +594,20 @@ else if (methodName.startsWith("get") || /** * If the first two characters are upper case, assume all characters are upper case to be returned as is. * Otherwise, return the name with the first character converted to lower case and the remaining part returned as is. - * @param fieldName is the property accessor name to be updated following Persistence property name rules. - * @return name that follows JavaBeans rules. + * + * @param name is the property accessor name to be updated following Persistence property name rules. + * @return name that follows JavaBeans rules, or {@code null} if the provided string is empty */ - private static String getJavaBeansFieldName(String fieldName) { - - if (fieldName.length() == 0 || - (fieldName.length() > 1 && Character.isUpperCase(fieldName.charAt(0)) && Character.isUpperCase(fieldName.charAt(1))) - ) { - return fieldName; + private static @Nullable String getJavaBeansFieldName(String name) { + if ( name.isEmpty() ) { + return null; + } + if ( name.length() > 1 && Character.isUpperCase( name.charAt( 1 ) ) && Character.isUpperCase( name.charAt( 0 ) ) ) { + return name; } - return Character.toLowerCase(fieldName.charAt(0)) + fieldName.substring(1); + final char[] chars = name.toCharArray(); + chars[0] = Character.toLowerCase( chars[0] ); + return new String( chars ); } private static void verifyVersions(TypeDescription managedCtClass, ByteBuddyEnhancementContext enhancementContext) { 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 902b19c95994..0e60fe6647b9 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,7 +9,10 @@ import jakarta.persistence.Basic; import jakarta.persistence.Entity; import jakarta.persistence.Id; -import jakarta.persistence.Table; +import jakarta.persistence.PostLoad; +import jakarta.persistence.PrePersist; +import jakarta.persistence.Transient; + import org.hibernate.bytecode.enhance.internal.bytebuddy.EnhancerImpl; import org.hibernate.bytecode.enhance.spi.EnhancementContext; import org.hibernate.bytecode.enhance.spi.EnhancementException; @@ -18,11 +21,13 @@ import org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState; import org.hibernate.bytecode.spi.ByteCodeHelper; import org.hibernate.testing.bytecode.enhancement.EnhancerTestContext; +import org.hibernate.testing.orm.junit.Jira; import org.hibernate.testing.orm.junit.JiraKey; import org.junit.jupiter.api.Test; import java.io.IOException; import java.io.InputStream; +import java.util.UUID; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -39,28 +44,31 @@ public UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() { return UnsupportedEnhancementStrategy.SKIP; } }; - byte[] originalBytes = getAsBytes( SomeEntity.class ); - byte[] enhancedBytes = doEnhance( SomeEntity.class, originalBytes, context ); + byte[] enhancedBytes = doEnhance( SomeEntity.class, context ); assertThat( enhancedBytes ).isNull(); // null means "not enhanced" } @Test public void fail() throws IOException { - var context = new EnhancerTestContext() { - @Override - public UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() { - return UnsupportedEnhancementStrategy.FAIL; - } - }; - byte[] originalBytes = getAsBytes( SomeEntity.class ); - assertThatThrownBy( () -> doEnhance( SomeEntity.class, originalBytes, context ) ) - .isInstanceOf( EnhancementException.class ) + var context = new UnsupportedEnhancerContext(); + assertThatThrownBy( () -> doEnhance( SomeEntity.class, context ) ).isInstanceOf( EnhancementException.class ) .hasMessageContainingAll( String.format( "Enhancement of [%s] failed because no field named [%s] could be found for property accessor method [%s].", - SomeEntity.class.getName(), "propertyMethod", "getPropertyMethod" ), - "To fix this, make sure all property accessor methods have a matching field." + SomeEntity.class.getName(), + "propertyMethod", + "getPropertyMethod" + ), "To fix this, make sure all property accessor methods have a matching field." ); + assertThatThrownBy( () -> doEnhance( SomeOtherEntity.class, context ) ).isInstanceOf( EnhancementException.class ) + .hasMessageContainingAll( + String.format( + "Enhancement of [%s] failed because no field named [%s] could be found for property accessor method [%s].", + SomeOtherEntity.class.getName(), + "propertyMethod", + "setPropertyMethod" + ), "To fix this, make sure all property accessor methods have a matching field." + ); } @Test @@ -73,26 +81,49 @@ public UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() { return UnsupportedEnhancementStrategy.LEGACY; } }; - byte[] originalBytes = getAsBytes( SomeEntity.class ); - byte[] enhancedBytes = doEnhance( SomeEntity.class, originalBytes, context ); + byte[] enhancedBytes = doEnhance( SomeEntity.class, context ); assertThat( enhancedBytes ).isNotNull(); // non-null means enhancement _was_ performed } - private byte[] doEnhance(Class someEntityClass, byte[] originalBytes, EnhancementContext context) { + @Test + @Jira( "https://hibernate.atlassian.net/browse/HHH-18903" ) + @Jira( "https://hibernate.atlassian.net/browse/HHH-18904" ) + public void testEntityListeners() throws IOException { + // non-null means check passed and enhancement _was_ performed + assertThat( doEnhance( EventListenersEntity.class, new UnsupportedEnhancerContext() ) ).isNotNull(); + } + + @Test + @Jira( "https://hibernate.atlassian.net/browse/HHH-18903" ) + @Jira( "https://hibernate.atlassian.net/browse/HHH-18904" ) + public void testAccessTypeFieldEntity() throws IOException { + var context = new UnsupportedEnhancerContext(); + // non-null means check passed and enhancement _was_ performed + assertThat( doEnhance( ExplicitAccessTypeFieldEntity.class, context ) ).isNotNull(); + assertThat( doEnhance( ImplicitAccessTypeFieldEntity.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 ); - return enhancer.enhance( someEntityClass.getName(), originalBytes ); + return enhancer.enhance( entityClass.getName(), getAsBytes( entityClass ) ); } - private byte[] getAsBytes(Class clazz) throws IOException { + private static byte[] getAsBytes(Class clazz) throws IOException { final String classFile = clazz.getName().replace( '.', '/' ) + ".class"; try (InputStream classFileStream = clazz.getClassLoader().getResourceAsStream( classFile )) { return ByteCodeHelper.readByteCode( classFileStream ); } } - @Entity - @Table(name = "SOME_ENTITY") + static class UnsupportedEnhancerContext extends EnhancerTestContext { + @Override + public UnsupportedEnhancementStrategy getUnsupportedEnhancementStrategy() { + return UnsupportedEnhancementStrategy.FAIL; + } + } + + @Entity(name = "SomeEntity") static class SomeEntity { @Id Long id; @@ -102,15 +133,6 @@ static class SomeEntity { String property; - public SomeEntity() { - } - - public SomeEntity(Long id, String field, String property) { - this.id = id; - this.field = field; - this.property = property; - } - /** * The following property accessor methods are purposely named incorrectly to * not match the "property" field. The HHH-16572 change ensures that @@ -128,4 +150,105 @@ public void setPropertyMethod(String property) { this.property = property; } } + + @Entity(name = "SomeOtherEntity") + static class SomeOtherEntity { + @Id + Long id; + + @Basic + String field; + + String property; + @Access(AccessType.PROPERTY) + public void setPropertyMethod(String property) { + this.property = property; + } + } + + @Entity(name = "EventListenersEntity") + static class EventListenersEntity { + private UUID id; + + private String status = "new"; + + @Id + public UUID getId() { + return id; + } + + public void setId(UUID id) { + this.id = id; + } + + // special case, we should let it through + public UUID get() { + return id; + } + + @PrePersist + public void setId() { + id = UUID.randomUUID(); + } + + @Transient + public String getState() { + return status; + } + + @PostLoad + public void setState() { + status = "loaded"; + } + + @Transient + public boolean isLoaded() { + return status.equals( "loaded" ); + } + } + + @Entity(name = "ExplicitAccessTypeFieldEntity") + @Access( AccessType.FIELD ) + static class ExplicitAccessTypeFieldEntity { + @Id + private Long id; + + public Long getId() { + return id; + } + + public void setId(Long id) { + this.id = id; + } + + public Long get() { + return id; + } + + public String getSomething() { + return "something"; + } + } + + @Entity(name = "ImplicitAccessTypeFieldEntity") + static class ImplicitAccessTypeFieldEntity { + @Id + private Long id; + + public Long getId() { + return id; + } + + public void setId(Long id) { + this.id = id; + } + + public Long get() { + return id; + } + + public String getAnother() { + return "another"; + } + } }