Skip to content

Commit f82dd1e

Browse files
sbrannensbrannen
committed
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)
1 parent 1d1eb85 commit f82dd1e

File tree

8 files changed

+184
-19
lines changed

8 files changed

+184
-19
lines changed

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

+10-2
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ JUnit repository on GitHub.
1515

1616
==== Bug Fixes
1717

18+
* Field predicates are now applied while searching the type hierarchy. This fixes bugs in
19+
`findFields(...)` and `streamFields(...)` in `ReflectionSupport` as well as
20+
`findAnnotatedFields(...)` and `findAnnotatedFieldValues(...)` in `AnnotationSupport`.
21+
- See link:https://github.com/junit-team/junit5/issues/3532[issue 3532] for details.
1822
* Method predicates are now applied while searching the type hierarchy. This fixes bugs
1923
in `findMethods(...)` and `streamMethods(...)` in `ReflectionSupport` as well as
2024
`findAnnotatedMethods(...)` in `AnnotationSupport`.
@@ -26,16 +30,20 @@ JUnit repository on GitHub.
2630

2731
==== Bug Fixes
2832

33+
* A package-private static field annotated with `@TempDir` is no longer _shadowed_ by a
34+
non-static field annotated with `@TempDir` when the non-static field resides in a
35+
different package and has the same name as the static field.
36+
- See link:https://github.com/junit-team/junit5/issues/3532[issue 3532] for details.
2937
* A package-private class-level lifecycle method annotated with `@BeforeAll` or
3038
`@AfterAll` is no longer _shadowed_ by a method-level lifecycle method annotated with
3139
`@BeforeEach` or `@AfterEach` when the method-level lifecycle method resides in a
3240
different package and has the same name as the class-level lifecycle method.
3341
- See link:https://github.com/junit-team/junit5/issues/3498[issue 3498] for details.
42+
* The `ON_SUCCESS` cleanup mode of `@TempDir` now takes into account failures of test
43+
methods and nested tests when it's declared on the class level, e.g. as a static field.
3444
* The `RandomNumberExtension` example in the
3545
<<../user-guide/index.adoc#extensions-RandomNumberExtension, User Guide>> has been
3646
updated to properly support `Integer` types as well as non-static field injection.
37-
* The `ON_SUCCESS` cleanup mode of `@TempDir` now takes into account failures of test
38-
methods and nested tests when it's declared on the class level, e.g. as a static field.
3947

4048
==== New Features and Improvements
4149

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

