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.

TODO:

- add release note entries

See #3498
Closes #3532

Author: sbrannen

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}.
@@ -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()));

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