Support SpEL compilation for public methods in private subtypes

This commit ensures that methods are invoked via a public interface or
public superclass whenever possible when compiling SpEL expressions.

See spring-projectsgh-29857

Author: sbrannen

spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java

@@ -299,8 +299,10 @@ public boolean isCompilable() {
 			return false;
 		}
 
-		Class<?> clazz = executor.getMethod().getDeclaringClass();
-		return (Modifier.isPublic(clazz.getModifiers()) || executor.getPublicDeclaringClass() != null);
+		Method method = executor.getMethod();
+		return ((Modifier.isPublic(method.getModifiers()) &&
+				(Modifier.isPublic(method.getDeclaringClass().getModifiers()) ||
+						executor.getPublicDeclaringClass() != null)));
 	}
 
 	@Override
@@ -310,6 +312,18 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
 			throw new IllegalStateException("No applicable cached executor found: " + executorToCheck);
 		}
 		Method method = methodExecutor.getMethod();
+
+		Class<?> publicDeclaringClass;
+		if (Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
+			publicDeclaringClass = method.getDeclaringClass();
+		}
+		else {
+			publicDeclaringClass = methodExecutor.getPublicDeclaringClass();
+		}
+		Assert.state(publicDeclaringClass != null,
+				() -> "Failed to find public declaring class for method: " + method);
+
+		String classDesc = publicDeclaringClass.getName().replace('.', '/');
 		boolean isStatic = Modifier.isStatic(method.getModifiers());
 		String descriptor = cf.lastDescriptor();
 
@@ -339,24 +353,15 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
 			CodeFlow.insertBoxIfNecessary(mv, descriptor.charAt(0));
 		}
 
-		String classDesc;
-		if (Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
-			classDesc = method.getDeclaringClass().getName().replace('.', '/');
-		}
-		else {
-			Class<?> publicDeclaringClass = methodExecutor.getPublicDeclaringClass();
-			Assert.state(publicDeclaringClass != null, "No public declaring class");
-			classDesc = publicDeclaringClass.getName().replace('.', '/');
-		}
-
 		if (!isStatic && (descriptor == null || !descriptor.substring(1).equals(classDesc))) {
 			CodeFlow.insertCheckCast(mv, "L" + classDesc);
 		}
 
 		generateCodeForArguments(mv, cf, method, this.children);
