diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGenerator.java b/spring-beans/src/main/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGenerator.java index 91cd2a14b98..49d7d8083db 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGenerator.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGenerator.java @@ -176,9 +176,9 @@ class BeanDefinitionPropertiesCodeGenerator { Method method = ReflectionUtils.findMethod(methodDeclaringClass, methodName); if (method != null) { this.hints.reflection().registerMethod(method, ExecutableMode.INVOKE); - Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(method, beanUserClass); - if (!interfaceMethod.equals(method)) { - this.hints.reflection().registerMethod(interfaceMethod, ExecutableMode.INVOKE); + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, beanUserClass); + if (!publiclyAccessibleMethod.equals(method)) { + this.hints.reflection().registerMethod(publiclyAccessibleMethod, ExecutableMode.INVOKE); } } } diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java index 3a460f41a20..d1ade588ec1 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java @@ -1899,7 +1899,7 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac if (logger.isTraceEnabled()) { logger.trace("Invoking init method '" + methodName + "' on bean with name '" + beanName + "'"); } - Method methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(initMethod, beanClass); + Method methodToInvoke = ClassUtils.getPubliclyAccessibleMethodIfPossible(initMethod, beanClass); try { ReflectionUtils.makeAccessible(methodToInvoke); diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/DisposableBeanAdapter.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/DisposableBeanAdapter.java index 14198c4b2f1..f95fd951e3f 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/DisposableBeanAdapter.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/DisposableBeanAdapter.java @@ -147,7 +147,7 @@ class DisposableBeanAdapter implements DisposableBean, Runnable, Serializable { beanName + "' has a non-boolean parameter - not supported as destroy method"); } } - destroyMethod = ClassUtils.getInterfaceMethodIfPossible(destroyMethod, bean.getClass()); + destroyMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(destroyMethod, bean.getClass()); destroyMethods.add(destroyMethod); } } @@ -253,8 +253,8 @@ class DisposableBeanAdapter implements DisposableBean, Runnable, Serializable { for (String destroyMethodName : this.destroyMethodNames) { Method destroyMethod = determineDestroyMethod(destroyMethodName); if (destroyMethod != null) { - invokeCustomDestroyMethod( - ClassUtils.getInterfaceMethodIfPossible(destroyMethod, this.bean.getClass())); + destroyMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(destroyMethod, this.bean.getClass()); + invokeCustomDestroyMethod(destroyMethod); } } } diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGeneratorTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGeneratorTests.java index 7a9f52e0e59..2fc485a5abd 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGeneratorTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGeneratorTests.java @@ -631,7 +631,7 @@ class BeanDefinitionPropertiesCodeGeneratorTests { } - interface Initializable { + public interface Initializable { void initialize(); } @@ -643,7 +643,7 @@ class BeanDefinitionPropertiesCodeGeneratorTests { } } - interface Disposable { + public interface Disposable { void dispose(); } 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 e191a0552dd..cab2e454bb4 100644 --- a/spring-core/src/main/java/org/springframework/util/ClassUtils.java +++ b/spring-core/src/main/java/org/springframework/util/ClassUtils.java @@ -135,10 +135,17 @@ public abstract class ClassUtils { private static final Set> javaLanguageInterfaces; /** - * Cache for equivalent methods on an interface implemented by the declaring class. + * Cache for equivalent methods on a public interface implemented by the declaring class. */ private static final Map interfaceMethodCache = new ConcurrentReferenceHashMap<>(256); + /** + * Cache for equivalent public methods in a public declaring type within the type hierarchy + * of the method's declaring class. + * @since 6.2 + */ + private static final Map publiclyAccessibleMethodCache = new ConcurrentReferenceHashMap<>(256); + static { primitiveWrapperTypeMap.put(Boolean.class, boolean.class); @@ -1394,7 +1401,7 @@ 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 #getInterfaceMethodIfPossible(Method, Class)} + * @deprecated in favor of {@link #getPubliclyAccessibleMethodIfPossible(Method, Class)} */ @Deprecated public static Method getInterfaceMethodIfPossible(Method method) { @@ -1407,37 +1414,45 @@ public abstract class ClassUtils { * Module System which allows the method to be invoked via reflection without an illegal * access warning. * @param method the method to be invoked, potentially from an implementation class - * @param targetClass the target class to check for declared interfaces + * @param targetClass the target class to invoke the method on, or {@code null} if unknown * @return the corresponding interface method, or the original method if none found * @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) { - if (!Modifier.isPublic(method.getModifiers()) || method.getDeclaringClass().isInterface()) { + Class declaringClass = method.getDeclaringClass(); + if (!Modifier.isPublic(method.getModifiers()) || declaringClass.isInterface()) { return method; } + String methodName = method.getName(); + Class[] parameterTypes = method.getParameterTypes(); + // Try cached version of method in its declaring class Method result = interfaceMethodCache.computeIfAbsent(method, - key -> findInterfaceMethodIfPossible(key, key.getParameterTypes(), key.getDeclaringClass(), - Object.class)); - if (result == method && targetClass != null) { + key -> findInterfaceMethodIfPossible(methodName, parameterTypes, declaringClass, Object.class)); + 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(method, method.getParameterTypes(), targetClass, - method.getDeclaringClass()); + result = findInterfaceMethodIfPossible(methodName, parameterTypes, targetClass, declaringClass); } - return result; + return (result != null ? result : method); } - private static Method findInterfaceMethodIfPossible(Method method, Class[] parameterTypes, + @Nullable + private static Method findInterfaceMethodIfPossible(String methodName, Class[] parameterTypes, Class startClass, Class endClass) { Class current = startClass; while (current != null && current != endClass) { for (Class ifc : current.getInterfaces()) { try { - return ifc.getMethod(method.getName(), parameterTypes); + if (Modifier.isPublic(ifc.getModifiers())) { + return ifc.getMethod(methodName, parameterTypes); + } } catch (NoSuchMethodException ex) { // ignore @@ -1445,7 +1460,72 @@ public abstract class ClassUtils { } current = current.getSuperclass(); } - return method; + return null; + } + + /** + * Get the first publicly accessible method in the supplied method's type hierarchy that + * has a method signature equivalent to the supplied method, if possible. + *

If the supplied method is {@code public} and declared in a {@code public} type, + * the supplied method will be returned. + *

Otherwise, this method recursively searches the class hierarchy and implemented + * interfaces for an equivalent method that is {@code public} and declared in a + * {@code public} type. + *

If a publicly accessible equivalent method cannot be found, the supplied method + * will be returned, indicating that no such equivalent method exists. Consequently, + * callers of this method must manually validate the accessibility of the returned method + * if public access is a requirement. + *

This is particularly useful for arriving at a public exported type on the Java + * Module System which allows the method to be invoked via reflection without an illegal + * access warning. This is also useful for invoking methods via a public API in bytecode + * — for example, for use with the Spring Expression Language (SpEL) compiler. + * For example, if a non-public class overrides {@code toString()}, this method will + * traverse up the type hierarchy to find the first public type that declares the method + * (if there is one). For {@code toString()}, it may traverse as far as {@link Object}. + * @param method the method to be invoked, potentially from an implementation class + * @param targetClass the target class to invoke the method on, or {@code null} if unknown + * @return the corresponding publicly accessible method, or the original method if none + * found + * @since 6.2 + * @see #getInterfaceMethodIfPossible(Method, Class) + * @see #getMostSpecificMethod(Method, Class) + */ + public static Method getPubliclyAccessibleMethodIfPossible(Method method, @Nullable Class targetClass) { + Class declaringClass = method.getDeclaringClass(); + // If the method is not public, we can abort the search immediately; or if the method's + // declaring class is public, the method is already publicly accessible. + if (!Modifier.isPublic(method.getModifiers()) || Modifier.isPublic(declaringClass.getModifiers())) { + return method; + } + + Method interfaceMethod = getInterfaceMethodIfPossible(method, targetClass); + // If we found a method in a public interface, return the interface method. + if (!interfaceMethod.equals(method)) { + return interfaceMethod; + } + + Method result = publiclyAccessibleMethodCache.computeIfAbsent(method, + key -> findPubliclyAccessibleMethodIfPossible(key.getName(), key.getParameterTypes(), declaringClass)); + return (result != null ? result : method); + } + + @Nullable + private static Method findPubliclyAccessibleMethodIfPossible( + String methodName, Class[] parameterTypes, Class declaringClass) { + + Class current = declaringClass.getSuperclass(); + while (current != null) { + if (Modifier.isPublic(current.getModifiers())) { + try { + return current.getDeclaredMethod(methodName, parameterTypes); + } + catch (NoSuchMethodException ex) { + // ignore + } + } + current = current.getSuperclass(); + } + return null; } /** 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 dd834e75dfa..bfc10a2d19a 100644 --- a/spring-core/src/test/java/org/springframework/util/ClassUtilsTests.java +++ b/spring-core/src/test/java/org/springframework/util/ClassUtilsTests.java @@ -23,12 +23,17 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Member; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.lang.reflect.Proxy; +import java.time.ZoneId; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; import java.util.List; import java.util.Set; import java.util.function.Supplier; @@ -37,6 +42,7 @@ import a.ClassHavingNestedClass; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; @@ -503,6 +509,278 @@ class ClassUtilsTests { } + @Nested // gh-33216 + class GetPubliclyAccessibleMethodTests { + + @Test + void nonPublicMethod(TestInfo testInfo) { + Method originalMethod = testInfo.getTestMethod().get(); + + // Prerequisites for this use case: + assertNotPublic(originalMethod); + + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod).isSameAs(originalMethod); + assertNotPubliclyAccessible(publiclyAccessibleMethod); + } + + @Test + // This method is intentionally public. + public void publicMethodInNonPublicType(TestInfo testInfo) { + Method originalMethod = testInfo.getTestMethod().get(); + + // Prerequisites for this use case: + assertPublic(originalMethod); + assertNotPublic(originalMethod.getDeclaringClass()); + + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod).isSameAs(originalMethod); + assertNotPubliclyAccessible(publiclyAccessibleMethod); + } + + @Test + void publicMethodInPublicType() throws Exception { + Class originalType = String.class; + Method originalMethod = originalType.getDeclaredMethod("toString"); + + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(originalType); + assertThat(publiclyAccessibleMethod).isSameAs(originalMethod); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @Test + void publicInterfaceMethodInPublicType() throws Exception { + Class originalType = ArrayList.class; + Method originalMethod = originalType.getDeclaredMethod("size"); + + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + // Should not find the interface method in List. + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(originalType); + assertThat(publiclyAccessibleMethod).isSameAs(originalMethod); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @Test + void publicMethodInJavaLangObjectDeclaredInNonPublicType() 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("toString"); + + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(Object.class); + assertThat(publiclyAccessibleMethod.getName()).isEqualTo("toString"); + assertThat(publiclyAccessibleMethod.getParameterTypes()).isEmpty(); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @Test + void publicMethodInJavaTimeZoneIdDeclaredInNonPublicSubclass() throws Exception { + // Returns a package-private java.time.ZoneRegion. + ZoneId zoneId = ZoneId.of("CET"); + Class targetClass = zoneId.getClass(); + + // Prerequisites for this use case: + assertNotPublic(targetClass); + + Method originalMethod = targetClass.getDeclaredMethod("getId"); + + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(ZoneId.class); + assertThat(publiclyAccessibleMethod.getName()).isEqualTo("getId"); + assertThat(publiclyAccessibleMethod.getParameterTypes()).isEmpty(); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @Test + void publicInterfaceMethodDeclaredInNonPublicTypeWithLateBindingOfClassMethodToSubclassDeclaredInterface() 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 publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, targetClass); + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(Iterator.class); + assertThat(publiclyAccessibleMethod.getName()).isEqualTo("hasNext"); + assertThat(publiclyAccessibleMethod.getParameterTypes()).isEmpty(); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @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 publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(PublicInterface.class); + assertThat(publiclyAccessibleMethod.getName()).isEqualTo("getText"); + assertThat(publiclyAccessibleMethod.getParameterTypes()).isEmpty(); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @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 publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + // Should not find the interface method in PrivateInterface. + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(PublicSuperclass.class); + assertThat(publiclyAccessibleMethod.getName()).isEqualTo("getMessage"); + assertThat(publiclyAccessibleMethod.getParameterTypes()).isEmpty(); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @Test + void privateSubclassOverridesPropertyInPublicSuperclass() throws Exception { + Method originalMethod = PrivateSubclass.class.getDeclaredMethod("getNumber"); + + // Prerequisite: type must not be public for this use case. + assertNotPublic(originalMethod.getDeclaringClass()); + + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(PublicSuperclass.class); + assertThat(publiclyAccessibleMethod.getName()).isEqualTo("getNumber"); + assertThat(publiclyAccessibleMethod.getParameterTypes()).isEmpty(); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @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 publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(Collection.class); + assertThat(publiclyAccessibleMethod.getName()).isEqualTo("contains"); + assertThat(publiclyAccessibleMethod.getParameterTypes()).containsExactly(Object.class); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @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 publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(PublicSuperclass.class); + assertThat(publiclyAccessibleMethod.getName()).isEqualTo("greet"); + assertThat(publiclyAccessibleMethod.getParameterTypes()).containsExactly(String.class); + assertPubliclyAccessible(publiclyAccessibleMethod); + } + + @Test + void privateSubclassOverridesMethodInPublicSuperclass() throws Exception { + Method originalMethod = PrivateSubclass.class.getMethod("process", int.class); + + // Prerequisite: type must not be public for this use case. + assertNotPublic(originalMethod.getDeclaringClass()); + + Method publiclyAccessibleMethod = ClassUtils.getPubliclyAccessibleMethodIfPossible(originalMethod, null); + assertThat(publiclyAccessibleMethod.getDeclaringClass()).isEqualTo(PublicSuperclass.class); + assertThat(publiclyAccessibleMethod.getName()).isEqualTo("process"); + assertThat(publiclyAccessibleMethod.getParameterTypes()).containsExactly(int.class); + 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 { + + 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"; + } + } + + } + + @Target(ElementType.METHOD) @Retention(RetentionPolicy.RUNTIME) @ValueSource(classes = { Boolean.class, Character.class, Byte.class, Short.class, diff --git a/spring-core/src/test/java/org/springframework/util/PublicInterface.java b/spring-core/src/test/java/org/springframework/util/PublicInterface.java new file mode 100644 index 00000000000..7812b38459c --- /dev/null +++ b/spring-core/src/test/java/org/springframework/util/PublicInterface.java @@ -0,0 +1,26 @@ +/* + * Copyright 2002-2024 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.util; + +/** + * This is intentionally a top-level public interface. + */ +public interface PublicInterface { + + String getText(); + +} diff --git a/spring-core/src/test/java/org/springframework/util/PublicSuperclass.java b/spring-core/src/test/java/org/springframework/util/PublicSuperclass.java new file mode 100644 index 00000000000..e976d9b26ba --- /dev/null +++ b/spring-core/src/test/java/org/springframework/util/PublicSuperclass.java @@ -0,0 +1,40 @@ +/* + * Copyright 2002-2024 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.util; + +/** + * This is intentionally a top-level public class. + */ +public class PublicSuperclass { + + public String getMessage() { + return "goodbye"; + } + + public int getNumber() { + return 1; + } + + public String greet(String name) { + return "Super, " + name; + } + + public int process(int num) { + return num + 1; + } + +} diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java index d9d3bbcbbfa..7a3d4489467 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java @@ -18,12 +18,10 @@ package org.springframework.expression.spel; import java.lang.reflect.Constructor; import java.lang.reflect.Method; -import java.lang.reflect.Modifier; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Deque; import java.util.List; -import java.util.Map; import org.springframework.asm.ClassWriter; import org.springframework.asm.MethodVisitor; @@ -31,9 +29,7 @@ import org.springframework.asm.Opcodes; import org.springframework.lang.Contract; import org.springframework.lang.Nullable; import org.springframework.util.Assert; -import org.springframework.util.ClassUtils; import org.springframework.util.CollectionUtils; -import org.springframework.util.ConcurrentReferenceHashMap; /** * Manages the class being generated by the compilation process. @@ -49,14 +45,6 @@ import org.springframework.util.ConcurrentReferenceHashMap; */ public class CodeFlow implements Opcodes { - /** - * Cache for equivalent methods in a public declaring class in the type - * hierarchy of the method's declaring class. - * @since 6.2 - */ - private static final Map> publicDeclaringClassCache = new ConcurrentReferenceHashMap<>(256); - - /** * Name of the class being generated. Typically used when generating code * that accesses freshly generated fields on the generated type. @@ -479,66 +467,6 @@ public class CodeFlow implements Opcodes { } } - /** - * Find the first public class or interface in the method's class hierarchy - * that declares the supplied method. - *

Sometimes the reflective method discovery logic finds a suitable method - * that can easily be called via reflection but cannot be called from generated - * code when compiling the expression because of visibility restrictions. For - * example, if a non-public class overrides {@code toString()}, this method - * will traverse up the type hierarchy to find the first public type that - * declares the method (if there is one). For {@code toString()}, it may - * traverse as far as {@link Object}. - * @param method the method to process - * @return the public class or interface that declares the method, or - * {@code null} if no such public type could be found - * @since 6.2 - */ - @Nullable - public static Class findPublicDeclaringClass(Method method) { - return publicDeclaringClassCache.computeIfAbsent(method, key -> { - // If the method is already defined in a public type, return that type. - if (Modifier.isPublic(key.getDeclaringClass().getModifiers())) { - return key.getDeclaringClass(); - } - Method interfaceMethod = ClassUtils.getInterfaceMethodIfPossible(key, null); - // If we found an interface method whose type is public, return the interface type. - if (!interfaceMethod.equals(key)) { - if (Modifier.isPublic(interfaceMethod.getDeclaringClass().getModifiers())) { - return interfaceMethod.getDeclaringClass(); - } - } - // Attempt to search the type hierarchy. - Class superclass = key.getDeclaringClass().getSuperclass(); - if (superclass != null) { - return findPublicDeclaringClass(superclass, key.getName(), key.getParameterTypes()); - } - // Otherwise, no public declaring class found. - return null; - }); - } - - @Nullable - private static Class findPublicDeclaringClass( - Class declaringClass, String methodName, Class[] parameterTypes) { - - if (Modifier.isPublic(declaringClass.getModifiers())) { - try { - declaringClass.getDeclaredMethod(methodName, parameterTypes); - return declaringClass; - } - catch (NoSuchMethodException ex) { - // Continue below... - } - } - - Class superclass = declaringClass.getSuperclass(); - if (superclass != null) { - return findPublicDeclaringClass(superclass, methodName, parameterTypes); - } - return null; - } - /** * Create the JVM signature descriptor for a method. This consists of the descriptors * for the method parameters surrounded with parentheses, followed by the diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java index 5006a03a493..e8d15e5b053 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java @@ -301,9 +301,7 @@ public class MethodReference extends SpelNodeImpl { } Method method = executor.getMethod(); - return ((Modifier.isPublic(method.getModifiers()) && - (Modifier.isPublic(method.getDeclaringClass().getModifiers()) || - executor.getPublicDeclaringClass() != null))); + return (Modifier.isPublic(method.getModifiers()) && executor.getPublicDeclaringClass() != null); } @Override @@ -314,13 +312,7 @@ public class MethodReference extends SpelNodeImpl { } Method method = methodExecutor.getMethod(); - Class publicDeclaringClass; - if (Modifier.isPublic(method.getDeclaringClass().getModifiers())) { - publicDeclaringClass = method.getDeclaringClass(); - } - else { - publicDeclaringClass = methodExecutor.getPublicDeclaringClass(); - } + Class publicDeclaringClass = methodExecutor.getPublicDeclaringClass(); Assert.state(publicDeclaringClass != null, () -> "Failed to find public declaring class for method: " + method); diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveIndexAccessor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveIndexAccessor.java index abf0ec7505a..601f2792c29 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveIndexAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveIndexAccessor.java @@ -159,7 +159,7 @@ public class ReflectiveIndexAccessor implements CompilableIndexAccessor { .formatted(readMethodName, getName(indexType), getName(targetType))); } - this.readMethodToInvoke = ClassUtils.getInterfaceMethodIfPossible(this.readMethod, targetType); + this.readMethodToInvoke = ClassUtils.getPubliclyAccessibleMethodIfPossible(this.readMethod, targetType); ReflectionUtils.makeAccessible(this.readMethodToInvoke); if (writeMethodName != null) { @@ -173,7 +173,7 @@ public class ReflectiveIndexAccessor implements CompilableIndexAccessor { .formatted(writeMethodName, getName(indexType), getName(indexedValueType), getName(targetType))); } - this.writeMethodToInvoke = ClassUtils.getInterfaceMethodIfPossible(writeMethod, targetType); + this.writeMethodToInvoke = ClassUtils.getPubliclyAccessibleMethodIfPossible(writeMethod, targetType); ReflectionUtils.makeAccessible(this.writeMethodToInvoke); } else { @@ -250,10 +250,7 @@ public class ReflectiveIndexAccessor implements CompilableIndexAccessor { public void generateCode(SpelNode index, MethodVisitor mv, CodeFlow cf) { // Find the public declaring class. Class publicDeclaringClass = this.readMethodToInvoke.getDeclaringClass(); - if (!Modifier.isPublic(publicDeclaringClass.getModifiers())) { - publicDeclaringClass = CodeFlow.findPublicDeclaringClass(this.readMethod); - } - Assert.state(publicDeclaringClass != null && Modifier.isPublic(publicDeclaringClass.getModifiers()), + Assert.state(Modifier.isPublic(publicDeclaringClass.getModifiers()), () -> "Failed to find public declaring class for read-method: " + this.readMethod); String classDesc = publicDeclaringClass.getName().replace('.', '/'); diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java index f93bea65503..b66312328f9 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java @@ -17,6 +17,7 @@ package org.springframework.expression.spel.support; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import org.springframework.core.MethodParameter; import org.springframework.core.convert.TypeDescriptor; @@ -24,7 +25,6 @@ import org.springframework.expression.AccessException; import org.springframework.expression.EvaluationContext; import org.springframework.expression.MethodExecutor; import org.springframework.expression.TypedValue; -import org.springframework.expression.spel.CodeFlow; import org.springframework.lang.Nullable; import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; @@ -42,18 +42,15 @@ public class ReflectiveMethodExecutor implements MethodExecutor { private final Method originalMethod; /** - * The method to invoke via reflection, which is not necessarily the method - * to invoke in a compiled expression. + * The method to invoke via reflection or in a compiled expression. */ private final Method methodToInvoke; @Nullable private final Integer varargsPosition; - private boolean computedPublicDeclaringClass = false; - @Nullable - private Class publicDeclaringClass; + private final Class publicDeclaringClass; private boolean argumentConversionOccurred = false; @@ -69,18 +66,15 @@ public class ReflectiveMethodExecutor implements MethodExecutor { /** * Create a new executor for the given method. * @param method the method to invoke - * @param targetClass the target class to invoke the method on + * @param targetClass the target class to invoke the method on, or {@code null} if unknown * @since 5.3.16 */ public ReflectiveMethodExecutor(Method method, @Nullable Class targetClass) { this.originalMethod = method; - this.methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, targetClass); - if (method.isVarArgs()) { - this.varargsPosition = method.getParameterCount() - 1; - } - else { - this.varargsPosition = null; - } + this.methodToInvoke = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, targetClass); + Class declaringClass = this.methodToInvoke.getDeclaringClass(); + this.publicDeclaringClass = (Modifier.isPublic(declaringClass.getModifiers()) ? declaringClass : null); + this.varargsPosition = (method.isVarArgs() ? method.getParameterCount() - 1 : null); } @@ -92,19 +86,15 @@ public class ReflectiveMethodExecutor implements MethodExecutor { } /** - * Find a public class or interface in the method's class hierarchy that - * declares the {@linkplain #getMethod() original method}. - *

See {@link CodeFlow#findPublicDeclaringClass(Method)} for + * Get the public class or interface in the method's type hierarchy that declares the + * {@linkplain #getMethod() original method}. + *

See {@link ClassUtils#getPubliclyAccessibleMethodIfPossible(Method, Class)} for * details. - * @return the public class or interface that declares the method, or - * {@code null} if no such public type could be found + * @return the public class or interface that declares the method, or {@code null} if + * no such public type could be found */ @Nullable public Class getPublicDeclaringClass() { - if (!this.computedPublicDeclaringClass) { - this.publicDeclaringClass = CodeFlow.findPublicDeclaringClass(this.originalMethod); - this.computedPublicDeclaringClass = true; - } return this.publicDeclaringClass; } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java index 52ca54de86f..b7b9644a8e1 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java @@ -136,8 +136,8 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { // The readerCache will only contain gettable properties (let's not worry about setters for now). Property property = new Property(type, method, null); TypeDescriptor typeDescriptor = new TypeDescriptor(property); - Method methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, type); - this.readerCache.put(cacheKey, new InvokerPair(methodToInvoke, typeDescriptor, method)); + Method methodToInvoke = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, type); + this.readerCache.put(cacheKey, new InvokerPair(methodToInvoke, typeDescriptor)); this.typeDescriptorCache.put(cacheKey, typeDescriptor); return true; } @@ -180,8 +180,8 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { // The readerCache will only contain gettable properties (let's not worry about setters for now). Property property = new Property(type, method, null); TypeDescriptor typeDescriptor = new TypeDescriptor(property); - methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, type); - invoker = new InvokerPair(methodToInvoke, typeDescriptor, method); + methodToInvoke = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, type); + invoker = new InvokerPair(methodToInvoke, typeDescriptor); this.readerCache.put(cacheKey, invoker); } } @@ -238,7 +238,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { // Treat it like a property Property property = new Property(type, null, method); TypeDescriptor typeDescriptor = new TypeDescriptor(property); - method = ClassUtils.getInterfaceMethodIfPossible(method, type); + method = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, type); this.writerCache.put(cacheKey, method); this.typeDescriptorCache.put(cacheKey, typeDescriptor); return true; @@ -287,7 +287,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { if (method == null) { method = findSetterForProperty(name, type, target); if (method != null) { - method = ClassUtils.getInterfaceMethodIfPossible(method, type); + method = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, type); cachedMember = method; this.writerCache.put(cacheKey, cachedMember); } @@ -512,7 +512,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { * field) to use each time {@link #read(EvaluationContext, Object, String)} * is called. *

This method will return this {@code ReflectivePropertyAccessor} instance - * if it is unable to build a optimized accessor. + * if it is unable to build an optimized accessor. *

Note: An optimized accessor is currently only usable for read attempts. * Do not call this method if you need a read-write accessor. */ @@ -536,9 +536,9 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { method = findGetterForProperty(name, type, target); if (method != null) { TypeDescriptor typeDescriptor = new TypeDescriptor(new MethodParameter(method, -1)); - Method methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, type); - invokerPair = new InvokerPair(methodToInvoke, typeDescriptor, method); + Method methodToInvoke = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, type); ReflectionUtils.makeAccessible(methodToInvoke); + invokerPair = new InvokerPair(methodToInvoke, typeDescriptor); this.readerCache.put(cacheKey, invokerPair); } } @@ -552,8 +552,8 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { if (field == null) { field = findField(name, type, target instanceof Class); if (field != null) { - invokerPair = new InvokerPair(field, new TypeDescriptor(field)); ReflectionUtils.makeAccessible(field); + invokerPair = new InvokerPair(field, new TypeDescriptor(field)); this.readerCache.put(cacheKey, invokerPair); } } @@ -576,13 +576,8 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { /** * Captures the member (method/field) to call reflectively to access a property value * and the type descriptor for the value returned by the reflective call. - *

The {@code originalMethod} is only used if the member is a method. */ - private record InvokerPair(Member member, TypeDescriptor typeDescriptor, @Nullable Method originalMethod) { - - InvokerPair(Member member, TypeDescriptor typeDescriptor) { - this(member, typeDescriptor, null); - } + private record InvokerPair(Member member, TypeDescriptor typeDescriptor) { } private record PropertyCacheKey(Class clazz, String property, boolean targetIsClass) @@ -617,16 +612,10 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { private final TypeDescriptor typeDescriptor; - /** - * The original method, or {@code null} if the member is not a method. - */ - @Nullable - private final Method originalMethod; OptimalPropertyAccessor(InvokerPair invokerPair) { this.member = invokerPair.member; this.typeDescriptor = invokerPair.typeDescriptor; - this.originalMethod = invokerPair.originalMethod; } @Override @@ -695,14 +684,8 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { @Override public boolean isCompilable() { - if (Modifier.isPublic(this.member.getModifiers()) && - Modifier.isPublic(this.member.getDeclaringClass().getModifiers())) { - return true; - } - if (this.originalMethod != null) { - return (CodeFlow.findPublicDeclaringClass(this.originalMethod) != null); - } - return false; + return (Modifier.isPublic(this.member.getModifiers()) && + Modifier.isPublic(this.member.getDeclaringClass().getModifiers())); } @Override @@ -718,12 +701,8 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { @Override public void generateCode(String propertyName, MethodVisitor mv, CodeFlow cf) { Class publicDeclaringClass = this.member.getDeclaringClass(); - if (!Modifier.isPublic(publicDeclaringClass.getModifiers()) && this.originalMethod != null) { - publicDeclaringClass = CodeFlow.findPublicDeclaringClass(this.originalMethod); - } - Assert.state(publicDeclaringClass != null && Modifier.isPublic(publicDeclaringClass.getModifiers()), - () -> "Failed to find public declaring class for: " + - (this.originalMethod != null ? this.originalMethod : this.member)); + Assert.state(Modifier.isPublic(publicDeclaringClass.getModifiers()), + () -> "Failed to find public declaring class for: " + this.member); String classDesc = publicDeclaringClass.getName().replace('.', '/'); boolean isStatic = Modifier.isStatic(this.member.getModifiers()); diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java index 3da7456189d..e2f8f79b4aa 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java @@ -21,6 +21,8 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.List; import org.junit.jupiter.api.Test; @@ -402,12 +404,32 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void methodOfClass() { + void methodOfJavaLangClass() { Expression expression = parser.parseExpression("getName()"); Object value = expression.getValue(new StandardEvaluationContext(String.class)); assertThat(value).isEqualTo("java.lang.String"); } + @Test + void methodOfJavaLangObject() { + Expression expression = parser.parseExpression("toString()"); + Object value = expression.getValue(new StandardEvaluationContext(42)); + assertThat(value).isEqualTo("42"); + } + + @Test // gh-33216 + void methodOfJavaLangObjectDeclaredInNonPublicType() throws Exception { + Expression expression = parser.parseExpression("toString()"); + List unmodifiableList = Collections.unmodifiableList(Arrays.asList("foo", "bar")); + Method toStringMethod = unmodifiableList.getClass().getMethod("toString"); + + // Prerequisite for this use case: + assertThat(toStringMethod.getDeclaringClass()).isPackagePrivate(); + + Object value = expression.getValue(new StandardEvaluationContext(unmodifiableList)); + assertThat(value).isEqualTo(unmodifiableList.toString()); + } + @Test void invokeMethodWithoutConversion() { final BytesService service = new BytesService(); diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java index d763088b7ce..24850844da5 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java @@ -16,6 +16,7 @@ package org.springframework.expression.spel; +import java.time.ZoneId; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -145,12 +146,27 @@ class PropertyAccessTests extends AbstractExpressionTests { } @Test - void accessingPropertyOfClass() { + void accessingPropertyOfJavaLangClass() { Expression expression = parser.parseExpression("name"); Object value = expression.getValue(new StandardEvaluationContext(String.class)); assertThat(value).isEqualTo("java.lang.String"); } + @Test // gh-33216 + void accessingPropertyOfJavaTimeZoneIdDeclaredInNonPublicSubclass() throws Exception { + String ID = "CET"; + ZoneId zoneId = ZoneId.of(ID); + + // Prerequisites for this use case: + assertThat(zoneId.getClass()).isPackagePrivate(); + assertThat(zoneId.getId()).isEqualTo(ID); + + Expression expression = parser.parseExpression("id"); + + String result = expression.getValue(new StandardEvaluationContext(zoneId), String.class); + assertThat(result).isEqualTo(ID); + } + @Test void shouldAlwaysUsePropertyAccessorFromEvaluationContext() { SpelExpressionParser parser = new SpelExpressionParser();