From 68c6cc5dc08ecd8f20eeff5c304b02f4f9453e31 Mon Sep 17 00:00:00 2001 From: Maksym Korshun Date: Thu, 30 Jul 2026 02:43:52 +0200 Subject: [PATCH 1/4] [LANG-1815] Fix AnnotationUtils.equals for package-private annotations. --- .../apache/commons/lang3/AnnotationUtils.java | 4 +- .../lang3/builder/AbstractReflection.java | 20 ++-- .../lang3/external/AnnotationEqualsTest.java | 91 +++++++++++++++++++ 3 files changed, 104 insertions(+), 11 deletions(-) create mode 100644 src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java diff --git a/src/main/java/org/apache/commons/lang3/AnnotationUtils.java b/src/main/java/org/apache/commons/lang3/AnnotationUtils.java index 3b21b0f6458..8996444fdce 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; } 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 49ce0a96ce1..19c7f6196e3 100644 --- a/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java +++ b/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java @@ -109,7 +109,7 @@ 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) */ - static boolean getForceAccessible() { + public static boolean getForceAccessible() { return SystemProperties.getBoolean(AbstractReflection.class, "forceAccessible", () -> true); } @@ -128,33 +128,33 @@ static void register(final Object lhs, final Object rhs, final Setonly if a field 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. + * @param accessibleObject The accessibleObject 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. * @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) { + return !accessibleObject.isAccessible() && forceAccessible && setAccessibleTrue(accessibleObject); } /** * Sets the field as accessible by calling {@link AccessibleObject#setAccessible(boolean) AccessibleObject#setAccessible(true)} but only if a field * is not already accessible. * - * @param field The field to set, may be null. + * @param accessibleObject The accessibleObject 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. * @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; } 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 00000000000..048dffdd24c --- /dev/null +++ b/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; + +/** + * Regression test for LANG-1815. + *

+ * Verifies that {@code AnnotationUtils.equals(Annotation, Annotation)} treats two equal + * package-private annotations as equal, and also wraps a possible ReflectiveOperationException. + *

+ * + *

Important

+ *

+ * This test relies on reflective access rules that differ depending on the caller's package. + * To reproduce the original bug, this class must remain outside the + * {@code org.apache.commons.lang3} package. + *

+ *

+ * Do not move this class into {@code org.apache.commons.lang3}, + * otherwise the test may no longer exercise the failing scenario from LANG-1815. + *

+ */ +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 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 { + Tag tagA = getClass().getDeclaredField("a").getAnnotation(Tag.class); + 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()); + } + +} \ No newline at end of file From b2947ab6d8a6abd449dd1b9f70894b8f8fdee8b0 Mon Sep 17 00:00:00 2001 From: Maksym Korshun Date: Thu, 30 Jul 2026 03:04:27 +0200 Subject: [PATCH 2/4] style: fix to follow style convention --- .../lang3/external/AnnotationEqualsTest.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java b/src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java index 048dffdd24c..cf1b5d20bb9 100644 --- a/src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java +++ b/src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java @@ -16,18 +16,18 @@ */ package org.apache.commons.lang3.external; -import org.apache.commons.lang3.AnnotationUtils; -import org.junit.jupiter.api.Test; +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 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 org.apache.commons.lang3.AnnotationUtils; +import org.junit.jupiter.api.Test; /** * Regression test for LANG-1815. @@ -72,8 +72,8 @@ public Class annotationType() { @Test void equalsWorksOnPackagePrivateAnnotations() throws Exception { - Tag tagA = getClass().getDeclaredField("a").getAnnotation(Tag.class); - Tag tagB = getClass().getDeclaredField("b").getAnnotation(Tag.class); + final Tag tagA = getClass().getDeclaredField("a").getAnnotation(Tag.class); + final Tag tagB = getClass().getDeclaredField("b").getAnnotation(Tag.class); assertTrue(AnnotationUtils.equals(tagA, tagB)); } @@ -88,4 +88,4 @@ void equalsWrapsReflectiveOperationException() throws Exception { assertEquals("boom", ((InvocationTargetException) ex.getCause()).getTargetException().getMessage()); } -} \ No newline at end of file +} From d046adaac4227bea86ddaa3594caa8b67965fbe9 Mon Sep 17 00:00:00 2001 From: Maksym Korshun Date: Thu, 30 Jul 2026 03:21:34 +0200 Subject: [PATCH 3/4] fix: AbstractReflection.setAccessible handle null safely, ensure accessability for AnnotationUtils hashCode and toString In addition, improve code coverage and update docs --- .../apache/commons/lang3/AnnotationUtils.java | 2 + .../lang3/builder/AbstractReflection.java | 39 +++++++----- .../AbstractReflectionSetAccessibleTest.java | 60 +++++++++++++++++++ .../lang3/external/AnnotationEqualsTest.java | 18 +++++- 4 files changed, 101 insertions(+), 18 deletions(-) create mode 100644 src/test/java/org/apache/commons/lang3/builder/AbstractReflectionSetAccessibleTest.java diff --git a/src/main/java/org/apache/commons/lang3/AnnotationUtils.java b/src/main/java/org/apache/commons/lang3/AnnotationUtils.java index 8996444fdce..bdaf76433ca 100644 --- a/src/main/java/org/apache/commons/lang3/AnnotationUtils.java +++ b/src/main/java/org/apache/commons/lang3/AnnotationUtils.java @@ -244,6 +244,7 @@ public static int hashCode(final Annotation a) { final Class 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)); @@ -338,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 19c7f6196e3..fbe224cf266 100644 --- a/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java +++ b/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java @@ -124,27 +124,34 @@ static void register(final Object lhs, final Object rhs, final Setonly 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 only if it is not already accessible. * - * @param forceAccessible Whether to call {@link AccessibleObject#setAccessible(boolean)} if a field is not already accessible. - * @param accessibleObject The accessibleObject 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 */ public static boolean setAccessible(final boolean forceAccessible, final AccessibleObject accessibleObject) { - return !accessibleObject.isAccessible() && forceAccessible && setAccessibleTrue(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 only if a field - * is not already accessible. + * Sets the accessible object as accessible by calling {@link AccessibleObject#setAccessible(boolean) AccessibleObject#setAccessible(true)} but + * only if it is not already accessible. * - * @param accessibleObject The accessibleObject 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 */ @@ -202,12 +209,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 only 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 only 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 00000000000..b6a379100e0 --- /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 index cf1b5d20bb9..9f215e6ef91 100644 --- a/src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java +++ b/src/test/java/org/apache/commons/lang3/external/AnnotationEqualsTest.java @@ -32,8 +32,9 @@ /** * Regression test for LANG-1815. *

- * Verifies that {@code AnnotationUtils.equals(Annotation, Annotation)} treats two equal - * package-private annotations as equal, and also wraps a possible ReflectiveOperationException. + * 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. *

* *

Important

@@ -88,4 +89,17 @@ void equalsWrapsReflectiveOperationException() throws Exception { 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); + } + } From ff0e207dc1ee5011131cb20faf7de1c0d7ba3bb1 Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Thu, 30 Jul 2026 07:51:55 -0400 Subject: [PATCH 4/4] Add @since tag to getForceAccessible method --- .../org/apache/commons/lang3/builder/AbstractReflection.java | 1 + 1 file changed, 1 insertion(+) 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 fbe224cf266..e422d56bf6d 100644 --- a/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java +++ b/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java @@ -108,6 +108,7 @@ 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 */ public static boolean getForceAccessible() { return SystemProperties.getBoolean(AbstractReflection.class, "forceAccessible", () -> true);