From d698446f7ce0018bcd35c17fe579db13e42f04a5 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Mon, 24 Jan 2022 13:45:09 +0100 Subject: [PATCH 1/2] Polish ReflectionUtils[Tests] --- .../springframework/util/ReflectionUtils.java | 4 ++-- .../util/ReflectionUtilsTests.java | 23 +++++++------------ 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/ReflectionUtils.java b/spring-core/src/main/java/org/springframework/util/ReflectionUtils.java index 2177481f0d3..36dfaa1e5eb 100644 --- a/spring-core/src/main/java/org/springframework/util/ReflectionUtils.java +++ b/spring-core/src/main/java/org/springframework/util/ReflectionUtils.java @@ -46,7 +46,7 @@ import org.springframework.lang.Nullable; public abstract class ReflectionUtils { /** - * Pre-built MethodFilter that matches all non-bridge non-synthetic methods + * Pre-built {@link MethodFilter} that matches all non-bridge non-synthetic methods * which are not declared on {@code java.lang.Object}. * @since 3.0.5 */ @@ -354,7 +354,6 @@ public abstract class ReflectionUtils { * @throws IllegalStateException if introspection fails */ public static void doWithMethods(Class clazz, MethodCallback mc, @Nullable MethodFilter mf) { - // Keep backing up the inheritance hierarchy. Method[] methods = getDeclaredMethods(clazz, false); for (Method method : methods) { if (mf != null && !mf.matches(method)) { @@ -367,6 +366,7 @@ public abstract class ReflectionUtils { throw new IllegalStateException("Not allowed to access method '" + method.getName() + "': " + ex); } } + // Keep backing up the inheritance hierarchy. if (clazz.getSuperclass() != null && (mf != USER_DECLARED_METHODS || clazz.getSuperclass() != Object.class)) { doWithMethods(clazz.getSuperclass(), mc, mf); } diff --git a/spring-core/src/test/java/org/springframework/util/ReflectionUtilsTests.java b/spring-core/src/test/java/org/springframework/util/ReflectionUtilsTests.java index 8ca69ddc9a0..06e2df61444 100644 --- a/spring-core/src/test/java/org/springframework/util/ReflectionUtilsTests.java +++ b/spring-core/src/test/java/org/springframework/util/ReflectionUtilsTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -184,27 +184,20 @@ class ReflectionUtilsTests { } @Test - void doWithProtectedMethods() { + void doWithMethodsUsingProtectedFilter() { ListSavingMethodCallback mc = new ListSavingMethodCallback(); ReflectionUtils.doWithMethods(TestObject.class, mc, method -> Modifier.isProtected(method.getModifiers())); - assertThat(mc.getMethodNames().isEmpty()).isFalse(); - assertThat(mc.getMethodNames().contains("clone")).as("Must find protected method on Object").isTrue(); - assertThat(mc.getMethodNames().contains("finalize")).as("Must find protected method on Object").isTrue(); - assertThat(mc.getMethodNames().contains("hashCode")).as("Public, not protected").isFalse(); - assertThat(mc.getMethodNames().contains("absquatulate")).as("Public, not protected").isFalse(); + assertThat(mc.getMethodNames()) + .hasSizeGreaterThanOrEqualTo(2) + .as("Must find protected methods on Object").contains("clone", "finalize") + .as("Public, not protected").doesNotContain("hashCode", "absquatulate"); } @Test - void duplicatesFound() { + void doWithMethodsFindsDuplicatesInClassHierarchy() { ListSavingMethodCallback mc = new ListSavingMethodCallback(); ReflectionUtils.doWithMethods(TestObjectSubclass.class, mc); - int absquatulateCount = 0; - for (String name : mc.getMethodNames()) { - if (name.equals("absquatulate")) { - ++absquatulateCount; - } - } - assertThat(absquatulateCount).as("Found 2 absquatulates").isEqualTo(2); + assertThat(mc.getMethodNames().stream()).filteredOn("absquatulate"::equals).as("Found 2 absquatulates").hasSize(2); } @Test From d01dca14e94b6ead5d8ed98c99ba4c7dfed0fefb Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Mon, 24 Jan 2022 17:03:16 +0100 Subject: [PATCH 2/2] Filter methods in Object in ReflectionUtils.USER_DECLARED_METHODS Prior to this commit, the USER_DECLARED_METHODS MethodFilter in ReflectionUtils did not actually filter methods declared in java.lang.Object as stated in its Javadoc. Consequently, if ReflectionUtils.doWithMethods was invoked with USER_DECLARED_METHODS and Object.class as the class to introspect, all non-bridge non-synthetic methods declared in java.lang.Object were passed to the supplied MethodCallback, which breaks the contract of USER_DECLARED_METHODS. In addition, if USER_DECLARED_METHODS was composed with a custom MethodFilter using `USER_DECLARED_METHODS.and()`, that composed method filter allowed all non-bridge non-synthetic methods declared in java.lang.Object to be passed to the supplied MethodCallback, which also breaks the contract of USER_DECLARED_METHODS. This behavior resulted in regressions in code that had previously used USER_DECLARED_METHODS by itself and then began using USER_DECLARED_METHODS in a composed filter. For example, since commit c419ea7ba7 ReflectiveAspectJAdvisorFactory has incorrectly processed methods in java.lang.Object as candidates for advice methods. This commit fixes this bug and associated regressions by ensuring that USER_DECLARED_METHODS actually filters methods declared in java.lang.Object. In addition, ReflectionUtils.doWithMethods now aborts its search algorithm early if invoked with the USER_DECLARED_METHODS filter and Object.class as the class to introspect. Closes gh-27970 --- .../springframework/util/ReflectionUtils.java | 6 ++++- .../util/ReflectionUtilsTests.java | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/spring-core/src/main/java/org/springframework/util/ReflectionUtils.java b/spring-core/src/main/java/org/springframework/util/ReflectionUtils.java index 36dfaa1e5eb..274cb718870 100644 --- a/spring-core/src/main/java/org/springframework/util/ReflectionUtils.java +++ b/spring-core/src/main/java/org/springframework/util/ReflectionUtils.java @@ -51,7 +51,7 @@ public abstract class ReflectionUtils { * @since 3.0.5 */ public static final MethodFilter USER_DECLARED_METHODS = - (method -> !method.isBridge() && !method.isSynthetic()); + (method -> !method.isBridge() && !method.isSynthetic() && (method.getDeclaringClass() != Object.class)); /** * Pre-built FieldFilter that matches all non-static, non-final fields. @@ -354,6 +354,10 @@ public abstract class ReflectionUtils { * @throws IllegalStateException if introspection fails */ public static void doWithMethods(Class clazz, MethodCallback mc, @Nullable MethodFilter mf) { + if (mf == USER_DECLARED_METHODS && clazz == Object.class) { + // nothing to introspect + return; + } Method[] methods = getDeclaredMethods(clazz, false); for (Method method : methods) { if (mf != null && !mf.matches(method)) { diff --git a/spring-core/src/test/java/org/springframework/util/ReflectionUtilsTests.java b/spring-core/src/test/java/org/springframework/util/ReflectionUtilsTests.java index 06e2df61444..ef089755251 100644 --- a/spring-core/src/test/java/org/springframework/util/ReflectionUtilsTests.java +++ b/spring-core/src/test/java/org/springframework/util/ReflectionUtilsTests.java @@ -28,6 +28,7 @@ import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.springframework.tests.sample.objects.TestObject; +import org.springframework.util.ReflectionUtils.MethodFilter; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; @@ -193,6 +194,32 @@ class ReflectionUtilsTests { .as("Public, not protected").doesNotContain("hashCode", "absquatulate"); } + @Test + void doWithMethodsUsingUserDeclaredMethodsFilterStartingWithObject() { + ListSavingMethodCallback mc = new ListSavingMethodCallback(); + ReflectionUtils.doWithMethods(Object.class, mc, ReflectionUtils.USER_DECLARED_METHODS); + assertThat(mc.getMethodNames()).isEmpty(); + } + + @Test + void doWithMethodsUsingUserDeclaredMethodsFilterStartingWithTestObject() { + ListSavingMethodCallback mc = new ListSavingMethodCallback(); + ReflectionUtils.doWithMethods(TestObject.class, mc, ReflectionUtils.USER_DECLARED_METHODS); + assertThat(mc.getMethodNames()) + .as("user declared methods").contains("absquatulate", "compareTo", "getName", "setName", "getAge", "setAge", "getSpouse", "setSpouse") + .as("methods on Object").doesNotContain("equals", "hashCode", "toString", "clone", "finalize", "getClass", "notify", "notifyAll", "wait"); + } + + @Test + void doWithMethodsUsingUserDeclaredMethodsComposedFilter() { + ListSavingMethodCallback mc = new ListSavingMethodCallback(); + // "q" because both absquatulate() and equals() contain "q" + MethodFilter isSetterMethodOrNameContainsQ = m -> m.getName().startsWith("set") || m.getName().contains("q"); + MethodFilter methodFilter = ReflectionUtils.USER_DECLARED_METHODS.and(isSetterMethodOrNameContainsQ); + ReflectionUtils.doWithMethods(TestObject.class, mc, methodFilter); + assertThat(mc.getMethodNames()).containsExactlyInAnyOrder("setName", "setAge", "setSpouse", "absquatulate"); + } + @Test void doWithMethodsFindsDuplicatesInClassHierarchy() { ListSavingMethodCallback mc = new ListSavingMethodCallback();