+27-17
Original file line numberDiff line numberDiff line change
@@ -1238,6 +1238,7 @@ public static List<Constructor<?>> findConstructors(Class<?> clazz, Predicate<Co
12381238
*/
12391239
public static List<Field> findFields(Class<?> clazz, Predicate<Field> predicate,
12401240
HierarchyTraversalMode traversalMode) {
1241+
12411242
return streamFields(clazz, predicate, traversalMode).collect(toUnmodifiableList());
12421243
}
12431244

@@ -1252,21 +1253,23 @@ public static Stream<Field> streamFields(Class<?> clazz, Predicate<Field> predic
12521253
Preconditions.notNull(predicate, "Predicate must not be null");
12531254
Preconditions.notNull(traversalMode, "HierarchyTraversalMode must not be null");
12541255

1255-
return findAllFieldsInHierarchy(clazz, traversalMode).stream().filter(predicate);
1256+
return findAllFieldsInHierarchy(clazz, predicate, traversalMode).stream();
12561257
}
12571258

1258-
private static List<Field> findAllFieldsInHierarchy(Class<?> clazz, HierarchyTraversalMode traversalMode) {
1259+
private static List<Field> findAllFieldsInHierarchy(Class<?> clazz, Predicate<Field> predicate,
1260+
HierarchyTraversalMode traversalMode) {
1261+
12591262
Preconditions.notNull(clazz, "Class must not be null");
12601263
Preconditions.notNull(traversalMode, "HierarchyTraversalMode must not be null");
12611264

12621265
// @formatter:off
1263-
List<Field> localFields = getDeclaredFields(clazz).stream()
1266+
List<Field> localFields = getDeclaredFields(clazz, predicate).stream()
12641267
.filter(field -> !field.isSynthetic())
12651268
.collect(toList());
1266-
List<Field> superclassFields = getSuperclassFields(clazz, traversalMode).stream()
1269+
List<Field> superclassFields = getSuperclassFields(clazz, predicate, traversalMode).stream()
12671270
.filter(field -> !isFieldShadowedByLocalFields(field, localFields))
12681271
.collect(toList());
1269-
List<Field> interfaceFields = getInterfaceFields(clazz, traversalMode).stream()
1272+
List<Field> interfaceFields = getInterfaceFields(clazz, predicate, traversalMode).stream()
12701273
.filter(field -> !isFieldShadowedByLocalFields(field, localFields))
12711274
.collect(toList());
12721275
// @formatter:on
@@ -1529,18 +1532,20 @@ private static List<Method> findAllMethodsInHierarchy(Class<?> clazz, Predicate<
15291532

15301533
/**
15311534
* Custom alternative to {@link Class#getFields()} that sorts the fields
1532-
* and converts them to a mutable list.
1535+
* which match the supplied predicate and converts them to a mutable list.
1536+
* @param predicate the field filter; never {@code null}
15331537
*/
1534-
private static List<Field> getFields(Class<?> clazz) {
1535-
return toSortedMutableList(clazz.getFields());
1538+
private static List<Field> getFields(Class<?> clazz, Predicate<Field> predicate) {
1539+
return toSortedMutableList(clazz.getFields(), predicate);
15361540
}
15371541

15381542
/**
15391543
* Custom alternative to {@link Class#getDeclaredFields()} that sorts the
1540-
* fields and converts them to a mutable list.
1544+
* fields which match the supplied predicate and converts them to a mutable list.
1545+
* @param predicate the field filter; never {@code null}
15411546
*/
1542-
private static List<Field> getDeclaredFields(Class<?> clazz) {
1543-
return toSortedMutableList(clazz.getDeclaredFields());
1547+
private static List<Field> getDeclaredFields(Class<?> clazz, Predicate<Field> predicate) {
1548+
return toSortedMutableList(clazz.getDeclaredFields(), predicate);
15441549
}
15451550

15461551
/**
@@ -1602,9 +1607,10 @@ private static List<Method> getDefaultMethods(Class<?> clazz) {
16021607
// @formatter:on
16031608
}
16041609

1605-
private static List<Field> toSortedMutableList(Field[] fields) {
1610+
private static List<Field> toSortedMutableList(Field[] fields, Predicate<Field> predicate) {
16061611
// @formatter:off
16071612
return Arrays.stream(fields)
1613+
.filter(predicate)
16081614
.sorted(ReflectionUtils::defaultFieldSorter)
16091615
// Use toCollection() instead of toList() to ensure list is mutable.
16101616
.collect(toCollection(ArrayList::new));
@@ -1672,13 +1678,15 @@ private static List<Method> getInterfaceMethods(Class<?> clazz, Predicate<Method
16721678
return allInterfaceMethods;
16731679
}
16741680

1675-
private static List<Field> getInterfaceFields(Class<?> clazz, HierarchyTraversalMode traversalMode) {
1681+
private static List<Field> getInterfaceFields(Class<?> clazz, Predicate<Field> predicate,
1682+
HierarchyTraversalMode traversalMode) {
1683+
16761684
List<Field> allInterfaceFields = new ArrayList<>();
16771685
for (Class<?> ifc : clazz.getInterfaces()) {
1678-
List<Field> localInterfaceFields = getFields(ifc);
1686+
List<Field> localInterfaceFields = getFields(ifc, predicate);
16791687

16801688
// @formatter:off
1681-
List<Field> superinterfaceFields = getInterfaceFields(ifc, traversalMode).stream()
1689+
List<Field> superinterfaceFields = getInterfaceFields(ifc, predicate, traversalMode).stream()
16821690
.filter(field -> !isFieldShadowedByLocalFields(field, localInterfaceFields))
16831691
.collect(toList());
16841692
// @formatter:on
@@ -1694,12 +1702,14 @@ private static List<Field> getInterfaceFields(Class<?> clazz, HierarchyTraversal
16941702
return allInterfaceFields;
16951703
}
16961704

1697-
private static List<Field> getSuperclassFields(Class<?> clazz, HierarchyTraversalMode traversalMode) {
1705+
private static List<Field> getSuperclassFields(Class<?> clazz, Predicate<Field> predicate,
1706+
HierarchyTraversalMode traversalMode) {
1707+
16981708
Class<?> superclass = clazz.getSuperclass();
16991709
if (!isSearchable(superclass)) {
17001710
return Collections.emptyList();
17011711
}
1702-
return findAllFieldsInHierarchy(superclass, traversalMode);
1712+
return findAllFieldsInHierarchy(superclass, predicate, traversalMode);
17031713
}
17041714

17051715
private static boolean isFieldShadowedByLocalFields(Field field, List<Field> localFields) {

‎platform-tests/src/test/java/org/junit/platform/commons/util/AnnotationUtilsTests.java

+26
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,12 @@
4646
import org.junit.jupiter.api.BeforeEach;
4747
import org.junit.jupiter.api.Test;
4848
import org.junit.platform.commons.PreconditionViolationException;
49+
import org.junit.platform.commons.util.pkg1.ClassLevelDir;
50+
import org.junit.platform.commons.util.pkg1.InstanceLevelDir;
4951
import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateBeforeMethod;
52+
import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateTempDirField;
5053
import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateBeforeMethod;
54+
import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateTempDirField;
5155

5256
/**
5357
* Unit tests for {@link AnnotationUtils}.
@@ -504,6 +508,28 @@ private List<Field> findShadowingAnnotatedFields(Class<? extends Annotation> ann
504508
return findAnnotatedFields(ClassWithShadowedAnnotatedFields.class, annotationType, isStringField);
505509
}
506510

511+
/**
512+
* @see https://github.com/junit-team/junit5/issues/3532
513+
*/
514+
@Test
515+
void findAnnotatedFieldsAppliesPredicateBeforeSearchingTypeHierarchy() throws Exception {
516+
final String TEMP_DIR = "tempDir";
517+
Class<?> superclass = SuperclassWithStaticPackagePrivateTempDirField.class;
518+
Field staticField = superclass.getDeclaredField(TEMP_DIR);
519+
Class<?> subclass = SubclassWithNonStaticPackagePrivateTempDirField.class;
520+
Field nonStaticField = subclass.getDeclaredField(TEMP_DIR);
521+
522+
// Prerequisite
523+
var fields = findAnnotatedFields(superclass, ClassLevelDir.class, field -> true);
524+
assertThat(fields).containsExactly(staticField);
525+
526+
// Actual use cases for this test
527+
fields = findAnnotatedFields(subclass, ClassLevelDir.class, field -> true);
528+
assertThat(fields).containsExactly(staticField);
529+
fields = findAnnotatedFields(subclass, InstanceLevelDir.class, field -> true);
530+
assertThat(fields).containsExactly(nonStaticField);
531+
}
532+
507533
// === findPublicAnnotatedFields() =========================================
508534

509535
@Test

‎platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java

+24
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,9 @@
7474
import org.junit.platform.commons.util.ReflectionUtilsTests.OuterClassImplementingInterface.InnerClassImplementingInterface;
7575
import org.junit.platform.commons.util.classes.CustomType;
7676
import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateBeforeMethod;
77+
import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateTempDirField;
7778
import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateBeforeMethod;
79+
import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateTempDirField;
7880

7981
/**
8082
* Unit tests for {@link ReflectionUtils}.
@@ -1380,6 +1382,28 @@ void isGeneric() {
13801382
}
13811383
}
13821384

1385+
/**
1386+
* @see https://github.com/junit-team/junit5/issues/3532
1387+
*/
1388+
@Test
1389+
void findFieldsAppliesPredicateBeforeSearchingTypeHierarchy() throws Exception {
1390+
final String TEMP_DIR = "tempDir";
1391+
Class<?> superclass = SuperclassWithStaticPackagePrivateTempDirField.class;
1392+
Field staticField = superclass.getDeclaredField(TEMP_DIR);
1393+
Class<?> subclass = SubclassWithNonStaticPackagePrivateTempDirField.class;
1394+
Field nonStaticField = subclass.getDeclaredField(TEMP_DIR);
1395+
1396+
// Prerequisite
1397+
var fields = findFields(superclass, ReflectionUtils::isStatic, TOP_DOWN);
1398+
assertThat(fields).containsExactly(staticField);
1399+
1400+
// Actual use cases for this test
1401+
fields = findFields(subclass, ReflectionUtils::isStatic, TOP_DOWN);
1402+
assertThat(fields).containsExactly(staticField);
1403+
fields = findFields(subclass, ReflectionUtils::isNotStatic, TOP_DOWN);
1404+
assertThat(fields).containsExactly(nonStaticField);
1405+
}
1406+
13831407
@Test
13841408
void readFieldValuesPreconditions() {
13851409
assertThrows(PreconditionViolationException.class, () -> ReflectionUtils.readFieldValues(null, new Object()));

‎platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/ClassLevelDir.java

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/*
2+
* Copyright 2015-2023 the original author or authors.
3+
*
4+
* All rights reserved. This program and the accompanying materials are
5+
* made available under the terms of the Eclipse Public License v2.0 which
6+
* accompanies this distribution and is available at
7+
*
8+
* https://www.eclipse.org/legal/epl-v20.html
9+
*/
10+
11+
package org.junit.platform.commons.util.pkg1;
12+
13+
import java.lang.annotation.ElementType;
14+
import java.lang.annotation.Retention;
15+
import java.lang.annotation.RetentionPolicy;
16+
import java.lang.annotation.Target;
17+
18+
/**
19+
* Mimics {@code @TempDir}.
20+
*/
21+
@Target(ElementType.FIELD)
22+
@Retention(RetentionPolicy.RUNTIME)
23+
public @interface ClassLevelDir {
24+
}

‎platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/InstanceLevelDir.java

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/*
2+
* Copyright 2015-2023 the original author or authors.
3+
*
4+
* All rights reserved. This program and the accompanying materials are
5+
* made available under the terms of the Eclipse Public License v2.0 which
6+
* accompanies this distribution and is available at
7+
*
8+
* https://www.eclipse.org/legal/epl-v20.html
9+
*/
10+
11+
package org.junit.platform.commons.util.pkg1;
12+
13+
import java.lang.annotation.ElementType;
14+
import java.lang.annotation.Retention;
15+
import java.lang.annotation.RetentionPolicy;
16+
import java.lang.annotation.Target;
17+
18+
/**
19+
* Mimics {@code @TempDir}.
20+
*/
21+
@Target(ElementType.FIELD)
22+
@Retention(RetentionPolicy.RUNTIME)
23+
public @interface InstanceLevelDir {
24+
}

‎platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/SuperclassWithStaticPackagePrivateTempDirField.java

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/*
2+
* Copyright 2015-2023 the original author or authors.
3+
*
4+
* All rights reserved. This program and the accompanying materials are
5+
* made available under the terms of the Eclipse Public License v2.0 which
6+
* accompanies this distribution and is available at
7+
*
8+
* https://www.eclipse.org/legal/epl-v20.html
9+
*/
10+
11+
package org.junit.platform.commons.util.pkg1;
12+
13+
import java.nio.file.Path;
14+
15+
/**
16+
* @see https://github.com/junit-team/junit5/issues/3532
17+
*/
18+
public class SuperclassWithStaticPackagePrivateTempDirField {
19+
20+
@ClassLevelDir
21+
static Path tempDir;
22+
23+
}

‎platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/subpkg/SubclassWithNonStaticPackagePrivateTempDirField.java

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/*
2+
* Copyright 2015-2023 the original author or authors.
3+
*
4+
* All rights reserved. This program and the accompanying materials are
5+
* made available under the terms of the Eclipse Public License v2.0 which
6+
* accompanies this distribution and is available at
7+
*
8+
* https://www.eclipse.org/legal/epl-v20.html
9+
*/
10+
11+
package org.junit.platform.commons.util.pkg1.subpkg;
12+
13+
import java.nio.file.Path;
14+
15+
import org.junit.platform.commons.util.pkg1.InstanceLevelDir;
16+
import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateTempDirField;
17+
18+
/**
19+
* @see https://github.com/junit-team/junit5/issues/3532
20+
*/
21+
public class SubclassWithNonStaticPackagePrivateTempDirField extends SuperclassWithStaticPackagePrivateTempDirField {
22+
23+
@InstanceLevelDir
24+
Path tempDir;
25+
26+
}

0 commit comments

Comments
 (0)