Skip to content

Commit

Permalink
Support SpEL compilation for public methods in private subtypes
Browse files Browse the repository at this point in the history
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 spring-projectsgh-29857
  • Loading branch information
sbrannen committed Mar 9, 2024
1 parent 107f47c commit 65d7762
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 65d7762

Please sign in to comment.