From 5f9ba76975d7ef4f04fd5be61e33a1d327452b90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Thu, 8 Jun 2023 09:26:26 +0200 Subject: [PATCH 1/5] Prepare PanacheFieldAccessMethodVisitor for further changes This does not change its behavior at all. --- .../PanacheFieldAccessMethodVisitor.java | 66 ++++++++++--------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java index ccd0c3d8acb6c..66277c09f04d4 100644 --- a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java +++ b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java @@ -9,49 +9,55 @@ public class PanacheFieldAccessMethodVisitor extends MethodVisitor { private final String methodName; - private String owner; - private String methodDescriptor; - private MetamodelInfo modelInfo; + private final String methodOwnerClassName; + private final String methodDescriptor; + private final MetamodelInfo modelInfo; - public PanacheFieldAccessMethodVisitor(MethodVisitor methodVisitor, String owner, + public PanacheFieldAccessMethodVisitor(MethodVisitor methodVisitor, String methodOwner, String methodName, String methodDescriptor, MetamodelInfo modelInfo) { super(Gizmo.ASM_API_VERSION, methodVisitor); - this.owner = owner; + this.methodOwnerClassName = methodOwner.replace('/', '.'); this.methodName = methodName; this.methodDescriptor = methodDescriptor; this.modelInfo = modelInfo; } @Override - public void visitFieldInsn(int opcode, String owner, String fieldName, String descriptor) { - String ownerName = owner.replace('/', '.'); - if ((opcode == Opcodes.GETFIELD - || opcode == Opcodes.PUTFIELD) + public void visitFieldInsn(int opcode, String fieldOwner, String fieldName, String descriptor) { + String fieldOwnerClassName = fieldOwner.replace('/', '.'); + if ( // we only care about non-static access + !(opcode == Opcodes.GETFIELD || opcode == Opcodes.PUTFIELD) // if we're in the constructor, do not replace field accesses to this type and its supertypes // otherwise we risk running setters that depend on initialisation - && (!this.methodName.equals("") - || !targetIsInHierarchy(this.owner.replace('/', '.'), ownerName)) - && isEntityField(ownerName, fieldName)) { - String methodName; - String methodDescriptor; - if (opcode == Opcodes.GETFIELD) { - methodName = JavaBeanUtil.getGetterName(fieldName, descriptor); - methodDescriptor = "()" + descriptor; - } else { - methodName = JavaBeanUtil.getSetterName(fieldName); - methodDescriptor = "(" + descriptor + ")V"; - } - if (!owner.equals(this.owner) - || !methodName.equals(this.methodName) - || !methodDescriptor.equals(this.methodDescriptor)) { - super.visitMethodInsn(Opcodes.INVOKEVIRTUAL, owner, methodName, methodDescriptor, false); - } else { - // do not substitute to accessors inside its own accessor - super.visitFieldInsn(opcode, owner, fieldName, descriptor); - } + || (this.methodName.equals("") + && targetIsInHierarchy(methodOwnerClassName, fieldOwnerClassName)) + // we only care about entity fields + || !isEntityField(fieldOwnerClassName, fieldName)) { + // In those cases, don't do anything. + super.visitFieldInsn(opcode, fieldOwner, fieldName, descriptor); + return; + } + + String methodName; + String methodDescriptor; + if (opcode == Opcodes.GETFIELD) { + methodName = JavaBeanUtil.getGetterName(fieldName, descriptor); + methodDescriptor = "()" + descriptor; + } else { + methodName = JavaBeanUtil.getSetterName(fieldName); + methodDescriptor = "(" + descriptor + ")V"; + } + if (fieldOwnerClassName.equals(this.methodOwnerClassName) + && methodName.equals(this.methodName) + && methodDescriptor.equals(this.methodDescriptor)) { + // The current method accessing the entity field is the corresponding getter/setter. + // We don't perform substitution at all. + super.visitFieldInsn(opcode, fieldOwner, fieldName, descriptor); } else { - super.visitFieldInsn(opcode, owner, fieldName, descriptor); + // The current method accessing the entity field is *not* the corresponding getter/setter. + // We found a relevant field access: replace it with a call to the getter/setter. + super.visitMethodInsn(Opcodes.INVOKEVIRTUAL, fieldOwner, methodName, methodDescriptor, false); } } From 5e72582c31b597ce1ce9847b20a635d1ba62edcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Mon, 24 Apr 2023 19:05:40 +0200 Subject: [PATCH 2/5] Panache: Replace access to non-public fields with calls to $$_hibernate_{read/write}_* methods when enhancing for Hibernate ORM --- .../BasePanacheMongoResourceProcessor.java | 13 ++--- .../common/deployment/EntityField.java | 41 ++++++++++++++- .../common/deployment/MetamodelInfo.java | 2 +- .../PanacheFieldAccessMethodVisitor.java | 49 +++++++++++++----- ...eEntityClassAccessorGenerationVisitor.java | 15 ++++-- .../deployment/pom.xml | 4 ++ ...nacheHibernateCommonResourceProcessor.java | 50 ++++++++++++++----- 7 files changed, 136 insertions(+), 38 deletions(-) diff --git a/extensions/panache/mongodb-panache-common/deployment/src/main/java/io/quarkus/mongodb/panache/deployment/BasePanacheMongoResourceProcessor.java b/extensions/panache/mongodb-panache-common/deployment/src/main/java/io/quarkus/mongodb/panache/deployment/BasePanacheMongoResourceProcessor.java index 199f8fae50735..92062687a109d 100644 --- a/extensions/panache/mongodb-panache-common/deployment/src/main/java/io/quarkus/mongodb/panache/deployment/BasePanacheMongoResourceProcessor.java +++ b/extensions/panache/mongodb-panache-common/deployment/src/main/java/io/quarkus/mongodb/panache/deployment/BasePanacheMongoResourceProcessor.java @@ -289,14 +289,14 @@ protected void processEntities(CombinedIndexBuildItem index, } private void replaceFieldAccesses(BuildProducer transformers, MetamodelInfo modelInfo) { - Set entitiesWithPublicFields = modelInfo.getEntitiesWithPublicFields(); - if (entitiesWithPublicFields.isEmpty()) { - // There are no public fields to be accessed in the first place. + Set entitiesWithExternallyAccessibleFields = modelInfo.getEntitiesWithExternallyAccessibleFields(); + if (entitiesWithExternallyAccessibleFields.isEmpty()) { + // There are no fields to be accessed in the first place. return; } Set entityClassNamesInternal = new HashSet<>(); - for (String entityClassName : entitiesWithPublicFields) { + for (String entityClassName : entitiesWithExternallyAccessibleFields) { entityClassNamesInternal.add(entityClassName.replace(".", "/")); } @@ -323,10 +323,11 @@ private EntityModel createEntityModel(ClassInfo classInfo) { EntityModel entityModel = new EntityModel(classInfo); for (FieldInfo fieldInfo : classInfo.fields()) { String name = fieldInfo.name(); - if (Modifier.isPublic(fieldInfo.flags()) + var visibility = EntityField.Visibility.get(fieldInfo.flags()); + if (EntityField.Visibility.PUBLIC.equals(visibility) && !Modifier.isStatic(fieldInfo.flags()) && !fieldInfo.hasAnnotation(BSON_IGNORE)) { - entityModel.addField(new EntityField(name, DescriptorUtils.typeToString(fieldInfo.type()))); + entityModel.addField(new EntityField(name, DescriptorUtils.typeToString(fieldInfo.type()), visibility)); } } return entityModel; diff --git a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/EntityField.java b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/EntityField.java index a9a4da67fc828..3645c592c95be 100644 --- a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/EntityField.java +++ b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/EntityField.java @@ -1,5 +1,6 @@ package io.quarkus.panache.common.deployment; +import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; @@ -10,6 +11,7 @@ import org.objectweb.asm.AnnotationVisitor; import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; import io.quarkus.deployment.bean.JavaBeanUtil; @@ -17,12 +19,24 @@ public class EntityField { public final String name; public final String descriptor; + public final Visibility visibility; + public final String librarySpecificGetterName; + public final String librarySpecificSetterName; + public String signature; public final Set annotations = new HashSet<>(2); - public EntityField(String name, String descriptor) { + public EntityField(String name, String descriptor, Visibility visibility) { + this(name, descriptor, visibility, null, null); + } + + public EntityField(String name, String descriptor, Visibility visibility, + String librarySpecificGetterName, String librarySpecificSetterName) { this.name = name; this.descriptor = descriptor; + this.visibility = visibility; + this.librarySpecificGetterName = librarySpecificGetterName; + this.librarySpecificSetterName = librarySpecificSetterName; } public String getGetterName() { @@ -42,6 +56,31 @@ public boolean hasAnnotation(String descriptor) { return false; } + public enum Visibility { + PUBLIC(Opcodes.ACC_PUBLIC), + PROTECTED(Opcodes.ACC_PROTECTED), + PACKAGE_PRIVATE(0), + PRIVATE(Opcodes.ACC_PRIVATE); + + public final int accessOpCode; + + Visibility(int accessOpCode) { + this.accessOpCode = accessOpCode; + } + + public static Visibility get(int flags) { + if (Modifier.isPublic(flags)) { + return PUBLIC; + } else if (Modifier.isPrivate(flags)) { + return PRIVATE; + } else if (Modifier.isProtected(flags)) { + return PROTECTED; + } else { + return PACKAGE_PRIVATE; + } + } + } + public static class EntityFieldAnnotation { public final String descriptor; public final Map attributes = new HashMap<>(2); diff --git a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/MetamodelInfo.java b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/MetamodelInfo.java index c6d3c3fd5afe8..d71e99f2f9129 100644 --- a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/MetamodelInfo.java +++ b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/MetamodelInfo.java @@ -16,7 +16,7 @@ public void addEntityModel(EntityModel entityModel) { entities.put(entityModel.name, entityModel); } - public Set getEntitiesWithPublicFields() { + public Set getEntitiesWithExternallyAccessibleFields() { return entities.entrySet().stream() .filter(e -> { EntityModel value = e.getValue(); diff --git a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java index 66277c09f04d4..821050668ce97 100644 --- a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java +++ b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java @@ -3,7 +3,6 @@ import org.objectweb.asm.MethodVisitor; import org.objectweb.asm.Opcodes; -import io.quarkus.deployment.bean.JavaBeanUtil; import io.quarkus.gizmo.Gizmo; public class PanacheFieldAccessMethodVisitor extends MethodVisitor { @@ -31,25 +30,49 @@ public void visitFieldInsn(int opcode, String fieldOwner, String fieldName, Stri // if we're in the constructor, do not replace field accesses to this type and its supertypes // otherwise we risk running setters that depend on initialisation || (this.methodName.equals("") - && targetIsInHierarchy(methodOwnerClassName, fieldOwnerClassName)) - // we only care about entity fields - || !isEntityField(fieldOwnerClassName, fieldName)) { + && targetIsInHierarchy(methodOwnerClassName, fieldOwnerClassName))) { // In those cases, don't do anything. super.visitFieldInsn(opcode, fieldOwner, fieldName, descriptor); return; } - String methodName; + EntityField entityField = getEntityField(fieldOwnerClassName, fieldName); + if (entityField == null) { + // Not an entity field: don't do anything. + super.visitFieldInsn(opcode, fieldOwner, fieldName, descriptor); + return; + } + + String javaBeanMethodName; + String librarySpecificMethodName; String methodDescriptor; if (opcode == Opcodes.GETFIELD) { - methodName = JavaBeanUtil.getGetterName(fieldName, descriptor); + javaBeanMethodName = entityField.getGetterName(); + librarySpecificMethodName = entityField.librarySpecificGetterName; methodDescriptor = "()" + descriptor; } else { - methodName = JavaBeanUtil.getSetterName(fieldName); + javaBeanMethodName = entityField.getSetterName(); + librarySpecificMethodName = entityField.librarySpecificSetterName; methodDescriptor = "(" + descriptor + ")V"; } + + // For public fields, we'll replace field access with (potentially generated) JavaBean accessors. + // For non-public fields, we won't be using JavaBean accessors, + // because we might not generate any and pre-existing ones might have unwanted side effects. + // Instead, we will replace the field access with a call to internal field access methods, if any, + // e.g. generated methods like $$_hibernate_read_myProperty() + boolean useJavaBeanAccessor = EntityField.Visibility.PUBLIC.equals(entityField.visibility); + if (!useJavaBeanAccessor && librarySpecificMethodName == null) { + // Field is non-public, so we won't be using JavaBean accessors, + // and there are no library-specific accessors: + // don't do anything. + super.visitFieldInsn(opcode, fieldOwner, fieldName, descriptor); + return; + } + String methodName = useJavaBeanAccessor ? javaBeanMethodName : librarySpecificMethodName; + if (fieldOwnerClassName.equals(this.methodOwnerClassName) - && methodName.equals(this.methodName) + && javaBeanMethodName.equals(this.methodName) && methodDescriptor.equals(this.methodDescriptor)) { // The current method accessing the entity field is the corresponding getter/setter. // We don't perform substitution at all. @@ -79,15 +102,15 @@ private boolean targetIsInHierarchy(String currentClass, String targetClass) { /** * Checks that the given field belongs to an entity (any entity) */ - boolean isEntityField(String className, String fieldName) { + EntityField getEntityField(String className, String fieldName) { EntityModel entityModel = modelInfo.getEntityModel(className); if (entityModel == null) - return false; + return null; EntityField field = entityModel.fields.get(fieldName); if (field != null) - return true; + return field; if (entityModel.superClassName != null) - return isEntityField(entityModel.superClassName, fieldName); - return false; + return getEntityField(entityModel.superClassName, fieldName); + return null; } } diff --git a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/visitors/PanacheEntityClassAccessorGenerationVisitor.java b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/visitors/PanacheEntityClassAccessorGenerationVisitor.java index 195c1714b73d4..f7b5bebaaa70b 100644 --- a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/visitors/PanacheEntityClassAccessorGenerationVisitor.java +++ b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/visitors/PanacheEntityClassAccessorGenerationVisitor.java @@ -62,8 +62,10 @@ public PanacheEntityClassAccessorGenerationVisitor(ClassVisitor outputClassVisit @Override public FieldVisitor visitField(int access, String name, String descriptor, String signature, Object value) { EntityField entityField = fields == null ? null : fields.get(name); - if (entityField == null || hasGetterForField(entityField)) { - // If the getter for this entity field already exists, + if (entityField == null || !EntityField.Visibility.PUBLIC.equals(entityField.visibility) + || hasGetterForField(entityField)) { + // If the field does not exist or is non-public, + // or if the getter for this entity field already exists, // we won't alter it in any way. return super.visitField(access, name, descriptor, signature, value); } @@ -128,11 +130,16 @@ private void generateAccessors() { if (fields == null) return; for (EntityField field : fields.values()) { + if (!EntityField.Visibility.PUBLIC.equals(field.visibility)) { + // We don't generate accessors for non-public fields + // (but may rely on library-specific accessors in other places) + continue; + } // Getter String getterName = field.getGetterName(); String getterDescriptor = "()" + field.descriptor; if (!userMethods.contains(getterName + "/" + getterDescriptor)) { - MethodVisitor mv = super.visitMethod(Opcodes.ACC_PUBLIC, + MethodVisitor mv = super.visitMethod(field.visibility.accessOpCode, getterName, getterDescriptor, field.signature == null ? null : "()" + field.signature, null); mv.visitCode(); mv.visitIntInsn(Opcodes.ALOAD, 0); @@ -165,7 +172,7 @@ private void generateAccessors() { String setterName = field.getSetterName(); String setterDescriptor = "(" + field.descriptor + ")V"; if (!userMethods.contains(setterName + "/" + setterDescriptor)) { - MethodVisitor mv = super.visitMethod(Opcodes.ACC_PUBLIC, + MethodVisitor mv = super.visitMethod(field.visibility.accessOpCode, setterName, setterDescriptor, field.signature == null ? null : "(" + field.signature + ")V", null); mv.visitCode(); mv.visitIntInsn(Opcodes.ALOAD, 0); diff --git a/extensions/panache/panache-hibernate-common/deployment/pom.xml b/extensions/panache/panache-hibernate-common/deployment/pom.xml index 6592af6cd1c03..dcd39aa2df0ae 100644 --- a/extensions/panache/panache-hibernate-common/deployment/pom.xml +++ b/extensions/panache/panache-hibernate-common/deployment/pom.xml @@ -32,6 +32,10 @@ org.ow2.asm asm + + org.hibernate.orm + hibernate-core + diff --git a/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheHibernateCommonResourceProcessor.java b/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheHibernateCommonResourceProcessor.java index 82cf77985d58c..d362d256c3b83 100644 --- a/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheHibernateCommonResourceProcessor.java +++ b/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheHibernateCommonResourceProcessor.java @@ -8,8 +8,12 @@ import java.util.Optional; import java.util.Set; +import jakarta.persistence.Embeddable; +import jakarta.persistence.Entity; +import jakarta.persistence.MappedSuperclass; import jakarta.persistence.Transient; +import org.hibernate.bytecode.enhance.spi.EnhancerConstants; import org.jboss.jandex.ClassInfo; import org.jboss.jandex.DotName; import org.jboss.jandex.FieldInfo; @@ -26,6 +30,9 @@ public final class PanacheHibernateCommonResourceProcessor { + private static final DotName DOTNAME_ENTITY = DotName.createSimple(Entity.class.getName()); + private static final DotName DOTNAME_MAPPED_SUPERCLASS = DotName.createSimple(MappedSuperclass.class.getName()); + private static final DotName DOTNAME_EMBEDDABLE = DotName.createSimple(Embeddable.class.getName()); private static final DotName DOTNAME_TRANSIENT = DotName.createSimple(Transient.class.getName()); private static final DotName DOTNAME_KOTLIN_METADATA = DotName.createSimple("kotlin.Metadata"); @@ -68,15 +75,15 @@ void findEntityClasses(CombinedIndexBuildItem index, // Share the metamodel for use in replaceFieldAccesses modelInfoBuildItem.produce(new HibernateMetamodelForFieldAccessBuildItem(modelInfo)); - Set entitiesWithPublicFields = modelInfo.getEntitiesWithPublicFields(); - if (entitiesWithPublicFields.isEmpty()) { - // There are no public fields to be accessed in the first place. + Set entitiesWithExternallyAccessibleFields = modelInfo.getEntitiesWithExternallyAccessibleFields(); + if (entitiesWithExternallyAccessibleFields.isEmpty()) { + // There are no fields to be accessed in the first place. return; } // Share with other extensions that we will generate accessors for some classes fieldAccessEnhancedEntityClasses - .produce(new PanacheEntityClassesBuildItem(entitiesWithPublicFields)); + .produce(new PanacheEntityClassesBuildItem(entitiesWithExternallyAccessibleFields)); } @BuildStep @@ -91,23 +98,23 @@ void replaceFieldAccesses(CombinedIndexBuildItem index, } MetamodelInfo modelInfo = modelInfoBuildItem.get().getMetamodelInfo(); - Set entitiesWithPublicFields = modelInfo.getEntitiesWithPublicFields(); - if (entitiesWithPublicFields.isEmpty()) { - // There are no public fields to be accessed in the first place. + Set entitiesWithExternallyAccessibleFields = modelInfo.getEntitiesWithExternallyAccessibleFields(); + if (entitiesWithExternallyAccessibleFields.isEmpty()) { + // There are no fields to be accessed in the first place. return; } - // Generate accessors for public fields in entities, mapped superclasses + // Generate accessors for externally accessible fields in entities, mapped superclasses // (and embeddables, see where we build modelInfo above). PanacheJpaEntityAccessorsEnhancer entityAccessorsEnhancer = new PanacheJpaEntityAccessorsEnhancer(index.getIndex(), modelInfo); - for (String entityClassName : entitiesWithPublicFields) { + for (String entityClassName : entitiesWithExternallyAccessibleFields) { transformers.produce(new BytecodeTransformerBuildItem(true, entityClassName, entityAccessorsEnhancer)); } // Replace field access in application code with calls to accessors Set entityClassNamesInternal = new HashSet<>(); - for (String entityClassName : entitiesWithPublicFields) { + for (String entityClassName : entitiesWithExternallyAccessibleFields) { entityClassNamesInternal.add(entityClassName.replace(".", "/")); } @@ -144,12 +151,29 @@ void replaceFieldAccesses(CombinedIndexBuildItem index, private EntityModel createEntityModel(ClassInfo classInfo) { EntityModel entityModel = new EntityModel(classInfo); + // Unfortunately, at the moment Hibernate ORM's enhancement ignores XML mapping, + // so we need to be careful when we enhance private fields, + // because the corresponding `$_hibernate_{read/write}_*()` methods + // will only be generated for classes mapped through *annotations*. + boolean willBeEnhancedByHibernateOrm = classInfo.hasAnnotation(DOTNAME_ENTITY) + || classInfo.hasAnnotation(DOTNAME_MAPPED_SUPERCLASS) + || classInfo.hasAnnotation(DOTNAME_EMBEDDABLE); for (FieldInfo fieldInfo : classInfo.fields()) { String name = fieldInfo.name(); - if (Modifier.isPublic(fieldInfo.flags()) - && !Modifier.isStatic(fieldInfo.flags()) + if (!Modifier.isStatic(fieldInfo.flags()) && !fieldInfo.hasAnnotation(DOTNAME_TRANSIENT)) { - entityModel.addField(new EntityField(name, DescriptorUtils.typeToString(fieldInfo.type()))); + String librarySpecificGetterName; + String librarySpecificSetterName; + if (willBeEnhancedByHibernateOrm) { + librarySpecificGetterName = EnhancerConstants.PERSISTENT_FIELD_READER_PREFIX + name; + librarySpecificSetterName = EnhancerConstants.PERSISTENT_FIELD_WRITER_PREFIX + name; + } else { + librarySpecificGetterName = null; + librarySpecificSetterName = null; + } + entityModel.addField(new EntityField(name, DescriptorUtils.typeToString(fieldInfo.type()), + EntityField.Visibility.get(fieldInfo.flags()), + librarySpecificGetterName, librarySpecificSetterName)); } } return entityModel; From 29089dc4e646afad3457fb618671720a9b7525ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Tue, 25 Apr 2023 10:28:33 +0200 Subject: [PATCH 3/5] Panache: Replace access to superclass fields with calls to $$_hibernate_{read/write}_* methods when enhancing for Hibernate ORM Instead of leaving the field access in place. --- .../PanacheFieldAccessMethodVisitor.java | 54 ++++++++++++------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java index 821050668ce97..053de5e227b15 100644 --- a/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java +++ b/extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheFieldAccessMethodVisitor.java @@ -36,12 +36,13 @@ && targetIsInHierarchy(methodOwnerClassName, fieldOwnerClassName))) { return; } - EntityField entityField = getEntityField(fieldOwnerClassName, fieldName); - if (entityField == null) { - // Not an entity field: don't do anything. + EntityModel declaringEntityModel = getDeclaringEntityModel(fieldOwnerClassName, fieldName); + if (declaringEntityModel == null) { + // Not an entity field, don't do anything. super.visitFieldInsn(opcode, fieldOwner, fieldName, descriptor); return; } + EntityField entityField = declaringEntityModel.fields.get(fieldName); String javaBeanMethodName; String librarySpecificMethodName; @@ -62,8 +63,29 @@ && targetIsInHierarchy(methodOwnerClassName, fieldOwnerClassName))) { // Instead, we will replace the field access with a call to internal field access methods, if any, // e.g. generated methods like $$_hibernate_read_myProperty() boolean useJavaBeanAccessor = EntityField.Visibility.PUBLIC.equals(entityField.visibility); + + if (fieldOwnerClassName.equals(this.methodOwnerClassName) + && javaBeanMethodName.equals(this.methodName) + && methodDescriptor.equals(this.methodDescriptor)) { + // The current method accessing the entity field is the corresponding getter/setter. + + // If the current method is defined in the class that declares the field, + // we don't perform substitution at all. + if (declaringEntityModel.name.equals(methodOwnerClassName)) { + super.visitFieldInsn(opcode, fieldOwner, fieldName, descriptor); + return; + } + + // Otherwise the current method is considered an override of + // the getter/setter which should be defined (manually or through bytecode generation) + // in the superclass that also declares the field. + // We will replace the field access with a call to internal field access methods, if any, + // e.g. generated methods like $$_hibernate_read_myProperty() + useJavaBeanAccessor = false; + } + if (!useJavaBeanAccessor && librarySpecificMethodName == null) { - // Field is non-public, so we won't be using JavaBean accessors, + // We won't be using JavaBean accessors, // and there are no library-specific accessors: // don't do anything. super.visitFieldInsn(opcode, fieldOwner, fieldName, descriptor); @@ -71,17 +93,8 @@ && targetIsInHierarchy(methodOwnerClassName, fieldOwnerClassName))) { } String methodName = useJavaBeanAccessor ? javaBeanMethodName : librarySpecificMethodName; - if (fieldOwnerClassName.equals(this.methodOwnerClassName) - && javaBeanMethodName.equals(this.methodName) - && methodDescriptor.equals(this.methodDescriptor)) { - // The current method accessing the entity field is the corresponding getter/setter. - // We don't perform substitution at all. - super.visitFieldInsn(opcode, fieldOwner, fieldName, descriptor); - } else { - // The current method accessing the entity field is *not* the corresponding getter/setter. - // We found a relevant field access: replace it with a call to the getter/setter. - super.visitMethodInsn(Opcodes.INVOKEVIRTUAL, fieldOwner, methodName, methodDescriptor, false); - } + // We found a relevant field access: replace it with a call to the correct accessor. + super.visitMethodInsn(Opcodes.INVOKEVIRTUAL, fieldOwner, methodName, methodDescriptor, false); } /** @@ -100,17 +113,18 @@ private boolean targetIsInHierarchy(String currentClass, String targetClass) { } /** - * Checks that the given field belongs to an entity (any entity) + * Returns the entity model that declares the given field in the hierarchy of the given class. */ - EntityField getEntityField(String className, String fieldName) { - EntityModel entityModel = modelInfo.getEntityModel(className); + EntityModel getDeclaringEntityModel(String encounteredClassName, String fieldName) { + EntityModel entityModel = modelInfo.getEntityModel(encounteredClassName); if (entityModel == null) return null; EntityField field = entityModel.fields.get(fieldName); if (field != null) - return field; + return entityModel; if (entityModel.superClassName != null) - return getEntityField(entityModel.superClassName, fieldName); + return getDeclaringEntityModel(entityModel.superClassName, fieldName); return null; } + } From fa3ce5626ea5666b6b322bc69b4a968ae838c619 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Mon, 24 Apr 2023 18:22:28 +0200 Subject: [PATCH 4/5] Test non-standard access to entity properties I.e. direct access to superclass fields and non-public fields --- .../NonStandardAccessTest.java | 917 ++++++++++++++++++ 1 file changed, 917 insertions(+) create mode 100644 extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/applicationfieldaccess/NonStandardAccessTest.java diff --git a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/applicationfieldaccess/NonStandardAccessTest.java b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/applicationfieldaccess/NonStandardAccessTest.java new file mode 100644 index 0000000000000..caf1700336110 --- /dev/null +++ b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/applicationfieldaccess/NonStandardAccessTest.java @@ -0,0 +1,917 @@ +package io.quarkus.hibernate.orm.applicationfieldaccess; + +import static org.assertj.core.api.Assertions.assertThat; + +import jakarta.inject.Inject; +import jakarta.persistence.Entity; +import jakarta.persistence.EntityManager; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.Id; +import jakarta.persistence.MappedSuperclass; + +import org.hibernate.Hibernate; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.quarkus.narayana.jta.QuarkusTransaction; +import io.quarkus.test.QuarkusUnitTest; + +/** + * Checks that non-standard access to fields by the application works correctly, + * i.e. when exposing fields through accessors with non-standard names, + * static accessors, accessors defined in a subclass, + * or accessors defined in an inner class. + */ +public class NonStandardAccessTest { + + @RegisterExtension + static QuarkusUnitTest runner = new QuarkusUnitTest() + .withApplicationRoot((jar) -> jar + .addClasses(MyAbstractMappedSuperClass.class, MyAbstractEntity.class, MyAbstractConfusingEntity.class, + MyConcreteEntity.class) + .addClass(ExternalClassAccessors.class) + .addClass(AccessDelegate.class)) + .withConfigurationResource("application.properties"); + + @Inject + EntityManager em; + + @Test + public void nonStandardInstanceGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForPublicField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForPublicField(); + } + }); + } + + @Test + public void nonStandardInstanceGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForProtectedField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForProtectedField(); + } + }); + } + + @Test + public void nonStandardInstanceGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForPackagePrivateField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForPackagePrivateField(); + } + }); + } + + @Test + public void nonStandardInstanceGetterSetterPrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForPrivateField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForPrivateField(); + } + }); + } + + @Test + public void staticGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + MyConcreteEntity.staticSetPublicField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return MyConcreteEntity.staticGetPublicField(entity); + } + }); + } + + @Test + public void staticGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + MyConcreteEntity.staticSetProtectedField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return MyConcreteEntity.staticGetProtectedField(entity); + } + }); + } + + @Test + public void staticGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + MyConcreteEntity.staticSetPackagePrivateField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return MyConcreteEntity.staticGetPackagePrivateField(entity); + } + }); + } + + @Test + public void staticGetterSetterPrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + MyConcreteEntity.staticSetPrivateField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return MyConcreteEntity.staticGetPrivateField(entity); + } + }); + } + + @Test + public void innerClassStaticGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + MyConcreteEntity.InnerClassAccessors.staticSetPublicField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return MyConcreteEntity.InnerClassAccessors.staticGetPublicField(entity); + } + }); + } + + @Test + public void innerClassStaticGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + MyConcreteEntity.InnerClassAccessors.staticSetProtectedField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return MyConcreteEntity.InnerClassAccessors.staticGetProtectedField(entity); + } + }); + } + + @Test + public void innerClassStaticGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + MyConcreteEntity.InnerClassAccessors.staticSetPackagePrivateField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return MyConcreteEntity.InnerClassAccessors.staticGetPackagePrivateField(entity); + } + }); + } + + @Test + public void innerClassStaticGetterSetterPrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + MyConcreteEntity.InnerClassAccessors.staticSetPrivateField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return MyConcreteEntity.InnerClassAccessors.staticGetPrivateField(entity); + } + }); + } + + @Test + public void innerClassInstanceGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + new MyConcreteEntity.InnerClassAccessors().instanceSetPublicField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return new MyConcreteEntity.InnerClassAccessors().instanceGetPublicField(entity); + } + }); + } + + @Test + public void innerClassInstanceGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + new MyConcreteEntity.InnerClassAccessors().instanceSetProtectedField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return new MyConcreteEntity.InnerClassAccessors().instanceGetProtectedField(entity); + } + }); + } + + @Test + public void innerClassInstanceGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + new MyConcreteEntity.InnerClassAccessors().instanceSetPackagePrivateField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return new MyConcreteEntity.InnerClassAccessors().instanceGetPackagePrivateField(entity); + } + }); + } + + @Test + public void innerClassInstanceGetterSetterPrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + new MyConcreteEntity.InnerClassAccessors().instanceSetPrivateField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return new MyConcreteEntity.InnerClassAccessors().instanceGetPrivateField(entity); + } + }); + } + + @Test + public void externalClassStaticGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + ExternalClassAccessors.staticSetPublicField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return ExternalClassAccessors.staticGetPublicField(entity); + } + }); + } + + @Test + public void externalClassStaticGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + ExternalClassAccessors.staticSetProtectedField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return ExternalClassAccessors.staticGetProtectedField(entity); + } + }); + } + + @Test + public void externalClassStaticGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + ExternalClassAccessors.staticSetPackagePrivateField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return ExternalClassAccessors.staticGetPackagePrivateField(entity); + } + }); + } + + @Test + public void externalClassInstanceGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + new ExternalClassAccessors().instanceSetPublicField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return new ExternalClassAccessors().instanceGetPublicField(entity); + } + }); + } + + @Test + public void externalClassInstanceGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + new ExternalClassAccessors().instanceSetProtectedField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return new ExternalClassAccessors().instanceGetProtectedField(entity); + } + }); + } + + @Test + public void externalClassInstanceGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + new ExternalClassAccessors().instanceSetPackagePrivateField(entity, value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return new ExternalClassAccessors().instanceGetPackagePrivateField(entity); + } + }); + } + + @Test + public void mappedSuperClassSubClassInstanceGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.setAbstractEntityPublicField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.getAbstractEntityPublicField(); + } + }); + } + + @Test + public void mappedSuperClassSubClassInstanceGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.setAbstractEntityProtectedField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.getAbstractEntityProtectedField(); + } + }); + } + + @Test + public void mappedSuperClassSubClassInstanceGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.setAbstractEntityPackagePrivateField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.getAbstractEntityPackagePrivateField(); + } + }); + } + + @Test + public void mappedSuperClassSubClassNonStandardInstanceGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForAbstractEntityPublicField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForAbstractEntityPublicField(); + } + }); + } + + @Test + public void mappedSuperClassSubClassNonStandardInstanceGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForAbstractEntityProtectedField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForAbstractEntityProtectedField(); + } + }); + } + + @Test + public void mappedSuperClassSubClassNonStandardInstanceGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForAbstractEntityPackagePrivateField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForAbstractEntityPackagePrivateField(); + } + }); + } + + @Test + public void entitySubClassInstanceGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.setAbstractEntityPublicField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.getAbstractEntityPublicField(); + } + }); + } + + @Test + public void entitySubClassInstanceGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.setAbstractEntityProtectedField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.getAbstractEntityProtectedField(); + } + }); + } + + @Test + public void entitySubClassInstanceGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.setAbstractEntityPackagePrivateField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.getAbstractEntityPackagePrivateField(); + } + }); + } + + @Test + public void entitySubClassNonStandardInstanceGetterSetterPublicField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForAbstractEntityPublicField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForAbstractEntityPublicField(); + } + }); + } + + @Test + public void entitySubClassNonStandardInstanceGetterSetterProtectedField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForAbstractEntityProtectedField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForAbstractEntityProtectedField(); + } + }); + } + + @Test + public void entitySubClassNonStandardInstanceGetterSetterPackagePrivateField() { + doTestFieldAccess(new AccessDelegate() { + @Override + public void setValue(MyConcreteEntity entity, Long value) { + entity.nonStandardSetterForAbstractEntityPackagePrivateField(value); + } + + @Override + public Long getValue(MyConcreteEntity entity) { + return entity.nonStandardGetterForAbstractEntityPackagePrivateField(); + } + }); + } + + // Ideally we'd make this a @ParameterizedTest and pass the access delegate as parameter, + // but we cannot do that due to JUnit using a different classloader than the test. + private void doTestFieldAccess(AccessDelegate delegate) { + Long id = QuarkusTransaction.disallowingExisting().call(() -> { + var entity = new MyConcreteEntity(); + em.persist(entity); + return entity.id; + }); + + QuarkusTransaction.disallowingExisting().run(() -> { + var entity = em.find(MyConcreteEntity.class, id); + assertThat(delegate.getValue(entity)) + .as("Loaded value before update") + .isNull(); + }); + + QuarkusTransaction.disallowingExisting().run(() -> { + var entity = em.getReference(MyConcreteEntity.class, id); + // Since field access is replaced with accessor calls, + // we expect this change to be detected by dirty tracking and persisted. + delegate.setValue(entity, 42L); + }); + + QuarkusTransaction.disallowingExisting().run(() -> { + var entity = em.find(MyConcreteEntity.class, id); + // We're working on an initialized entity. + assertThat(entity) + .as("find() should return uninitialized entity") + .returns(true, Hibernate::isInitialized); + // The above should have persisted a value that passes the assertion. + assertThat(delegate.getValue(entity)) + .as("Loaded value after update") + .isEqualTo(42L); + }); + + QuarkusTransaction.disallowingExisting().run(() -> { + var entity = em.getReference(MyConcreteEntity.class, id); + // We're working on an uninitialized entity. + assertThat(entity) + .as("getReference() should return uninitialized entity") + .returns(false, Hibernate::isInitialized); + // The above should have persisted a value that passes the assertion. + assertThat(delegate.getValue(entity)) + .as("Lazily loaded value after update") + .isEqualTo(42L); + // Accessing the value should trigger initialization of the entity. + assertThat(entity) + .as("Getting the value should initialize the entity") + .returns(true, Hibernate::isInitialized); + }); + } + + @MappedSuperclass + public static abstract class MyAbstractMappedSuperClass { + public Long abstractMappedSuperClassPublicField; + + protected Long abstractMappedSuperClassProtectedField; + + Long abstractMappedSuperClassPackagePrivateField; + } + + @Entity(name = "abstract") + public static abstract class MyAbstractEntity extends MyAbstractMappedSuperClass { + @Id + @GeneratedValue + public long id; + + public Long abstractEntityPublicField; + + protected Long abstractEntityProtectedField; + + Long abstractEntityPackagePrivateField; + } + + // Just to confuse panache: the fields are not declared in the *direct* superclass. + @Entity(name = "abstract2") + public static abstract class MyAbstractConfusingEntity extends MyAbstractEntity { + } + + @Entity(name = "concrete") + public static class MyConcreteEntity extends MyAbstractConfusingEntity { + public Long publicField; + protected Long protectedField; + Long packagePrivateField; + private Long privateField; + + public Long getAbstractMappedSuperClassPublicField() { + return abstractMappedSuperClassPublicField; + } + + public void setAbstractMappedSuperClassPublicField(Long abstractMappedSuperClassPublicField) { + this.abstractMappedSuperClassPublicField = abstractMappedSuperClassPublicField; + } + + public Long nonStandardGetterForAbstractMappedSuperClassPublicField() { + return abstractMappedSuperClassPublicField; + } + + public void nonStandardSetterForAbstractMappedSuperClassPublicField(Long value) { + abstractMappedSuperClassPublicField = value; + } + + public Long getAbstractMappedSuperClassProtectedField() { + return abstractMappedSuperClassProtectedField; + } + + public void setAbstractMappedSuperClassProtectedField(Long abstractMappedSuperClassProtectedField) { + this.abstractMappedSuperClassProtectedField = abstractMappedSuperClassProtectedField; + } + + public Long nonStandardGetterForAbstractMappedSuperClassProtectedField() { + return abstractMappedSuperClassProtectedField; + } + + public void nonStandardSetterForAbstractMappedSuperClassProtectedField(Long value) { + abstractMappedSuperClassProtectedField = value; + } + + public Long getAbstractMappedSuperClassPackagePrivateField() { + return abstractMappedSuperClassPackagePrivateField; + } + + public void setAbstractMappedSuperClassPackagePrivateField(Long abstractMappedSuperClassPackagePrivateField) { + this.abstractMappedSuperClassPackagePrivateField = abstractMappedSuperClassPackagePrivateField; + } + + public Long nonStandardGetterForAbstractMappedSuperClassPackagePrivateField() { + return abstractMappedSuperClassPackagePrivateField; + } + + public void nonStandardSetterForAbstractMappedSuperClassPackagePrivateField(Long value) { + abstractMappedSuperClassPackagePrivateField = value; + } + + public Long getAbstractEntityPublicField() { + return abstractEntityPublicField; + } + + public void setAbstractEntityPublicField(Long abstractEntityPublicField) { + this.abstractEntityPublicField = abstractEntityPublicField; + } + + public Long nonStandardGetterForAbstractEntityPublicField() { + return abstractEntityPublicField; + } + + public void nonStandardSetterForAbstractEntityPublicField(Long value) { + abstractEntityPublicField = value; + } + + public Long getAbstractEntityProtectedField() { + return abstractEntityProtectedField; + } + + public void setAbstractEntityProtectedField(Long abstractEntityProtectedField) { + this.abstractEntityProtectedField = abstractEntityProtectedField; + } + + public Long nonStandardGetterForAbstractEntityProtectedField() { + return abstractEntityProtectedField; + } + + public void nonStandardSetterForAbstractEntityProtectedField(Long value) { + abstractEntityProtectedField = value; + } + + public Long getAbstractEntityPackagePrivateField() { + return abstractEntityPackagePrivateField; + } + + public void setAbstractEntityPackagePrivateField(Long abstractEntityPackagePrivateField) { + this.abstractEntityPackagePrivateField = abstractEntityPackagePrivateField; + } + + public Long nonStandardGetterForAbstractEntityPackagePrivateField() { + return abstractEntityPackagePrivateField; + } + + public void nonStandardSetterForAbstractEntityPackagePrivateField(Long value) { + abstractEntityPackagePrivateField = value; + } + + public Long nonStandardGetterForPublicField() { + return publicField; + } + + public void nonStandardSetterForPublicField(Long value) { + publicField = value; + } + + public Long nonStandardGetterForPackagePrivateField() { + return packagePrivateField; + } + + public void nonStandardSetterForPackagePrivateField(Long value) { + packagePrivateField = value; + } + + public Long nonStandardGetterForProtectedField() { + return protectedField; + } + + public void nonStandardSetterForProtectedField(Long value) { + protectedField = value; + } + + public Long nonStandardGetterForPrivateField() { + return privateField; + } + + public void nonStandardSetterForPrivateField(Long value) { + privateField = value; + } + + public static Long staticGetPublicField(MyConcreteEntity entity) { + return entity.publicField; + } + + public static void staticSetPublicField(MyConcreteEntity entity, Long value) { + entity.publicField = value; + } + + public static Long staticGetProtectedField(MyConcreteEntity entity) { + return entity.protectedField; + } + + public static void staticSetProtectedField(MyConcreteEntity entity, Long value) { + entity.protectedField = value; + } + + public static Long staticGetPackagePrivateField(MyConcreteEntity entity) { + return entity.packagePrivateField; + } + + public static void staticSetPackagePrivateField(MyConcreteEntity entity, Long value) { + entity.packagePrivateField = value; + } + + public static Long staticGetPrivateField(MyConcreteEntity entity) { + return entity.privateField; + } + + public static void staticSetPrivateField(MyConcreteEntity entity, Long value) { + entity.privateField = value; + } + + public static final class InnerClassAccessors { + public static Long staticGetPublicField(MyConcreteEntity entity) { + return entity.publicField; + } + + public static void staticSetPublicField(MyConcreteEntity entity, Long value) { + entity.publicField = value; + } + + public static Long staticGetProtectedField(MyConcreteEntity entity) { + return entity.protectedField; + } + + public static void staticSetProtectedField(MyConcreteEntity entity, Long value) { + entity.protectedField = value; + } + + public static Long staticGetPackagePrivateField(MyConcreteEntity entity) { + return entity.packagePrivateField; + } + + public static void staticSetPackagePrivateField(MyConcreteEntity entity, Long value) { + entity.packagePrivateField = value; + } + + public static Long staticGetPrivateField(MyConcreteEntity entity) { + return entity.privateField; + } + + public static void staticSetPrivateField(MyConcreteEntity entity, Long value) { + entity.privateField = value; + } + + public Long instanceGetPublicField(MyConcreteEntity entity) { + return entity.publicField; + } + + public void instanceSetPublicField(MyConcreteEntity entity, Long value) { + entity.publicField = value; + } + + public Long instanceGetProtectedField(MyConcreteEntity entity) { + return entity.protectedField; + } + + public void instanceSetProtectedField(MyConcreteEntity entity, Long value) { + entity.protectedField = value; + } + + public Long instanceGetPackagePrivateField(MyConcreteEntity entity) { + return entity.packagePrivateField; + } + + public void instanceSetPackagePrivateField(MyConcreteEntity entity, Long value) { + entity.packagePrivateField = value; + } + + public Long instanceGetPrivateField(MyConcreteEntity entity) { + return entity.privateField; + } + + public void instanceSetPrivateField(MyConcreteEntity entity, Long value) { + entity.privateField = value; + } + } + } + + public static final class ExternalClassAccessors { + public static Long staticGetPublicField(MyConcreteEntity entity) { + return entity.publicField; + } + + public static void staticSetPublicField(MyConcreteEntity entity, Long value) { + entity.publicField = value; + } + + public static Long staticGetProtectedField(MyConcreteEntity entity) { + return entity.protectedField; + } + + public static void staticSetProtectedField(MyConcreteEntity entity, Long value) { + entity.protectedField = value; + } + + public static Long staticGetPackagePrivateField(MyConcreteEntity entity) { + return entity.packagePrivateField; + } + + public static void staticSetPackagePrivateField(MyConcreteEntity entity, Long value) { + entity.packagePrivateField = value; + } + + public Long instanceGetPublicField(MyConcreteEntity entity) { + return entity.publicField; + } + + public void instanceSetPublicField(MyConcreteEntity entity, Long value) { + entity.publicField = value; + } + + public Long instanceGetProtectedField(MyConcreteEntity entity) { + return entity.protectedField; + } + + public void instanceSetProtectedField(MyConcreteEntity entity, Long value) { + entity.protectedField = value; + } + + public Long instanceGetPackagePrivateField(MyConcreteEntity entity) { + return entity.packagePrivateField; + } + + public void instanceSetPackagePrivateField(MyConcreteEntity entity, Long value) { + entity.packagePrivateField = value; + } + } + + private interface AccessDelegate { + void setValue(MyConcreteEntity entity, Long value); + + Long getValue(MyConcreteEntity entity); + } +} From ab26345d9f689b4785e27754eb088b19b2347bb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Tue, 13 Jun 2023 14:58:49 +0200 Subject: [PATCH 5/5] Avoid enhancing field access in non-managed ORM model classes (converters, listeners) 1. Because it's not necessary 2. Because it won't work since those fields don't get corresponding $_hibernate_{read,write}_*() methods generated by Hibernate ORM's enhancements. --- .../orm/deployment/HibernateOrmProcessor.java | 4 ++-- .../orm/deployment/JpaJandexScavenger.java | 16 +++++----------- .../orm/deployment/JpaModelBuildItem.java | 18 ++++++++++++++++-- ...ClassCandidatesForFieldAccessBuildItem.java | 10 +++++----- ...anacheHibernateCommonResourceProcessor.java | 2 +- 5 files changed, 29 insertions(+), 21 deletions(-) diff --git a/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java b/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java index 02c744f3472ba..54b0625fe234d 100644 --- a/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java +++ b/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java @@ -620,7 +620,7 @@ public HibernateEnhancersRegisteredBuildItem enhancerDomainObjects(JpaModelBuild @BuildStep public HibernateModelClassCandidatesForFieldAccessBuildItem candidatesForFieldAccess(JpaModelBuildItem jpaModel) { // Ask Panache to replace direct access to public fields with calls to accessors for all model classes. - return new HibernateModelClassCandidatesForFieldAccessBuildItem(jpaModel.getAllModelClassNames()); + return new HibernateModelClassCandidatesForFieldAccessBuildItem(jpaModel.getManagedClassNames()); } @BuildStep @@ -1235,7 +1235,7 @@ private void enhanceEntities(final JpaModelBuildItem jpaModel, List additionalJpaModelBuildItems, BuildProducer additionalClasses) { HibernateEntityEnhancer hibernateEntityEnhancer = new HibernateEntityEnhancer(); - for (String i : jpaModel.getAllModelClassNames()) { + for (String i : jpaModel.getManagedClassNames()) { transformers.produce(new BytecodeTransformerBuildItem(true, i, hibernateEntityEnhancer, true)); } for (AdditionalJpaModelBuildItem additionalJpaModel : additionalJpaModelBuildItems) { diff --git a/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaJandexScavenger.java b/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaJandexScavenger.java index 004eea2b6af15..28dc8f1a5238f 100644 --- a/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaJandexScavenger.java +++ b/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaJandexScavenger.java @@ -102,9 +102,9 @@ public JpaModelBuildItem discoverModelAndRegisterForReflection() { enlistExplicitMappings(collector, persistenceUnitContribution); } - Set allModelClassNames = new HashSet<>(collector.entityTypes); - allModelClassNames.addAll(collector.modelTypes); - for (String className : allModelClassNames) { + Set managedClassNames = new HashSet<>(collector.entityTypes); + managedClassNames.addAll(collector.modelTypes); + for (String className : managedClassNames) { reflectiveClass.produce(ReflectiveClassBuildItem.builder(className).methods().fields().build()); } @@ -121,12 +121,6 @@ public JpaModelBuildItem discoverModelAndRegisterForReflection() { reflectiveClass.produce(ReflectiveClassBuildItem.builder(javaType).methods().build()); } - // Converters need to be in the list of model types in order for @Converter#autoApply to work, - // but they don't need reflection enabled. - for (DotName potentialCdiBeanType : collector.potentialCdiBeanTypes) { - allModelClassNames.add(potentialCdiBeanType.toString()); - } - if (!collector.unindexedClasses.isEmpty()) { Set unIgnorableIndexedClasses = collector.unindexedClasses.stream().map(DotName::toString) .collect(Collectors.toSet()); @@ -143,8 +137,8 @@ public JpaModelBuildItem discoverModelAndRegisterForReflection() { } } - return new JpaModelBuildItem(collector.packages, collector.entityTypes, collector.potentialCdiBeanTypes, - allModelClassNames, collector.xmlMappingsByPU); + return new JpaModelBuildItem(collector.packages, collector.entityTypes, managedClassNames, + collector.potentialCdiBeanTypes, collector.xmlMappingsByPU); } private void enlistExplicitMappings(Collector collector, diff --git a/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaModelBuildItem.java b/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaModelBuildItem.java index 00eed878c7aeb..540c1a1846dd2 100644 --- a/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaModelBuildItem.java +++ b/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaModelBuildItem.java @@ -22,17 +22,24 @@ public final class JpaModelBuildItem extends SimpleBuildItem { private final Set allModelPackageNames = new TreeSet<>(); private final Set entityClassNames = new TreeSet<>(); + private final Set managedClassNames = new TreeSet<>(); private final Set potentialCdiBeanClassNames = new TreeSet<>(); private final Set allModelClassNames = new TreeSet<>(); private final Map> xmlMappingsByPU = new HashMap<>(); public JpaModelBuildItem(Set allModelPackageNames, Set entityClassNames, - Set potentialCdiBeanClassNames, Set allModelClassNames, + Set managedClassNames, Set potentialCdiBeanClassNames, Map> xmlMappingsByPU) { this.allModelPackageNames.addAll(allModelPackageNames); this.entityClassNames.addAll(entityClassNames); + this.managedClassNames.addAll(managedClassNames); this.potentialCdiBeanClassNames.addAll(potentialCdiBeanClassNames); - this.allModelClassNames.addAll(allModelClassNames); + this.allModelClassNames.addAll(managedClassNames); + // Converters need to be in the list of model types in order for @Converter#autoApply to work, + // but they don't need reflection enabled. + for (DotName potentialCdiBeanClassName : potentialCdiBeanClassNames) { + this.allModelClassNames.add(potentialCdiBeanClassName.toString()); + } this.xmlMappingsByPU.putAll(xmlMappingsByPU); } @@ -50,6 +57,13 @@ public Set getEntityClassNames() { return entityClassNames; } + /** + * @return the list of managed class names: entities, mapped super classes and embeddables only. + */ + public Set getManagedClassNames() { + return managedClassNames; + } + /** * @return the list of classes that might be retrieved by Hibernate ORM as CDI beans, * e.g. converters, listeners, ... diff --git a/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/HibernateModelClassCandidatesForFieldAccessBuildItem.java b/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/HibernateModelClassCandidatesForFieldAccessBuildItem.java index 6f98ea1845dcc..680b31ea65cf1 100644 --- a/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/HibernateModelClassCandidatesForFieldAccessBuildItem.java +++ b/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/HibernateModelClassCandidatesForFieldAccessBuildItem.java @@ -5,13 +5,13 @@ import io.quarkus.builder.item.SimpleBuildItem; public final class HibernateModelClassCandidatesForFieldAccessBuildItem extends SimpleBuildItem { - private final Set allModelClassNames; + private final Set managedClassNames; - public HibernateModelClassCandidatesForFieldAccessBuildItem(Set allModelClassNames) { - this.allModelClassNames = allModelClassNames; + public HibernateModelClassCandidatesForFieldAccessBuildItem(Set managedClassNames) { + this.managedClassNames = managedClassNames; } - public Set getAllModelClassNames() { - return allModelClassNames; + public Set getManagedClassNames() { + return managedClassNames; } } diff --git a/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheHibernateCommonResourceProcessor.java b/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheHibernateCommonResourceProcessor.java index d362d256c3b83..e86a99b5ed72b 100644 --- a/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheHibernateCommonResourceProcessor.java +++ b/extensions/panache/panache-hibernate-common/deployment/src/main/java/io/quarkus/panache/common/deployment/PanacheHibernateCommonResourceProcessor.java @@ -56,7 +56,7 @@ void findEntityClasses(CombinedIndexBuildItem index, MetamodelInfo modelInfo = new MetamodelInfo(); // Technically we wouldn't need to process embeddables, but we don't have an easy way to exclude them. - for (String entityClassName : candidatesForFieldAccess.get().getAllModelClassNames()) { + for (String entityClassName : candidatesForFieldAccess.get().getManagedClassNames()) { ClassInfo entityClass = index.getIndex().getClassByName(DotName.createSimple(entityClassName)); if (entityClass == null) { // Probably a synthetic entity, such as Envers' DefaultRevisionEntity.