[
https://issues.apache.org/jira/browse/LANG-1544?focusedWorklogId=527237&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-527237
]
ASF GitHub Bot logged work on LANG-1544:
----------------------------------------
Author: ASF GitHub Bot
Created on: 22/Dec/20 15:21
Start Date: 22/Dec/20 15:21
Worklog Time Spent: 10m
Work Description: garydgregory commented on a change in pull request #680:
URL: https://github.com/apache/commons-lang/pull/680#discussion_r547326472
##########
File path: src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java
##########
@@ -742,35 +745,51 @@ public static Method getMatchingMethod(final Class<?>
cls, final String methodNa
Validate.notNull(cls, "cls");
Validate.notEmpty(methodName, "methodName");
- // Address methods in superclasses
- Method[] methodArray = cls.getDeclaredMethods();
- final List<Class<?>> superclassList =
ClassUtils.getAllSuperclasses(cls);
- for (final Class<?> klass : superclassList) {
- methodArray = ArrayUtils.addAll(methodArray,
klass.getDeclaredMethods());
- }
+ final List<Method> methods = Arrays.stream(cls.getDeclaredMethods())
+ .filter(method -> method.getName().equals(methodName))
+ .collect(toList());
+
+ ClassUtils.getAllSuperclasses(cls).stream()
+ .map(Class::getDeclaredMethods)
+ .flatMap(Arrays::stream)
+ .filter(method -> method.getName().equals(methodName))
+ .forEach(methods::add);
- Method inexactMatch = null;
- for (final Method method : methodArray) {
- if (methodName.equals(method.getName()) &&
- Objects.deepEquals(parameterTypes,
method.getParameterTypes())) {
+ for (Method method : methods) {
+ if (Arrays.deepEquals(method.getParameterTypes(), parameterTypes))
{
return method;
- } else if (methodName.equals(method.getName()) &&
- ClassUtils.isAssignable(parameterTypes,
method.getParameterTypes(), true)) {
- if ((inexactMatch == null) || (distance(parameterTypes,
method.getParameterTypes())
- < distance(parameterTypes,
inexactMatch.getParameterTypes()))) {
- inexactMatch = method;
- }
}
+ }
+
+ final TreeMap<Double, List<Method>> candidates = new TreeMap<>();
+ methods.stream()
+ .filter(method -> ClassUtils.isAssignable(parameterTypes,
method.getParameterTypes(), true))
+ .forEach(method -> {
+ final double distance = distance(parameterTypes,
method.getParameterTypes());
+ List<Method> methods1 =
candidates.computeIfAbsent(distance, k -> new ArrayList<>());
+ methods1.add(method);
+ });
+
+ if (candidates.isEmpty()) {
+ return null;
}
- return inexactMatch;
+
+ final List<Method> bestCandidates =
candidates.values().iterator().next();
+ if (bestCandidates.size() == 1) {
+ return bestCandidates.get(0);
+ }
+
+ final String target = methodName +
Arrays.stream(parameterTypes).map(String::valueOf).collect(Collectors.joining(",",
"(", ")"));
+ final String strCandidates =
bestCandidates.stream().map(Method::toString).collect(Collectors.joining("\n
"));
+ throw new IllegalStateException("Found multiple candidates for method
" + target + " on class " + cls + ":\n " + strCandidates);
Review comment:
Let's not hard-code Linux-specific new-line chars in error messages,
but, if you really want new-lines, use `%n` with `String.format()`.
##########
File path: src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java
##########
@@ -742,35 +745,51 @@ public static Method getMatchingMethod(final Class<?>
cls, final String methodNa
Validate.notNull(cls, "cls");
Validate.notEmpty(methodName, "methodName");
- // Address methods in superclasses
- Method[] methodArray = cls.getDeclaredMethods();
- final List<Class<?>> superclassList =
ClassUtils.getAllSuperclasses(cls);
- for (final Class<?> klass : superclassList) {
- methodArray = ArrayUtils.addAll(methodArray,
klass.getDeclaredMethods());
- }
+ final List<Method> methods = Arrays.stream(cls.getDeclaredMethods())
+ .filter(method -> method.getName().equals(methodName))
+ .collect(toList());
+
+ ClassUtils.getAllSuperclasses(cls).stream()
+ .map(Class::getDeclaredMethods)
+ .flatMap(Arrays::stream)
+ .filter(method -> method.getName().equals(methodName))
+ .forEach(methods::add);
- Method inexactMatch = null;
- for (final Method method : methodArray) {
- if (methodName.equals(method.getName()) &&
- Objects.deepEquals(parameterTypes,
method.getParameterTypes())) {
+ for (Method method : methods) {
+ if (Arrays.deepEquals(method.getParameterTypes(), parameterTypes))
{
return method;
- } else if (methodName.equals(method.getName()) &&
- ClassUtils.isAssignable(parameterTypes,
method.getParameterTypes(), true)) {
- if ((inexactMatch == null) || (distance(parameterTypes,
method.getParameterTypes())
- < distance(parameterTypes,
inexactMatch.getParameterTypes()))) {
- inexactMatch = method;
- }
}
+ }
+
+ final TreeMap<Double, List<Method>> candidates = new TreeMap<>();
+ methods.stream()
+ .filter(method -> ClassUtils.isAssignable(parameterTypes,
method.getParameterTypes(), true))
+ .forEach(method -> {
+ final double distance = distance(parameterTypes,
method.getParameterTypes());
+ List<Method> methods1 =
candidates.computeIfAbsent(distance, k -> new ArrayList<>());
+ methods1.add(method);
+ });
+
+ if (candidates.isEmpty()) {
+ return null;
}
- return inexactMatch;
+
+ final List<Method> bestCandidates =
candidates.values().iterator().next();
+ if (bestCandidates.size() == 1) {
+ return bestCandidates.get(0);
+ }
+
+ final String target = methodName +
Arrays.stream(parameterTypes).map(String::valueOf).collect(Collectors.joining(",",
"(", ")"));
+ final String strCandidates =
bestCandidates.stream().map(Method::toString).collect(Collectors.joining("\n
"));
+ throw new IllegalStateException("Found multiple candidates for method
" + target + " on class " + cls + ":\n " + strCandidates);
}
/**
* <p>Returns the aggregate number of inheritance hops between assignable
argument class types. Returns -1
* if the arguments aren't assignable. Fills a specific purpose for
getMatchingMethod and is not generalized.</p>
- * @param classArray
- * @param toClassArray
+ * @param classArray the Class array to calculate the distance from.
Review comment:
Maybe rename to `fromClassArray` to match `toClassArray`?
##########
File path: src/test/java/org/apache/commons/lang3/reflect/MethodUtilsTest.java
##########
@@ -1018,4 +1019,83 @@ public void testDistance() throws Exception {
distanceMethod.setAccessible(false);
}
+
+ @Test
+ public void testGetMatchingMethod() throws NoSuchMethodException {
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod"),
+ GetMatchingMethodClass.class.getMethod("testMethod"));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod", Long.TYPE),
+ GetMatchingMethodClass.class.getMethod("testMethod",
Long.TYPE));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod", Long.class),
+ GetMatchingMethodClass.class.getMethod("testMethod",
Long.class));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod", (Class<?>) null),
+ GetMatchingMethodClass.class.getMethod("testMethod",
Long.class));
+
+ assertThrows(IllegalStateException.class,
+ () ->
MethodUtils.getMatchingMethod(GetMatchingMethodClass.class, "testMethod2",
(Class<?>) null));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod3", Long.TYPE, Long.class),
+ GetMatchingMethodClass.class.getMethod("testMethod3",
Long.TYPE, Long.class));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod3", Long.class, Long.TYPE),
+ GetMatchingMethodClass.class.getMethod("testMethod3",
Long.class, Long.TYPE));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod3", null, Long.TYPE),
+ GetMatchingMethodClass.class.getMethod("testMethod3",
Long.class, Long.TYPE));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod3", Long.TYPE, null),
+ GetMatchingMethodClass.class.getMethod("testMethod3",
Long.TYPE, Long.class));
+
+ assertThrows(IllegalStateException.class,
+ () ->
MethodUtils.getMatchingMethod(GetMatchingMethodClass.class, "testMethod4",
null, null));
+ }
+
+ private static final class GetMatchingMethodClass {
+ public String testMethod() {
+ return "testMethod";
+ }
+
+ public String testMethod(Long aLong) {
Review comment:
Use final where you can, like here for parms.
##########
File path: src/test/java/org/apache/commons/lang3/reflect/MethodUtilsTest.java
##########
@@ -1018,4 +1019,83 @@ public void testDistance() throws Exception {
distanceMethod.setAccessible(false);
}
+
+ @Test
+ public void testGetMatchingMethod() throws NoSuchMethodException {
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod"),
+ GetMatchingMethodClass.class.getMethod("testMethod"));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod", Long.TYPE),
+ GetMatchingMethodClass.class.getMethod("testMethod",
Long.TYPE));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod", Long.class),
+ GetMatchingMethodClass.class.getMethod("testMethod",
Long.class));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod", (Class<?>) null),
+ GetMatchingMethodClass.class.getMethod("testMethod",
Long.class));
+
+ assertThrows(IllegalStateException.class,
+ () ->
MethodUtils.getMatchingMethod(GetMatchingMethodClass.class, "testMethod2",
(Class<?>) null));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod3", Long.TYPE, Long.class),
+ GetMatchingMethodClass.class.getMethod("testMethod3",
Long.TYPE, Long.class));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod3", Long.class, Long.TYPE),
+ GetMatchingMethodClass.class.getMethod("testMethod3",
Long.class, Long.TYPE));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod3", null, Long.TYPE),
+ GetMatchingMethodClass.class.getMethod("testMethod3",
Long.class, Long.TYPE));
+
+
assertEquals(MethodUtils.getMatchingMethod(GetMatchingMethodClass.class,
"testMethod3", Long.TYPE, null),
+ GetMatchingMethodClass.class.getMethod("testMethod3",
Long.TYPE, Long.class));
+
+ assertThrows(IllegalStateException.class,
+ () ->
MethodUtils.getMatchingMethod(GetMatchingMethodClass.class, "testMethod4",
null, null));
+ }
+
+ private static final class GetMatchingMethodClass {
Review comment:
Please make the test fixture as simple as possible: All of these methods
can just return `void`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 527237)
Time Spent: 2h 40m (was: 2.5h)
> MethodUtils.invokeMethod NullPointerException in case of null in args list
> --------------------------------------------------------------------------
>
> Key: LANG-1544
> URL: https://issues.apache.org/jira/browse/LANG-1544
> Project: Commons Lang
> Issue Type: Bug
> Components: lang.reflect.*
> Affects Versions: 3.10
> Reporter: Peter Nagy
> Priority: Critical
> Time Spent: 2h 40m
> Remaining Estimate: 0h
>
> MethodUtils:774
>
> if (classArray[offset].equals(toClassArray[offset])) {
> continue;
> } else if (ClassUtils.isAssignable(classArray[offset], toClassArray[offset],
> true)
>
> cause NPE if classArray[offset] is null. Can you please extend the if
> condition with a null check, like this?
>
> if (classArray[offset] != null &&
> classArray[offset].equals(toClassArray[offset]))
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
