From 96844a0757d68d0ba4386bc07d11747e36df0859 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sun, 3 Mar 2024 16:44:00 +0100 Subject: [PATCH] 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 gh-29857 --- .../expression/spel/ast/MethodReference.java | 33 ++-- .../spel/support/ReflectionHelper.java | 71 ++++++++ .../support/ReflectiveMethodExecutor.java | 37 +---- .../support/ReflectivePropertyAccessor.java | 55 +++++-- .../expression/spel/PublicInterface.java | 26 +++ .../expression/spel/PublicSuperclass.java | 40 +++++ .../spel/SpelCompilationCoverageTests.java | 153 ++++++++++++++++++ 7 files changed, 357 insertions(+), 58 deletions(-) create mode 100644 spring-expression/src/test/java/org/springframework/expression/spel/PublicInterface.java create mode 100644 spring-expression/src/test/java/org/springframework/expression/spel/PublicSuperclass.java diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java index 2bd823376c41..00be16e8556a 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java +++ b/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) { diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java index 375f9f6163c8..2df7a155a987 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java +++ b/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> publicDeclaringClassCache = new ConcurrentReferenceHashMap<>(256); + + /** * Compare argument arrays and return information about whether they match. *

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. + *

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. diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java index d155b6dd5789..8b93e69135b4 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java +++ b/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. - *

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}. + *

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; } 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 c761b45671db..547dcb861246 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 @@ -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,6 +171,7 @@ 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) { @@ -178,15 +179,15 @@ public TypedValue read(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); - 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. + *

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 { @@ -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); diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/PublicInterface.java b/spring-expression/src/test/java/org/springframework/expression/spel/PublicInterface.java new file mode 100644 index 000000000000..587a86769b03 --- /dev/null +++ b/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(); + +} diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/PublicSuperclass.java b/spring-expression/src/test/java/org/springframework/expression/spel/PublicSuperclass.java new file mode 100644 index 000000000000..0c8a2f8d54b5 --- /dev/null +++ b/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; + } + +} 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 22e9e7d3e109..9b7ec2e94583 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 @@ -42,6 +42,7 @@ 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 @@ else if (object instanceof int[] ints) { } + @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)");