From 31681982de16a3496e0ebb6aa1efa3901ac6e6ef Mon Sep 17 00:00:00 2001 From: Mark Struberg Date: Wed, 24 May 2023 10:38:44 +0200 Subject: [PATCH] OPENJPA-2911 move PCSubclassValidator to ASM work in progress. For now we compare old and new mechanism to catch errors. --- .../apache/openjpa/enhance/PCEnhancer.java | 155 +++++++++-- .../openjpa/enhance/PCSubclassValidator.java | 251 ++++++++++-------- .../meta/AbstractMetaDataDefaults.java | 82 +++--- .../apache/openjpa/util/asm/AsmHelper.java | 30 +++ .../enhance/TestPCEnhancerFindField.java | 2 +- .../enhance/TestSubclassValidator.java | 126 +++++++++ 6 files changed, 470 insertions(+), 176 deletions(-) create mode 100644 openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/TestSubclassValidator.java diff --git a/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java b/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java index 72b6380c8..092c8ef9b 100644 --- a/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java +++ b/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java @@ -52,6 +52,7 @@ import java.util.Locale; import java.util.Map; import java.util.Properties; import java.util.Set; +import java.util.function.Predicate; import org.apache.openjpa.conf.OpenJPAConfiguration; import org.apache.openjpa.conf.OpenJPAConfigurationImpl; @@ -96,6 +97,14 @@ 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; +import org.apache.xbean.asm9.tree.ClassNode; +import org.apache.xbean.asm9.tree.FieldInsnNode; +import org.apache.xbean.asm9.tree.MethodNode; +import org.apache.xbean.asm9.tree.VarInsnNode; import serp.bytecode.BCClass; import serp.bytecode.BCField; @@ -541,14 +550,15 @@ public class PCEnhancer { Class type = _managedType.getType(); try { // if enum, skip, no need of any meta - if (_pc.isEnum()) + if (type.isEnum()) return ENHANCE_NONE; // if managed interface, skip - if (_pc.isInterface()) + if (type.isInterface()) return ENHANCE_INTERFACE; // check if already enhanced + // we cannot simply use instanceof or isAssignableFrom as we have a temp ClassLoader inbetween ClassLoader loader = AccessController.doPrivileged(J2DoPrivHelper.getClassLoaderAction(type)); for (String iface : _managedType.getDeclaredInterfaceNames()) { if (iface.equals(PCTYPE.getName())) { @@ -558,10 +568,10 @@ public class PCEnhancer { return ENHANCE_NONE; } } - if (_log.isTraceEnabled()) { - _log.trace(_loc.get("enhance-start", type, loader)); - } + if (_log.isTraceEnabled()) { + _log.trace(_loc.get("enhance-start", type)); + } configureBCs(); @@ -569,8 +579,9 @@ public class PCEnhancer { // we build up a record of backing fields, etc if (isPropertyAccess(_meta)) { validateProperties(); - if (getCreateSubclass()) + if (getCreateSubclass()) { addAttributeTranslation(); + } } replaceAndValidateFieldAccess(); processViolations(); @@ -710,7 +721,7 @@ public class PCEnhancer { true); continue; } - returned = getReturnedField(getter); + returned = getReturnedField_old(getter); if (returned != null) registerBackingFieldInfo(fmd, getter, returned); @@ -738,7 +749,7 @@ public class PCEnhancer { } if (setter != null) - assigned = getAssignedField(setter); + assigned = getAssignedField_old(setter); if (assigned != null) { if (setter != null) @@ -848,28 +859,35 @@ public class PCEnhancer { * Return the field returned by the given method, or null if none. * Package-protected and static for testing. */ - static BCField getReturnedField(BCMethod meth) { - return findField(meth, (AccessController.doPrivileged( - J2DoPrivHelper.newCodeAction())).xreturn() + static BCField getReturnedField_old(BCMethod meth) { + return findField_old(meth, new Code().xreturn() .setType(meth.getReturnType()), false); } + static Field getReturnedField(Method meth) { + return findField(meth, (ain) -> ain.getOpcode() == AsmHelper.getReturnInsn(meth.getReturnType()), false); + } + + /** * Return the field assigned in the given method, or null if none. * Package-protected and static for testing. */ - static BCField getAssignedField(BCMethod meth) { - return findField(meth, (AccessController.doPrivileged( + static BCField getAssignedField_old(BCMethod meth) { + return findField_old(meth, (AccessController.doPrivileged( J2DoPrivHelper.newCodeAction())).putfield(), true); } + static Field getAssignedField(Method meth) { + return findField(meth, (ain) -> ain.getOpcode() == Opcodes.PUTFIELD, true); + } + /** * Return the field returned / assigned by meth. Returns * null if non-fields (methods, literals, parameters, variables) are * returned, or if non-parameters are assigned to fields. */ - private static BCField findField(BCMethod meth, Instruction template, - boolean findAccessed) { + private static BCField findField_old(BCMethod meth, Instruction template, boolean findAccessed) { // ignore any static methods. OpenJPA only currently supports // non-static setters and getters if (meth.isStatic()) @@ -918,8 +936,7 @@ public class PCEnhancer { && ((LoadInstruction) prevIns).getParam() == 0) { final FieldInstruction fTemplateIns = (FieldInstruction) templateIns; - cur = AccessController.doPrivileged(J2DoPrivHelper - .getFieldInstructionFieldAction(fTemplateIns)); + cur = fTemplateIns.getField(); } else return null; @@ -936,6 +953,103 @@ public class PCEnhancer { return field; } + + private static Field findField(Method meth, Predicate 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; + try { + cr = new ClassReader(meth.getDeclaringClass().getName()); + } + 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(); + + Field field = null; + Field cur; + AbstractInsnNode prevInsn, earlierInsn; + for (AbstractInsnNode insn : methodNode.instructions) { + if (!ain.test(insn)) { + continue; + } + + prevInsn = insn.getPrevious(); + if (prevInsn == null) { + return null; + } + + if (prevInsn.getOpcode() == Opcodes.INSTANCEOF && prevInsn.getPrevious() != null ) { + prevInsn = prevInsn.getPrevious(); + } + + if (prevInsn.getPrevious() == null) { + return null; + } + + earlierInsn = prevInsn.getPrevious(); + + // if the opcode two before the template was an aload_0, check + // against the middle instruction based on what type of find + // we're doing + if (!AsmHelper.isLoadInsn(earlierInsn) + || !AsmHelper.isThisInsn(earlierInsn)) { + return null; + } + + // if the middle instruction was a getfield, then it's the + // field that's being accessed + if (!findAccessed && prevInsn.getOpcode() == Opcodes.GETFIELD) { + final FieldInsnNode fieldInsn = (FieldInsnNode) prevInsn; + + cur = getField(meth.getDeclaringClass(), fieldInsn); + + // if the middle instruction was an xload_1, then the + // matched instruction is the field that's being set. + } else if (findAccessed && AsmHelper.isLoadInsn(prevInsn) + && ((VarInsnNode) prevInsn).var == 1) { + final FieldInsnNode fieldInsn = (FieldInsnNode)insn; + cur = getField(meth.getDeclaringClass(), fieldInsn); + } else { + return null; + } + + + if (field != null && cur != field) { + return null; + } + field = cur; + } + + + return field; + } + + private static Field getField(Class clazz, FieldInsnNode fieldInsn) { + try { + final Class fieldOwner = clazz.getClassLoader().loadClass(fieldInsn.owner.replace("/", ".")); + return fieldOwner.getDeclaredField(fieldInsn.name); + } + catch (NoSuchFieldException e) { + if (clazz.getSuperclass() == Object.class) { + throw new IllegalStateException("Cannot find field " + fieldInsn + " in Class " + clazz); + } + return getField(clazz.getSuperclass(), fieldInsn); + } + catch (ClassNotFoundException e) { + throw new RuntimeException(e); + } + } + + /** * Record a violation of the property access restrictions. */ @@ -4895,8 +5009,8 @@ public class PCEnhancer { if (args == null || args.length == 0) { classes = repos.getPersistentTypeNames(true, loader); if (classes == null) { - log.warn(_loc.get("no-class-to-enhance")); - return false; + log.warn(_loc.get("no-class-to-enhance")); + return false; } } else { ClassArgParser cap = conf.getMetaDataRepositoryInstance(). @@ -4923,8 +5037,9 @@ public class PCEnhancer { else bc = project.loadClass((Class) o); enhancer = new PCEnhancer(conf, bc, repos, loader); - if (writer != null) + if (writer != null) { enhancer.setBytecodeWriter(writer); + } enhancer.setDirectory(flags.directory); enhancer.setAddDefaultConstructor(flags.addDefaultConstructor); status = enhancer.run(); diff --git a/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCSubclassValidator.java b/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCSubclassValidator.java index cee8f03a4..19018417e 100644 --- a/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCSubclassValidator.java +++ b/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCSubclassValidator.java @@ -21,6 +21,7 @@ package org.apache.openjpa.enhance; import java.io.Externalizable; import java.io.ObjectInput; import java.lang.reflect.Constructor; +import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Method; import java.lang.reflect.Modifier; @@ -34,7 +35,6 @@ import org.apache.openjpa.lib.util.StringUtil; import org.apache.openjpa.meta.AccessCode; import org.apache.openjpa.meta.ClassMetaData; import org.apache.openjpa.meta.FieldMetaData; -import org.apache.openjpa.util.InternalException; import org.apache.openjpa.util.UserException; import serp.bytecode.BCClass; @@ -42,48 +42,48 @@ import serp.bytecode.BCField; import serp.bytecode.BCMethod; /** - *

Validates that a given type meets the JPA contract, plus a few - * OpenJPA-specific additions for subclassing / redefinition: + *

Validates that a given type meets the JPA contract, plus a few + * OpenJPA-specific additions for subclassing / redefinition: * - *

* - *

If you use this technique and use the new keyword instead - * of a OpenJPA-supplied construction routine, OpenJPA will need to do extra - * work with persistent-new-flushed instances, since OpenJPA cannot in this - * case track what happens to such an instance.

- * - * @since 1.0.0 + *

If you use this technique and use the new keyword instead + * of a OpenJPA-supplied construction routine, OpenJPA will need to do extra + * work with persistent-new-flushed instances, since OpenJPA cannot in this + * case track what happens to such an instance.

+ *

+ * @since 1.0.0 */ public class PCSubclassValidator { private static final Localizer loc = - Localizer.forPackage(PCSubclassValidator.class); + Localizer.forPackage(PCSubclassValidator.class); private final ClassMetaData meta; - private final BCClass pc; + private final BCClass bc; private final Log log; private final boolean failOnContractViolations; @@ -91,52 +91,51 @@ public class PCSubclassValidator { private Collection contractViolations; public PCSubclassValidator(ClassMetaData meta, BCClass bc, Log log, - boolean enforceContractViolations) { + boolean enforceContractViolations) { this.meta = meta; - this.pc = bc; + this.bc = bc; this.log = log; this.failOnContractViolations = enforceContractViolations; } public void assertCanSubclass() { - Class superclass = meta.getDescribedType(); + Class superclass = meta.getDescribedType(); String name = superclass.getName(); - if (superclass.isInterface()) + if (superclass.isInterface()) { addError(loc.get("subclasser-no-ifaces", name), meta); - if (Modifier.isFinal(superclass.getModifiers())) + } + if (Modifier.isFinal(superclass.getModifiers())) { addError(loc.get("subclasser-no-final-classes", name), meta); - if (Modifier.isPrivate(superclass.getModifiers())) + } + if (Modifier.isPrivate(superclass.getModifiers())) { addError(loc.get("subclasser-no-private-classes", name), meta); - if (PersistenceCapable.class.isAssignableFrom(superclass)) + } + if (PersistenceCapable.class.isAssignableFrom(superclass)) { addError(loc.get("subclasser-super-already-pc", name), meta); + } try { - Constructor c = superclass.getDeclaredConstructor(new Class[0]); + Constructor c = superclass.getDeclaredConstructor(); if (!(Modifier.isProtected(c.getModifiers()) - || Modifier.isPublic(c.getModifiers()))) + || Modifier.isPublic(c.getModifiers()))) { addError(loc.get("subclasser-private-ctor", name), meta); + } } catch (NoSuchMethodException e) { - addError(loc.get("subclasser-no-void-ctor", name), - meta); + addError(loc.get("subclasser-no-void-ctor", name), meta); } - // if the BCClass we loaded is already pc and the superclass is not, - // then we should never get here, so let's make sure that the - // calling context is caching correctly by throwing an exception. - if (pc.isInstanceOf(PersistenceCapable.class) && - !PersistenceCapable.class.isAssignableFrom(superclass)) - throw new InternalException( - loc.get("subclasser-class-already-pc", name)); - - if (AccessCode.isProperty(meta.getAccessType())) + if (AccessCode.isProperty(meta.getAccessType())) { checkPropertiesAreInterceptable(); + } - if (errors != null && !errors.isEmpty()) + if (errors != null && !errors.isEmpty()) { throw new UserException(errors.toString()); + } else if (contractViolations != null && - !contractViolations.isEmpty() && log.isWarnEnabled()) + !contractViolations.isEmpty() && log.isWarnEnabled()) { log.warn(contractViolations.toString()); + } } private void checkPropertiesAreInterceptable() { @@ -146,7 +145,7 @@ public class PCSubclassValidator { Method getter = getBackingMember(fmd); if (getter == null) { addError(loc.get("subclasser-no-getter", - fmd.getName()), fmd); + fmd.getName()), fmd); continue; } BCField returnedField = checkGetterIsSubclassable(getter, fmd); @@ -154,18 +153,19 @@ public class PCSubclassValidator { Method setter = setterForField(fmd); if (setter == null) { addError(loc.get("subclasser-no-setter", fmd.getName()), - fmd); + fmd); continue; } BCField assignedField = checkSetterIsSubclassable(setter, fmd); - if (assignedField == null) + if (assignedField == null) { continue; + } - if (assignedField != returnedField) - addContractViolation(loc.get - ("subclasser-setter-getter-field-mismatch", - fmd.getName(), returnedField, assignedField), - fmd); + if (assignedField != returnedField) { + addContractViolation(loc.get("subclasser-setter-getter-field-mismatch", + fmd.getName(), returnedField, assignedField), + fmd); + } // ### scan through all the rest of the class to make sure it // ### doesn't use the field. @@ -173,22 +173,21 @@ public class PCSubclassValidator { } private Method getBackingMember(FieldMetaData fmd) { - Member back = fmd.getBackingMember(); - if (Method.class.isInstance(back)) - return (Method)back; + Member back = fmd.getBackingMember(); + if (back instanceof Method) { + return (Method) back; + } - Method getter = Reflection.findGetter(meta.getDescribedType(), - fmd.getName(), false); - if (getter != null) - fmd.backingMember(getter); - return getter; + Method getter = Reflection.findGetter(meta.getDescribedType(), fmd.getName(), false); + if (getter != null) { + fmd.backingMember(getter); + } + return getter; } private Method setterForField(FieldMetaData fmd) { try { - return fmd.getDeclaringType().getDeclaredMethod( - "set" + StringUtil.capitalize(fmd.getName()), - new Class[]{ fmd.getDeclaredType() }); + return fmd.getDeclaringType().getDeclaredMethod("set" + StringUtil.capitalize(fmd.getName()), fmd.getDeclaredType()); } catch (NoSuchMethodException e) { return null; @@ -197,93 +196,111 @@ public class PCSubclassValidator { /** * @return the name of the field that is returned by meth, or - * null if something other than a single field is - * returned, or if it cannot be determined what is returned. + * null 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) { checkMethodIsSubclassable(meth, fmd); - BCField field = PCEnhancer.getReturnedField(getBCMethod(meth)); - if (field == null) { - addContractViolation(loc.get("subclasser-invalid-getter", - fmd.getName()), fmd); + BCField bcField = PCEnhancer.getReturnedField_old(getBCMethod(meth)); + Field field = PCEnhancer.getReturnedField(meth); + + //X TODO remove + if (field == null && bcField != null || field != null && bcField == null || + field != null && bcField != null && !field.getName().equals(bcField.getName())) { + throw new IllegalStateException("MSX ASMTODO " + bcField + " " + field); + } + + if (bcField == null) { + addContractViolation(loc.get("subclasser-invalid-getter", fmd.getName()), fmd); return null; - } else { - return field; + } + else { + return bcField; } } /** * @return the field that is set in meth, or - * null if something other than a single field is - * set, or if it cannot be determined what is set. + * null 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) { checkMethodIsSubclassable(meth, fmd); - BCField field = PCEnhancer.getAssignedField(getBCMethod(meth)); - if (field == null) { - addContractViolation(loc.get("subclasser-invalid-setter", - fmd.getName()), fmd); + BCField bcField = PCEnhancer.getAssignedField_old(getBCMethod(meth)); + Field field = PCEnhancer.getAssignedField(meth); + + //X TODO remove + if (field == null && bcField != null || field != null && bcField == null || + field != null && bcField != null && !field.getName().equals(bcField.getName())) { + throw new IllegalStateException("MSX ASMTODO " + bcField + " " + field); + } + + if (bcField == null) { + addContractViolation(loc.get("subclasser-invalid-setter", fmd.getName()), fmd); return null; - } else { - return field; + } + else { + return bcField; } } private BCMethod getBCMethod(Method meth) { - BCClass bc = pc.getProject().loadClass(meth.getDeclaringClass()); + BCClass bc = this.bc.getProject().loadClass(meth.getDeclaringClass()); return bc.getDeclaredMethod(meth.getName(), meth.getParameterTypes()); } private void checkMethodIsSubclassable(Method meth, FieldMetaData fmd) { String className = fmd.getDefiningMetaData(). - getDescribedType().getName(); + getDescribedType().getName(); if (!(Modifier.isProtected(meth.getModifiers()) - || Modifier.isPublic(meth.getModifiers()))) - addError(loc.get("subclasser-private-accessors-unsupported", - className, meth.getName()), fmd); - if (Modifier.isFinal(meth.getModifiers())) - addError(loc.get("subclasser-final-methods-not-allowed", - className, meth.getName()), fmd); - if (Modifier.isNative(meth.getModifiers())) - addContractViolation(loc.get - ("subclasser-native-methods-not-allowed", className, - meth.getName()), - fmd); - if (Modifier.isStatic(meth.getModifiers())) - addError(loc.get("subclasser-static-methods-not-supported", - className, meth.getName()), fmd); + || Modifier.isPublic(meth.getModifiers()))) { + addError(loc.get("subclasser-private-accessors-unsupported", className, meth.getName()), fmd); + } + if (Modifier.isFinal(meth.getModifiers())) { + addError(loc.get("subclasser-final-methods-not-allowed", className, meth.getName()), fmd); + } + if (Modifier.isNative(meth.getModifiers())) { + addContractViolation(loc.get("subclasser-native-methods-not-allowed", className, meth.getName()), fmd); + } + if (Modifier.isStatic(meth.getModifiers())) { + addError(loc.get("subclasser-static-methods-not-supported", className, meth.getName()), fmd); + } } private void addError(Message s, ClassMetaData cls) { - if (errors == null) + if (errors == null) { errors = new ArrayList(); + } errors.add(loc.get("subclasser-error-meta", s, - cls.getDescribedType().getName(), - cls.getSourceFile())); + cls.getDescribedType().getName(), + cls.getSourceFile())); } private void addError(Message s, FieldMetaData fmd) { - if (errors == null) + if (errors == null) { errors = new ArrayList(); + } errors.add(loc.get("subclasser-error-field", s, - fmd.getFullName(), - fmd.getDeclaringMetaData().getSourceFile())); + fmd.getFullName(), + fmd.getDeclaringMetaData().getSourceFile())); } private void addContractViolation(Message m, FieldMetaData fmd) { // add the violation as an error in case we're processing violations // as errors; this keeps them in the order that they were found rather // than just adding the violations to the end of the list. - if (failOnContractViolations) + if (failOnContractViolations) { addError(m, fmd); + } - if (contractViolations == null) + if (contractViolations == null) { contractViolations = new ArrayList(); + } contractViolations.add(loc.get - ("subclasser-contract-violation-field", m.getMessage(), - fmd.getFullName(), fmd.getDeclaringMetaData().getSourceFile())); + ("subclasser-contract-violation-field", m.getMessage(), + fmd.getFullName(), fmd.getDeclaringMetaData().getSourceFile())); } } diff --git a/openjpa-kernel/src/main/java/org/apache/openjpa/meta/AbstractMetaDataDefaults.java b/openjpa-kernel/src/main/java/org/apache/openjpa/meta/AbstractMetaDataDefaults.java index 6108d7daa..6c7b73340 100644 --- a/openjpa-kernel/src/main/java/org/apache/openjpa/meta/AbstractMetaDataDefaults.java +++ b/openjpa-kernel/src/main/java/org/apache/openjpa/meta/AbstractMetaDataDefaults.java @@ -187,11 +187,11 @@ public abstract class AbstractMetaDataDefaults Member member; FieldMetaData fmd; for (int i = 0; i < fieldNames.length; i ++) { - String property = fieldNames[i]; - member = getMemberByProperty(meta, property, - AccessCode.UNKNOWN, true); - if (member == null) // transient or indeterminable access - continue; + String property = fieldNames[i]; + member = getMemberByProperty(meta, property, AccessCode.UNKNOWN, true); + if (member == null) { // transient or indeterminable access + continue; + } fmd = meta.addDeclaredField(property, fieldTypes[i]); fmd.backingMember(member); populate(fmd); @@ -285,21 +285,27 @@ public abstract class AbstractMetaDataDefaults * the next letter lower-cased. For other methods, returns null. */ public static String getFieldName(Member member) { - if (member instanceof Field) + if (member instanceof Field) { return member.getName(); - if (!(member instanceof Method)) - return null; + } + if (!(member instanceof Method)) { + return null; + } Method method = (Method) member; String name = method.getName(); - if (isNormalGetter(method)) - name = name.substring("get".length()); - else if (isBooleanGetter(method)) - name = name.substring("is".length()); - else + if (isNormalGetter(method)) { + name = name.substring("get".length()); + } + else if (isBooleanGetter(method)) { + name = name.substring("is".length()); + } + else { return null; + } - if (name.length() == 1) + if (name.length() == 1) { return name.toLowerCase(); + } return Character.toLowerCase(name.charAt(0)) + name.substring(1); } @@ -335,7 +341,7 @@ public abstract class AbstractMetaDataDefaults if (fmd == null) return null; if (fmd.getBackingMember() != null) - return fmd.getBackingMember(); + return fmd.getBackingMember(); return getMemberByProperty(fmd.getDeclaringMetaData(), fmd.getName(), fmd.getAccessType(), true); } @@ -362,10 +368,10 @@ public abstract class AbstractMetaDataDefaults * where T is any non-void type. */ public static boolean isNormalGetter(Method method) { - String methodName = method.getName(); - return startsWith(methodName, "get") - && method.getParameterTypes().length == 0 - && method.getReturnType() != void.class; + String methodName = method.getName(); + return startsWith(methodName, "get") + && method.getParameterTypes().length == 0 + && method.getReturnType() != void.class; } /** @@ -374,10 +380,10 @@ public abstract class AbstractMetaDataDefaults * public Boolean isXXX() */ public static boolean isBooleanGetter(Method method) { - String methodName = method.getName(); - return startsWith(methodName, "is") - && method.getParameterTypes().length == 0 - && isBoolean(method.getReturnType()); + String methodName = method.getName(); + return startsWith(methodName, "is") + && method.getParameterTypes().length == 0 + && isBoolean(method.getReturnType()); } /** @@ -388,16 +394,16 @@ public abstract class AbstractMetaDataDefaults * public T isXXX() where T is boolean or Boolean.
*/ public static boolean isGetter(Method method, boolean includePrivate) { - if (method == null) - return false; - int mods = method.getModifiers(); - if (!(Modifier.isPublic(mods) - || Modifier.isProtected(mods) - || (Modifier.isPrivate(mods) && includePrivate)) - || Modifier.isNative(mods) - || Modifier.isStatic(mods)) - return false; - return isNormalGetter(method) || isBooleanGetter(method); + if (method == null) + return false; + int mods = method.getModifiers(); + if (!(Modifier.isPublic(mods) + || Modifier.isProtected(mods) + || (Modifier.isPrivate(mods) && includePrivate)) + || Modifier.isNative(mods) + || Modifier.isStatic(mods)) + return false; + return isNormalGetter(method) || isBooleanGetter(method); } /** @@ -409,14 +415,14 @@ public abstract class AbstractMetaDataDefaults } public static boolean isBoolean(Class cls) { - return cls == boolean.class || cls == Boolean.class; + return cls == boolean.class || cls == Boolean.class; } public static List toNames(List members) { - List result = new ArrayList<>(); - for (Member m : members) - result.add(m.getName()); - return result; + List result = new ArrayList<>(); + for (Member m : members) + result.add(m.getName()); + return result; } } diff --git a/openjpa-kernel/src/main/java/org/apache/openjpa/util/asm/AsmHelper.java b/openjpa-kernel/src/main/java/org/apache/openjpa/util/asm/AsmHelper.java index 5f6ac6860..fcebd4c49 100644 --- a/openjpa-kernel/src/main/java/org/apache/openjpa/util/asm/AsmHelper.java +++ b/openjpa-kernel/src/main/java/org/apache/openjpa/util/asm/AsmHelper.java @@ -23,6 +23,8 @@ import org.apache.xbean.asm9.ClassReader; 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.VarInsnNode; import serp.bytecode.BCClass; import serp.bytecode.Project; @@ -211,4 +213,32 @@ public final class AsmHelper { return types; } + /** + * @return true if the instruction is an LOAD instruction + */ + public static boolean isLoadInsn(AbstractInsnNode insn) { + return insn.getOpcode() == Opcodes.ALOAD + || insn.getOpcode() == Opcodes.ILOAD + || insn.getOpcode() == Opcodes.IALOAD + || insn.getOpcode() == Opcodes.LLOAD + || insn.getOpcode() == Opcodes.LALOAD + || insn.getOpcode() == Opcodes.FLOAD + || insn.getOpcode() == Opcodes.FALOAD + || insn.getOpcode() == Opcodes.DLOAD + || insn.getOpcode() == Opcodes.DALOAD + || insn.getOpcode() == Opcodes.BALOAD + || insn.getOpcode() == Opcodes.CALOAD + || insn.getOpcode() == Opcodes.SALOAD; + + + } + + /** + * @return true if the instruction is an ALOAD_0 + */ + public static boolean isThisInsn(AbstractInsnNode insn) { + return insn instanceof VarInsnNode + && insn.getOpcode() == Opcodes.ALOAD + && ((VarInsnNode)insn).var == 0; + } } diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/TestPCEnhancerFindField.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/TestPCEnhancerFindField.java index 44077384f..069a54b55 100644 --- a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/TestPCEnhancerFindField.java +++ b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/TestPCEnhancerFindField.java @@ -40,7 +40,7 @@ public class TestPCEnhancerFindField Project proj = new Project(); BCClass bc = proj.loadClass(getClass()); BCMethod meth = bc.getMethods("myMethod")[0]; - BCField field = PCEnhancer.getReturnedField(meth); + BCField field = PCEnhancer.getReturnedField_old(meth); assertEquals("field", field.getName()); } } diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/TestSubclassValidator.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/TestSubclassValidator.java new file mode 100644 index 000000000..681ca6a11 --- /dev/null +++ b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/TestSubclassValidator.java @@ -0,0 +1,126 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.openjpa.enhance; + +import org.apache.openjpa.conf.OpenJPAConfiguration; +import org.apache.openjpa.lib.log.Log; +import org.apache.openjpa.lib.util.TemporaryClassLoader; +import org.apache.openjpa.meta.ClassMetaData; +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.junit.Test; + +import jakarta.persistence.Access; +import jakarta.persistence.AccessType; +import jakarta.persistence.Basic; +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import serp.bytecode.BCClass; +import serp.bytecode.Project; + +public class TestSubclassValidator extends SingleEMFTestCase { + @Override + public void setUp() throws Exception { + setUp("openjpa.RuntimeUnenhancedClasses", "supported", + Department.class, + RuntimeTest2.class, + EnhancableGetterEntity.class, + UnenhancedPropertyAccess.class, + CLEAR_TABLES); + } + + @Test + public void testBcSubclassValidator() throws Exception { + Project project = new Project(); + TemporaryClassLoader tempCl = new TemporaryClassLoader(this.getClass().getClassLoader()); + final OpenJPAConfiguration conf = emf.getConfiguration(); + + final MetaDataRepository repos = conf.newMetaDataRepositoryInstance(); + repos.setSourceMode(MetaDataModes.MODE_META); + + final Log log = conf.getLog(OpenJPAConfiguration.LOG_ENHANCE); +/* + + { + final BCClass bcClass = project.loadClass(Department.class.getName(), tempCl); + final ClassMetaData meta = repos.getMetaData(tempCl.loadClass(Department.class.getName()), tempCl, false); + PCSubclassValidator subclassValidator = new PCSubclassValidator(meta, bcClass, log, false); + subclassValidator.assertCanSubclass(); + } + + try { + final BCClass bcClass = project.loadClass(RuntimeTest2.class.getName(), tempCl); + final ClassMetaData meta = repos.getMetaData(tempCl.loadClass(RuntimeTest2.class.getName()), tempCl, false); + PCSubclassValidator subclassValidator = new PCSubclassValidator(meta, bcClass, log, true); + subclassValidator.assertCanSubclass(); + fail("RuntimeTest2 validation should fail"); + } + catch (UserException ue) { + // all fine + } +*/ + + { + final BCClass bcClass = project.loadClass(EnhancableGetterEntity.class.getName(), tempCl); + final ClassMetaData meta = repos.getMetaData(tempCl.loadClass(EnhancableGetterEntity.class.getName()), tempCl, false); + PCSubclassValidator subclassValidator = new PCSubclassValidator(meta, bcClass, log, true); + subclassValidator.assertCanSubclass(); + } + + { + 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); + subclassValidator.assertCanSubclass(); + } + } + + @Entity + @Access(AccessType.PROPERTY) + public static class EnhancableGetterEntity { + + @Id + private String id; + + @Basic + private String name; + + @Basic + private String another; + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public String getDifferent() { + return another; + } + + public void setDifferent(String another) { + this.another = another; + } + } +}