Skip to content

Commit 356ea20

Browse files
scottmarlowsebersole
scottmarlow
authored and
sebersole
committed
HHH-16572 - Skip enhancement for PROPERTY attributes with mismatched field and method names
Signed-off-by: Scott Marlow <[email protected]>
1 parent 14e0f12 commit 356ea20

File tree

3 files changed

+214
-31
lines changed

3 files changed

+214
-31
lines changed

‎hibernate-core/src/main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/EnhancerImpl.java‎

Lines changed: 122 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,25 @@
44
*/
55
package org.hibernate.bytecode.enhance.internal.bytebuddy;
66

7-
import java.lang.annotation.Annotation;
8-
import java.lang.reflect.Modifier;
9-
import java.util.ArrayList;
10-
import java.util.Collection;
11-
import java.util.Collections;
12-
import java.util.List;
13-
import java.util.Map;
14-
import java.util.Objects;
15-
import java.util.Optional;
16-
import java.util.function.Supplier;
17-
7+
import jakarta.persistence.Access;
8+
import jakarta.persistence.AccessType;
9+
import jakarta.persistence.metamodel.Type;
10+
import net.bytebuddy.asm.Advice;
11+
import net.bytebuddy.description.annotation.AnnotationDescription;
12+
import net.bytebuddy.description.annotation.AnnotationList;
13+
import net.bytebuddy.description.field.FieldDescription;
14+
import net.bytebuddy.description.field.FieldDescription.InDefinedShape;
15+
import net.bytebuddy.description.method.MethodDescription;
16+
import net.bytebuddy.description.type.TypeDefinition;
17+
import net.bytebuddy.description.type.TypeDescription;
18+
import net.bytebuddy.description.type.TypeDescription.Generic;
19+
import net.bytebuddy.description.type.TypeList;
20+
import net.bytebuddy.dynamic.DynamicType;
21+
import net.bytebuddy.dynamic.scaffold.MethodGraph;
22+
import net.bytebuddy.implementation.FieldAccessor;
23+
import net.bytebuddy.implementation.FixedValue;
24+
import net.bytebuddy.implementation.Implementation;
25+
import net.bytebuddy.implementation.StubMethod;
1826
import org.hibernate.Version;
1927
import org.hibernate.bytecode.enhance.VersionMismatchException;
2028
import org.hibernate.bytecode.enhance.internal.tracker.CompositeOwnerTracker;
@@ -39,23 +47,16 @@
3947
import org.hibernate.internal.CoreLogging;
4048
import org.hibernate.internal.CoreMessageLogger;
4149

42-
import jakarta.persistence.Access;
43-
import jakarta.persistence.AccessType;
44-
import jakarta.persistence.metamodel.Type;
45-
import net.bytebuddy.asm.Advice;
46-
import net.bytebuddy.description.annotation.AnnotationDescription;
47-
import net.bytebuddy.description.annotation.AnnotationList;
48-
import net.bytebuddy.description.field.FieldDescription;
49-
import net.bytebuddy.description.field.FieldDescription.InDefinedShape;
50-
import net.bytebuddy.description.method.MethodDescription;
51-
import net.bytebuddy.description.type.TypeDefinition;
52-
import net.bytebuddy.description.type.TypeDescription;
53-
import net.bytebuddy.description.type.TypeDescription.Generic;
54-
import net.bytebuddy.dynamic.DynamicType;
55-
import net.bytebuddy.implementation.FieldAccessor;
56-
import net.bytebuddy.implementation.FixedValue;
57-
import net.bytebuddy.implementation.Implementation;
58-
import net.bytebuddy.implementation.StubMethod;
50+
import java.lang.annotation.Annotation;
51+
import java.lang.reflect.Modifier;
52+
import java.util.ArrayList;
53+
import java.util.Collection;
54+
import java.util.Collections;
55+
import java.util.List;
56+
import java.util.Map;
57+
import java.util.Objects;
58+
import java.util.Optional;
59+
import java.util.function.Supplier;
5960

6061
import static net.bytebuddy.matcher.ElementMatchers.isDefaultFinalizer;
6162

@@ -170,6 +171,11 @@ private DynamicType.Builder<?> doEnhance(Supplier<DynamicType.Builder<?>> builde
170171
}
171172

