Support SpEL compilation for public methods in private subtypes

Although the Spring Expression Language (SpEL) generally does a good
job of locating the public declaring class or interface on which to
invoke a method in a compiled expression, prior to this commit there
were still a few unsupported use cases.

To address those remaining use cases, this commit ensures that methods
are invoked via a public interface or public superclass whenever
possible when compiling SpEL expressions.

See gh-29857

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

@@ -299,8 +299,10 @@ public class MethodReference extends SpelNodeImpl {
 			return false;
 		}
 
-		Class<?> clazz = executor.getMethod().getDeclaringClass();
+		Method method = executor.getMethod();
-		return (Modifier.isPublic(clazz.getModifiers()) || executor.getPublicDeclaringClass() != null);
+		return ((Modifier.isPublic(method.getModifiers()) &&
+				(Modifier.isPublic(method.getDeclaringClass().getModifiers()) ||
+						executor.getPublicDeclaringClass() != null)));
 	}
 
 	@Override
@@ -310,6 +312,18 @@ public class MethodReference extends SpelNodeImpl {
 			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 class MethodReference extends SpelNodeImpl {
 			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.invoke.MethodType;
 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.lang.Nullable;
 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 @@ import org.springframework.util.MethodInvoker;
  */
 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 abstract class ReflectionHelper {
 		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 @@ import org.springframework.util.ReflectionUtils;
  *
  * @author Andy Clement
  * @author Juergen Hoeller
+ * @author Sam Brannen
  * @since 3.0
  */
 public class ReflectiveMethodExecutor implements MethodExecutor {
@@ -91,43 +91,22 @@ public class ReflectiveMethodExecutor implements MethodExecutor {
 	}
 
 	/**
-	 * Find the first public class in the method's declaring class hierarchy that
+	 * Find a public class or interface in the method's class hierarchy that
-	 * declares this method.
+	 * declares the {@linkplain #getMethod() original method}.
-	 * <p>Sometimes the reflective method discovery logic finds a suitable method
+	 * <p>See {@link ReflectionHelper#findPublicDeclaringClass(Method)} for
-	 * that can easily be called via reflection but cannot be called from generated
+	 * details.
-	 * code when compiling the expression because of visibility restrictions. For
+	 * @return the public class or interface that declares the method, or
-	 * example, if a non-public class overrides {@code toString()}, this helper
+	 * {@code null} if no such public type could be found
-	 * 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.
 	 */
 	@Nullable
 	public Class<?> getPublicDeclaringClass() {
 		if (!this.computedPublicDeclaringClass) {
-			this.publicDeclaringClass =
+			this.publicDeclaringClass = ReflectionHelper.findPublicDeclaringClass(this.originalMethod);
-					discoverPublicDeclaringClass(this.originalMethod, this.originalMethod.getDeclaringClass());
 			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 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 = ClassUtils.getInterfaceMethodIfPossible(method, type);
+			Method methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, type);
-			this.readerCache.put(cacheKey, new InvokerPair(method, typeDescriptor));
+			this.readerCache.put(cacheKey, new InvokerPair(methodToInvoke, typeDescriptor, method));
 			this.typeDescriptorCache.put(cacheKey, typeDescriptor);
 			return true;
 		}
@@ -171,6 +171,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 
 		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) {
@@ -178,15 +179,15 @@ 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 = ClassUtils.getInterfaceMethodIfPossible(method, type);
+					methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, type);
-					invoker = new InvokerPair(method, typeDescriptor);
+					invoker = new InvokerPair(methodToInvoke, typeDescriptor, method);
 					this.readerCache.put(cacheKey, invoker);
 				}
 			}
-			if (method != null) {
+			if (methodToInvoke != null) {
 				try {
-					ReflectionUtils.makeAccessible(method);
+					ReflectionUtils.makeAccessible(methodToInvoke);
-					Object value = method.invoke(target);
+					Object value = methodToInvoke.invoke(target);
 					return new TypedValue(value, invoker.typeDescriptor.narrow(value));
 				}
 				catch (Exception ex) {
@@ -532,9 +533,9 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 				method = findGetterForProperty(name, type, target);
 				if (method != null) {
 					TypeDescriptor typeDescriptor = new TypeDescriptor(new MethodParameter(method, -1));
-					method = ClassUtils.getInterfaceMethodIfPossible(method, type);
+					Method methodToInvoke = ClassUtils.getInterfaceMethodIfPossible(method, type);
-					invokerPair = new InvokerPair(method, typeDescriptor);
+					invokerPair = new InvokerPair(methodToInvoke, typeDescriptor, method);
-					ReflectionUtils.makeAccessible(method);
+					ReflectionUtils.makeAccessible(methodToInvoke);
 					this.readerCache.put(cacheKey, invokerPair);
 				}
 			}
@@ -572,8 +573,14 @@ 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.
+	 * <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 class ReflectivePropertyAccessor implements PropertyAccessor {
 
 		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 class ReflectivePropertyAccessor implements PropertyAccessor {
 
 		@Override
 		public boolean isCompilable() {
-			return (Modifier.isPublic(this.member.getModifiers()) &&
+			if (Modifier.isPublic(this.member.getModifiers()) &&
-					Modifier.isPublic(this.member.getDeclaringClass().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 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 = 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 class ReflectivePropertyAccessor implements PropertyAccessor {
 			}
 
 			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();
+
+}

spring-expression/src/test/java/org/springframework/expression/spel/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.expression.spel;
+
+/**
+ * This is intentionally a top-level public class.
+ */
+public class PublicSuperclass {
+
+	public int process(int num) {
+		return num + 1;
+	}
+
+	public int getNumber() {
+		return 1;
+	}
+
+	public String getMessage() {
+		return "goodbye";
+	}
+
+	public String greet(String name) {
+		return "Super, " + name;
+	}
+
+}

spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java

@@ -42,6 +42,7 @@ import org.springframework.expression.EvaluationContext;
 import org.springframework.expression.Expression;
 import org.springframework.expression.TypedValue;
 import org.springframework.expression.spel.ast.CompoundExpression;
+import org.springframework.expression.spel.ast.InlineList;
 import org.springframework.expression.spel.ast.OpLT;
 import org.springframework.expression.spel.ast.SpelNodeImpl;
 import org.springframework.expression.spel.ast.Ternary;
@@ -678,6 +679,158 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 
 	}
 
+	@Nested
+	class PropertyVisibilityTests {
+
+		@Test
+		void privateSubclassOverridesPropertyInPublicInterface() {
+			expression = parser.parseExpression("text");
+			PrivateSubclass privateSubclass = new PrivateSubclass();
+
+			// Prerequisite: type must not be public for this use case.
+			assertNotPublic(privateSubclass.getClass());
+
+			String result = expression.getValue(context, privateSubclass, String.class);
+			assertThat(result).isEqualTo("enigma");
+
+			assertCanCompile(expression);
+			result = expression.getValue(context, privateSubclass, String.class);
+			assertThat(result).isEqualTo("enigma");
+		}
+
+		@Test
+		void privateSubclassOverridesPropertyInPrivateInterface() {
+			expression = parser.parseExpression("message");
+			PrivateSubclass privateSubclass = new PrivateSubclass();
+
+			// Prerequisite: type must not be public for this use case.
+			assertNotPublic(privateSubclass.getClass());
+
+			String result = expression.getValue(context, privateSubclass, String.class);
+			assertThat(result).isEqualTo("hello");
+
+			assertCanCompile(expression);
+			result = expression.getValue(context, privateSubclass, String.class);
+			assertThat(result).isEqualTo("hello");
+		}
+
+		@Test
+		void privateSubclassOverridesPropertyInPublicSuperclass() {
+			expression = parser.parseExpression("number");
+			PrivateSubclass privateSubclass = new PrivateSubclass();
+
+			// Prerequisite: type must not be public for this use case.
+			assertNotPublic(privateSubclass.getClass());
+
+			Integer result = expression.getValue(context, privateSubclass, Integer.class);
+			assertThat(result).isEqualTo(2);
+
+			assertCanCompile(expression);
+			result = expression.getValue(context, privateSubclass, Integer.class);
+			assertThat(result).isEqualTo(2);
+		}
+
+		private interface PrivateInterface {
+
+			String getMessage();
+		}
+
+		private static class PrivateSubclass extends PublicSuperclass implements PublicInterface, PrivateInterface {
+
+			@Override
+			public int getNumber() {
+				return 2;
+			}
+
+			@Override
+			public String getText() {
+				return "enigma";
+			}
+
+			@Override
+			public String getMessage() {
+				return "hello";
+			}
+		}
+	}
+
+	@Nested
+	class MethodVisibilityTests {
+
+		/**
+		 * Note that {@link InlineList} creates a list and wraps it via
+		 * {@link Collections#unmodifiableList(List)}, whose concrete type is
+		 * package private.
+		 */
+		@Test
+		void packagePrivateSubclassOverridesMethodInPublicInterface() {
+			expression = parser.parseExpression("{2021, 2022}");
+			List<?> inlineList = expression.getValue(List.class);
+
+			// Prerequisite: type must not be public for this use case.
+			assertNotPublic(inlineList.getClass());
+
+			expression = parser.parseExpression("{2021, 2022}.contains(2022)");
+			Boolean result = expression.getValue(context, Boolean.class);
+			assertThat(result).isTrue();
+
+			assertCanCompile(expression);
+			result = expression.getValue(context, Boolean.class);
+			assertThat(result).isTrue();
+		}
+
+		@Test
+		void packagePrivateSubclassOverridesMethodInPrivateInterface() {
+			expression = parser.parseExpression("greet('Jane')");
+			PrivateSubclass privateSubclass = new PrivateSubclass();
+
+			// Prerequisite: type must not be public for this use case.
+			assertNotPublic(privateSubclass.getClass());
+
+			String result = expression.getValue(context, privateSubclass, String.class);
+			assertThat(result).isEqualTo("Hello, Jane");
+
+			assertCanCompile(expression);
+			result = expression.getValue(context, privateSubclass, String.class);
+			assertThat(result).isEqualTo("Hello, Jane");
+		}
+
+		@Test
+		void privateSubclassOverridesMethodInPublicSuperclass() {
+			expression = parser.parseExpression("process(2)");
+			PrivateSubclass privateSubclass = new PrivateSubclass();
+
+			// Prerequisite: type must not be public for this use case.
+			assertNotPublic(privateSubclass.getClass());
+
+			Integer result = expression.getValue(context, privateSubclass, Integer.class);
+			assertThat(result).isEqualTo(2 * 2);
+
+			assertCanCompile(expression);
+			result = expression.getValue(context, privateSubclass, Integer.class);
+			assertThat(result).isEqualTo(2 * 2);
+		}
+
+		private interface PrivateInterface {
+
+			String greet(String name);
+		}
+
+		private static class PrivateSubclass extends PublicSuperclass implements PrivateInterface {
+
+			@Override
+			public int process(int num) {
+				return num * 2;
+			}
+
+			@Override
+			public String greet(String name) {
+				return "Hello, " + name;
+			}
+		}
+	}
+
+
 	@Test
 	void typeReference() {
 		expression = parse("T(String)");