Support SpEL compilation for public methods in private subtypes

Commit c79436f ensured that methods are invoked via a public
interface or public superclass when compiling Spring Expression
Language (SpEL) expressions involving method references or property
access (see MethodReference, PropertyOrFieldReference, and
collaborating support classes). However, compilation of expressions
that access properties by indexing into an object by property name is
still not properly supported in all scenarios.

To address those remaining use cases, this commit ensures that methods
are invoked via a public interface or public superclass when accessing
a property by indexing into an object by the property name – for
example, `person['name']` instead of `person.name`.

In addition, SpEL's Indexer now properly relies on the
CompilablePropertyAccessor abstraction instead of hard-coding support
for only OptimalPropertyAccessor. This greatly reduces the complexity
of the Indexer and simultaneously allows the Indexer to potentially
support other CompilablePropertyAccessor implementations.

Closes gh-29857

Author: sbrannen

spring-expression/src/main/java/org/springframework/expression/spel/CompilablePropertyAccessor.java

@@ -19,6 +19,7 @@
 import org.springframework.asm.MethodVisitor;
 import org.springframework.asm.Opcodes;
 import org.springframework.expression.PropertyAccessor;
+import org.springframework.lang.Nullable;
 
 /**
  * A compilable {@link PropertyAccessor} is able to generate bytecode that represents
@@ -41,13 +42,17 @@ public interface CompilablePropertyAccessor extends PropertyAccessor, Opcodes {
 	Class<?> getPropertyType();
 
 	/**
-	 * Generate the bytecode the performs the access operation into the specified
+	 * Generate the bytecode that performs the access operation into the specified
 	 * {@link MethodVisitor} using context information from the {@link CodeFlow}
 	 * where necessary.
-	 * @param propertyName the name of the property
+	 * <p>Concrete implementations of {@code CompilablePropertyAccessor} typically
+	 * have access to the property name via other means (for example, supplied as
+	 * an argument when they were instantiated). Thus, the {@code propertyName}
+	 * supplied to this method may be {@code null}.
+	 * @param propertyName the name of the property, or {@code null} if not available
 	 * @param methodVisitor the ASM method visitor into which code should be generated
 	 * @param codeFlow the current state of the expression compiler
 	 */
-	void generateCode(String propertyName, MethodVisitor methodVisitor, CodeFlow codeFlow);
+	void generateCode(@Nullable String propertyName, MethodVisitor methodVisitor, CodeFlow codeFlow);
 
 }

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

@@ -17,10 +17,6 @@
 package org.springframework.expression.spel.ast;
 
 import java.lang.reflect.Constructor;
-import java.lang.reflect.Field;
-import java.lang.reflect.Member;
-import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
@@ -35,6 +31,7 @@
 import org.springframework.expression.TypeConverter;
 import org.springframework.expression.TypedValue;
 import org.springframework.expression.spel.CodeFlow;
+import org.springframework.expression.spel.CompilablePropertyAccessor;
 import org.springframework.expression.spel.ExpressionState;
 import org.springframework.expression.spel.SpelEvaluationException;
 import org.springframework.expression.spel.SpelMessage;
@@ -223,10 +220,11 @@ else if (this.indexedType == IndexedType.MAP) {
 			return (this.children[0] instanceof PropertyOrFieldReference || this.children[0].isCompilable());
 		}
 		else if (this.indexedType == IndexedType.OBJECT) {
-			// If the string name is changing, the accessor is clearly going to change (so no compilation possible)
-			return (this.cachedReadAccessor != null &&
-					this.cachedReadAccessor instanceof ReflectivePropertyAccessor.OptimalPropertyAccessor &&
-					getChild(0) instanceof StringLiteral);
+			// If the string name is changing, the accessor is clearly going to change.
+			// So compilation is only possible if the index expression is a StringLiteral.
+			return (getChild(0) instanceof StringLiteral &&
+					this.cachedReadAccessor instanceof CompilablePropertyAccessor compilablePropertyAccessor &&
+					compilablePropertyAccessor.isCompilable());
 		}
 		return false;
 	}
