OPENJPA-2911 move PCSubclassValidator to ASM

openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java

@@ -97,7 +97,6 @@ import org.apache.openjpa.util.StringId;
 import org.apache.openjpa.util.UserException;
 import org.apache.openjpa.util.asm.AsmHelper;
 import org.apache.openjpa.util.asm.ClassWriterTracker;
-import org.apache.xbean.asm9.ClassReader;
 import org.apache.xbean.asm9.Opcodes;
 import org.apache.xbean.asm9.Type;
 import org.apache.xbean.asm9.tree.AbstractInsnNode;
@@ -225,9 +224,9 @@ public class PCEnhancer {
     private Set _violations = null;
     private File _dir = null;
     private BytecodeWriter _writer = null;
-    private Map _backingFields = null; // map of set / get names => field names
+    private Map<String, String> _backingFields = null; // map of set / get names => field names
-    private Map _attrsToFields = null; // map of attr names => field names
+    private Map<String, String> _attrsToFields = null; // map of attr names => field names
-    private Map _fieldsToAttrs = null; // map of field names => attr names
+    private Map<String, String> _fieldsToAttrs = null; // map of field names => attr names
     private boolean _isAlreadyRedefined = false;
     private boolean _isAlreadySubclassed = false;
     private boolean _bcsConfigured = false;
@@ -573,12 +572,14 @@ public class PCEnhancer {
                 _log.trace(_loc.get("enhance-start", type));
             }
 
-            configureBCs();
+            final ClassNode classNode = AsmHelper.readClassNode(_managedType.toByteArray());
+
+            configureBCs(classNode);
 
             // validate properties before replacing field access so that
             // we build up a record of backing fields, etc
             if (isPropertyAccess(_meta)) {
-                validateProperties();
+                validateProperties(classNode);
                 if (getCreateSubclass()) {
                     addAttributeTranslation();
                 }
@@ -607,22 +608,21 @@ public class PCEnhancer {
         }
     }
 
-    private void configureBCs() {
+    private void configureBCs(final ClassNode classNode) {
         if (!_bcsConfigured) {
             if (getRedefine()) {
-                if (_managedType.getAttribute(REDEFINED_ATTRIBUTE) == null)
+                if (_managedType.getAttribute(REDEFINED_ATTRIBUTE) == null) {
                     _managedType.addAttribute(REDEFINED_ATTRIBUTE);
-                else
+                } else {
                     _isAlreadyRedefined = true;
+                }
             }
 
             if (getCreateSubclass()) {
-                PCSubclassValidator val = new PCSubclassValidator(
+                PCSubclassValidator val = new PCSubclassValidator(_meta, classNode, _managedType, _log, _fail);
-                    _meta, _managedType, _log, _fail);
                 val.assertCanSubclass();
 
-                _pc = _managedType.getProject().loadClass(
+                _pc = _managedType.getProject().loadClass(toPCSubclassName(_managedType.getType()));
-                    toPCSubclassName(_managedType.getType()));
                 if (_pc.getSuperclassBC() != _managedType) {
                     _pc.setSuperclass(_managedType);
                     _pc.setAbstract(_managedType.isAbstract());
@@ -699,12 +699,14 @@ public class PCEnhancer {
      * written correctly. This method also gathers information on each
      * property's backing field.
      */
-    private void validateProperties() {
+    private void validateProperties(ClassNode classNode) {
         FieldMetaData[] fmds;
-        if (getCreateSubclass())
+        if (getCreateSubclass()) {
             fmds = _meta.getFields();
-        else
+        }
+        else {
             fmds = _meta.getDeclaredFields();
+        }
         Method meth;
 
         BCMethod bcGetter, bcSetter;
@@ -728,6 +730,7 @@ public class PCEnhancer {
             }
 
             meth = (Method) fmd.getBackingMember();
+
             // ##### this will fail if we override and don't call super.
             BCClass declaringType = _managedType.getProject()
                     .loadClass(fmd.getDeclaringType());
@@ -739,12 +742,13 @@ public class PCEnhancer {
                 continue;
             }
             bcReturned = getReturnedField_old(bcGetter);
-            returned = getReturnedField(meth);
+            getter = meth;
+            returned = getReturnedField(classNode, getter);
 
             //X TODO remove
             PCEnhancer.assertSameField(returned, bcReturned);
 
-            if (bcReturned != null) {
+            if (returned != null) {
                 registerBackingFieldInfo(fmd, bcGetter, bcReturned);
             }
 
@@ -774,7 +778,7 @@ public class PCEnhancer {
             if (bcSetter != null) {
                 bcAssigned = getAssignedField_old(bcSetter);
 
-                assigned = getAssignedField(getMethod(fmd.getDeclaringType(), fmd.getSetterName(), new Class[]{fmd.getDeclaredType()}));
+                assigned = getAssignedField(classNode, getMethod(fmd.getDeclaringType(), fmd.getSetterName(), new Class[]{fmd.getDeclaredType()}));
                 assertSameField(assigned, bcAssigned);
             }
 
@@ -790,18 +794,20 @@ public class PCEnhancer {
         }
     }
 
-    private void registerBackingFieldInfo(FieldMetaData fmd, BCMethod method,
+    private void registerBackingFieldInfo(FieldMetaData fmd, BCMethod method, BCField field) {
-        BCField field) {
+        if (_backingFields == null) {
-        if (_backingFields == null)
             _backingFields = new HashMap();
+        }
         _backingFields.put(method.getName(), field.getName());
 
-        if (_attrsToFields == null)
+        if (_attrsToFields == null) {
             _attrsToFields = new HashMap();
+        }
         _attrsToFields.put(fmd.getName(), field.getName());
 
-        if (_fieldsToAttrs == null)
+        if (_fieldsToAttrs == null) {
             _fieldsToAttrs = new HashMap();
+        }
         _fieldsToAttrs.put(field.getName(), fmd.getName());
     }
 
@@ -886,13 +892,14 @@ public class PCEnhancer {
      * Return the field returned by the given method, or null if none.
      * Package-protected and static for testing.
      */
+    @Deprecated
     static BCField getReturnedField_old(BCMethod meth) {
         return findField_old(meth, new Code().xreturn()
             .setType(meth.getReturnType()), false);
     }
 
-    static Field getReturnedField(Method meth) {
+    static Field getReturnedField(ClassNode classNode, Method meth) {
-        return findField(meth, (ain) -> ain.getOpcode() == AsmHelper.getReturnInsn(meth.getReturnType()), false);
+        return findField(classNode, meth, (ain) -> ain.getOpcode() == AsmHelper.getReturnInsn(meth.getReturnType()), false);
     }
 
 
@@ -900,13 +907,14 @@ public class PCEnhancer {
      * Return the field assigned in the given method, or null if none.
      * Package-protected and static for testing.
      */
+    @Deprecated
     static BCField getAssignedField_old(BCMethod meth) {
         return findField_old(meth, (AccessController.doPrivileged(
             J2DoPrivHelper.newCodeAction())).putfield(), true);
     }
 
-    static Field getAssignedField(Method meth) {
+    static Field getAssignedField(ClassNode classNode, Method meth) {
-        return findField(meth, (ain) -> ain.getOpcode() == Opcodes.PUTFIELD, true);
+        return findField(classNode, meth, (ain) -> ain.getOpcode() == Opcodes.PUTFIELD, true);
     }
 
     /**
@@ -914,6 +922,7 @@ public class PCEnhancer {
      * null if non-fields (methods, literals, parameters, variables) are
      * returned, or if non-parameters are assigned to fields.
      */
+    @Deprecated
     private static BCField findField_old(BCMethod meth, Instruction template, boolean findAccessed) {
         // ignore any static methods. OpenJPA only currently supports
         // non-static setters and getters
@@ -980,24 +989,17 @@ public class PCEnhancer {
         return field;
     }
 
-
+    private static Field findField(ClassNode classNode, Method meth, Predicate<AbstractInsnNode> ain, boolean findAccessed) {
-    private static Field findField(Method meth, Predicate<AbstractInsnNode> ain, boolean findAccessed) {
         // ignore any static methods. OpenJPA only currently supports
         // non-static setters and getters
         if (Modifier.isStatic(meth.getModifiers())) {
             return null;
         }
 
-        ClassReader cr;
+        if (meth.getDeclaringClass().isInterface()) {
-        final String classResourceName = meth.getDeclaringClass().getName().replace(".", "/") + ".class";
+            return null;
-        try (final InputStream classBytesStream = meth.getDeclaringClass().getClassLoader().getResourceAsStream(classResourceName)) {
-            cr = new ClassReader(classBytesStream);
         }
-        catch (IOException e) {
+
-            throw new RuntimeException(e);
-        }
-        ClassNode classNode = new ClassNode();
-        cr.accept(classNode, 0);
         final MethodNode methodNode = classNode.methods.stream()
                 .filter(mn -> mn.name.equals(meth.getName()) && mn.desc.equals(Type.getMethodDescriptor(meth)))
                 .findAny().get();
@@ -3437,7 +3439,7 @@ public class PCEnhancer {
 
     /**
      * Adds a custom readObject method that delegates to the
-     * {@link ObjectInputStream#readObject} method.
+     * {@link ObjectInputStream#readObject()} method.
      */
     private void modifyReadObjectMethod(BCMethod method, boolean full) {
         Code code = method.getCode(true);
@@ -4935,7 +4937,7 @@ public class PCEnhancer {
      * <ul>
      * <li><i>-properties/-p &lt;properties file&gt;</i>: The path to a OpenJPA
      * properties file containing information as outlined in
-     * {@link Configuration}; optional.</li>
+     * {@link org.apache.openjpa.lib.conf.Configuration}; optional.</li>
      * <li><i>-&lt;property name&gt; &lt;property value&gt;</i>: All bean
      * properties of the standard OpenJPA {@link OpenJPAConfiguration} can be
      * set by using their names and supplying a value; for example:

openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCSubclassValidator.java

@@ -27,6 +27,7 @@ import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Objects;
 
 import org.apache.openjpa.lib.log.Log;
 import org.apache.openjpa.lib.util.Localizer;
@@ -36,6 +37,7 @@ import org.apache.openjpa.meta.AccessCode;
 import org.apache.openjpa.meta.ClassMetaData;
 import org.apache.openjpa.meta.FieldMetaData;
 import org.apache.openjpa.util.UserException;
+import org.apache.xbean.asm9.tree.ClassNode;
 
 import serp.bytecode.BCClass;
 import serp.bytecode.BCField;
@@ -83,6 +85,7 @@ public class PCSubclassValidator {
             Localizer.forPackage(PCSubclassValidator.class);
 
     private final ClassMetaData meta;
+    private final ClassNode classNode;
     private final BCClass bc;
     private final Log log;
     private final boolean failOnContractViolations;
@@ -90,9 +93,10 @@ public class PCSubclassValidator {
     private Collection errors;
     private Collection contractViolations;
 
-    public PCSubclassValidator(ClassMetaData meta, BCClass bc, Log log,
+    public PCSubclassValidator(ClassMetaData meta, ClassNode classNode, BCClass bc, Log log,
                                boolean enforceContractViolations) {
         this.meta = meta;
+        this.classNode = classNode;
         this.bc = bc;
         this.log = log;
         this.failOnContractViolations = enforceContractViolations;
@@ -148,7 +152,7 @@ public class PCSubclassValidator {
                                  fmd.getName()), fmd);
                 continue;
             }
-            BCField returnedField = checkGetterIsSubclassable(getter, fmd);
+            Field returnedField = checkGetterIsSubclassable(getter, fmd);
 
             Method setter = setterForField(fmd);
             if (setter == null) {
@@ -156,12 +160,12 @@ public class PCSubclassValidator {
                          fmd);
                 continue;
             }
-            BCField assignedField = checkSetterIsSubclassable(setter, fmd);
+            Field assignedField = checkSetterIsSubclassable(setter, fmd);
             if (assignedField == null) {
                 continue;
             }
 
-            if (assignedField != returnedField) {
+            if (!Objects.equals(assignedField, returnedField)) {
                 addContractViolation(loc.get("subclasser-setter-getter-field-mismatch",
                                              fmd.getName(), returnedField, assignedField),
                                      fmd);
@@ -199,20 +203,20 @@ public class PCSubclassValidator {
      * <code>null</code> if something other than a single field is
      * returned, or if it cannot be determined what is returned.
      */
-    private BCField checkGetterIsSubclassable(Method meth, FieldMetaData fmd) {
+    private Field checkGetterIsSubclassable(Method meth, FieldMetaData fmd) {
         checkMethodIsSubclassable(meth, fmd);
         BCField bcField = PCEnhancer.getReturnedField_old(getBCMethod(meth));
-        Field field = PCEnhancer.getReturnedField(meth);
+        Field field = PCEnhancer.getReturnedField(classNode, meth);
 
         //X TODO remove
         PCEnhancer.assertSameField(field, bcField);
 
-        if (bcField == null) {
+        if (field == null) {
             addContractViolation(loc.get("subclasser-invalid-getter", fmd.getName()), fmd);
             return null;
         }
         else {
-            return bcField;
+            return field;
         }
     }
 
@@ -221,20 +225,20 @@ public class PCSubclassValidator {
      * <code>null</code> if something other than a single field is
      * set, or if it cannot be determined what is set.
      */
-    private BCField checkSetterIsSubclassable(Method meth, FieldMetaData fmd) {
+    private Field checkSetterIsSubclassable(Method meth, FieldMetaData fmd) {
         checkMethodIsSubclassable(meth, fmd);
         BCField bcField = PCEnhancer.getAssignedField_old(getBCMethod(meth));
-        Field field = PCEnhancer.getAssignedField(meth);
+        Field field = PCEnhancer.getAssignedField(classNode, meth);
 
         //X TODO remove
         PCEnhancer.assertSameField(field, bcField);
 
-        if (bcField == null) {
+        if (field == null) {
             addContractViolation(loc.get("subclasser-invalid-setter", fmd.getName()), fmd);
             return null;
         }
         else {
-            return bcField;
+            return field;
         }
     }
 

openjpa-kernel/src/main/java/org/apache/openjpa/util/asm/AsmHelper.java

@@ -17,6 +17,8 @@
 package org.apache.openjpa.util.asm;
 
 import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
 import java.lang.reflect.Method;
 
 import org.apache.xbean.asm9.ClassReader;
@@ -24,6 +26,7 @@ import org.apache.xbean.asm9.ClassWriter;
 import org.apache.xbean.asm9.Opcodes;
 import org.apache.xbean.asm9.Type;
 import org.apache.xbean.asm9.tree.AbstractInsnNode;
+import org.apache.xbean.asm9.tree.ClassNode;
 import org.apache.xbean.asm9.tree.VarInsnNode;
 
 import serp.bytecode.BCClass;
@@ -39,6 +42,41 @@ public final class AsmHelper {
         // utility class ct
     }
 
+
+    /**
+     * Read the binary bytecode from the class with the given name
+     * @param classBytes the binary of the class
+     * @return the ClassNode constructed from that class
+     */
+    public static ClassNode readClassNode(byte[] classBytes) {
+        ClassReader cr = new ClassReader(classBytes);
+        ClassNode classNode = new ClassNode();
+        cr.accept(classNode, 0);
+
+        return classNode;
+    }
+
+    /**
+     * Read the binary bytecode from the class with the given name
+     * @param classLoader the ClassLoader to use
+     * @param className the fully qualified class name to read. e.g. "org.mycorp.mypackage.MyEntity"
+     * @return the ClassNode constructed from that class
+     */
+    public static ClassNode readClassNode(ClassLoader classLoader, String className) {
+        ClassReader cr;
+        final String classResourceName = className.replace(".", "/") + ".class";
+        try (final InputStream classBytesStream = classLoader.getResourceAsStream(classResourceName)) {
+            cr = new ClassReader(classBytesStream);
+        }
+        catch (IOException e) {
+            throw new RuntimeException(e);
+        }
+        ClassNode classNode = new ClassNode();
+        cr.accept(classNode, 0);
+
+        return classNode;
+    }
+
     /**
      * temporary helper class to convert BCClass to ASM
      * @deprecated must get removed when done with migrating from Serp to ASM

openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/TestSubclassValidator.java

@@ -24,9 +24,9 @@ import org.apache.openjpa.meta.MetaDataModes;
 import org.apache.openjpa.meta.MetaDataRepository;
 import org.apache.openjpa.persistence.common.apps.Department;
 import org.apache.openjpa.persistence.common.apps.RuntimeTest2;
-import org.apache.openjpa.enhance.UnenhancedPropertyAccess;
 import org.apache.openjpa.persistence.test.SingleEMFTestCase;
-import org.apache.openjpa.util.UserException;
+import org.apache.openjpa.util.asm.AsmHelper;
+import org.apache.xbean.asm9.tree.ClassNode;
 import org.junit.Test;
 
 import jakarta.persistence.Access;
@@ -34,6 +34,7 @@ import jakarta.persistence.AccessType;
 import jakarta.persistence.Basic;
 import jakarta.persistence.Entity;
 import jakarta.persistence.Id;
+import jakarta.persistence.MappedSuperclass;
 import serp.bytecode.BCClass;
 import serp.bytecode.Project;
 
@@ -43,7 +44,7 @@ public class TestSubclassValidator extends SingleEMFTestCase {
         setUp("openjpa.RuntimeUnenhancedClasses", "supported",
                 Department.class,
                 RuntimeTest2.class,
-                EnhancableGetterEntity.class,
+                EnhanceableGetterEntity.class,
                 UnenhancedPropertyAccess.class,
                 CLEAR_TABLES);
     }
@@ -80,29 +81,31 @@ public class TestSubclassValidator extends SingleEMFTestCase {
 */
 
         {
-            final BCClass bcClass = project.loadClass(EnhancableGetterEntity.class.getName(), tempCl);
+            ClassNode classNode = AsmHelper.readClassNode(EnhanceableGetterEntity.class.getClassLoader(), EnhanceableGetterEntity.class.getName());
-            final ClassMetaData meta = repos.getMetaData(tempCl.loadClass(EnhancableGetterEntity.class.getName()), tempCl, false);
+            final BCClass bcClass = project.loadClass(EnhanceableGetterEntity.class.getName(), tempCl);
-            PCSubclassValidator subclassValidator = new PCSubclassValidator(meta, bcClass, log, true);
+            final ClassMetaData meta = repos.getMetaData(tempCl.loadClass(EnhanceableGetterEntity.class.getName()), tempCl, false);
+            PCSubclassValidator subclassValidator = new PCSubclassValidator(meta, classNode, bcClass, log, true);
             subclassValidator.assertCanSubclass();
         }
 
         {
+            ClassNode classNode = AsmHelper.readClassNode(UnenhancedPropertyAccess.class.getClassLoader(), UnenhancedPropertyAccess.class.getName());
             final BCClass bcClass = project.loadClass(UnenhancedPropertyAccess.class.getName(), tempCl);
             final ClassMetaData meta = repos.getMetaData(tempCl.loadClass(UnenhancedPropertyAccess.class.getName()), tempCl, false);
-            PCSubclassValidator subclassValidator = new PCSubclassValidator(meta, bcClass, log, true);
+            PCSubclassValidator subclassValidator = new PCSubclassValidator(meta, classNode, bcClass, log, true);
             subclassValidator.assertCanSubclass();
         }
     }
 
     @Entity
     @Access(AccessType.PROPERTY)
-    public static class EnhancableGetterEntity {
+    public static class EnhanceableGetterEntity {
 
         @Id
         private String id;
 
         @Basic
-        private String name;
+        protected String name;
 
         @Basic
         private String another;