Copilot commented on code in PR #1760:
URL: https://github.com/apache/commons-lang/pull/1760#discussion_r3679130542
##########
src/main/java/org/apache/commons/lang3/AnnotationUtils.java:
##########
@@ -212,6 +213,7 @@ public static boolean equals(final Annotation a1, final
Annotation a2) {
for (final Method m : type1.getDeclaredMethods()) {
if (m.getParameterTypes().length == 0
&& isValidAnnotationMemberType(m.getReturnType())) {
+
AbstractReflection.setAccessible(AbstractReflection.getForceAccessible(), m);
final Object v1 = m.invoke(a1);
final Object v2 = m.invoke(a2);
Review Comment:
equals(...) now forces accessibility on annotation member methods to handle
package-private annotation types, but hashCode(...) and toString(...) in this
same class still invoke declared methods without ensuring accessibility. That
means the same package-private annotation scenario can still fail
(IllegalAccessException) when computing hashCode/toString, which is
inconsistent with the updated equals behavior.
This issue also appears on line 224 of the same file.
##########
src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.lang3.external;
+
+import org.apache.commons.lang3.AnnotationUtils;
+import org.junit.jupiter.api.Test;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.reflect.InvocationTargetException;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
Review Comment:
Import groups appear to follow the project convention of: static imports
first, then java.* imports, then org.* imports (with blank lines between
groups). This file currently has org.* before java.* and static imports last,
which may violate the style/checkstyle rules.
##########
src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java:
##########
@@ -128,33 +128,33 @@ static void register(final Object lhs, final Object rhs,
final Set<Pair<IDKey, I
* AccessibleObject#setAccessible(true)} but <em>only</em> if a field is
not already accessible.
*
* @param forceAccessible Whether to call {@link
AccessibleObject#setAccessible(boolean)} if a field is not already accessible.
Review Comment:
AbstractReflection.setAccessible(...) is now public and can be called with
null; the current implementation will throw a NullPointerException when
accessibleObject is null. Also, the Javadoc still refers to a "field" and
describes the return value as "field is accessible" even though the method
returns false when already accessible.
This issue also appears on line 141 of the same file.
--
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]
