From ba774c6290cf5a91329ed68e1ea60d0904bedbb4 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Tue, 27 Aug 2024 11:42:07 +0200 Subject: [PATCH] Restore behavior for ClassUtils.getInterfaceMethodIfPossible() Commit 47f88e123f introduced support for invoking init/destroy/SpEL methods via public types whenever possible. To achieve that, getInterfaceMethodIfPossible() was modified so that it only returned interface methods from public interfaces; however, we have learned that third parties relied on the previous behavior which found any interface method (even in non-public interfaces). In light of the above, this commit partially reverts commit 47f88e123f in order to reinstate getInterfaceMethodIfPossible() in non-deprecated form with its previous behavior. See gh-33216 --- .../org/springframework/util/ClassUtils.java | 37 ++- .../springframework/util/ClassUtilsTests.java | 285 ++++++++++++++---- 2 files changed, 247 insertions(+), 75 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/ClassUtils.java b/spring-core/src/main/java/org/springframework/util/ClassUtils.java index ee00015fa8a..40824be3000 100644 --- a/spring-core/src/main/java/org/springframework/util/ClassUtils.java +++ b/spring-core/src/main/java/org/springframework/util/ClassUtils.java @@ -134,11 +134,18 @@ public abstract class ClassUtils { */ private static final Set> javaLanguageInterfaces; + /** + * Cache for equivalent methods on a interface implemented by the declaring class. + *

A {@code null} value signals that no interface method was found for the key. + */ + private static final Map interfaceMethodCache = new ConcurrentReferenceHashMap<>(256); + /** * Cache for equivalent methods on a public interface implemented by the declaring class. *

A {@code null} value signals that no public interface method was found for the key. + * @since 6.2 */ - private static final Map interfaceMethodCache = new ConcurrentReferenceHashMap<>(256); + private static final Map publicInterfaceMethodCache = new ConcurrentReferenceHashMap<>(256); /** * Cache for equivalent public methods in a public declaring type within the type hierarchy @@ -1403,7 +1410,8 @@ public abstract class ClassUtils { * @param method the method to be invoked, potentially from an implementation class * @return the corresponding interface method, or the original method if none found * @since 5.1 - * @deprecated in favor of {@link #getPubliclyAccessibleMethodIfPossible(Method, Class)} + * @see #getPubliclyAccessibleMethodIfPossible(Method, Class) + * @deprecated in favor of {@link #getInterfaceMethodIfPossible(Method, Class)} */ @Deprecated public static Method getInterfaceMethodIfPossible(Method method) { @@ -1421,38 +1429,45 @@ public abstract class ClassUtils { * @since 5.3.16 * @see #getPubliclyAccessibleMethodIfPossible(Method, Class) * @see #getMostSpecificMethod - * @deprecated in favor of {@link #getPubliclyAccessibleMethodIfPossible(Method, Class)} */ - @Deprecated(since = "6.2") public static Method getInterfaceMethodIfPossible(Method method, @Nullable Class targetClass) { + return getInterfaceMethodIfPossible(method, targetClass, false); + } + + private static Method getInterfaceMethodIfPossible(Method method, @Nullable Class targetClass, + boolean requirePublicInterface) { + Class declaringClass = method.getDeclaringClass(); - if (!Modifier.isPublic(method.getModifiers()) || declaringClass.isInterface()) { + if (!Modifier.isPublic(method.getModifiers()) || (declaringClass.isInterface() && + (!requirePublicInterface || Modifier.isPublic(declaringClass.getModifiers())))) { return method; } String methodName = method.getName(); Class[] parameterTypes = method.getParameterTypes(); + Map methodCache = (requirePublicInterface ? publicInterfaceMethodCache : interfaceMethodCache); // Try cached version of method in its declaring class - Method result = interfaceMethodCache.computeIfAbsent(method, - key -> findInterfaceMethodIfPossible(methodName, parameterTypes, declaringClass, Object.class)); + Method result = methodCache.computeIfAbsent(method, key -> findInterfaceMethodIfPossible( + methodName, parameterTypes, declaringClass, Object.class, requirePublicInterface)); if (result == null && targetClass != null) { // No interface method found yet -> try given target class (possibly a subclass of the // declaring class, late-binding a base class method to a subclass-declared interface: // see e.g. HashMap.HashIterator.hasNext) - result = findInterfaceMethodIfPossible(methodName, parameterTypes, targetClass, declaringClass); + result = findInterfaceMethodIfPossible( + methodName, parameterTypes, targetClass, declaringClass, requirePublicInterface); } return (result != null ? result : method); } @Nullable private static Method findInterfaceMethodIfPossible(String methodName, Class[] parameterTypes, - Class startClass, Class endClass) { + Class startClass, Class endClass, boolean requirePublicInterface) { Class current = startClass; while (current != null && current != endClass) { for (Class ifc : current.getInterfaces()) { try { - if (Modifier.isPublic(ifc.getModifiers())) { + if (!requirePublicInterface || Modifier.isPublic(ifc.getModifiers())) { return ifc.getMethod(methodName, parameterTypes); } } @@ -1500,7 +1515,7 @@ public abstract class ClassUtils { return method; } - Method interfaceMethod = getInterfaceMethodIfPossible(method, targetClass); + Method interfaceMethod = getInterfaceMethodIfPossible(method, targetClass, true); // If we found a method in a public interface, return the interface method. if (!interfaceMethod.equals(method)) { return interfaceMethod; diff --git a/spring-core/src/test/java/org/springframework/util/ClassUtilsTests.java b/spring-core/src/test/java/org/springframework/util/ClassUtilsTests.java index bfc10a2d19a..ef73b41b001 100644 --- a/spring-core/src/test/java/org/springframework/util/ClassUtilsTests.java +++ b/spring-core/src/test/java/org/springframework/util/ClassUtilsTests.java @@ -509,6 +509,140 @@ class ClassUtilsTests { } + @Nested // gh-33216 + class GetInterfaceMethodTests { + + @Test + void publicMethodInPublicClass() throws Exception { + Class originalType = String.class; + Method originalMethod = originalType.getDeclaredMethod("getBytes"); + + Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(originalMethod, null); + assertThat(interfaceMethod.getDeclaringClass()).isEqualTo(originalType); + assertThat(interfaceMethod).isSameAs(originalMethod); + assertNotInterfaceMethod(interfaceMethod); + assertPubliclyAccessible(interfaceMethod); + } + + @Test + void publicMethodInNonPublicInterface() throws Exception { + Class originalType = PrivateInterface.class; + Method originalMethod = originalType.getDeclaredMethod("getMessage"); + + // Prerequisites for this use case: + assertPublic(originalMethod); + assertNotPublic(originalMethod.getDeclaringClass()); + + Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(originalMethod, null); + assertThat(interfaceMethod).isSameAs(originalMethod); + assertInterfaceMethod(interfaceMethod); + assertNotPubliclyAccessible(interfaceMethod); + } + + @Test + void publicInterfaceMethodInPublicClass() throws Exception { + Class originalType = ArrayList.class; + Method originalMethod = originalType.getDeclaredMethod("size"); + + Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(originalMethod, null); + assertThat(interfaceMethod.getDeclaringClass()).isEqualTo(List.class); + assertThat(interfaceMethod.getName()).isEqualTo("size"); + assertThat(interfaceMethod.getParameterTypes()).isEmpty(); + assertInterfaceMethod(interfaceMethod); + assertPubliclyAccessible(interfaceMethod); + } + + @Test + void publicInterfaceMethodDeclaredInNonPublicClassWithLateBindingOfClassMethodToSubclassDeclaredInterface() throws Exception { + HashMap hashMap = new HashMap<>(); + // Returns a package-private java.util.HashMap.KeyIterator which extends java.util.HashMap.HashIterator + // which declares hasNext(), even though HashIterator does not implement Iterator. Rather, KeyIterator + // implements HashIterator. + Iterator iterator = hashMap.keySet().iterator(); + Class targetClass = iterator.getClass(); + + // Prerequisites for this use case: + assertNotPublic(targetClass); + + Method originalMethod = targetClass.getMethod("hasNext"); + + Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(originalMethod, targetClass); + assertThat(interfaceMethod.getDeclaringClass()).isEqualTo(Iterator.class); + assertThat(interfaceMethod.getName()).isEqualTo("hasNext"); + assertThat(interfaceMethod.getParameterTypes()).isEmpty(); + assertInterfaceMethod(interfaceMethod); + assertPubliclyAccessible(interfaceMethod); + } + + @Test + void privateSubclassOverridesPropertyInPublicInterface() throws Exception { + Method originalMethod = PrivateSubclass.class.getDeclaredMethod("getText"); + + // Prerequisite: type must not be public for this use case. + assertNotPublic(originalMethod.getDeclaringClass()); + + Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(originalMethod, null); + assertThat(interfaceMethod.getDeclaringClass()).isEqualTo(PublicInterface.class); + assertThat(interfaceMethod.getName()).isEqualTo("getText"); + assertThat(interfaceMethod.getParameterTypes()).isEmpty(); + assertInterfaceMethod(interfaceMethod); + assertPubliclyAccessible(interfaceMethod); + } + + @Test + void privateSubclassOverridesPropertyInPrivateInterface() throws Exception { + Method originalMethod = PrivateSubclass.class.getDeclaredMethod("getMessage"); + + // Prerequisite: type must not be public for this use case. + assertNotPublic(originalMethod.getDeclaringClass()); + + Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(originalMethod, null); + assertThat(interfaceMethod.getDeclaringClass()).isEqualTo(PrivateInterface.class); + assertThat(interfaceMethod.getName()).isEqualTo("getMessage"); + assertThat(interfaceMethod.getParameterTypes()).isEmpty(); + assertInterfaceMethod(interfaceMethod); + assertNotPubliclyAccessible(interfaceMethod); + } + + @Test + void packagePrivateSubclassOverridesMethodInPublicInterface() throws Exception { + List unmodifiableList = Collections.unmodifiableList(Arrays.asList("foo", "bar")); + Class targetClass = unmodifiableList.getClass(); + + // Prerequisites for this use case: + assertNotPublic(targetClass); + + Method originalMethod = targetClass.getMethod("contains", Object.class); + + // Prerequisite: type must not be public for this use case. + assertNotPublic(originalMethod.getDeclaringClass()); + + Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(originalMethod, null); + assertThat(interfaceMethod.getDeclaringClass()).isEqualTo(Collection.class); + assertThat(interfaceMethod.getName()).isEqualTo("contains"); + assertThat(interfaceMethod.getParameterTypes()).containsExactly(Object.class); + assertInterfaceMethod(interfaceMethod); + assertPubliclyAccessible(interfaceMethod); + } + + @Test + void privateSubclassOverridesMethodInPrivateInterface() throws Exception { + Method originalMethod = PrivateSubclass.class.getMethod("greet", String.class); + + // Prerequisite: type must not be public for this use case. + assertNotPublic(originalMethod.getDeclaringClass()); + + Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(originalMethod, null); + assertThat(interfaceMethod.getDeclaringClass()).isEqualTo(PrivateInterface.class); + assertThat(interfaceMethod.getName()).isEqualTo("greet"); + assertThat(interfaceMethod.getParameterTypes()).containsExactly(String.class); + assertInterfaceMethod(interfaceMethod); + assertNotPubliclyAccessible(interfaceMethod); + } + + } + + @Nested // gh-33216 class GetPubliclyAccessibleMethodTests { @@ -526,7 +660,7 @@ class ClassUtilsTests { @Test // This method is intentionally public. - public void publicMethodInNonPublicType(TestInfo testInfo) { + public void publicMethodInNonPublicClass(TestInfo testInfo) { Method originalMethod = testInfo.getTestMethod().get(); // Prerequisites for this use case: @@ -539,7 +673,21 @@ class ClassUtilsTests { } @Test - void publicMethodInPublicType() throws Exception { + void publicMethodInNonPublicInterface() throws Exception { + Class originalType = PrivateInterface.class; + Method originalMethod = originalType.getDeclaredMethod("getMessage"); + + // Prerequisites for this use case: + assertPublic(originalMethod); + assertNotPublic(originalMethod.getDeclaringClass()); + + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod).isSameAs(originalMethod); + assertNotPubliclyAccessible(publiclyAccessibleMethod); + } + + @Test + void publicMethodInPublicClass() throws Exception { Class originalType = String.class; Method originalMethod = originalType.getDeclaredMethod("toString"); @@ -550,7 +698,7 @@ class ClassUtilsTests { } @Test - void publicInterfaceMethodInPublicType() throws Exception { + void publicInterfaceMethodInPublicClass() throws Exception { Class originalType = ArrayList.class; Method originalMethod = originalType.getDeclaredMethod("size"); @@ -562,7 +710,7 @@ class ClassUtilsTests { } @Test - void publicMethodInJavaLangObjectDeclaredInNonPublicType() throws Exception { + void publicMethodInJavaLangObjectDeclaredInNonPublicClass() throws Exception { List unmodifiableList = Collections.unmodifiableList(Arrays.asList("foo", "bar")); Class targetClass = unmodifiableList.getClass(); @@ -597,7 +745,7 @@ class ClassUtilsTests { } @Test - void publicInterfaceMethodDeclaredInNonPublicTypeWithLateBindingOfClassMethodToSubclassDeclaredInterface() throws Exception { + void publicInterfaceMethodDeclaredInNonPublicClassWithLateBindingOfClassMethodToSubclassDeclaredInterface() throws Exception { HashMap hashMap = new HashMap<>(); // Returns a package-private java.util.HashMap.KeyIterator which extends java.util.HashMap.HashIterator // which declares hasNext(), even though HashIterator does not implement Iterator. Rather, KeyIterator @@ -708,76 +856,50 @@ class ClassUtilsTests { assertPubliclyAccessible(publiclyAccessibleMethod); } - private static void assertPubliclyAccessible(Method method) { - assertPublic(method); - assertPublic(method.getDeclaringClass()); - } - - private static void assertNotPubliclyAccessible(Method method) { - assertThat(!isPublic(method) || !isPublic(method.getDeclaringClass())) - .as("%s must not be publicly accessible", method) - .isTrue(); - } - - private static void assertPublic(Member member) { - assertThat(isPublic(member)).as("%s must be public", member).isTrue(); - } - - private static void assertPublic(Class clazz) { - assertThat(isPublic(clazz)).as("%s must be public", clazz).isTrue(); - } - - private static void assertNotPublic(Member member) { - assertThat(!isPublic(member)).as("%s must be not be public", member).isTrue(); - } - - private static void assertNotPublic(Class clazz) { - assertThat(!isPublic(clazz)).as("%s must be not be public", clazz).isTrue(); - } - - private static boolean isPublic(Class clazz) { - return Modifier.isPublic(clazz.getModifiers()); - } + } - private static boolean isPublic(Member member) { - return Modifier.isPublic(member.getModifiers()); - } - private interface PrivateInterface { + private static void assertInterfaceMethod(Method method) { + assertThat(method.getDeclaringClass()).as("%s must be an interface method", method).isInterface(); + } - String getMessage(); + private static void assertNotInterfaceMethod(Method method) { + assertThat(method.getDeclaringClass()).as("%s must not be an interface method", method).isNotInterface(); + } - String greet(String name); - } + private static void assertPubliclyAccessible(Method method) { + assertPublic(method); + assertPublic(method.getDeclaringClass()); + } - private static class PrivateSubclass extends PublicSuperclass implements PublicInterface, PrivateInterface { + private static void assertNotPubliclyAccessible(Method method) { + assertThat(!isPublic(method) || !isPublic(method.getDeclaringClass())) + .as("%s must not be publicly accessible", method) + .isTrue(); + } - @Override - public int getNumber() { - return 2; - } + private static void assertPublic(Member member) { + assertThat(isPublic(member)).as("%s must be public", member).isTrue(); + } - @Override - public String getMessage() { - return "hello"; - } + private static void assertPublic(Class clazz) { + assertThat(isPublic(clazz)).as("%s must be public", clazz).isTrue(); + } - @Override - public String greet(String name) { - return "Hello, " + name; - } + private static void assertNotPublic(Member member) { + assertThat(!isPublic(member)).as("%s must be not be public", member).isTrue(); + } - @Override - public int process(int num) { - return num * 2; - } + private static void assertNotPublic(Class clazz) { + assertThat(!isPublic(clazz)).as("%s must be not be public", clazz).isTrue(); + } - @Override - public String getText() { - return "enigma"; - } - } + private static boolean isPublic(Class clazz) { + return Modifier.isPublic(clazz.getModifiers()); + } + private static boolean isPublic(Member member) { + return Modifier.isPublic(member.getModifiers()); } @@ -914,4 +1036,39 @@ class ClassUtilsTests { } + private interface PrivateInterface { + + String getMessage(); + + String greet(String name); + } + + private static class PrivateSubclass extends PublicSuperclass implements PublicInterface, PrivateInterface { + + @Override + public int getNumber() { + return 2; + } + + @Override + public String getMessage() { + return "hello"; + } + + @Override + public String greet(String name) { + return "Hello, " + name; + } + + @Override + public int process(int num) { + return num * 2; + } + + @Override + public String getText() { + return "enigma"; + } + } + }