From f82dd1e716f8717e012152b1d1d5cc0da10d33cd 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. See #3498 Closes #3532 Closes #3533 (cherry picked from commit f30a8d551795653843e3c44c1af01001bddfd846) --- .../release-notes/release-notes-5.10.1.adoc | 12 ++++- .../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 +++++++++++ 8 files changed, 184 insertions(+), 19 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/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc index 65c3aa6055b4..515aa64f41b1 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc +++ b/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 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 3e11222be2e3..7cccbe13a70c 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}. @@ -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())); 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; + +}