You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "zemiles (via GitHub)" <gi...@apache.org> on 2026/10/04 04:23:25 UTC
[PR] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
zemiles opened a new pull request, #1810:
URL: https://github.com/apache/commons-lang/pull/1810
Fixes [LANG-1835](https://issues.apache.org/jira/browse/LANG-1835).
AnnotationUtils can invoke compiler-generated methods in annotation types, causing lambda bodies to execute, exceptions to be thrown, or incorrect hash codes and string representations.
Restrict equals(), hashCode(), and toString() to public, abstract, non-synthetic annotation members.
Add six regression tests for annotations containing Runnable, Supplier, and Function constants, including equality between distinct annotation instances without executing lambda bodies. Also use package-private visibility for the existing JUnit 5 setup method.
Validation:
- All six new regression tests fail without the fix.
- All 16 AnnotationUtilsTest tests pass with the fix.
- The default Maven build passes on JDK 25: 95,978 tests, zero failures or errors, and 17 skipped.
- Coverage, license, Checkstyle, API compatibility, SpotBugs, PMD, and Javadoc checks pass.
- AnnotationUtilsTest and Checkstyle were rerun successfully after the setup visibility adjustment.
Checklist:
- [ ] Read the [contribution guidelines](https://github.com/apache/commons-lang/blob/master/CONTRIBUTING.md).
- [ ] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html).
- [x] I used AI to create part of this pull request. OpenAI Codex generated the implementation and regression tests, and helped with validation and the PR description.
- [x] Run a successful build using the default Maven goal.
- [x] Write regression tests that fail without the runtime fix.
- [x] Write a description explaining the change.
- [x] Use a meaningful commit subject and body.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#discussion_r4218916356
##########
src/main/java/org/apache/commons/lang3/AnnotationUtils.java:
##########
@@ -280,6 +284,17 @@ private static int hashMember(final String name, final Object value) {
return part1 ^ value.hashCode();
}
+ /**
+ * Tests whether the specified method declares an annotation member.
+ *
+ * @param method The method to check
+ * @return whether the method is public, abstract and non-synthetic
+ */
+ private static boolean isAnnotationMember(final Method method) {
+ final int modifiers = method.getModifiers();
+ return Modifier.isPublic(modifiers) && Modifier.isAbstract(modifiers) && !method.isSynthetic();
Review Comment:
`isAnnotationMember` is described (and named) as determining whether a method “declares an annotation member”, but it currently only checks modifiers and synthetic-ness. Annotation members are also defined by having zero parameters (and, in practice for this class, by having a valid annotation member return type). Consider folding `method.getParameterCount() == 0` (and optionally `isValidAnnotationMemberType(method.getReturnType())`) into `isAnnotationMember`, then simplifying the call sites to avoid duplicated/partial filtering logic.
##########
src/main/java/org/apache/commons/lang3/AnnotationUtils.java:
##########
@@ -217,7 +218,7 @@ public static boolean equals(final Annotation a1, final Annotation a2) {
}
try {
for (final Method m : type1.getDeclaredMethods()) {
- if (m.getParameterTypes().length == 0
+ if (isAnnotationMember(m) && m.getParameterTypes().length == 0
&& isValidAnnotationMemberType(m.getReturnType())) {
AbstractReflection.setAccessible(AbstractReflection.getForceAccessible(), m);
Review Comment:
Now that `equals()` restricts invoked methods to `public` + `abstract` members, the unconditional `setAccessible(...)` call is no longer needed for access control and can increase the chance of reflective-access problems under stronger encapsulation (and can add overhead). Consider removing the call in this path, or only calling it when `!m.canAccess(a1)`/when access is actually 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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "zemiles (via GitHub)" <gi...@apache.org>.
zemiles commented on PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#issuecomment-6062475408
Thanks for the suggestion. Although annotation member methods are public, their declaring annotation type can be package-private. The accessibility override is still needed in that case.
Removing this call breaks two existing regression tests in AnnotationEqualsTest due to IllegalAccessException on Java 8, 11, 17, 21, 25, and 27.
Restoring the call while keeping the consolidated member checks passes the full Maven build on Java 21. I therefore propose retaining this call to preserve support for non-public annotation types.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#discussion_r4230388843
##########
src/test/java/org/apache/commons/lang3/AnnotationUtilsTest.java:
##########
@@ -470,6 +526,24 @@ void testHashCode() {
});
}
+ @Test
+ void testHashCodeWithFunction() {
+ final FunctionAnnotation annotation = AnnotatedClass.class.getAnnotation(FunctionAnnotation.class);
+ assertEquals(annotation.hashCode(), AnnotationUtils.hashCode(annotation));
+ }
+
+ @Test
+ void testHashCodeWithRunnable() {
+ final RunnableAnnotation annotation = AnnotatedClass.class.getAnnotation(RunnableAnnotation.class);
+ assertEquals(annotation.hashCode(), AnnotationUtils.hashCode(annotation));
+ }
+
+ @Test
+ void testHashCodeWithSupplier() {
+ final SupplierAnnotation annotation = AnnotatedClass.class.getAnnotation(SupplierAnnotation.class);
+ assertEquals(annotation.hashCode(), AnnotationUtils.hashCode(annotation));
+ }
Review Comment:
The regression tests for `hashCode()` don’t currently prove that synthetic lambda-body methods are *not* invoked, because the lambdas used here are non-throwing. To make this a true regression guard for LANG-1835, add a `hashCode` test using `ThrowingSupplierAnnotation` (or a throwing `Runnable`) and assert `AnnotationUtils.hashCode(annotation)` completes without triggering the `AssertionError`.
##########
src/test/java/org/apache/commons/lang3/AnnotationUtilsTest.java:
##########
@@ -519,4 +593,16 @@ void testToString() {
});
}
+ @Test
+ void testToStringWithRunnable() {
+ final RunnableAnnotation annotation = AnnotatedClass.class.getAnnotation(RunnableAnnotation.class);
+ assertEquals("@" + RunnableAnnotation.class.getName() + "(value=member)", AnnotationUtils.toString(annotation));
+ }
+
+ @Test
+ void testToStringWithSupplier() {
+ final SupplierAnnotation annotation = AnnotatedClass.class.getAnnotation(SupplierAnnotation.class);
+ assertEquals("@" + SupplierAnnotation.class.getName() + "(value=member)", AnnotationUtils.toString(annotation));
+ }
+
Review Comment:
Similar to `hashCode`, the `toString()` regressions here don’t validate the “don’t execute lambda bodies” requirement since the lambdas are harmless. Consider adding a `toString` test using `ThrowingSupplierAnnotation` and assert `AnnotationUtils.toString(annotation)` returns the expected value string without throwing, which would directly catch a regression where synthetic methods are invoked.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#issuecomment-6046055312
@zemiles
The build is broken. You didn't test your code. Setting to draft.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#discussion_r4229628552
##########
src/test/java/org/apache/commons/lang3/AnnotationUtilsTest.java:
##########
@@ -421,6 +467,15 @@ void testEquivalence() {
assertTrue(AnnotationUtils.equals(field2.getAnnotation(TestAnnotation.class), field1.getAnnotation(TestAnnotation.class)));
}
+ @Test
+ void testEquivalenceWithoutInvokingSupplier() {
+ final ThrowingSupplierAnnotation first = AnnotatedClass.class.getAnnotation(ThrowingSupplierAnnotation.class);
+ final ThrowingSupplierAnnotation second = EquivalentAnnotatedClass.class.getAnnotation(ThrowingSupplierAnnotation.class);
+
+ assertNotSame(first, second);
+ assertTrue(AnnotationUtils.equals(first, second));
Review Comment:
`assertNotSame(first, second)` makes the test depend on annotation instance identity, which is not something the Java reflection/annotation APIs guarantee across different annotated elements. This assertion is not required to validate the regression (synthetic methods not being invoked) and could make the test flaky on JVMs that aggressively cache/reuse annotation proxies. Suggest removing the identity assertion and keeping the behavioral equality assertion.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#issuecomment-6081528856
@zemiles
I committed a Copilot suggestion, please pull to your local branch and review Copilot's other comments.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "zemiles (via GitHub)" <gi...@apache.org>.
zemiles commented on code in PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#discussion_r4237772532
##########
src/main/java/org/apache/commons/lang3/AnnotationUtils.java:
##########
@@ -217,7 +218,7 @@ public static boolean equals(final Annotation a1, final Annotation a2) {
}
try {
for (final Method m : type1.getDeclaredMethods()) {
- if (m.getParameterTypes().length == 0
+ if (isAnnotationMember(m) && m.getParameterTypes().length == 0
&& isValidAnnotationMemberType(m.getReturnType())) {
AbstractReflection.setAccessible(AbstractReflection.getForceAccessible(), m);
Review Comment:
Thanks for the suggestion. I updated equals(), hashCode(), and toString() to try normal invocation first, adjusting accessibility and retrying only after IllegalAccessException when permitted by the existing forceAccessible setting.
This preserves support for non-public annotation types and Java 8 compatibility without using canAccess(), which was introduced in Java 9.
All 22 related tests pass on Java 8, 11, 17, 21, 25, and 27. The full default Maven build also passes on Java 21, with no Checkstyle violations.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "zemiles (via GitHub)" <gi...@apache.org>.
zemiles commented on PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#issuecomment-6082008865
Thanks for adding the toString() regression test. I've pulled your commit and added the corresponding hashCode() test using ThrowingSupplierAnnotation.
All 22 related tests pass on Java 21, and Checkstyle reports no violations. I also confirmed that the new hashCode() regression test fails against the original implementation and passes with the fix.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "zemiles (via GitHub)" <gi...@apache.org>.
zemiles commented on code in PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#discussion_r4230708141
##########
src/test/java/org/apache/commons/lang3/AnnotationUtilsTest.java:
##########
@@ -470,6 +526,24 @@ void testHashCode() {
});
}
+ @Test
+ void testHashCodeWithFunction() {
+ final FunctionAnnotation annotation = AnnotatedClass.class.getAnnotation(FunctionAnnotation.class);
+ assertEquals(annotation.hashCode(), AnnotationUtils.hashCode(annotation));
+ }
+
+ @Test
+ void testHashCodeWithRunnable() {
+ final RunnableAnnotation annotation = AnnotatedClass.class.getAnnotation(RunnableAnnotation.class);
+ assertEquals(annotation.hashCode(), AnnotationUtils.hashCode(annotation));
+ }
+
+ @Test
+ void testHashCodeWithSupplier() {
+ final SupplierAnnotation annotation = AnnotatedClass.class.getAnnotation(SupplierAnnotation.class);
+ assertEquals(annotation.hashCode(), AnnotationUtils.hashCode(annotation));
+ }
Review Comment:
Added testHashCodeWithoutInvokingSupplier() using ThrowingSupplierAnnotation. The test fails against the original implementation and passes with the fix.
All 22 related tests pass on Java 21, and Checkstyle reports no violations.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "zemiles (via GitHub)" <gi...@apache.org>.
zemiles commented on code in PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#discussion_r4230704354
##########
src/test/java/org/apache/commons/lang3/AnnotationUtilsTest.java:
##########
@@ -519,4 +593,16 @@ void testToString() {
});
}
+ @Test
+ void testToStringWithRunnable() {
+ final RunnableAnnotation annotation = AnnotatedClass.class.getAnnotation(RunnableAnnotation.class);
+ assertEquals("@" + RunnableAnnotation.class.getName() + "(value=member)", AnnotationUtils.toString(annotation));
+ }
+
+ @Test
+ void testToStringWithSupplier() {
+ final SupplierAnnotation annotation = AnnotatedClass.class.getAnnotation(SupplierAnnotation.class);
+ assertEquals("@" + SupplierAnnotation.class.getName() + "(value=member)", AnnotationUtils.toString(annotation));
+ }
+
Review Comment:
Addressed by @garydgregory in 5dcd1d2. I've pulled the commit and verified that the test passes on Java 21.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "zemiles (via GitHub)" <gi...@apache.org>.
zemiles commented on PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#issuecomment-6049551729
Thanks for catching this, and sorry for missing validation on older JDKs.
The failing assertion called the annotation proxy's native equals(), causing AnnotationFormatError on Java 8 and 11 before reaching AnnotationUtils.equals(). I've removed that assertion while retaining the distinct-instance check and the regression assertion.
The default Maven build with -Ddoclint=all now passes locally on Windows with Java 8, 11, 17, 21, 25, and 27. I also confirmed that the retained regression test still fails against the original implementation.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "zemiles (via GitHub)" <gi...@apache.org>.
zemiles commented on PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#issuecomment-6103975815
Thank you for the thorough review and for merging this PR! I appreciate your guidance and help with improving the regression tests.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#issuecomment-6100374232
Thank you @zemiles , 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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "zemiles (via GitHub)" <gi...@apache.org>.
zemiles commented on code in PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#discussion_r4229945321
##########
src/test/java/org/apache/commons/lang3/AnnotationUtilsTest.java:
##########
@@ -421,6 +467,15 @@ void testEquivalence() {
assertTrue(AnnotationUtils.equals(field2.getAnnotation(TestAnnotation.class), field1.getAnnotation(TestAnnotation.class)));
}
+ @Test
+ void testEquivalenceWithoutInvokingSupplier() {
+ final ThrowingSupplierAnnotation first = AnnotatedClass.class.getAnnotation(ThrowingSupplierAnnotation.class);
+ final ThrowingSupplierAnnotation second = EquivalentAnnotatedClass.class.getAnnotation(ThrowingSupplierAnnotation.class);
+
+ assertNotSame(first, second);
+ assertTrue(AnnotationUtils.equals(first, second));
Review Comment:
Thanks for the suggestion. I replaced the second annotation with a freshly created proxy, avoiding reliance on JVM annotation caching. I retained assertNotSame to ensure the test exercises member comparison rather than the identity shortcut.
The 20 related tests pass on Java 8, 11, 17, 21, 25, and 27. The full default Maven build also passes on Java 21.
I confirmed that the updated regression test still fails against the original implementation and passes with the fix.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#issuecomment-6060041054
Hello @zemiles Please review Copilot again.
--
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] [LANG-1835] Ignore synthetic methods in AnnotationUtils [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1810:
URL: https://github.com/apache/commons-lang/pull/1810#issuecomment-6097490359
@zemiles
Thank you for the update. Please review the new Copilot comment.
--
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