From 0fda94a9a5ab4984ddd013ca1a4a635c0e6ee57d Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 24 May 2021 17:25:20 -0400 Subject: [PATCH 1/6] Remove ASM dep from `ClassDescriptor` --- core/pom.xml | 5 - .../CaptureParameterNameTransformation.java | 1 + .../stapler/CapturedParameterNames.java | 1 + .../org/kohsuke/stapler/ClassDescriptor.java | 132 +++--------------- .../kohsuke/stapler/ClassDescriptorTest.java | 15 +- 5 files changed, 23 insertions(+), 131 deletions(-) diff --git a/core/pom.xml b/core/pom.xml index fce8cdd130..c03db644ec 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -87,11 +87,6 @@ 1.8.3 true - - org.kohsuke - asm5 - 5.0.1 - org.jenkins-ci annotation-indexer diff --git a/core/src/main/java/org/kohsuke/stapler/CaptureParameterNameTransformation.java b/core/src/main/java/org/kohsuke/stapler/CaptureParameterNameTransformation.java index db99a13ec2..c0948ec77e 100644 --- a/core/src/main/java/org/kohsuke/stapler/CaptureParameterNameTransformation.java +++ b/core/src/main/java/org/kohsuke/stapler/CaptureParameterNameTransformation.java @@ -44,6 +44,7 @@ * Groovy AST transformation that capture necessary parameter names. * * @author Kohsuke Kawaguchi + * @see CapturedParameterNames */ @MetaInfServices @GroovyASTTransformation diff --git a/core/src/main/java/org/kohsuke/stapler/CapturedParameterNames.java b/core/src/main/java/org/kohsuke/stapler/CapturedParameterNames.java index 75904c84d3..51074d44c3 100644 --- a/core/src/main/java/org/kohsuke/stapler/CapturedParameterNames.java +++ b/core/src/main/java/org/kohsuke/stapler/CapturedParameterNames.java @@ -32,6 +32,7 @@ * to do than generating the same files that the annotation processor does. * * @author Kohsuke Kawaguchi + * @see CaptureParameterNameTransformation */ @Retention(RUNTIME) public @interface CapturedParameterNames { diff --git a/core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java b/core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java index 3aaee0349c..01db645c9b 100644 --- a/core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java +++ b/core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java @@ -25,18 +25,15 @@ import org.apache.commons.io.IOUtils; import org.jvnet.tiger_types.Types; -import org.kohsuke.asm5.ClassReader; -import org.kohsuke.asm5.ClassVisitor; -import org.kohsuke.asm5.Label; -import org.kohsuke.asm5.MethodVisitor; -import org.kohsuke.asm5.Type; import java.io.IOException; import java.io.InputStream; import java.lang.reflect.Constructor; +import java.lang.reflect.Executable; import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.lang.reflect.Parameter; import java.lang.reflect.ParameterizedType; import java.net.URL; import java.util.ArrayList; @@ -49,12 +46,10 @@ import java.util.Map; import java.util.Properties; import java.util.Set; -import java.util.TreeMap; import java.util.logging.Logger; -import static java.util.logging.Level.FINE; import static java.util.logging.Level.WARNING; -import static org.kohsuke.asm5.Opcodes.ASM5; +import java.util.stream.Stream; /** * Reflection information of a {@link Class}. @@ -192,13 +187,9 @@ public static String[] loadParameterNames(Method m) { if(cpn!=null) return cpn.value(); // debug information, if present, is more trustworthy - try { - String[] n = ASM.loadParametersFromAsm(m); - if (n!=null) return n; - } catch (LinkageError e) { - LOGGER.log(FINE, "Incompatible ASM", e); - } catch (IOException e) { - LOGGER.log(WARNING, "Failed to load a class file", e); + String[] n = loadParameterNamesFromReflection(m); + if (n != null) { + return n; } // otherwise check the .stapler file @@ -229,19 +220,24 @@ public static String[] loadParameterNames(Constructor m) { if(cpn!=null) return cpn.value(); // debug information, if present, is more trustworthy - try { - String[] n = ASM.loadParametersFromAsm(m); - if (n!=null) return n; - } catch (LinkageError e) { - LOGGER.log(FINE, "Incompatible ASM", e); - } catch (IOException e) { - LOGGER.log(WARNING, "Failed to load a class file", e); + String[] n = loadParameterNamesFromReflection(m); + if (n != null) { + return n; } // couldn't find it return EMPTY_ARRAY; } + static String[] loadParameterNamesFromReflection(final Executable m) { + Parameter[] ps = m.getParameters(); + if (Stream.of(ps).allMatch(Parameter::isNamePresent)) { + return Stream.of(ps).map(Parameter::getName).toArray(String[]::new); + } else { + return null; + } + } + /** * Determines the constructor parameter names. * @@ -293,98 +289,6 @@ public String[] loadConstructorParamNames() { "Run 'mvn clean compile' once to run the annotation processor."); } - - /** - * Isolate the ASM dependency to its own class, as otherwise this seems to cause linkage error on the whole {@link ClassDescriptor}. - */ - /*package*/ static class ASM { - /** - * Try to load parameter names from the debug info by using ASM. - */ - private static String[] loadParametersFromAsm(final Method m) throws IOException { - final String[] paramNames = new String[m.getParameterTypes().length]; - if (paramNames.length==0) return paramNames; - Class c = m.getDeclaringClass(); - URL clazz = c.getClassLoader().getResource(c.getName().replace('.', '/') + ".class"); - if (clazz==null) return null; - - final TreeMap localVars = new TreeMap(); - ClassReader r = new ClassReader(clazz.openStream()); - r.accept(new ClassVisitor(ASM5) { - final String md = Type.getMethodDescriptor(m); - // First localVariable is "this" for non-static method - final int limit = (m.getModifiers() & Modifier.STATIC) != 0 ? 0 : 1; - @Override public MethodVisitor visitMethod(int access, String methodName, String desc, String signature, String[] exceptions) { - if (methodName.equals(m.getName()) && desc.equals(md)) - return new MethodVisitor(ASM5) { - @Override public void visitLocalVariable(String name, String desc, String signature, Label start, Label end, int index) { - if (index >= limit) - localVars.put(index, name); - } - }; - else - return null; // ignore this method - } - }, 0); - - // Indexes may not be sequential, but first set of local variables are method params - int i = 0; - for (String s : localVars.values()) { - paramNames[i] = s; - if (++i == paramNames.length) return paramNames; - } - return null; // Not enough data found to fill array - } - - /** - * Try to load parameter names from the debug info by using ASM. - */ - private static String[] loadParametersFromAsm(final Constructor m) throws IOException { - final String[] paramNames = new String[m.getParameterTypes().length]; - if (paramNames.length==0) return paramNames; - Class c = m.getDeclaringClass(); - URL clazz = c.getClassLoader().getResource(c.getName().replace('.', '/') + ".class"); - if (clazz==null) return null; - - final TreeMap localVars = new TreeMap(); - InputStream is = clazz.openStream(); - try { - ClassReader r = new ClassReader(is); - r.accept(new ClassVisitor(ASM5) { - final String md = getConstructorDescriptor(m); - public MethodVisitor visitMethod(int access, String methodName, String desc, String signature, String[] exceptions) { - if (methodName.equals("") && desc.equals(md)) - return new MethodVisitor(ASM5) { - @Override public void visitLocalVariable(String name, String desc, String signature, Label start, Label end, int index) { - if (index>0) // 0 is 'this' - localVars.put(index, name); - } - }; - else - return null; // ignore this method - } - }, 0); - } finally { - is.close(); - } - - // Indexes may not be sequential, but first set of local variables are method params - int i = 0; - for (String s : localVars.values()) { - paramNames[i] = s; - if (++i == paramNames.length) return paramNames; - } - return null; // Not enough data found to fill array - } - - private static String getConstructorDescriptor(Constructor c) { - StringBuilder buf = new StringBuilder("("); - for (Class p : c.getParameterTypes()) - buf.append(Type.getDescriptor(p)); - return buf.append(")V").toString(); - } - } - final static class MethodMirror { final Signature sig; final Method method; diff --git a/core/src/test/java/org/kohsuke/stapler/ClassDescriptorTest.java b/core/src/test/java/org/kohsuke/stapler/ClassDescriptorTest.java index 84df994e09..43dbbe2830 100644 --- a/core/src/test/java/org/kohsuke/stapler/ClassDescriptorTest.java +++ b/core/src/test/java/org/kohsuke/stapler/ClassDescriptorTest.java @@ -3,19 +3,14 @@ import java.io.IOException; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; -import java.lang.annotation.Target; -import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Arrays; import java.util.HashMap; import java.util.Map; -import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.servlet.ServletException; import static org.junit.Assert.*; import org.junit.Test; -import org.kohsuke.stapler.verb.GET; -import org.kohsuke.stapler.verb.POST; /** * @author Alan Harder @@ -28,11 +23,7 @@ public class ClassDescriptorTest { assertEquals("[a, b, x]",Arrays.asList(names).toString()); } - @Test public void loadParametersFromAsm() throws Exception { - // get private method that is being tested - Method lpfa = ClassDescriptor.ASM.class.getDeclaredMethod( - "loadParametersFromAsm", Method.class); - lpfa.setAccessible(true); + @Test public void loadParameterNamesFromReflection() throws Exception { // collect test cases Map testCases = new HashMap(); for (Method m : ClassDescriptorTest.class.getDeclaredMethods()) @@ -48,8 +39,8 @@ public class ClassDescriptorTest { for (Map.Entry entry : expected.entrySet()) { Method testMethod = testCases.get(entry.getKey()); assertNotNull("Method missing for " + entry.getKey(), testMethod); - String[] result = (String[])lpfa.invoke(null, testMethod); - assertNotNull("Null result for " + entry.getKey()); + String[] result = ClassDescriptor.loadParameterNamesFromReflection(testMethod); + assertNotNull("Null result for " + entry.getKey(), result); if (!Arrays.equals(entry.getValue(), result)) { StringBuilder buf = new StringBuilder('|'); for (String s : result) buf.append(s).append('|'); From 0d8bac1b95f1a2690bef90638add3f520f982ec3 Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Sun, 30 May 2021 08:41:21 -0700 Subject: [PATCH 2/6] Generate metadata for reflection on method parameters --- pom.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pom.xml b/pom.xml index 170626e3f7..9c4951c189 100644 --- a/pom.xml +++ b/pom.xml @@ -60,6 +60,9 @@ 999999-SNAPSHOT HEAD ${project.basedir}/../src/spotbugs/spotbugs-excludes.xml + + + true From e11ea45a9dd95816bfef04cb0572dd2523cb65b4 Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Sun, 30 May 2021 08:42:02 -0700 Subject: [PATCH 3/6] Restore ASM as a fallback for when method parameter metadata is not available via reflection --- core/pom.xml | 5 + .../org/kohsuke/stapler/ClassDescriptor.java | 126 +++++++++++++++++- .../kohsuke/stapler/ClassDescriptorTest.java | 35 +++++ 3 files changed, 163 insertions(+), 3 deletions(-) diff --git a/core/pom.xml b/core/pom.xml index ad551c0bb6..7aac9f3424 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -87,6 +87,11 @@ 2.4.12 true + + org.ow2.asm + asm + 9.1 + org.jenkins-ci annotation-indexer diff --git a/core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java b/core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java index 01db645c9b..c21d4e8908 100644 --- a/core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java +++ b/core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java @@ -25,6 +25,12 @@ import org.apache.commons.io.IOUtils; import org.jvnet.tiger_types.Types; +import org.objectweb.asm.ClassReader; +import org.objectweb.asm.ClassVisitor; +import org.objectweb.asm.Label; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; import java.io.IOException; import java.io.InputStream; @@ -46,10 +52,12 @@ import java.util.Map; import java.util.Properties; import java.util.Set; +import java.util.TreeMap; import java.util.logging.Logger; +import java.util.stream.Stream; +import static java.util.logging.Level.FINE; import static java.util.logging.Level.WARNING; -import java.util.stream.Stream; /** * Reflection information of a {@link Class}. @@ -186,12 +194,22 @@ public static String[] loadParameterNames(Method m) { CapturedParameterNames cpn = m.getAnnotation(CapturedParameterNames.class); if(cpn!=null) return cpn.value(); - // debug information, if present, is more trustworthy + // reflection is the most efficient and supported system String[] n = loadParameterNamesFromReflection(m); if (n != null) { return n; } + // debug information, if present, is more trustworthy + try { + n = ASM.loadParametersFromAsm(m); + if (n!=null) return n; + } catch (LinkageError e) { + LOGGER.log(FINE, "Incompatible ASM", e); + } catch (IOException e) { + LOGGER.log(WARNING, "Failed to load a class file", e); + } + // otherwise check the .stapler file Class c = m.getDeclaringClass(); URL url = c.getClassLoader().getResource( @@ -219,12 +237,22 @@ public static String[] loadParameterNames(Constructor m) { CapturedParameterNames cpn = m.getAnnotation(CapturedParameterNames.class); if(cpn!=null) return cpn.value(); - // debug information, if present, is more trustworthy + // reflection is the most efficient and supported system String[] n = loadParameterNamesFromReflection(m); if (n != null) { return n; } + // debug information, if present, is more trustworthy + try { + n = ASM.loadParametersFromAsm(m); + if (n!=null) return n; + } catch (LinkageError e) { + LOGGER.log(FINE, "Incompatible ASM", e); + } catch (IOException e) { + LOGGER.log(WARNING, "Failed to load a class file", e); + } + // couldn't find it return EMPTY_ARRAY; } @@ -289,6 +317,98 @@ public String[] loadConstructorParamNames() { "Run 'mvn clean compile' once to run the annotation processor."); } + + /** + * Isolate the ASM dependency to its own class, as otherwise this seems to cause linkage error on the whole {@link ClassDescriptor}. + */ + /*package*/ static class ASM { + /** + * Try to load parameter names from the debug info by using ASM. + */ + private static String[] loadParametersFromAsm(final Method m) throws IOException { + final String[] paramNames = new String[m.getParameterTypes().length]; + if (paramNames.length==0) return paramNames; + Class c = m.getDeclaringClass(); + URL clazz = c.getClassLoader().getResource(c.getName().replace('.', '/') + ".class"); + if (clazz==null) return null; + + final TreeMap localVars = new TreeMap(); + ClassReader r = new ClassReader(clazz.openStream()); + r.accept(new ClassVisitor(Opcodes.ASM9) { + final String md = Type.getMethodDescriptor(m); + // First localVariable is "this" for non-static method + final int limit = (m.getModifiers() & Modifier.STATIC) != 0 ? 0 : 1; + @Override public MethodVisitor visitMethod(int access, String methodName, String desc, String signature, String[] exceptions) { + if (methodName.equals(m.getName()) && desc.equals(md)) + return new MethodVisitor(Opcodes.ASM9) { + @Override public void visitLocalVariable(String name, String desc, String signature, Label start, Label end, int index) { + if (index >= limit) + localVars.put(index, name); + } + }; + else + return null; // ignore this method + } + }, 0); + + // Indexes may not be sequential, but first set of local variables are method params + int i = 0; + for (String s : localVars.values()) { + paramNames[i] = s; + if (++i == paramNames.length) return paramNames; + } + return null; // Not enough data found to fill array + } + + /** + * Try to load parameter names from the debug info by using ASM. + */ + private static String[] loadParametersFromAsm(final Constructor m) throws IOException { + final String[] paramNames = new String[m.getParameterTypes().length]; + if (paramNames.length==0) return paramNames; + Class c = m.getDeclaringClass(); + URL clazz = c.getClassLoader().getResource(c.getName().replace('.', '/') + ".class"); + if (clazz==null) return null; + + final TreeMap localVars = new TreeMap(); + InputStream is = clazz.openStream(); + try { + ClassReader r = new ClassReader(is); + r.accept(new ClassVisitor(Opcodes.ASM9) { + final String md = getConstructorDescriptor(m); + public MethodVisitor visitMethod(int access, String methodName, String desc, String signature, String[] exceptions) { + if (methodName.equals("") && desc.equals(md)) + return new MethodVisitor(Opcodes.ASM9) { + @Override public void visitLocalVariable(String name, String desc, String signature, Label start, Label end, int index) { + if (index>0) // 0 is 'this' + localVars.put(index, name); + } + }; + else + return null; // ignore this method + } + }, 0); + } finally { + is.close(); + } + + // Indexes may not be sequential, but first set of local variables are method params + int i = 0; + for (String s : localVars.values()) { + paramNames[i] = s; + if (++i == paramNames.length) return paramNames; + } + return null; // Not enough data found to fill array + } + + private static String getConstructorDescriptor(Constructor c) { + StringBuilder buf = new StringBuilder("("); + for (Class p : c.getParameterTypes()) + buf.append(Type.getDescriptor(p)); + return buf.append(")V").toString(); + } + } + final static class MethodMirror { final Signature sig; final Method method; diff --git a/core/src/test/java/org/kohsuke/stapler/ClassDescriptorTest.java b/core/src/test/java/org/kohsuke/stapler/ClassDescriptorTest.java index 43dbbe2830..008374a46c 100644 --- a/core/src/test/java/org/kohsuke/stapler/ClassDescriptorTest.java +++ b/core/src/test/java/org/kohsuke/stapler/ClassDescriptorTest.java @@ -3,14 +3,19 @@ import java.io.IOException; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Arrays; import java.util.HashMap; import java.util.Map; +import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.servlet.ServletException; import static org.junit.Assert.*; import org.junit.Test; +import org.kohsuke.stapler.verb.GET; +import org.kohsuke.stapler.verb.POST; /** * @author Alan Harder @@ -49,6 +54,36 @@ public class ClassDescriptorTest { } } + @Test public void loadParametersFromAsm() throws Exception { + // get private method that is being tested + Method lpfa = ClassDescriptor.ASM.class.getDeclaredMethod( + "loadParametersFromAsm", Method.class); + lpfa.setAccessible(true); + // collect test cases + Map testCases = new HashMap(); + for (Method m : ClassDescriptorTest.class.getDeclaredMethods()) + if (m.getName().startsWith("methodWith")) + testCases.put(m.getName().substring(10), m); + // expected results + Map expected = new HashMap(); + expected.put("NoParams", new String[0]); + expected.put("NoParams_static", new String[0]); + expected.put("ManyParams", new String[] { "a", "b", "c", "d", "e", "f", "g", "h", "i" }); + expected.put("Params_static", new String[] { "abc", "def", "ghi" }); + // run tests + for (Map.Entry entry : expected.entrySet()) { + Method testMethod = testCases.get(entry.getKey()); + assertNotNull("Method missing for " + entry.getKey(), testMethod); + String[] result = (String[])lpfa.invoke(null, testMethod); + assertNotNull("Null result for " + entry.getKey()); + if (!Arrays.equals(entry.getValue(), result)) { + StringBuilder buf = new StringBuilder('|'); + for (String s : result) buf.append(s).append('|'); + fail("Unexpected result for " + entry.getKey() + ": " + buf); + } + } + } + @Test public void inheritedWebMethods() throws Exception { // http://bugs.sun.com/view_bug.do?bug_id=6342411 assertEquals(1, new ClassDescriptor(Sub.class).methods.name("doDynamic").signature(StaplerRequest.class, StaplerResponse.class).size()); From 0fa4befbb637212f6e0388f6f56c5947b0070369 Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Tue, 1 Jun 2021 11:32:28 -0700 Subject: [PATCH 4/6] Use plugin configuration rather than properties --- pom.xml | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/pom.xml b/pom.xml index 9c4951c189..c53d47fa79 100644 --- a/pom.xml +++ b/pom.xml @@ -60,9 +60,6 @@ 999999-SNAPSHOT HEAD ${project.basedir}/../src/spotbugs/spotbugs-excludes.xml - - - true @@ -92,6 +89,21 @@ + + org.apache.maven.plugins + maven-compiler-plugin + + + + compile + + + + + + true + + From 3c9f18e977ac5594ce3b80ae0a85d417978bec26 Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Tue, 1 Jun 2021 11:47:08 -0700 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Jesse Glick --- pom.xml | 7 ------- 1 file changed, 7 deletions(-) diff --git a/pom.xml b/pom.xml index c53d47fa79..5297f34689 100644 --- a/pom.xml +++ b/pom.xml @@ -92,13 +92,6 @@ org.apache.maven.plugins maven-compiler-plugin - - - - compile - - - true From 0e81ad0c4a50e0b1155a8e6a0dafc88249ba2788 Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Tue, 1 Jun 2021 12:42:08 -0700 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Jesse Glick --- pom.xml | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pom.xml b/pom.xml index 0d7a62af4f..3e3de18ec3 100644 --- a/pom.xml +++ b/pom.xml @@ -89,14 +89,6 @@ - - org.apache.maven.plugins - maven-compiler-plugin - - - true - -