@@ -315,30 +313,9 @@ else if (this.indexedType == IndexedType.MAP) {
 		}
 
 		else if (this.indexedType == IndexedType.OBJECT) {
-			ReflectivePropertyAccessor.OptimalPropertyAccessor accessor =
-					(ReflectivePropertyAccessor.OptimalPropertyAccessor) this.cachedReadAccessor;
-			Assert.state(accessor != null, "No cached read accessor");
-			Member member = accessor.member;
-			boolean isStatic = Modifier.isStatic(member.getModifiers());
-			String classDesc = member.getDeclaringClass().getName().replace('.', '/');
-
-			if (!isStatic) {
-				if (descriptor == null) {
-					cf.loadTarget(mv);
-				}
-				if (descriptor == null || !classDesc.equals(descriptor.substring(1))) {
-					mv.visitTypeInsn(CHECKCAST, classDesc);
-				}
-			}
-
-			if (member instanceof Method method) {
-				mv.visitMethodInsn((isStatic? INVOKESTATIC : INVOKEVIRTUAL), classDesc, member.getName(),
-						CodeFlow.createSignatureDescriptor(method), false);
-			}
-			else {
-				mv.visitFieldInsn((isStatic ? GETSTATIC : GETFIELD), classDesc, member.getName(),
-						CodeFlow.toJvmDescriptor(((Field) member).getType()));
-			}
+			CompilablePropertyAccessor compilablePropertyAccessor = (CompilablePropertyAccessor) this.cachedReadAccessor;
+			Assert.state(compilablePropertyAccessor != null, "No cached read accessor");
+			compilablePropertyAccessor.generateCode(null, mv, cf);
 		}
 
 		cf.pushDescriptor(this.exitTypeDescriptor);
@@ -600,10 +577,8 @@ public TypedValue getValue() {
 						Indexer.this.cachedReadAccessor = accessor;
 						Indexer.this.cachedReadName = this.name;
 						Indexer.this.cachedReadTargetType = targetObjectRuntimeClass;
-						if (accessor instanceof ReflectivePropertyAccessor.OptimalPropertyAccessor optimalAccessor) {
-							Member member = optimalAccessor.member;
-							Indexer.this.exitTypeDescriptor = CodeFlow.toDescriptor(member instanceof Method method ?
-									method.getReturnType() : ((Field) member).getType());
+						if (accessor instanceof CompilablePropertyAccessor compilablePropertyAccessor) {
+							Indexer.this.exitTypeDescriptor = CodeFlow.toDescriptor(compilablePropertyAccessor.getPropertyType());
 						}
 						return accessor.read(this.evaluationContext, this.targetObject, this.name);
 					}

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

@@ -712,7 +712,7 @@ public Class<?> getPropertyType() {
 		}
 
 		@Override
-		public void generateCode(String propertyName, MethodVisitor mv, CodeFlow cf) {
+		public void generateCode(@Nullable String propertyName, MethodVisitor mv, CodeFlow cf) {
 			Class<?> publicDeclaringClass = this.member.getDeclaringClass();
 			if (!Modifier.isPublic(publicDeclaringClass.getModifiers()) && this.originalMethod != null) {
 				publicDeclaringClass = ReflectionHelper.findPublicDeclaringClass(this.originalMethod);

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

@@ -764,6 +764,54 @@ void privateSubclassOverridesPropertyInPublicSuperclass() {
 			assertThat(result).isEqualTo(2);
 		}
 
+		@Test
+		void indexIntoPropertyInPrivateSubclassThatOverridesPropertyInPublicInterface() {
+			expression = parser.parseExpression("#root['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 indexIntoPropertyInPrivateSubclassThatOverridesPropertyInPrivateInterface() {
+			expression = parser.parseExpression("#root['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 indexIntoPropertyInPrivateSubclassThatOverridesPropertyInPublicSuperclass() {
+			expression = parser.parseExpression("#root['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();