Apply field predicate before searching type hierarchy

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. See #3498 Closes #3532 Closes #3533 (cherry picked from commit f30a8d5)

Author: sbrannen

documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc

@@ -15,6 +15,10 @@ JUnit repository on GitHub.
 
 ==== Bug Fixes
 
+* Field predicates are now applied while searching the type hierarchy. This fixes bugs in
+ `findFields(...)` and `streamFields(...)` in `ReflectionSupport` as well as
+ `findAnnotatedFields(...)` and `findAnnotatedFieldValues(...)` in `AnnotationSupport`.
+ - See link:https://github.com/junit-team/junit5/issues/3532[issue 3532] for details.
 * Method predicates are now applied while searching the type hierarchy. This fixes bugs
  in `findMethods(...)` and `streamMethods(...)` in `ReflectionSupport` as well as
  `findAnnotatedMethods(...)` in `AnnotationSupport`.
@@ -26,16 +30,20 @@ JUnit repository on GitHub.
 
 ==== Bug Fixes
 
+* A package-private static field annotated with `@TempDir` is no longer _shadowed_ by a
+ non-static field annotated with `@TempDir` when the non-static field resides in a
+ different package and has the same name as the static field.
+ - See link:https://github.com/junit-team/junit5/issues/3532[issue 3532] for details.
 * A package-private class-level lifecycle method annotated with `@BeforeAll` or
  `@AfterAll` is no longer _shadowed_ by a method-level lifecycle method annotated with
  `@BeforeEach` or `@AfterEach` when the method-level lifecycle method resides in a
  different package and has the same name as the class-level lifecycle method.
  - See link:https://github.com/junit-team/junit5/issues/3498[issue 3498] for details.
+* The `ON_SUCCESS` cleanup mode of `@TempDir` now takes into account failures of test
+ methods and nested tests when it's declared on the class level, e.g. as a static field.
 * The `RandomNumberExtension` example in the
  <<../user-guide/index.adoc#extensions-RandomNumberExtension, User Guide>> has been
  updated to properly support `Integer` types as well as non-static field injection.
-* The `ON_SUCCESS` cleanup mode of `@TempDir` now takes into account failures of test
- methods and nested tests when it's declared on the class level, e.g. as a static field.
 
 ==== New Features and Improvements
 

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java

@@ -1238,6 +1238,7 @@ public static List<Constructor<?>> findConstructors(Class<?> clazz, Predicate<Co
  */
 public static List<Field> findFields(Class<?> clazz, Predicate<Field> predicate,
 HierarchyTraversalMode traversalMode) {
+
 return streamFields(clazz, predicate, traversalMode).collect(toUnmodifiableList());
 }
 
@@ -1252,21 +1253,23 @@ public static Stream<Field> streamFields(Class<?> clazz, Predicate<Field> 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<Field> findAllFieldsInHierarchy(Class<?> clazz, HierarchyTraversalMode traversalMode) {
+private static List<Field> findAllFieldsInHierarchy(Class<?> clazz, Predicate<Field> predicate,
+HierarchyTraversalMode traversalMode) {
+
 Preconditions.notNull(clazz, "Class must not be null");
 Preconditions.notNull(traversalMode, "HierarchyTraversalMode must not be null");
 
 // @formatter:off
-List<Field> localFields = getDeclaredFields(clazz).stream()
+List<Field> localFields = getDeclaredFields(clazz, predicate).stream()
 .filter(field -> !field.isSynthetic())
 .collect(toList());
-List<Field> superclassFields = getSuperclassFields(clazz, traversalMode).stream()
+List<Field> superclassFields = getSuperclassFields(clazz, predicate, traversalMode).stream()
 .filter(field -> !isFieldShadowedByLocalFields(field, localFields))
 .collect(toList());
-List<Field> interfaceFields = getInterfaceFields(clazz, traversalMode).stream()
+List<Field> interfaceFields = getInterfaceFields(clazz, predicate, traversalMode).stream()
 .filter(field -> !isFieldShadowedByLocalFields(field, localFields))
 .collect(toList());
 // @formatter:on
@@ -1529,18 +1532,20 @@ private static List<Method> 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<Field> getFields(Class<?> clazz) {
-return toSortedMutableList(clazz.getFields());
+private static List<Field> getFields(Class<?> clazz, Predicate<Field> 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<Field> getDeclaredFields(Class<?> clazz) {
-return toSortedMutableList(clazz.getDeclaredFields());
+private static List<Field> getDeclaredFields(Class<?> clazz, Predicate<Field> predicate) {
+return toSortedMutableList(clazz.getDeclaredFields(), predicate);
 }
 
 /**
@@ -1602,9 +1607,10 @@ private static List<Method> getDefaultMethods(Class<?> clazz) {
 // @formatter:on
 }
 
-private static List<Field> toSortedMutableList(Field[] fields) {
+private static List<Field> toSortedMutableList(Field[] fields, Predicate<Field> 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<Method> getInterfaceMethods(Class<?> clazz, Predicate<Method
 return allInterfaceMethods;
 }
 
-private static List<Field> getInterfaceFields(Class<?> clazz, HierarchyTraversalMode traversalMode) {
+private static List<Field> getInterfaceFields(Class<?> clazz, Predicate<Field> predicate,
+HierarchyTraversalMode traversalMode) {
+
 List<Field> allInterfaceFields = new ArrayList<>();
 for (Class<?> ifc : clazz.getInterfaces()) {
-List<Field> localInterfaceFields = getFields(ifc);
+List<Field> localInterfaceFields = getFields(ifc, predicate);
 
 // @formatter:off
-List<Field> superinterfaceFields = getInterfaceFields(ifc, traversalMode).stream()
+List<Field> superinterfaceFields = getInterfaceFields(ifc, predicate, traversalMode).stream()
 .filter(field -> !isFieldShadowedByLocalFields(field, localInterfaceFields))
 .collect(toList());
 // @formatter:on
@@ -1694,12 +1702,14 @@ private static List<Field> getInterfaceFields(Class<?> clazz, HierarchyTraversal
 return allInterfaceFields;
 }
 
-private static List<Field> getSuperclassFields(Class<?> clazz, HierarchyTraversalMode traversalMode) {
+private static List<Field> getSuperclassFields(Class<?> clazz, Predicate<Field> 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<Field> localFields) {

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<Field> findShadowingAnnotatedFields(Class<? extends Annotation> 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

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}.
@@ -1380,6 +1382,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()));

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 {
+}

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 {
+}

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;
+
+}

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;
+
+}