Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce ASM dep in ClassDescriptor #233

Merged
merged 9 commits into from
Jun 1, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
* Groovy AST transformation that capture necessary parameter names.
*
* @author Kohsuke Kawaguchi
* @see CapturedParameterNames
*/
@MetaInfServices
@GroovyASTTransformation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
28 changes: 26 additions & 2 deletions core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,11 @@
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;
Expand All @@ -52,6 +54,7 @@
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;
Expand Down Expand Up @@ -191,9 +194,15 @@ public static String[] loadParameterNames(Method m) {
CapturedParameterNames cpn = m.getAnnotation(CapturedParameterNames.class);
if(cpn!=null) return cpn.value();

// 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 {
String[] n = ASM.loadParametersFromAsm(m);
n = ASM.loadParametersFromAsm(m);
if (n!=null) return n;
} catch (LinkageError e) {
LOGGER.log(FINE, "Incompatible ASM", e);
Expand Down Expand Up @@ -228,9 +237,15 @@ public static String[] loadParameterNames(Constructor<?> m) {
CapturedParameterNames cpn = m.getAnnotation(CapturedParameterNames.class);
if(cpn!=null) return cpn.value();

// 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 {
String[] n = ASM.loadParametersFromAsm(m);
n = ASM.loadParametersFromAsm(m);
if (n!=null) return n;
} catch (LinkageError e) {
LOGGER.log(FINE, "Incompatible ASM", e);
Expand All @@ -242,6 +257,15 @@ public static String[] loadParameterNames(Constructor<?> m) {
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.
*
Expand Down
26 changes: 26 additions & 0 deletions core/src/test/java/org/kohsuke/stapler/ClassDescriptorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,32 @@ public class ClassDescriptorTest {
assertEquals("[a, b, x]",Arrays.asList(names).toString());
}

@Test public void loadParameterNamesFromReflection() throws Exception {
// collect test cases
Map<String,Method> testCases = new HashMap<String,Method>();
for (Method m : ClassDescriptorTest.class.getDeclaredMethods())
if (m.getName().startsWith("methodWith"))
testCases.put(m.getName().substring(10), m);
// expected results
Map<String,String[]> expected = new HashMap<String,String[]>();
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<String,String[]> entry : expected.entrySet()) {
Method testMethod = testCases.get(entry.getKey());
assertNotNull("Method missing for " + entry.getKey(), testMethod);
String[] result = ClassDescriptor.loadParameterNamesFromReflection(testMethod);
assertNotNull("Null result for " + entry.getKey(), result);
if (!Arrays.equals(entry.getValue(), result)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use a standard Matcher for this I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just trying to change the existing code as little as possible. This is pretty much a copy of loadParametersFromAsm. There is a lot that can be cleaned up in this repository. Once the dependencies are up-to-date I'll start looking into cleanups like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular the use of reflection in the ASM test is unnecessary (fixed in #226).

StringBuilder buf = new StringBuilder('|');
for (String s : result) buf.append(s).append('|');
fail("Unexpected result for " + entry.getKey() + ": " + buf);
}
}
}

@Test public void loadParametersFromAsm() throws Exception {
// get private method that is being tested
Method lpfa = ClassDescriptor.ASM.class.getDeclaredMethod(
Expand Down
15 changes: 15 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,21 @@
</ignores>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, for official plugins this can be omitted if you like:

Suggested change
<groupId>org.apache.maven.plugins</groupId>

or kept for legibility.

<artifactId>maven-compiler-plugin</artifactId>
<executions>
<execution>
<goals>
<goal>compile</goal>
</goals>
</execution>
</executions>
basil marked this conversation as resolved.
Show resolved Hide resolved
<configuration>
<!-- Generate metadata for reflection on method parameters -->
<parameters>true</parameters>
</configuration>
</plugin>
</plugins>
</pluginManagement>
</build>
Expand Down