From b64d7529064e56b3d573c93bb9b466ef92c25dcd Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Sat, 12 Dec 2015 00:50:02 +0100 Subject: [PATCH] Fix SpEL compilation of static method/property/field operations Before this change the compilation of a method reference or property/field access was not properly cleaning up the stack if compilation meant calling a static method or accessing a static field. In these cases there is no need for a target object on the stack and it should be removed if present. For a simple expression it is harmless since the end result of the expression is the thing on the top of the stack, but for nested expressions if the inner expression suffered this issue, the outer expression can find itself operating on the wrong element. The particular issue covered the case of a static field access but this fix (and associated tests) cover static method, property and field access. Issue: SPR-13781 (cherry picked from commit a28fc76) --- .../spel/ast/CompoundExpression.java | 12 +- .../expression/spel/ast/MethodReference.java | 39 +++--- .../support/ReflectivePropertyAccessor.java | 98 +++++++------- .../spel/SpelCompilationCoverageTests.java | 122 ++++++++++++++++++ 4 files changed, 199 insertions(+), 72 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.java index c934c631828..6d739ed30c8 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2015 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. @@ -62,7 +62,7 @@ public class CompoundExpression extends SpelNodeImpl { } try { state.pushActiveContextObject(result); - nextNode = this.children[cc-1]; + nextNode = this.children[cc - 1]; return nextNode.getValueRef(state); } finally { @@ -124,14 +124,8 @@ public class CompoundExpression extends SpelNodeImpl { @Override public void generateCode(MethodVisitor mv, CodeFlow cf) { - // TODO could optimize T(SomeType).staticMethod - no need to generate the T() part for (int i = 0; i < this.children.length;i++) { - SpelNodeImpl child = this.children[i]; - if (child instanceof TypeReference && (i + 1) < this.children.length && - this.children[i + 1] instanceof MethodReference) { - continue; - } - child.generateCode(mv, cf); + this.children[i].generateCode(mv, cf); } cf.pushDescriptor(this.exitTypeDescriptor); } 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 98b9e6f85ca..e7884d7e6aa 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2015 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. @@ -287,35 +287,40 @@ public class MethodReference extends SpelNodeImpl { throw new IllegalStateException("No applicable cached executor found: " + executorToCheck); } - ReflectiveMethodExecutor methodExecutor = (ReflectiveMethodExecutor)executorToCheck.get(); + ReflectiveMethodExecutor methodExecutor = (ReflectiveMethodExecutor) executorToCheck.get(); Method method = methodExecutor.getMethod(); boolean isStaticMethod = Modifier.isStatic(method.getModifiers()); String descriptor = cf.lastDescriptor(); - if (descriptor == null && !isStaticMethod) { - cf.loadTarget(mv); + if (descriptor == null) { + if (!isStaticMethod) { + // Nothing on the stack but something is needed + cf.loadTarget(mv); + } + } + else { + if (isStaticMethod) { + // Something on the stack when nothing is needed + mv.visitInsn(POP); + } } if (CodeFlow.isPrimitive(descriptor)) { CodeFlow.insertBoxIfNecessary(mv, descriptor.charAt(0)); } - boolean itf = method.getDeclaringClass().isInterface(); - String methodDeclaringClassSlashedDescriptor = null; - if (Modifier.isPublic(method.getDeclaringClass().getModifiers())) { - methodDeclaringClassSlashedDescriptor = method.getDeclaringClass().getName().replace('.', '/'); - } - else { - methodDeclaringClassSlashedDescriptor = methodExecutor.getPublicDeclaringClass().getName().replace('.', '/'); - } + String classDesc = (Modifier.isPublic(method.getDeclaringClass().getModifiers()) ? + method.getDeclaringClass().getName().replace('.', '/') : + methodExecutor.getPublicDeclaringClass().getName().replace('.', '/')); if (!isStaticMethod) { - if (descriptor == null || !descriptor.substring(1).equals(methodDeclaringClassSlashedDescriptor)) { - CodeFlow.insertCheckCast(mv, "L"+ methodDeclaringClassSlashedDescriptor); + if (descriptor == null || !descriptor.substring(1).equals(classDesc)) { + CodeFlow.insertCheckCast(mv, "L" + classDesc); } } - generateCodeForArguments(mv, cf, method, children); - mv.visitMethodInsn(isStaticMethod ? INVOKESTATIC : INVOKEVIRTUAL, - methodDeclaringClassSlashedDescriptor, method.getName(), CodeFlow.createSignatureDescriptor(method), itf); + + generateCodeForArguments(mv, cf, method, this.children); + mv.visitMethodInsn((isStaticMethod ? INVOKESTATIC : INVOKEVIRTUAL), classDesc, method.getName(), + CodeFlow.createSignatureDescriptor(method), method.getDeclaringClass().isInterface()); cf.pushDescriptor(this.exitTypeDescriptor); } 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 ea805a331d4..4a3a5cfe82e 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2015 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. @@ -58,7 +58,10 @@ import org.springframework.util.StringUtils; */ public class ReflectivePropertyAccessor implements PropertyAccessor { + private static final Set> ANY_TYPES = Collections.emptySet(); + private static final Set> BOOLEAN_TYPES; + static { Set> booleanTypes = new HashSet>(); booleanTypes.add(Boolean.class); @@ -66,8 +69,6 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { BOOLEAN_TYPES = Collections.unmodifiableSet(booleanTypes); } - private static final Set> ANY_TYPES = Collections.emptySet(); - private final Map readerCache = new ConcurrentHashMap(64); @@ -120,9 +121,9 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { } return false; } - + public Member getLastReadInvokerPair() { - return lastReadInvokerPair.member; + return this.lastReadInvokerPair.member; } @Override @@ -165,7 +166,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { return new TypedValue(value, invoker.typeDescriptor.narrow(value)); } catch (Exception ex) { - throw new AccessException("Unable to access property '" + name + "' through getter", ex); + throw new AccessException("Unable to access property '" + name + "' through getter method", ex); } } } @@ -187,12 +188,12 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { return new TypedValue(value, invoker.typeDescriptor.narrow(value)); } catch (Exception ex) { - throw new AccessException("Unable to access field: " + name, ex); + throw new AccessException("Unable to access field '" + name + "'", ex); } } } - throw new AccessException("Neither getter nor field found for property '" + name + "'"); + throw new AccessException("Neither getter method nor field found for property '" + name + "'"); } @Override @@ -240,7 +241,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { newValue, TypeDescriptor.forObject(newValue), typeDescriptor); } catch (EvaluationException evaluationException) { - throw new AccessException("Type conversion failure",evaluationException); + throw new AccessException("Type conversion failure", evaluationException); } } CacheKey cacheKey = new CacheKey(type, name, target instanceof Class); @@ -262,7 +263,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { return; } catch (Exception ex) { - throw new AccessException("Unable to access property '" + name + "' through setter", ex); + throw new AccessException("Unable to access property '" + name + "' through setter method", ex); } } } @@ -283,12 +284,12 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { return; } catch (Exception ex) { - throw new AccessException("Unable to access field: " + name, ex); + throw new AccessException("Unable to access field '" + name + "'", ex); } } } - throw new AccessException("Neither setter nor field found for property '" + name + "'"); + throw new AccessException("Neither setter method nor field found for property '" + name + "'"); } private TypeDescriptor getTypeDescriptor(EvaluationContext context, Object target, String name) { @@ -469,11 +470,11 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { InvokerPair invocationTarget = this.readerCache.get(cacheKey); if (invocationTarget == null || invocationTarget.member instanceof Method) { - Method method = (Method) (invocationTarget==null?null:invocationTarget.member); + Method method = (Method) (invocationTarget != null ? invocationTarget.member : null); if (method == null) { method = findGetterForProperty(name, type, target); if (method != null) { - invocationTarget = new InvokerPair(method,new TypeDescriptor(new MethodParameter(method,-1))); + invocationTarget = new InvokerPair(method, new TypeDescriptor(new MethodParameter(method, -1))); ReflectionUtils.makeAccessible(method); this.readerCache.put(cacheKey, invocationTarget); } @@ -497,6 +498,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { return new OptimalPropertyAccessor(invocationTarget); } } + return this; } @@ -577,16 +579,8 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { OptimalPropertyAccessor(InvokerPair target) { this.member = target.member; this.typeDescriptor = target.typeDescriptor; - if (this.member instanceof Field) { - Field field = (Field) this.member; - this.needsToBeMadeAccessible = (!Modifier.isPublic(field.getModifiers()) || - !Modifier.isPublic(field.getDeclaringClass().getModifiers())) && !field.isAccessible(); - } - else { - Method method = (Method) this.member; - this.needsToBeMadeAccessible = ((!Modifier.isPublic(method.getModifiers()) || - !Modifier.isPublic(method.getDeclaringClass().getModifiers())) && !method.isAccessible()); - } + this.needsToBeMadeAccessible = (!Modifier.isPublic(this.member.getModifiers()) || + !Modifier.isPublic(this.member.getDeclaringClass().getModifiers())); } @Override @@ -599,10 +593,12 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { if (target == null) { return false; } + Class type = (target instanceof Class ? (Class) target : target.getClass()); if (type.isArray()) { return false; } + if (this.member instanceof Method) { Method method = (Method) this.member; String getterName = "get" + StringUtils.capitalize(name); @@ -621,30 +617,31 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { @Override public TypedValue read(EvaluationContext context, Object target, String name) throws AccessException { if (this.member instanceof Method) { + Method method = (Method) this.member; try { - if (this.needsToBeMadeAccessible) { - ReflectionUtils.makeAccessible((Method) this.member); + if (this.needsToBeMadeAccessible && !method.isAccessible()) { + method.setAccessible(true); } - Object value = ((Method) this.member).invoke(target); + Object value = method.invoke(target); return new TypedValue(value, this.typeDescriptor.narrow(value)); } catch (Exception ex) { - throw new AccessException("Unable to access property '" + name + "' through getter", ex); + throw new AccessException("Unable to access property '" + name + "' through getter method", ex); } } - if (this.member instanceof Field) { + else { + Field field = (Field) this.member; try { - if (this.needsToBeMadeAccessible) { - ReflectionUtils.makeAccessible((Field) this.member); + if (this.needsToBeMadeAccessible && !field.isAccessible()) { + field.setAccessible(true); } - Object value = ((Field) this.member).get(target); + Object value = field.get(target); return new TypedValue(value, this.typeDescriptor.narrow(value)); } catch (Exception ex) { - throw new AccessException("Unable to access field: " + name, ex); + throw new AccessException("Unable to access field '" + name + "'", ex); } } - throw new AccessException("Neither getter nor field found for property '" + name + "'"); } @Override @@ -656,7 +653,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { public void write(EvaluationContext context, Object target, String name, Object newValue) { throw new UnsupportedOperationException("Should not be called on an OptimalPropertyAccessor"); } - + @Override public boolean isCompilable() { return (Modifier.isPublic(this.member.getModifiers()) && @@ -665,11 +662,11 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { @Override public Class getPropertyType() { - if (this.member instanceof Field) { - return ((Field) this.member).getType(); + if (this.member instanceof Method) { + return ((Method) this.member).getReturnType(); } else { - return ((Method) this.member).getReturnType(); + return ((Field) this.member).getType(); } } @@ -677,22 +674,31 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { public void generateCode(String propertyName, MethodVisitor mv, CodeFlow cf) { boolean isStatic = Modifier.isStatic(this.member.getModifiers()); String descriptor = cf.lastDescriptor(); - String memberDeclaringClassSlashedDescriptor = this.member.getDeclaringClass().getName().replace('.', '/'); + String classDesc = this.member.getDeclaringClass().getName().replace('.', '/'); + if (!isStatic) { if (descriptor == null) { cf.loadTarget(mv); } - if (descriptor == null || !memberDeclaringClassSlashedDescriptor.equals(descriptor.substring(1))) { - mv.visitTypeInsn(CHECKCAST, memberDeclaringClassSlashedDescriptor); + if (descriptor == null || !classDesc.equals(descriptor.substring(1))) { + mv.visitTypeInsn(CHECKCAST, classDesc); } } - if (this.member instanceof Field) { - mv.visitFieldInsn(isStatic ? GETSTATIC : GETFIELD, memberDeclaringClassSlashedDescriptor, - this.member.getName(), CodeFlow.toJvmDescriptor(((Field) this.member).getType())); + else { + if (descriptor != null) { + // A static field/method call will not consume what is on the stack, + // it needs to be popped off. + mv.visitInsn(POP); + } + } + + if (this.member instanceof Method) { + mv.visitMethodInsn((isStatic ? INVOKESTATIC : INVOKEVIRTUAL), classDesc, this.member.getName(), + CodeFlow.createSignatureDescriptor((Method) this.member), false); } else { - mv.visitMethodInsn(isStatic ? INVOKESTATIC : INVOKEVIRTUAL, memberDeclaringClassSlashedDescriptor, - this.member.getName(), CodeFlow.createSignatureDescriptor((Method) this.member),false); + mv.visitFieldInsn((isStatic ? GETSTATIC : GETFIELD), classDesc, this.member.getName(), + CodeFlow.toJvmDescriptor(((Field) this.member).getType())); } } } 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 855361f1742..e203e71f7e3 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 @@ -2008,6 +2008,103 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertTrue(expression.getValue(Boolean.class)); } + + /** + * Test variants of using T(...) and static/non-static method/property/field references. + */ + @Test + public void constructorReference_SPR13781() { + // Static field access on a T() referenced type + expression = parser.parseExpression("T(java.util.Locale).ENGLISH"); + assertEquals("en",expression.getValue().toString()); + assertCanCompile(expression); + assertEquals("en",expression.getValue().toString()); + + // The actual expression from the bug report. It fails if the ENGLISH reference fails + // to pop the type reference for Locale off the stack (if it isn't popped then + // toLowerCase() will be called with a Locale parameter). In this situation the + // code generation for ENGLISH should notice there is something on the stack that + // is not required and pop it off. + expression = parser.parseExpression("#userId.toString().toLowerCase(T(java.util.Locale).ENGLISH)"); + StandardEvaluationContext context = + new StandardEvaluationContext(); + context.setVariable("userId", "RoDnEy"); + assertEquals("rodney",expression.getValue(context)); + assertCanCompile(expression); + assertEquals("rodney",expression.getValue(context)); + + // Property access on a class object + expression = parser.parseExpression("T(String).name"); + assertEquals("java.lang.String",expression.getValue()); + assertCanCompile(expression); + assertEquals("java.lang.String",expression.getValue()); + + // Now the type reference isn't on the stack, and needs loading + context = new StandardEvaluationContext(String.class); + expression = parser.parseExpression("name"); + assertEquals("java.lang.String",expression.getValue(context)); + assertCanCompile(expression); + assertEquals("java.lang.String",expression.getValue(context)); + + expression = parser.parseExpression("T(String).getName()"); + assertEquals("java.lang.String",expression.getValue()); + assertCanCompile(expression); + assertEquals("java.lang.String",expression.getValue()); + + // These tests below verify that the chain of static accesses (either method/property or field) + // leave the right thing on top of the stack for processing by any outer consuming code. + // Here the consuming code is the String.valueOf() function. If the wrong thing were on + // the stack (for example if the compiled code for static methods wasn't popping the + // previous thing off the stack) the valueOf() would operate on the wrong value. + + String shclass = StaticsHelper.class.getName(); + // Basic chain: property access then method access + expression = parser.parseExpression("T(String).valueOf(T(String).name.valueOf(1))"); + assertEquals("1",expression.getValue()); + assertCanCompile(expression); + assertEquals("1",expression.getValue()); + + // chain of statics ending with static method + expression = parser.parseExpression("T(String).valueOf(T("+shclass+").methoda().methoda().methodb())"); + assertEquals("mb",expression.getValue()); + assertCanCompile(expression); + assertEquals("mb",expression.getValue()); + + // chain of statics ending with static field + expression = parser.parseExpression("T(String).valueOf(T("+shclass+").fielda.fielda.fieldb)"); + assertEquals("fb",expression.getValue()); + assertCanCompile(expression); + assertEquals("fb",expression.getValue()); + + // chain of statics ending with static property access + expression = parser.parseExpression("T(String).valueOf(T("+shclass+").propertya.propertya.propertyb)"); + assertEquals("pb",expression.getValue()); + assertCanCompile(expression); + assertEquals("pb",expression.getValue()); + + // variety chain + expression = parser.parseExpression("T(String).valueOf(T("+shclass+").fielda.methoda().propertya.fieldb)"); + assertEquals("fb",expression.getValue()); + assertCanCompile(expression); + assertEquals("fb",expression.getValue()); + + expression = parser.parseExpression("T(String).valueOf(fielda.fieldb)"); + assertEquals("fb",expression.getValue(StaticsHelper.sh)); + assertCanCompile(expression); + assertEquals("fb",expression.getValue(StaticsHelper.sh)); + + expression = parser.parseExpression("T(String).valueOf(propertya.propertyb)"); + assertEquals("pb",expression.getValue(StaticsHelper.sh)); + assertCanCompile(expression); + assertEquals("pb",expression.getValue(StaticsHelper.sh)); + + expression = parser.parseExpression("T(String).valueOf(methoda().methodb())"); + assertEquals("mb",expression.getValue(StaticsHelper.sh)); + assertCanCompile(expression); + assertEquals("mb",expression.getValue(StaticsHelper.sh)); + + } + @Test public void constructorReference_SPR12326() { String type = this.getClass().getName(); @@ -4353,4 +4450,29 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { } } + public static class StaticsHelper { + static StaticsHelper sh = new StaticsHelper(); + public static StaticsHelper methoda() { + return sh; + } + public static String methodb() { + return "mb"; + } + + public static StaticsHelper getPropertya() { + return sh; + } + + public static String getPropertyb() { + return "pb"; + } + + + public static StaticsHelper fielda = sh; + public static String fieldb = "fb"; + + public String toString() { + return "sh"; + } + } }