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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
