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"; + } + } }