From 7c30fd34dd0955180f0abf900769cf40bbfebddf Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 3 Nov 2023 18:25:52 +0100 Subject: [PATCH] Apply field predicate before searching type hierarchy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, findFields() and streamFields() in ReflectionSupport as well as findAnnotatedFields() and findAnnotatedFieldValues() in AnnotationSupport first searched for all fields in the type hierarchy and then applied the user-supplied predicate (or "is annotated?" predicate) afterwards. This resulted in fields in subclasses incorrectly "shadowing" package-private fields in superclasses (in a different package) even if the predicate would otherwise exclude the field in such a subclass. For example, given a superclass that declares a package-private static @⁠TempDir "tempDir" field and a subclass (in a different package) that declares a @⁠TempDir "tempDir" field, when JUnit Jupiter looked up @⁠TempDir fields for the subclass, the @⁠TempDir "tempDir" field in the superclass was not found because the @⁠TempDir "tempDir" field shadowed it based solely on the field signature, ignoring the type of annotation sought. To address that, this commit modifies the internal search algorithms in ReflectionUtils so that field predicates are applied while searching the hierarchy for fields. TODO: - add release note entries See #3498 Closes #3532 --- .../commons/util/ReflectionUtils.java | 44 ++++++++++++------- .../commons/util/AnnotationUtilsTests.java | 26 +++++++++++ .../commons/util/ReflectionUtilsTests.java | 24 ++++++++++ .../commons/util/pkg1/ClassLevelDir.java | 24 ++++++++++ .../commons/util/pkg1/InstanceLevelDir.java | 24 ++++++++++ ...sWithStaticPackagePrivateTempDirField.java | 23 ++++++++++ ...thNonStaticPackagePrivateTempDirField.java | 26 +++++++++++ 7 files changed, 174 insertions(+), 17 deletions(-) create mode 100644 platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/ClassLevelDir.java create mode 100644 platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/InstanceLevelDir.java create mode 100644 platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/SuperclassWithStaticPackagePrivateTempDirField.java create mode 100644 platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/subpkg/SubclassWithNonStaticPackagePrivateTempDirField.java diff --git a/junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java b/junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java index 927b97451e0a..ea824ebd23a9 100644 --- a/junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java +++ b/junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java @@ -1238,6 +1238,7 @@ public static List> findConstructors(Class clazz, Predicate findFields(Class clazz, Predicate predicate, HierarchyTraversalMode traversalMode) { + return streamFields(clazz, predicate, traversalMode).collect(toUnmodifiableList()); } @@ -1252,21 +1253,23 @@ public static Stream streamFields(Class clazz, Predicate predic Preconditions.notNull(predicate, "Predicate must not be null"); Preconditions.notNull(traversalMode, "HierarchyTraversalMode must not be null"); - return findAllFieldsInHierarchy(clazz, traversalMode).stream().filter(predicate); + return findAllFieldsInHierarchy(clazz, predicate, traversalMode).stream(); } - private static List findAllFieldsInHierarchy(Class clazz, HierarchyTraversalMode traversalMode) { + private static List findAllFieldsInHierarchy(Class clazz, Predicate predicate, + HierarchyTraversalMode traversalMode) { + Preconditions.notNull(clazz, "Class must not be null"); Preconditions.notNull(traversalMode, "HierarchyTraversalMode must not be null"); // @formatter:off - List localFields = getDeclaredFields(clazz).stream() + List localFields = getDeclaredFields(clazz, predicate).stream() .filter(field -> !field.isSynthetic()) .collect(toList()); - List superclassFields = getSuperclassFields(clazz, traversalMode).stream() + List superclassFields = getSuperclassFields(clazz, predicate, traversalMode).stream() .filter(field -> !isFieldShadowedByLocalFields(field, localFields)) .collect(toList()); - List interfaceFields = getInterfaceFields(clazz, traversalMode).stream() + List interfaceFields = getInterfaceFields(clazz, predicate, traversalMode).stream() .filter(field -> !isFieldShadowedByLocalFields(field, localFields)) .collect(toList()); // @formatter:on @@ -1529,18 +1532,20 @@ private static List findAllMethodsInHierarchy(Class clazz, Predicate< /** * Custom alternative to {@link Class#getFields()} that sorts the fields - * and converts them to a mutable list. + * which match the supplied predicate and converts them to a mutable list. + * @param predicate the field filter; never {@code null} */ - private static List getFields(Class clazz) { - return toSortedMutableList(clazz.getFields()); + private static List getFields(Class clazz, Predicate predicate) { + return toSortedMutableList(clazz.getFields(), predicate); } /** * Custom alternative to {@link Class#getDeclaredFields()} that sorts the - * fields and converts them to a mutable list. + * fields which match the supplied predicate and converts them to a mutable list. + * @param predicate the field filter; never {@code null} */ - private static List getDeclaredFields(Class clazz) { - return toSortedMutableList(clazz.getDeclaredFields()); + private static List getDeclaredFields(Class clazz, Predicate predicate) { + return toSortedMutableList(clazz.getDeclaredFields(), predicate); } /** @@ -1602,9 +1607,10 @@ private static List getDefaultMethods(Class clazz) { // @formatter:on } - private static List toSortedMutableList(Field[] fields) { + private static List toSortedMutableList(Field[] fields, Predicate predicate) { // @formatter:off return Arrays.stream(fields) + .filter(predicate) .sorted(ReflectionUtils::defaultFieldSorter) // Use toCollection() instead of toList() to ensure list is mutable. .collect(toCollection(ArrayList::new)); @@ -1672,13 +1678,15 @@ private static List getInterfaceMethods(Class clazz, Predicate getInterfaceFields(Class clazz, HierarchyTraversalMode traversalMode) { + private static List getInterfaceFields(Class clazz, Predicate predicate, + HierarchyTraversalMode traversalMode) { + List allInterfaceFields = new ArrayList<>(); for (Class ifc : clazz.getInterfaces()) { - List localInterfaceFields = getFields(ifc); + List localInterfaceFields = getFields(ifc, predicate); // @formatter:off - List superinterfaceFields = getInterfaceFields(ifc, traversalMode).stream() + List superinterfaceFields = getInterfaceFields(ifc, predicate, traversalMode).stream() .filter(field -> !isFieldShadowedByLocalFields(field, localInterfaceFields)) .collect(toList()); // @formatter:on @@ -1694,12 +1702,14 @@ private static List getInterfaceFields(Class clazz, HierarchyTraversal return allInterfaceFields; } - private static List getSuperclassFields(Class clazz, HierarchyTraversalMode traversalMode) { + private static List getSuperclassFields(Class clazz, Predicate predicate, + HierarchyTraversalMode traversalMode) { + Class superclass = clazz.getSuperclass(); if (!isSearchable(superclass)) { return Collections.emptyList(); } - return findAllFieldsInHierarchy(superclass, traversalMode); + return findAllFieldsInHierarchy(superclass, predicate, traversalMode); } private static boolean isFieldShadowedByLocalFields(Field field, List localFields) { diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/AnnotationUtilsTests.java b/platform-tests/src/test/java/org/junit/platform/commons/util/AnnotationUtilsTests.java index f26f5ab755d3..5dde2c34cbd9 100644 --- a/platform-tests/src/test/java/org/junit/platform/commons/util/AnnotationUtilsTests.java +++ b/platform-tests/src/test/java/org/junit/platform/commons/util/AnnotationUtilsTests.java @@ -46,8 +46,12 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.platform.commons.PreconditionViolationException; +import org.junit.platform.commons.util.pkg1.ClassLevelDir; +import org.junit.platform.commons.util.pkg1.InstanceLevelDir; import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateBeforeMethod; +import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateTempDirField; import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateBeforeMethod; +import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateTempDirField; /** * Unit tests for {@link AnnotationUtils}. @@ -504,6 +508,28 @@ private List findShadowingAnnotatedFields(Class ann return findAnnotatedFields(ClassWithShadowedAnnotatedFields.class, annotationType, isStringField); } + /** + * @see https://github.com/junit-team/junit5/issues/3532 + */ + @Test + void findAnnotatedFieldsAppliesPredicateBeforeSearchingTypeHierarchy() throws Exception { + final String TEMP_DIR = "tempDir"; + Class superclass = SuperclassWithStaticPackagePrivateTempDirField.class; + Field staticField = superclass.getDeclaredField(TEMP_DIR); + Class subclass = SubclassWithNonStaticPackagePrivateTempDirField.class; + Field nonStaticField = subclass.getDeclaredField(TEMP_DIR); + + // Prerequisite + var fields = findAnnotatedFields(superclass, ClassLevelDir.class, field -> true); + assertThat(fields).containsExactly(staticField); + + // Actual use cases for this test + fields = findAnnotatedFields(subclass, ClassLevelDir.class, field -> true); + assertThat(fields).containsExactly(staticField); + fields = findAnnotatedFields(subclass, InstanceLevelDir.class, field -> true); + assertThat(fields).containsExactly(nonStaticField); + } + // === findPublicAnnotatedFields() ========================================= @Test diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java b/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java index 6544ac5b4c69..11b3963c3e8e 100644 --- a/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java +++ b/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java @@ -74,7 +74,9 @@ import org.junit.platform.commons.util.ReflectionUtilsTests.OuterClassImplementingInterface.InnerClassImplementingInterface; import org.junit.platform.commons.util.classes.CustomType; import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateBeforeMethod; +import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateTempDirField; import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateBeforeMethod; +import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateTempDirField; /** * Unit tests for {@link ReflectionUtils}. @@ -1379,6 +1381,28 @@ void isGeneric() { } } + /** + * @see https://github.com/junit-team/junit5/issues/3532 + */ + @Test + void findFieldsAppliesPredicateBeforeSearchingTypeHierarchy() throws Exception { + final String TEMP_DIR = "tempDir"; + Class superclass = SuperclassWithStaticPackagePrivateTempDirField.class; + Field staticField = superclass.getDeclaredField(TEMP_DIR); + Class subclass = SubclassWithNonStaticPackagePrivateTempDirField.class; + Field nonStaticField = subclass.getDeclaredField(TEMP_DIR); + + // Prerequisite + var fields = findFields(superclass, ReflectionUtils::isStatic, TOP_DOWN); + assertThat(fields).containsExactly(staticField); + + // Actual use cases for this test + fields = findFields(subclass, ReflectionUtils::isStatic, TOP_DOWN); + assertThat(fields).containsExactly(staticField); + fields = findFields(subclass, ReflectionUtils::isNotStatic, TOP_DOWN); + assertThat(fields).containsExactly(nonStaticField); + } + @Test void readFieldValuesPreconditions() { assertThrows(PreconditionViolationException.class, () -> ReflectionUtils.readFieldValues(null, new Object())); diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/ClassLevelDir.java b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/ClassLevelDir.java new file mode 100644 index 000000000000..d6b94d9d2076 --- /dev/null +++ b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/ClassLevelDir.java @@ -0,0 +1,24 @@ +/* + * Copyright 2015-2023 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.platform.commons.util.pkg1; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Mimics {@code @TempDir}. + */ +@Target(ElementType.FIELD) +@Retention(RetentionPolicy.RUNTIME) +public @interface ClassLevelDir { +} diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/InstanceLevelDir.java b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/InstanceLevelDir.java new file mode 100644 index 000000000000..bfa4e4ad8b95 --- /dev/null +++ b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/InstanceLevelDir.java @@ -0,0 +1,24 @@ +/* + * Copyright 2015-2023 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.platform.commons.util.pkg1; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Mimics {@code @TempDir}. + */ +@Target(ElementType.FIELD) +@Retention(RetentionPolicy.RUNTIME) +public @interface InstanceLevelDir { +} diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/SuperclassWithStaticPackagePrivateTempDirField.java b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/SuperclassWithStaticPackagePrivateTempDirField.java new file mode 100644 index 000000000000..4e2bbe7ec696 --- /dev/null +++ b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/SuperclassWithStaticPackagePrivateTempDirField.java @@ -0,0 +1,23 @@ +/* + * Copyright 2015-2023 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.platform.commons.util.pkg1; + +import java.nio.file.Path; + +/** + * @see https://github.com/junit-team/junit5/issues/3532 + */ +public class SuperclassWithStaticPackagePrivateTempDirField { + + @ClassLevelDir + static Path tempDir; + +} diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/subpkg/SubclassWithNonStaticPackagePrivateTempDirField.java b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/subpkg/SubclassWithNonStaticPackagePrivateTempDirField.java new file mode 100644 index 000000000000..d7eb33f6a326 --- /dev/null +++ b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/subpkg/SubclassWithNonStaticPackagePrivateTempDirField.java @@ -0,0 +1,26 @@ +/* + * Copyright 2015-2023 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.platform.commons.util.pkg1.subpkg; + +import java.nio.file.Path; + +import org.junit.platform.commons.util.pkg1.InstanceLevelDir; +import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateTempDirField; + +/** + * @see https://github.com/junit-team/junit5/issues/3532 + */ +public class SubclassWithNonStaticPackagePrivateTempDirField extends SuperclassWithStaticPackagePrivateTempDirField { + + @InstanceLevelDir + Path tempDir; + +}