diff --git a/src/main/java/org/apache/commons/lang3/AnnotationUtils.java b/src/main/java/org/apache/commons/lang3/AnnotationUtils.java index 3b21b0f6458..bdaf76433ca 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 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 49ce0a96ce1..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,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 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 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 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 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 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 new file mode 100644 index 00000000000..9f215e6ef91 --- /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 LANG-1815. + *

+ * 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

+ *

+ * 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 { + 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); + } + +}