From bf20b736363bf1b454e51b6de149da71b27820b6 Mon Sep 17 00:00:00 2001 From: Scott Marlow Date: Mon, 4 Nov 2024 08:31:01 -0500 Subject: [PATCH 1/3] HHH-16572 LazyLoadingByEnhancerSetterTest is expected to pass now Signed-off-by: Scott Marlow --- .../enhancement/lazy/LazyLoadingByEnhancerSetterTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/lazy/LazyLoadingByEnhancerSetterTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/lazy/LazyLoadingByEnhancerSetterTest.java index c490d8068c9f..d98dd6c865f1 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/lazy/LazyLoadingByEnhancerSetterTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/lazy/LazyLoadingByEnhancerSetterTest.java @@ -10,7 +10,6 @@ import org.hibernate.testing.bytecode.enhancement.extension.BytecodeEnhanced; import org.hibernate.testing.orm.junit.DomainModel; -import org.hibernate.testing.orm.junit.FailureExpected; import org.hibernate.testing.orm.junit.JiraKey; import org.hibernate.testing.orm.junit.ServiceRegistry; import org.hibernate.testing.orm.junit.SessionFactory; @@ -79,7 +78,7 @@ public void testField(SessionFactoryScope scope) { } @Test - @FailureExpected( jiraKey = "HHH-10747" ) + // failure doesn't occur with HHH-16572 change @FailureExpected( jiraKey = "HHH-10747" ) public void testProperty(SessionFactoryScope scope) { scope.inTransaction( s -> { ItemProperty input = new ItemProperty(); From f09fecd7f5d442ad6fe86ad7c7b3bcf80d00d00d Mon Sep 17 00:00:00 2001 From: Scott Marlow Date: Mon, 4 Nov 2024 08:33:52 -0500 Subject: [PATCH 2/3] HHH-16572 Incorrect enhancement for PROPERTY attributes with mismatched field and method names Signed-off-by: Scott Marlow --- .../internal/bytebuddy/EnhancerImpl.java | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) 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 e696d1beb802..b11f23d10040 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 @@ -17,6 +17,8 @@ import java.util.Optional; import java.util.function.Supplier; +import net.bytebuddy.description.type.TypeList; +import net.bytebuddy.dynamic.scaffold.MethodGraph; import org.hibernate.Version; import org.hibernate.bytecode.enhance.VersionMismatchException; import org.hibernate.bytecode.enhance.internal.tracker.CompositeOwnerTracker; @@ -172,6 +174,12 @@ private DynamicType.Builder doEnhance(Supplier> builde } if ( enhancementContext.isEntityClass( managedCtClass ) ) { + + // Check for HHH-16572 (PROPERTY attributes with mismatched field and method names) + if ( !allowedEnhancementCheck( managedCtClass ) ) { + return null; + } + log.debugf( "Enhancing [%s] as Entity", managedCtClass.getName() ); DynamicType.Builder builder = builderSupplier.get(); builder = builder.implement( ManagedEntity.class ) @@ -331,6 +339,12 @@ private DynamicType.Builder doEnhance(Supplier> builde return createTransformer( managedCtClass ).applyTo( builder ); } else if ( enhancementContext.isCompositeClass( managedCtClass ) ) { + + // Check for HHH-16572 (PROPERTY attributes with mismatched field and method names) + if ( !allowedEnhancementCheck( managedCtClass ) ) { + return null; + } + log.debugf( "Enhancing [%s] as Composite", managedCtClass.getName() ); DynamicType.Builder builder = builderSupplier.get(); @@ -364,6 +378,12 @@ 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 ( !allowedEnhancementCheck( managedCtClass ) ) { + return null; + } + log.debugf( "Enhancing [%s] as MappedSuperclass", managedCtClass.getName() ); DynamicType.Builder builder = builderSupplier.get(); @@ -380,6 +400,79 @@ else if ( enhancementContext.doExtendedEnhancement( managedCtClass ) ) { } } + // See HHH-16572 + // return true if enhancement is supported + private boolean allowedEnhancementCheck(TypeDescription managedCtClass) { + // For process access rules, See https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#default-access-type + // and https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#a122 + // + // This check will determine if entity field names do not match Property accessor method name + // For example: + // @Entity + // class Book { + // Integer id; + // String smtg; + // + // @Id Integer getId() { return id; } + // String getSomething() { return smtg; } + // } + // + // Check name of the getter/setter method with Peristence 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 + continue; + } + + String methodName = methodDescription.getActualName(); + if (methodName.equals("") || + (!methodName.startsWith("get") && !methodName.startsWith("set") && !methodName.startsWith("is"))) { + continue; + } + 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); + } + else { + // not a property accessor method so ignore it + continue; + } + boolean propertyNameMatchesFieldName = false; + // convert field letter to lower case + methodFieldName = methodFieldName.substring(0, 1).toLowerCase() + methodFieldName.substring(1); + TypeList typeList = methodDescription.getDeclaredAnnotations().asTypeList(); + 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); + boolean containsPropertyAccessorMethods = false; + if (enhancementContext.isPersistentField(annotatedField)) { + if (methodFieldName.equals(ctField.getActualName())) { + propertyNameMatchesFieldName = true; + break; + } + } + } + } + if (propertyHasAnnotation && !propertyNameMatchesFieldName) { + log.debugf("Skipping enhancement of [%s]: due to class [%s] not having a property accessor method name matching field name [%s]", + managedCtClass, methodDescription.getDeclaringType().getActualName(), methodFieldName); + return false; + } + } + return true; + } + private static void verifyVersions(TypeDescription managedCtClass, ByteBuddyEnhancementContext enhancementContext) { final AnnotationDescription.Loadable existingInfo = managedCtClass .getDeclaredAnnotations() From 472a0a1de3038d177efdf5b72a2755152fc3f570 Mon Sep 17 00:00:00 2001 From: Scott Marlow Date: Mon, 4 Nov 2024 12:32:47 -0500 Subject: [PATCH 3/3] HHH-16572 Add InvalidPropertyNameTest Signed-off-by: Scott Marlow --- .../access/InvalidPropertyNameTest.java | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/InvalidPropertyNameTest.java diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/InvalidPropertyNameTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/InvalidPropertyNameTest.java new file mode 100644 index 000000000000..3817c20230de --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/InvalidPropertyNameTest.java @@ -0,0 +1,87 @@ +package org.hibernate.orm.test.bytecode.enhancement.access; + +import jakarta.persistence.*; +import org.hibernate.testing.bytecode.enhancement.extension.BytecodeEnhanced; +import org.hibernate.testing.orm.junit.*; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +@DomainModel( + annotatedClasses = { + InvalidPropertyNameTest.SomeEntity.class, + } +) +@SessionFactory +@JiraKey("HHH-16572") +@BytecodeEnhanced +public class InvalidPropertyNameTest { + + + @Test + @FailureExpected(jiraKey = "HHH-16572") + public void test(SessionFactoryScope scope) { + scope.inTransaction( session -> { + session.persist( new SomeEntity( 1L, "field", "property" ) ); + } ); + + scope.inTransaction( session -> { + SomeEntity entity = session.get( SomeEntity.class, 1L ); + assertThat( entity.property ).isEqualTo( "from getter: property" ); + + entity.setPropertyMethod( "updated" ); + } ); + + scope.inTransaction( session -> { + SomeEntity entity = session.get( SomeEntity.class, 1L ); + assertThat( entity.property ).isEqualTo( "from getter: updated" ); + } ); + } + + @AfterEach + public void cleanup(SessionFactoryScope scope) { + // uncomment the following when @FailureExpected is removed above + // scope.inTransaction( session -> { + // session.remove( session.get( SomeEntity.class, 1L ) ); + // PropertyAccessTest} ); + } + + @Entity + @Table(name = "SOME_ENTITY") + static class SomeEntity { + @Id + Long id; + + @Basic + String field; + + 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 + * this entity is not (bytecode) enhanced. Eventually further changes will be made + * such that this entity is enhanced in which case the FailureExpected can be removed + * and the cleanup() uncommented. + */ + @Basic + @Access(AccessType.PROPERTY) + public String getPropertyMethod() { + return "from getter: " + property; + } + + public void setPropertyMethod(String property) { + this.property = property; + } + } +}