This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-lang.git
The following commit(s) were added to refs/heads/master by this push:
new 7e5906fb0 [LANG-1815] Fix AnnotationUtils.equals for package-private
annotations. (#1760)
7e5906fb0 is described below
commit 7e5906fb0cb679d630527fc1bc18018f935518dc
Author: Maksym Korshun <[email protected]>
AuthorDate: Thu Jul 30 14:04:27 2026 +0200
[LANG-1815] Fix AnnotationUtils.equals for package-private annotations.
(#1760)
* [LANG-1815] Fix AnnotationUtils.equals for package-private annotations.
* style: fix to follow style convention
* fix: AbstractReflection.setAccessible handle null safely, ensure
accessability for AnnotationUtils hashCode and toString
In addition, improve code coverage and update docs
* Add @since tag to getForceAccessible method
---------
Co-authored-by: Gary Gregory <[email protected]>
---
.../org/apache/commons/lang3/AnnotationUtils.java | 6 +-
.../commons/lang3/builder/AbstractReflection.java | 54 ++++++-----
.../AbstractReflectionSetAccessibleTest.java | 60 ++++++++++++
.../lang3/external/AnnotationEqualsTest.java | 105 +++++++++++++++++++++
4 files changed, 201 insertions(+), 24 deletions(-)
diff --git a/src/main/java/org/apache/commons/lang3/AnnotationUtils.java
b/src/main/java/org/apache/commons/lang3/AnnotationUtils.java
index 3b21b0f64..bdaf76433 100644
--- a/src/main/java/org/apache/commons/lang3/AnnotationUtils.java
+++ b/src/main/java/org/apache/commons/lang3/AnnotationUtils.java
@@ -20,6 +20,7 @@
import java.lang.reflect.Method;
import java.util.Arrays;
+import org.apache.commons.lang3.builder.AbstractReflection;
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.apache.commons.lang3.builder.ToStringStyle;
import org.apache.commons.lang3.exception.UncheckedException;
@@ -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);
if (!memberEquals(m.getReturnType(), v1, v2)) {
@@ -220,7 +222,7 @@ && isValidAnnotationMemberType(m.getReturnType())) {
}
}
} catch (final ReflectiveOperationException ex) {
- return false;
+ throw new IllegalStateException(ex);
}
return true;
}
@@ -242,6 +244,7 @@ public static int hashCode(final Annotation a) {
final Class<? extends Annotation> type = a.annotationType();
for (final Method m : type.getDeclaredMethods()) {
try {
+
AbstractReflection.setAccessible(AbstractReflection.getForceAccessible(), m);
final Object value = m.invoke(a);
if (value == null) {
throw new IllegalStateException(String.format("Annotation
method %s returned null", m));
@@ -336,6 +339,7 @@ public static String toString(final Annotation a) {
continue; // what?
}
try {
+
AbstractReflection.setAccessible(AbstractReflection.getForceAccessible(), m);
builder.append(m.getName(), m.invoke(a));
} catch (final ReflectiveOperationException ex) {
throw new UncheckedException(ex);
diff --git
a/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java
b/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java
index 49ce0a96c..e422d56bf 100644
--- a/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java
+++ b/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java
@@ -108,8 +108,9 @@ public B setForceAccessible(final boolean forceAccessible) {
*
* @return whether the system property {@code
"AbstractReflection.forceAccessible"} is set to true with true as the default.
* @see Boolean#parseBoolean(String)
+ * @since 3.21.0
*/
- static boolean getForceAccessible() {
+ public static boolean getForceAccessible() {
return SystemProperties.getBoolean(AbstractReflection.class,
"forceAccessible", () -> true);
}
@@ -124,37 +125,44 @@ static void register(final Object lhs, final Object rhs,
final Set<Pair<IDKey, I
}
/**
- * If {@code forceAccessible} flag is true, then the field is made
accessible by calling {@link AccessibleObject#setAccessible(boolean)
- * AccessibleObject#setAccessible(true)} but <em>only</em> if a field is
not already accessible.
+ * If {@code forceAccessible} is true, makes {@code accessibleObject}
accessible by calling
+ * {@link AccessibleObject#setAccessible(boolean)
AccessibleObject#setAccessible(true)} but <em>only</em> if it is not already
accessible.
*
- * @param forceAccessible Whether to call {@link
AccessibleObject#setAccessible(boolean)} if a field is not already accessible.
- * @param field The field to set.
- * @return true if the field is accessible, false otherwise.
- * @throws SecurityException Thrown if {@code forceAccessible} flag is
true and the request is denied.
+ * @param forceAccessible Whether to call {@link
AccessibleObject#setAccessible(boolean)} if the object is not already
accessible.
+ * @param accessibleObject The accessible object to set; may be {@code
null}.
+ * @return {@code true} if {@code accessibleObject} is non-null and
accessible after this call; {@code false} otherwise
+ * (including when {@code accessibleObject} is {@code null}, or
when it is inaccessible and {@code forceAccessible} is {@code false}).
+ * @throws SecurityException Thrown if {@code forceAccessible} is true and
the request is denied.
* @see AccessibleObject#setAccessible(boolean)
* @see SecurityManager#checkPermission
*/
- static boolean setAccessible(final boolean forceAccessible, final Field
field) {
- return !field.isAccessible() && forceAccessible &&
setAccessibleTrue(field);
+ public static boolean setAccessible(final boolean forceAccessible, final
AccessibleObject accessibleObject) {
+ if (accessibleObject == null) {
+ return false;
+ }
+ if (accessibleObject.isAccessible()) {
+ return true;
+ }
+ return forceAccessible && setAccessibleTrue(accessibleObject);
}
/**
- * Sets the field as accessible by calling {@link
AccessibleObject#setAccessible(boolean) AccessibleObject#setAccessible(true)}
but <em>only</em> if a field
- * is not already accessible.
+ * Sets the accessible object as accessible by calling {@link
AccessibleObject#setAccessible(boolean) AccessibleObject#setAccessible(true)}
but
+ * <em>only</em> if it is not already accessible.
*
- * @param field The field to set, may be null.
- * @return true if the field is accessible, false otherwise.
- * @throws SecurityException Thrown if {@code forceAccessible} flag is
true and the request is denied.
+ * @param accessibleObject The accessible object to set, may be {@code
null}.
+ * @return {@code true} if {@code accessibleObject} is non-null and
accessible after this call; {@code false} otherwise.
+ * @throws SecurityException Thrown if the request is denied.
* @see AccessibleObject#setAccessible(boolean)
* @see SecurityManager#checkPermission
*/
- private static boolean setAccessibleTrue(final Field field) {
- if (field != null) {
+ private static boolean setAccessibleTrue(final AccessibleObject
accessibleObject) {
+ if (accessibleObject != null) {
// Test isAccessible() to avoid the permission check.
- if (!field.isAccessible()) {
- field.setAccessible(true);
+ if (!accessibleObject.isAccessible()) {
+ accessibleObject.setAccessible(true);
}
- return field.isAccessible();
+ return accessibleObject.isAccessible();
}
return false;
}
@@ -202,12 +210,12 @@ protected boolean isForceAccessible() {
}
/**
- * If {@code forceAccessible} flag is true, each field in the given array
is made accessible by calling {@link AccessibleObject#setAccessible(boolean)
- * AccessibleObject#setAccessible(true)} but <em>only</em> if a field is
not already accessible.
+ * If {@code forceAccessible} flag is true, the field is made accessible
by calling {@link AccessibleObject#setAccessible(boolean)
+ * AccessibleObject#setAccessible(true)} but <em>only</em> if the field is
not already accessible.
*
- * @param field The fields to set.
+ * @param field The field to set; may be {@code null}.
+ * @return {@code true} if {@code field} is non-null and accessible after
this call; {@code false} otherwise.
* @throws SecurityException Thrown if {@code forceAccessible} flag is
true and the request is denied.
- * @return true if the field is accessible, false otherwise.
* @see AccessibleObject#setAccessible(boolean)
* @see SecurityManager#checkPermission
*/
diff --git
a/src/test/java/org/apache/commons/lang3/builder/AbstractReflectionSetAccessibleTest.java
b/src/test/java/org/apache/commons/lang3/builder/AbstractReflectionSetAccessibleTest.java
new file mode 100644
index 000000000..b6a379100
--- /dev/null
+++
b/src/test/java/org/apache/commons/lang3/builder/AbstractReflectionSetAccessibleTest.java
@@ -0,0 +1,60 @@
+/*
+ * 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.builder;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.lang.reflect.Field;
+
+import org.apache.commons.lang3.AbstractLangTest;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests {@link AbstractReflection#setAccessible(boolean,
java.lang.reflect.AccessibleObject)}.
+ */
+class AbstractReflectionSetAccessibleTest extends AbstractLangTest {
+
+ @SuppressWarnings("unused")
+ private final String privateField = "value";
+
+ @Test
+ void setAccessibleHandlesNull() {
+ assertFalse(AbstractReflection.setAccessible(true, null));
+ assertFalse(AbstractReflection.setAccessible(false, null));
+ }
+
+ @Test
+ void setAccessibleReturnsTrueWhenAlreadyAccessible() throws Exception {
+ final Field field = getClass().getDeclaredField("privateField");
+ field.setAccessible(true);
+ assertTrue(field.isAccessible());
+ assertTrue(AbstractReflection.setAccessible(false, field));
+ assertTrue(AbstractReflection.setAccessible(true, field));
+ }
+
+ @Test
+ void setAccessibleRespectsForceFlag() throws Exception {
+ final Field field = getClass().getDeclaredField("privateField");
+ field.setAccessible(false);
+ assertFalse(field.isAccessible());
+ assertFalse(AbstractReflection.setAccessible(false, field));
+ assertFalse(field.isAccessible());
+ assertTrue(AbstractReflection.setAccessible(true, field));
+ assertTrue(field.isAccessible());
+ }
+}
diff --git
a/src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java
b/src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java
new file mode 100644
index 000000000..9f215e6ef
--- /dev/null
+++ b/src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java
@@ -0,0 +1,105 @@
+/*
+ * 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 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;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.reflect.InvocationTargetException;
+
+import org.apache.commons.lang3.AnnotationUtils;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Regression test for <a
href="https://issues.apache.org/jira/browse/LANG-1815";>LANG-1815</a>.
+ * <p>
+ * Verifies that {@link AnnotationUtils} can reflectively read package-private
annotation members
+ * for {@code equals}, {@code hashCode}, and {@code toString}, and that
reflective invocation failures
+ * are not treated as inequality.
+ * </p>
+ *
+ * <h2>Important</h2>
+ * <p>
+ * This test relies on reflective access rules that differ depending on the
caller's package.
+ * To reproduce the original bug, this class <strong>must remain
outside</strong> the
+ * {@code org.apache.commons.lang3} package.
+ * </p>
+ * <p>
+ * Do <strong>not</strong> move this class into {@code
org.apache.commons.lang3},
+ * otherwise the test may no longer exercise the failing scenario from
LANG-1815.
+ * </p>
+ */
+public class AnnotationEqualsTest {
+ @Retention(RetentionPolicy.RUNTIME)
+ @interface Tag {
+ String value();
+ }
+
+ static class ThrowingTag implements Tag {
+ @Override
+ public String value() {
+ throw new IllegalArgumentException("boom");
+ }
+
+ @Override
+ public Class<? extends Annotation> annotationType() {
+ return Tag.class;
+ }
+ }
+
+ @Tag("value")
+ private final Object a = new Object();
+ @Tag("value")
+ private final Object b = new Object();
+
+ @Test
+ void equalsWorksOnPackagePrivateAnnotations() throws Exception {
+ final Tag tagA =
getClass().getDeclaredField("a").getAnnotation(Tag.class);
+ final Tag tagB =
getClass().getDeclaredField("b").getAnnotation(Tag.class);
+ assertTrue(AnnotationUtils.equals(tagA, tagB));
+ }
+
+ @Test
+ void equalsWrapsReflectiveOperationException() throws Exception {
+ final Tag tagA = new ThrowingTag();
+ final Tag tagB =
getClass().getDeclaredField("b").getAnnotation(Tag.class);
+
+ final IllegalStateException ex =
+ assertThrows(IllegalStateException.class, () ->
AnnotationUtils.equals(tagA, tagB));
+ assertInstanceOf(InvocationTargetException.class, ex.getCause());
+ assertEquals("boom", ((InvocationTargetException)
ex.getCause()).getTargetException().getMessage());
+ }
+
+ @Test
+ void hashCodeWorksOnPackagePrivateAnnotations() throws Exception {
+ final Tag tag =
getClass().getDeclaredField("a").getAnnotation(Tag.class);
+ assertEquals(tag.hashCode(), AnnotationUtils.hashCode(tag));
+ }
+
+ @Test
+ void toStringWorksOnPackagePrivateAnnotations() throws Exception {
+ final Tag tag =
getClass().getDeclaredField("a").getAnnotation(Tag.class);
+ final String text = AnnotationUtils.toString(tag);
+ assertTrue(text.contains("value=value"), text);
+ }
+
+}