-		int opcode = (isStatic ? INVOKESTATIC : method.isDefault() ? INVOKEINTERFACE : INVOKEVIRTUAL);
+		boolean isInterface = publicDeclaringClass.isInterface();
+		int opcode = (isStatic ? INVOKESTATIC : isInterface ? INVOKEINTERFACE : INVOKEVIRTUAL);
 		mv.visitMethodInsn(opcode, classDesc, method.getName(), CodeFlow.createSignatureDescriptor(method),
-				method.getDeclaringClass().isInterface());
+				isInterface);
 		cf.pushDescriptor(this.exitTypeDescriptor);
 
 		if (this.originalPrimitiveExitTypeDescriptor != null) {

spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java

@@ -21,7 +21,9 @@
 import java.lang.reflect.Array;
 import java.lang.reflect.Executable;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 
 import org.springframework.core.MethodParameter;
@@ -34,6 +36,7 @@
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
 import org.springframework.util.CollectionUtils;
+import org.springframework.util.ConcurrentReferenceHashMap;
 import org.springframework.util.MethodInvoker;
 
 /**
@@ -47,6 +50,14 @@
  */
 public abstract class ReflectionHelper {
 
+	/**
+	 * 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<Method, Class<?>> publicDeclaringClassCache = new ConcurrentReferenceHashMap<>(256);
+
+
 	/**
 	 * Compare argument arrays and return information about whether they match.
 	 * <p>A supplied type converter and conversionAllowed flag allow for matches to take
@@ -488,6 +499,66 @@ public static Object[] setupArgumentsForVarargsInvocation(Class<?>[] requiredPar
 		return args;
 	}
 
+	/**
+	 * Find the first public class or interface in the method's class hierarchy
+	 * that declares the supplied method.
+	 * <p>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;
+	}
+
 
 	/**
 	 * Arguments match kinds.

spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java

@@ -17,7 +17,6 @@
 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;
@@ -34,6 +33,7 @@
  *
  * @author Andy Clement
  * @author Juergen Hoeller
+ * @author Sam Brannen
  * @since 3.0
  */
 public class ReflectiveMethodExecutor implements MethodExecutor {
@@ -91,43 +91,22 @@ public final Method getMethod() {
 	}
 
 	/**
-	 * Find the first public class in the method's declaring class hierarchy that
-	 * declares this method.
-	 * <p>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 helper
-	 * 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 Object.
+	 * Find a public class or interface in the method's class hierarchy that
+	 * declares the {@linkplain #getMethod() original method}.
+	 * <p>See {@link ReflectionHelper#findPublicDeclaringClass(Method)} for
+	 * details.
+	 * @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 =
-					discoverPublicDeclaringClass(this.originalMethod, this.originalMethod.getDeclaringClass());
+			this.publicDeclaringClass = ReflectionHelper.findPublicDeclaringClass(this.originalMethod);
 			this.computedPublicDeclaringClass = true;
 		}
 		return this.publicDeclaringClass;
 	}
 
-	@Nullable
-	private Class<?> discoverPublicDeclaringClass(Method method, Class<?> clazz) {
-		if (Modifier.isPublic(clazz.getModifiers())) {
-			try {
-				clazz.getDeclaredMethod(method.getName(), method.getParameterTypes());
-				return clazz;
-			}
-			catch (NoSuchMethodException ex) {
-				// Continue below...
-			}
-		}
-		if (clazz.getSuperclass() != null) {
-			return discoverPublicDeclaringClass(method, clazz.getSuperclass());
-		}
-		return null;
-	}
-
 	public boolean didArgumentConversionOccur() {
 		return this.argumentConversionOccurred;
 	}

spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java

@@ -136,8 +136,8 @@ public boolean canRead(EvaluationContext context, @Nullable Object target, Strin
 			// 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 = ClassUtils.getInterfaceMethodIfPossible(method, type);
-			this.readerCache.put(cacheKey, new InvokerPair(method, typeDescriptor));
+			Method methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, type);
+			this.readerCache.put(cacheKey, new InvokerPair(methodToInvoke, typeDescriptor, method));
 			this.typeDescriptorCache.put(cacheKey, typeDescriptor);
 			return true;
 		}
@@ -171,22 +171,23 @@ public TypedValue read(EvaluationContext context, @Nullable Object target, Strin
 
 		if (invoker == null || invoker.member instanceof Method) {
 			Method method = (Method) (invoker != null ? invoker.member : null);
+			Method methodToInvoke = method;
 			if (method == null) {
 				method = findGetterForProperty(name, type, target);
 				if (method != null) {
 					// Treat it like a property...
 					// 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 = ClassUtils.getInterfaceMethodIfPossible(method, type);
-					invoker = new InvokerPair(method, typeDescriptor);
+					methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, type);
+					invoker = new InvokerPair(methodToInvoke, typeDescriptor, method);
 					this.readerCache.put(cacheKey, invoker);
 				}
 			}
-			if (method != null) {
+			if (methodToInvoke != null) {
 				try {
-					ReflectionUtils.makeAccessible(method);
-					Object value = method.invoke(target);
+					ReflectionUtils.makeAccessible(methodToInvoke);
+					Object value = methodToInvoke.invoke(target);
 					return new TypedValue(value, invoker.typeDescriptor.narrow(value));
 				}
 				catch (Exception ex) {
@@ -532,9 +533,9 @@ public PropertyAccessor createOptimalAccessor(EvaluationContext context, @Nullab
 				method = findGetterForProperty(name, type, target);
 				if (method != null) {
 					TypeDescriptor typeDescriptor = new TypeDescriptor(new MethodParameter(method, -1));
-					method = ClassUtils.getInterfaceMethodIfPossible(method, type);
-					invokerPair = new InvokerPair(method, typeDescriptor);
-					ReflectionUtils.makeAccessible(method);
+					Method methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, type);
+					invokerPair = new InvokerPair(methodToInvoke, typeDescriptor, method);
+					ReflectionUtils.makeAccessible(methodToInvoke);
 					this.readerCache.put(cacheKey, invokerPair);
 				}
 			}
@@ -572,8 +573,14 @@ private static boolean isKotlinProperty(Method method, String methodSuffix) {
 	/**
 	 * 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.
+	 * <p>The {@code originalMethod} is only used if the member is a method.
 	 */
-	private record InvokerPair(Member member, TypeDescriptor typeDescriptor) {}
+	private record InvokerPair(Member member, TypeDescriptor typeDescriptor, @Nullable Method originalMethod) {
+
+		InvokerPair(Member member, TypeDescriptor typeDescriptor) {
+			this(member, typeDescriptor, null);
+		}
+	}
 
 	private record PropertyCacheKey(Class<?> clazz, String property, boolean targetIsClass)
 			implements Comparable<PropertyCacheKey> {
@@ -606,9 +613,13 @@ public static class OptimalPropertyAccessor implements CompilablePropertyAccesso
 
 		private final TypeDescriptor typeDescriptor;
 
+		@Nullable
+		private final Method originalMethod;
+
 		OptimalPropertyAccessor(InvokerPair invokerPair) {
 			this.member = invokerPair.member;
 			this.typeDescriptor = invokerPair.typeDescriptor;
+			this.originalMethod = invokerPair.originalMethod;
 		}
 
 		@Override
@@ -677,8 +688,14 @@ public void write(EvaluationContext context, @Nullable Object target, String nam
 
 		@Override
 		public boolean isCompilable() {
-			return (Modifier.isPublic(this.member.getModifiers()) &&
-					Modifier.isPublic(this.member.getDeclaringClass().getModifiers()));
+			if (Modifier.isPublic(this.member.getModifiers()) &&
+					Modifier.isPublic(this.member.getDeclaringClass().getModifiers())) {
+				return true;
+			}
+			if (this.originalMethod != null) {
+				return (ReflectionHelper.findPublicDeclaringClass(this.originalMethod) != null);
+			}
+			return false;
 		}
 
 		@Override
@@ -693,9 +710,17 @@ public Class<?> getPropertyType() {
 
 		@Override
 		public void generateCode(String propertyName, MethodVisitor mv, CodeFlow cf) {
+			Class<?> publicDeclaringClass = this.member.getDeclaringClass();
+			if (this.originalMethod != null) {
+				publicDeclaringClass = ReflectionHelper.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));
+
+			String classDesc = publicDeclaringClass.getName().replace('.', '/');
 			boolean isStatic = Modifier.isStatic(this.member.getModifiers());
 			String descriptor = cf.lastDescriptor();
-			String classDesc = this.member.getDeclaringClass().getName().replace('.', '/');
 
 			if (!isStatic) {
 				if (descriptor == null) {
@@ -714,7 +739,7 @@ public void generateCode(String propertyName, MethodVisitor mv, CodeFlow cf) {
 			}
 
 			if (this.member instanceof Method method) {
-				boolean isInterface = method.getDeclaringClass().isInterface();
+				boolean isInterface = publicDeclaringClass.isInterface();
 				int opcode = (isStatic ? INVOKESTATIC : isInterface ? INVOKEINTERFACE : INVOKEVIRTUAL);
 				mv.visitMethodInsn(opcode, classDesc, method.getName(),
 						CodeFlow.createSignatureDescriptor(method), isInterface);

spring-expression/src/test/java/org/springframework/expression/spel/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.expression.spel;
+
+/**
+ * This is intentionally a top-level public interface.
+ */
+public interface PublicInterface {
+
+	String getText();
+
+}