172173
if ( enhancementContext.isEntityClass( managedCtClass ) ) {
174+
if ( hasUnsupportedAttributeNaming( managedCtClass ) ) {
175+
// do not enhance classes with mismatched names for PROPERTY-access persistent attributes
176+
return null;
177+
}
178+
173179
log.debugf( "Enhancing [%s] as Entity", managedCtClass.getName() );
174180
DynamicType.Builder<?> builder = builderSupplier.get();
175181
builder = builder.implement( ManagedEntity.class )
@@ -329,6 +335,11 @@ private DynamicType.Builder<?> doEnhance(Supplier<DynamicType.Builder<?>> builde
329335
return createTransformer( managedCtClass ).applyTo( builder );
330336
}
331337
else if ( enhancementContext.isCompositeClass( managedCtClass ) ) {
338+
if ( hasUnsupportedAttributeNaming( managedCtClass ) ) {
339+
// do not enhance classes with mismatched names for PROPERTY-access persistent attributes
340+
return null;
341+
}
342+
332343
log.debugf( "Enhancing [%s] as Composite", managedCtClass.getName() );
333344

334345
DynamicType.Builder<?> builder = builderSupplier.get();
@@ -362,6 +373,12 @@ else if ( enhancementContext.isCompositeClass( managedCtClass ) ) {
362373
return createTransformer( managedCtClass ).applyTo( builder );
363374
}
364375
else if ( enhancementContext.isMappedSuperclassClass( managedCtClass ) ) {
376+
377+
// Check for HHH-16572 (PROPERTY attributes with mismatched field and method names)
378+
if ( hasUnsupportedAttributeNaming( managedCtClass ) ) {
379+
return null;
380+
}
381+
365382
log.debugf( "Enhancing [%s] as MappedSuperclass", managedCtClass.getName() );
366383

367384
DynamicType.Builder<?> builder = builderSupplier.get();
@@ -378,6 +395,82 @@ else if ( enhancementContext.doExtendedEnhancement( managedCtClass ) ) {
378395
}
379396
}
380397

398+
/**
399+
* Check whether an entity class ({@code managedCtClass}) has mismatched names between a persistent field and its
400+
* getter/setter when using {@link AccessType#PROPERTY}, which Hibernate does not currently support for enhancement.
401+
* See https://hibernate.atlassian.net/browse/HHH-16572
402+
*/
403+
private boolean hasUnsupportedAttributeNaming(TypeDescription managedCtClass) {
404+
// For process access rules, See https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#default-access-type
405+
// and https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#a122
406+
//
407+
// This check will determine if entity field names do not match Property accessor method name
408+
// For example:
409+
// @Entity
410+
// class Book {
411+
// Integer id;
412+
// String smtg;
413+
//
414+
// @Id Integer getId() { return id; }
415+
// String getSomething() { return smtg; }
416+
// }
417+
//
418+
// Check name of the getter/setter method with persistence annotation and getter/setter method name that doesn't refer to an entity field
419+
// and will return false. If the property accessor method(s) are named to match the field name(s), return true.
420+
boolean propertyHasAnnotation = false;
421+
MethodGraph.Linked methodGraph = MethodGraph.Compiler.Default.forJavaHierarchy().compile((TypeDefinition) managedCtClass);
422+
for (MethodGraph.Node node : methodGraph.listNodes()) {
423+
MethodDescription methodDescription = node.getRepresentative();
424+
if (methodDescription.getDeclaringType().represents(Object.class)) { // skip class java.lang.Object methods
425+
continue;
426+
}
427+
428+
String methodName = methodDescription.getActualName();
429+
if (methodName.equals("") ||
430+
(!methodName.startsWith("get") && !methodName.startsWith("set") && !methodName.startsWith("is"))) {
431+
continue;
432+
}
433+
String methodFieldName;
434+
if (methodName.startsWith("is")) { // skip past "is"
435+
methodFieldName = methodName.substring(2);
436+
}
437+
else if (methodName.startsWith("get") ||
438+
methodName.startsWith("set")) { // skip past "get" or "set"
439+
methodFieldName = methodName.substring(3);
440+
}
441+
else {
442+
// not a property accessor method so ignore it
443+
continue;
444+
}
445+
boolean propertyNameMatchesFieldName = false;
446+
// convert field letter to lower case
447+
methodFieldName = methodFieldName.substring(0, 1).toLowerCase() + methodFieldName.substring(1);
448+
TypeList typeList = methodDescription.getDeclaredAnnotations().asTypeList();
449+
if (typeList.stream().anyMatch(typeDefinitions ->
450+
(typeDefinitions.getName().contains("jakarta.persistence")))) {
451+
propertyHasAnnotation = true;
452+
}
453+
for (FieldDescription ctField : methodDescription.getDeclaringType().getDeclaredFields()) {
454+
if (!Modifier.isStatic(ctField.getModifiers())) {
455+
AnnotatedFieldDescription annotatedField = new AnnotatedFieldDescription(enhancementContext, ctField);
456+
boolean containsPropertyAccessorMethods = false;
457+
if (enhancementContext.isPersistentField(annotatedField)) {
458+
if (methodFieldName.equals(ctField.getActualName())) {
459+
propertyNameMatchesFieldName = true;
460+
break;
461+
}
462+
}
463+
}
464+
}
465+
if (propertyHasAnnotation && !propertyNameMatchesFieldName) {
466+
log.debugf("Skipping enhancement of [%s]: due to class [%s] not having a property accessor method name matching field name [%s]",
467+
managedCtClass, methodDescription.getDeclaringType().getActualName(), methodFieldName);
468+
return true;
469+
}
470+
}
471+
return false;
472+
}
473+
381474
private static void verifyVersions(TypeDescription managedCtClass, ByteBuddyEnhancementContext enhancementContext) {
382475
final AnnotationDescription.Loadable<EnhancementInfo> existingInfo = managedCtClass
383476
.getDeclaredAnnotations()
@@ -493,7 +586,7 @@ private Collection<AnnotatedFieldDescription> collectInheritCollectionFields(Typ
493586
AnnotatedFieldDescription annotatedField = new AnnotatedFieldDescription( enhancementContext, ctField );
494587
if ( enhancementContext.isPersistentField( annotatedField ) && enhancementContext.isMappedCollection( annotatedField ) ) {
495588
if ( ctField.getType().asErasure().isAssignableTo( Collection.class ) || ctField.getType().asErasure().isAssignableTo( Map.class ) ) {
496-
collectionList.add( annotatedField );
589+
collectionList.add( annotatedField );
497590
}
498591
}
499592
}

‎hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/access/InvalidPropertyNameTest.java‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/*
2+
* SPDX-License-Identifier: LGPL-2.1-or-later
3+
* Copyright Red Hat Inc. and Hibernate Authors
4+
*/
5+
package org.hibernate.orm.test.bytecode.enhancement.access;
6+
7+
import jakarta.persistence.*;
8+
import org.hibernate.testing.bytecode.enhancement.extension.BytecodeEnhanced;
9+
import org.hibernate.testing.orm.junit.*;
10+
import org.junit.jupiter.api.AfterEach;
11+
import org.junit.jupiter.api.Test;
12+
13+
import static org.assertj.core.api.Assertions.assertThat;
14+
15+
@DomainModel(
16+
annotatedClasses = {
17+
InvalidPropertyNameTest.SomeEntity.class,
18+
}
19+
)
20+
@SessionFactory
21+
@JiraKey("HHH-16572")
22+
@BytecodeEnhanced
23+
public class InvalidPropertyNameTest {
24+
25+
26+
@Test
27+
@FailureExpected(jiraKey = "HHH-16572")
28+
public void test(SessionFactoryScope scope) {
29+
scope.inTransaction( session -> {
30+
session.persist( new SomeEntity( 1L, "field", "property" ) );
31+
} );
32+
33+
scope.inTransaction( session -> {
34+
SomeEntity entity = session.get( SomeEntity.class, 1L );
35+
assertThat( entity.property ).isEqualTo( "from getter: property" );
36+
37+
entity.setPropertyMethod( "updated" );
38+
} );
39+
40+
scope.inTransaction( session -> {
41+
SomeEntity entity = session.get( SomeEntity.class, 1L );
42+
assertThat( entity.property ).isEqualTo( "from getter: updated" );
43+
} );
44+
}
45+
46+
@AfterEach
47+
public void cleanup(SessionFactoryScope scope) {
48+
// uncomment the following when @FailureExpected is removed above
49+
// scope.inTransaction( session -> {
50+
// session.remove( session.get( SomeEntity.class, 1L ) );
51+
// PropertyAccessTest} );
52+
}
53+
54+
@Entity
55+
@Table(name = "SOME_ENTITY")
56+
static class SomeEntity {
57+
@Id
58+
Long id;
59+
60+
@Basic
61+
String field;
62+
63+
String property;
64+
65+
public SomeEntity() {
66+
}
67+
68+
public SomeEntity(Long id, String field, String property) {
69+
this.id = id;
70+
this.field = field;
71+
this.property = property;
72+
}
73+
74+
/**
75+
* The following property accessor methods are purposely named incorrectly to
76+
* not match the "property" field. The HHH-16572 change ensures that
77+
* this entity is not (bytecode) enhanced. Eventually further changes will be made
78+
* such that this entity is enhanced in which case the FailureExpected can be removed
79+
* and the cleanup() uncommented.
80+
*/
81+
@Basic
82+
@Access(AccessType.PROPERTY)
83+
public String getPropertyMethod() {
84+
return "from getter: " + property;
85+
}
86+
87+
public void setPropertyMethod(String property) {
88+
this.property = property;
89+
}
90+
}
91+
}

‎hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/lazy/LazyLoadingByEnhancerSetterTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
import org.hibernate.testing.bytecode.enhancement.extension.BytecodeEnhanced;
1010
import org.hibernate.testing.orm.junit.DomainModel;
11-
import org.hibernate.testing.orm.junit.FailureExpected;
1211
import org.hibernate.testing.orm.junit.JiraKey;
1312
import org.hibernate.testing.orm.junit.ServiceRegistry;
1413
import org.hibernate.testing.orm.junit.SessionFactory;
@@ -77,7 +76,7 @@ public void testField(SessionFactoryScope scope) {
7776
}
7877

7978
@Test
80-
@FailureExpected( jiraKey = "HHH-10747" )
79+
// failure doesn't occur with HHH-16572 change @FailureExpected( jiraKey = "HHH-10747" )
8180
public void testProperty(SessionFactoryScope scope) {
8281
scope.inTransaction( s -> {
8382
ItemProperty input = new ItemProperty();

0 commit comments

Comments
 (0)