You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "lenamonj (via GitHub)" <gi...@apache.org> on 2026/09/07 20:47:01 UTC
[PR] Fix MethodUtils.invokeMethod on instances of non-public classes [commons-lang]
lenamonj opened a new pull request, #1783:
URL: https://github.com/apache/commons-lang/pull/1783
`getMatchingAccessibleMethod` has an exact-signature fast path, added in #1427, that returns the `Method` declared on the receiver's own class without the accessible-declaration lookup the search path does. When that class is not public, which is every JDK collection factory result (`Collections.emptyList()`, `Arrays.asList()`, `Collections.emptyMap()`), `invokeMethod` throws `IllegalAccessException` although a public declaration exists on the interface.
The fast path now re-resolves the hit through `getAccessibleMethod` and keeps the original only when no public declaration exists, so a package-private class that `setAccessible` can open behaves as before.
Tests: four in `MethodUtilsTest`, red on master (three failures, one error) and green with the change. Default Maven goal green.
- [x] Read the [contribution guidelines](CONTRIBUTING.md) for this project.
- [x] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) if you use Artificial Intelligence (AI).
- [x] I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute? Claude (Anthropic) found the defect and drafted the fix and tests; I reviewed them, reproduced the failure on a fresh clone of master, and ran the default Maven goal.
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [x] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MethodUtils.invokeMethod on instances of non-public classes [commons-lang]
Posted by "lenamonj (via GitHub)" <gi...@apache.org>.
lenamonj commented on PR #1783:
URL: https://github.com/apache/commons-lang/pull/1783#issuecomment-5593812143
Both cases reproduced on the first commit and are fixed in efc8e2eb6: a static candidate is kept as is, since a static method hides rather than overrides, and the interface walk skips static interface methods, which are not inherited. Tests for both use your examples (`child`, `instance`), red on the first commit and green now; the original four still pass. On #1427: the fast path came in with ee09f69 on master, whose subject cites #1427, and #1427 itself was closed unmerged; the body is corrected. Default goal green with -Ddoclint=all; marked ready for review.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MethodUtils.invokeMethod on instances of non-public classes [commons-lang]
Posted by "lenamonj (via GitHub)" <gi...@apache.org>.
lenamonj commented on code in PR #1783:
URL: https://github.com/apache/commons-lang/pull/1783#discussion_r3963339988
##########
src/test/java/org/apache/commons/lang3/reflect/MethodUtilsTest.java:
##########
@@ -605,6 +607,39 @@ void testGetAccessibleInterfaceMethodFromDescription(final Class<?> clazz) {
}
}
+ @ParameterizedTest
+ @ValueSource(classes = {TestMutable.class, TestMutableSubclass.class})
+ void testGetMatchingAccessibleMethodOnNonPublicClass(final Class<?> clazz) {
+ assertSame(Mutable.class, MethodUtils.getMatchingAccessibleMethod(clazz, "getValue").getDeclaringClass());
+ assertSame(Mutable.class,
+ MethodUtils.getMatchingAccessibleMethod(clazz, "setValue", Object.class).getDeclaringClass());
+ }
+
+ @Test
+ void testGetMatchingAccessibleMethodOnNonPublicJdkClass() {
+ assertSame(List.class,
+ MethodUtils.getMatchingAccessibleMethod(Collections.emptyList().getClass(), "size").getDeclaringClass());
+ assertSame(List.class,
+ MethodUtils.getMatchingAccessibleMethod(Arrays.asList(1, 2).getClass(), "size").getDeclaringClass());
Review Comment:
`java.util.List` declares `size()` itself, so the interface walk in `getAccessibleMethodFromInterfaceNest` stops at `List` before it reaches `Collection`, and `Map` declares its own `size()` the same way. The assertions hold on every JDK in the matrix, 8 through 27.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MethodUtils.invokeMethod on instances of non-public classes [commons-lang]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1783:
URL: https://github.com/apache/commons-lang/pull/1783#discussion_r3962902432
##########
src/test/java/org/apache/commons/lang3/reflect/MethodUtilsTest.java:
##########
@@ -605,6 +607,39 @@ void testGetAccessibleInterfaceMethodFromDescription(final Class<?> clazz) {
}
}
+ @ParameterizedTest
+ @ValueSource(classes = {TestMutable.class, TestMutableSubclass.class})
+ void testGetMatchingAccessibleMethodOnNonPublicClass(final Class<?> clazz) {
+ assertSame(Mutable.class, MethodUtils.getMatchingAccessibleMethod(clazz, "getValue").getDeclaringClass());
+ assertSame(Mutable.class,
+ MethodUtils.getMatchingAccessibleMethod(clazz, "setValue", Object.class).getDeclaringClass());
+ }
+
+ @Test
+ void testGetMatchingAccessibleMethodOnNonPublicJdkClass() {
+ assertSame(List.class,
+ MethodUtils.getMatchingAccessibleMethod(Collections.emptyList().getClass(), "size").getDeclaringClass());
+ assertSame(List.class,
+ MethodUtils.getMatchingAccessibleMethod(Arrays.asList(1, 2).getClass(), "size").getDeclaringClass());
Review Comment:
For `size()` on JDK `List` implementations, the public declaration is on `java.util.Collection`, not `java.util.List` (List inherits it). `getAccessibleMethodFromInterfaceNest` will typically resolve to `Collection.size()`, so these assertions are likely brittle / incorrect.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MethodUtils.invokeMethod on instances of non-public classes [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #1783:
URL: https://github.com/apache/commons-lang/pull/1783
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MethodUtils.invokeMethod on instances of non-public classes [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1783:
URL: https://github.com/apache/commons-lang/pull/1783#issuecomment-5593205702
I think this PR creates 2 bugs so it looks like we are missing some tests since the build was green, so please add these missing tests and update the main code:
- At the new `getAccessibleMethod(cls, candidate)` call, a package-private subclass’s public static method can be replaced by a same-signature method on its public superclass. Static methods do not dispatch to subclass implementations.
With `Child.who()` returning `"child"` and `Parent.who()` returning `"parent"`:
`MethodUtils.invokeStaticMethod(Child.class, "who")`
Before PR: `"child"`
After PR: `"parent"`
So you should preserve the existing exact-match behavior for static candidates.
- Reject static interface methods as replacements for instance methods.
The interface lookup uses `getDeclaredMethod()` without excluding static methods. A public interface can declare `static String label()`, while its package-private implementing class independently declares an instance `String label()`.
`MethodUtils.invokeMethod(instance, "label")`
Before PR: `"instance"`
After PR: `"interface-static"`
Reflection ignores the receiver when invoking that static method. Skipping resolution for static candidates alone does not fix this case: replacement declarations must also be checked.
Note: Your AI is imagining things when it talks about PR https://github.com/apache/commons-lang/pull/1427 because that PR was closed without being merged.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MethodUtils.invokeMethod on instances of non-public classes [commons-lang]
Posted by "lenamonj (via GitHub)" <gi...@apache.org>.
lenamonj commented on PR #1783:
URL: https://github.com/apache/commons-lang/pull/1783#issuecomment-5594908129
Pushed the private interface method case. getAccessibleMethodFromInterfaceNest now returns a declared interface method only when it is public and not static, so a private one falls through to the class's own public method. Without that change the new test errors with IllegalAccessException on your example, and it passes with it.
The fixture pair is compiled with javac release 9 and checked in under src/test/resources next to its sources, since test sources build at 1.8. The test assumes Java 9 or later, so the Java 8 jobs skip it. Default goal green locally with -Ddoclint=all. The workflow runs on this head are action_required.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MethodUtils.invokeMethod on instances of non-public classes [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1783:
URL: https://github.com/apache/commons-lang/pull/1783#issuecomment-5594307955
Here's another wrinkle: On Java 9 and up, interfaces can contain private instance methods, which are not inherited and cannot serve as accessible replacements. So you need to exclude private interface methods as well as static methods.
Since our code is Java 8-based, the only way to test this is to add a precompiled Java code to the test resource folder.
For example:
```java
public interface Labels {
private String label() { return "helper"; }
}
static class Bean implements Labels {
public String label() { return "bean"; }
}
```
For `MethodUtils.invokeMethod(new Bean(), "label")`:
- The original code: returns "bean".
- The PE selects the `Labels` private method and throws `IllegalAccessException`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MethodUtils.invokeMethod on instances of non-public classes [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1783:
URL: https://github.com/apache/commons-lang/pull/1783#issuecomment-5597138054
Thank you @lenamonj , merged 🚀
--
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org