From 65d77624d1864db8b5355024cbbaa925488c295b Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sat, 9 Mar 2024 13:49:10 +0100 Subject: [PATCH] Support SpEL compilation for public methods in private subtypes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit c79436f832 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 --- .../spel/CompilablePropertyAccessor.java | 11 +++-- .../expression/spel/ast/Indexer.java | 47 +++++------------- .../support/ReflectivePropertyAccessor.java | 2 +- .../spel/SpelCompilationCoverageTests.java | 48 +++++++++++++++++++ 4 files changed, 68 insertions(+), 40 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/CompilablePropertyAccessor.java b/spring-expression/src/main/java/org/springframework/expression/spel/CompilablePropertyAccessor.java index 6651bd68887..0792cb1d506 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/CompilablePropertyAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/CompilablePropertyAccessor.java @@ -19,6 +19,7 @@ package org.springframework.expression.spel; 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 + *

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); } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java index 5205b24eb12..85665adf501 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java +++ b/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.PropertyAccessor; 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 @@ public class Indexer extends SpelNodeImpl { 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 @@ public class Indexer extends SpelNodeImpl { } 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 class Indexer extends SpelNodeImpl { 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); } 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 322a275c62b..307984d7e73 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 @@ -712,7 +712,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { } @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); diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java index c7eced339d9..d19f50f00e6 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java @@ -764,6 +764,54 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { 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();