From aae221cb159b9b0de4d4bc7cc0174d12a015b510 Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Fri, 17 Oct 2014 10:26:54 -0700 Subject: [PATCH] Fix SpEL compilation of constructor invocation The argument processing for compiling constructor references was very basic and this fix removes that and ensures the comprehensive logic written for method argument processing (under SPR-12328) is now used for both method and constructor argument handling. This fixes the reported issue and ensures varargs constructor references can be compiled. This also includes a couple of small fixes for the secondary testcase reported in SPR-12326. The first is to ensure the right root context object is used when it is passed to getValue() indirectly through the evaluation context. The final fix is to ensure correct boxing of primitives is done when a method is called upon a primitive. Issue: SPR-12326 --- .../expression/spel/CodeFlow.java | 20 +- .../spel/ast/ConstructorReference.java | 20 +- .../expression/spel/ast/MethodReference.java | 4 + .../spel/standard/SpelExpression.java | 6 +- .../spel/SpelCompilationCoverageTests.java | 183 ++++++++++++++++++ .../spel/SpelCompilationPerformanceTests.java | 2 +- .../spel/testdata/PersonInOtherPackage.java | 38 ++++ 7 files changed, 252 insertions(+), 21 deletions(-) create mode 100644 spring-expression/src/test/java/org/springframework/expression/spel/testdata/PersonInOtherPackage.java diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java index acdef4d84f7..8b98c97f396 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java @@ -17,6 +17,7 @@ package org.springframework.expression.spel; import java.lang.reflect.Constructor; +import java.lang.reflect.Member; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; @@ -837,12 +838,23 @@ public class CodeFlow implements Opcodes { * packaged into an array. * @param mv the method visitor where code should be generated * @param cf the current codeflow - * @param method the method for which arguments are being setup + * @param member the method or constructor for which arguments are being setup * @param arguments the expression nodes for the expression supplied argument values */ - public static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Method method, SpelNodeImpl[] arguments) { - String[] paramDescriptors = CodeFlow.toParamDescriptors(method); - if (method.isVarArgs()) { + public static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Member member, SpelNodeImpl[] arguments) { + String[] paramDescriptors = null; + boolean isVarargs = false; + if (member instanceof Constructor) { + Constructor ctor = (Constructor)member; + paramDescriptors = toDescriptors(ctor.getParameterTypes()); + isVarargs = ctor.isVarArgs(); + } + else { // Method + Method method = (Method)member; + paramDescriptors = toDescriptors(method.getParameterTypes()); + isVarargs = method.isVarArgs(); + } + if (isVarargs) { // The final parameter may or may not need packaging into an array, or nothing may // have been passed to satisfy the varargs and so something needs to be built. int p = 0; // Current supplied argument being processed diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java index 7b72627195d..27319ad60ae 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java @@ -437,29 +437,21 @@ public class ConstructorReference extends SpelNodeImpl { ReflectiveConstructorExecutor executor = (ReflectiveConstructorExecutor) this.cachedExecutor; Constructor constructor = executor.getConstructor(); - return (!constructor.isVarArgs() && Modifier.isPublic(constructor.getModifiers()) && + return (Modifier.isPublic(constructor.getModifiers()) && Modifier.isPublic(constructor.getDeclaringClass().getModifiers())); } @Override public void generateCode(MethodVisitor mv, CodeFlow cf) { ReflectiveConstructorExecutor executor = ((ReflectiveConstructorExecutor) this.cachedExecutor); - Constructor constructor = executor.getConstructor(); - + Constructor constructor = executor.getConstructor(); String classSlashedDescriptor = constructor.getDeclaringClass().getName().replace('.', '/'); - String[] paramDescriptors = CodeFlow.toParamDescriptors(constructor); mv.visitTypeInsn(NEW, classSlashedDescriptor); mv.visitInsn(DUP); - for (int c = 1; c < this.children.length; c++) { // children[0] is the type of the constructor - SpelNodeImpl child = this.children[c]; - cf.enterCompilationScope(); - child.generateCode(mv, cf); - // Check if need to box it for the method reference? - if (CodeFlow.isPrimitive(cf.lastDescriptor()) && paramDescriptors[c-1].charAt(0) == 'L') { - CodeFlow.insertBoxIfNecessary(mv, cf.lastDescriptor().charAt(0)); - } - cf.exitCompilationScope(); - } + // children[0] is the type of the constructor, don't want to include that in argument processing + SpelNodeImpl[] arguments = new SpelNodeImpl[children.length-1]; + System.arraycopy(children, 1, arguments, 0, children.length-1); + CodeFlow.generateCodeForArguments(mv, cf, constructor, arguments); mv.visitMethodInsn(INVOKESPECIAL, classSlashedDescriptor, "", CodeFlow.createSignatureDescriptor(constructor), false); 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 d86a71df5e4..addce1f6f3c 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 @@ -295,6 +295,10 @@ public class MethodReference extends SpelNodeImpl { if (descriptor == null && !isStaticMethod) { cf.loadTarget(mv); } + + if (CodeFlow.isPrimitive(descriptor)) { + CodeFlow.insertBoxIfNecessary(mv, descriptor.charAt(0)); + } boolean itf = method.getDeclaringClass().isInterface(); String methodDeclaringClassSlashedDescriptor = null; diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java b/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java index 9c5012902bf..eba325e4158 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java @@ -221,7 +221,8 @@ public class SpelExpression implements Expression { Assert.notNull(context, "The EvaluationContext is required"); if (compiledAst!= null) { try { - Object result = this.compiledAst.getValue(null,context); + TypedValue contextRoot = context == null ? null : context.getRootObject(); + Object result = this.compiledAst.getValue(contextRoot==null?null:contextRoot.getValue(),context); return result; } catch (Throwable ex) { @@ -272,7 +273,8 @@ public class SpelExpression implements Expression { public T getValue(EvaluationContext context, Class expectedResultType) throws EvaluationException { if (this.compiledAst != null) { try { - Object result = this.compiledAst.getValue(null,context); + TypedValue contextRoot = context == null ? null : context.getRootObject(); + Object result = this.compiledAst.getValue(contextRoot==null?null:contextRoot.getValue(),context); if (expectedResultType != null) { return ExpressionUtils.convertTypedValue(context, new TypedValue(result), expectedResultType); } 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 956d9e7911e..e69c52a7ed6 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 @@ -38,6 +38,7 @@ import org.springframework.expression.spel.standard.SpelCompiler; import org.springframework.expression.spel.standard.SpelExpression; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.StandardEvaluationContext; +import org.springframework.expression.spel.testdata.PersonInOtherPackage; import static org.junit.Assert.*; @@ -1683,6 +1684,128 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertEquals(1.0f,expression.getValue()); } + @Test + public void constructorReference_SPR12326() { + String type = this.getClass().getName(); + String prefix = "new "+type+".Obj"; + + expression = parser.parseExpression(prefix+"([0])"); + assertEquals("test", ((Obj) expression.getValue(new Object[] { "test" })).param1); + assertCanCompile(expression); + assertEquals("test", ((Obj) expression.getValue(new Object[] { "test" })).param1); + + expression = parser.parseExpression(prefix+"2('foo','bar').output"); + assertEquals("foobar", expression.getValue(String.class)); + assertCanCompile(expression); + assertEquals("foobar", expression.getValue(String.class)); + + expression = parser.parseExpression(prefix+"2('foo').output"); + assertEquals("foo", expression.getValue(String.class)); + assertCanCompile(expression); + assertEquals("foo", expression.getValue(String.class)); + + expression = parser.parseExpression(prefix+"2().output"); + assertEquals("", expression.getValue(String.class)); + assertCanCompile(expression); + assertEquals("", expression.getValue(String.class)); + + expression = parser.parseExpression(prefix+"3(1,2,3).output"); + assertEquals("123", expression.getValue(String.class)); + assertCanCompile(expression); + assertEquals("123", expression.getValue(String.class)); + + expression = parser.parseExpression(prefix+"3(1).output"); + assertEquals("1", expression.getValue(String.class)); + assertCanCompile(expression); + assertEquals("1", expression.getValue(String.class)); + + expression = parser.parseExpression(prefix+"3().output"); + assertEquals("", expression.getValue(String.class)); + assertCanCompile(expression); + assertEquals("", expression.getValue(String.class)); + + expression = parser.parseExpression(prefix+"3('abc',5.0f,1,2,3).output"); + assertEquals("abc:5.0:123", expression.getValue(String.class)); + assertCanCompile(expression); + assertEquals("abc:5.0:123", expression.getValue(String.class)); + + expression = parser.parseExpression(prefix+"3('abc',5.0f,1).output"); + assertEquals("abc:5.0:1", expression.getValue(String.class)); + assertCanCompile(expression); + assertEquals("abc:5.0:1", expression.getValue(String.class)); + + expression = parser.parseExpression(prefix+"3('abc',5.0f).output"); + assertEquals("abc:5.0:", expression.getValue(String.class)); + assertCanCompile(expression); + assertEquals("abc:5.0:", expression.getValue(String.class)); + + expression = parser.parseExpression(prefix+"4(#root).output"); + assertEquals("123", expression.getValue(new int[]{1,2,3},String.class)); + assertCanCompile(expression); + assertEquals("123", expression.getValue(new int[]{1,2,3},String.class)); + } + + @Test + public void methodReferenceMissingCastAndRootObjectAccessing_SPR12326() { + // Need boxing code on the 1 so that toString() can be called + expression = parser.parseExpression("1.toString()"); + assertEquals("1", expression.getValue()); + assertCanCompile(expression); + assertEquals("1", expression.getValue()); + + expression = parser.parseExpression("#it?.age.equals([0])"); + Person person = new Person(1); + StandardEvaluationContext context = + new StandardEvaluationContext(new Object[] { person.getAge() }); + context.setVariable("it", person); + assertTrue(expression.getValue(context, Boolean.class)); + assertCanCompile(expression); + assertTrue(expression.getValue(context, Boolean.class)); + + // Variant of above more like what was in the bug report: + SpelExpressionParser parser = new SpelExpressionParser( + new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, + this.getClass().getClassLoader())); + + SpelExpression ex = parser.parseRaw("#it?.age.equals([0])"); + context = new StandardEvaluationContext(new Object[] { person.getAge() }); + context.setVariable("it", person); + assertTrue(ex.getValue(context, Boolean.class)); + assertTrue(ex.getValue(context, Boolean.class)); + + PersonInOtherPackage person2 = new PersonInOtherPackage(1); + ex = parser.parseRaw("#it?.age.equals([0])"); + context = + new StandardEvaluationContext(new Object[] { person2.getAge() }); + context.setVariable("it", person2); + assertTrue(ex.getValue(context, Boolean.class)); + assertTrue(ex.getValue(context, Boolean.class)); + + ex = parser.parseRaw("#it?.age.equals([0])"); + context = + new StandardEvaluationContext(new Object[] { person2.getAge() }); + context.setVariable("it", person2); + assertTrue((Boolean)ex.getValue(context)); + assertTrue((Boolean)ex.getValue(context)); + } + + public class Person { + + private int age; + + public Person(int age) { + this.age = age; + } + + public int getAge() { + return age; + } + + public void setAge(int age) { + this.age = age; + } + } + @Test public void constructorReference() throws Exception { // simple ctor @@ -3440,6 +3563,66 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { this.s = a+b; } } + + public static class Obj { + + private final String param1; + + public Obj(String param1){ + this.param1 = param1; + } + } + + public static class Obj2 { + + public final String output; + + public Obj2(String... params){ + StringBuilder b = new StringBuilder(); + for (String param: params) { + b.append(param); + } + output = b.toString(); + } + } + + public static class Obj3 { + + public final String output; + + public Obj3(int... params) { + StringBuilder b = new StringBuilder(); + for (int param: params) { + b.append(Integer.toString(param)); + } + output = b.toString(); + } + + public Obj3(String s, Float f, int... ints) { + StringBuilder b = new StringBuilder(); + b.append(s); + b.append(":"); + b.append(Float.toString(f)); + b.append(":"); + for (int param: ints) { + b.append(Integer.toString(param)); + } + output = b.toString(); + } + } + + public static class Obj4 { + + public final String output; + + public Obj4(int[] params) { + StringBuilder b = new StringBuilder(); + for (int param: params) { + b.append(Integer.toString(param)); + } + output = b.toString(); + } + } @SuppressWarnings("unused") private static class TestClass9 { diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationPerformanceTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationPerformanceTests.java index 6b3a821570c..87f37e394fa 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationPerformanceTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationPerformanceTests.java @@ -156,7 +156,7 @@ public class SpelCompilationPerformanceTests extends AbstractExpressionTests { Object o = expression.getValue(g); assertEquals("helloworld spring", o); - System.out.println("Performance check for SpEL expression: '{'abcde','ijklm'}[0].substring({1,3,4}[0],{1,3,4}[1])'"); + System.out.println("Performance check for SpEL expression: 'hello' + getWorld() + ' spring'"); long stime = System.currentTimeMillis(); for (int i=0;i<1000000;i++) { o = expression.getValue(g); diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/testdata/PersonInOtherPackage.java b/spring-expression/src/test/java/org/springframework/expression/spel/testdata/PersonInOtherPackage.java new file mode 100644 index 00000000000..add606359f8 --- /dev/null +++ b/spring-expression/src/test/java/org/springframework/expression/spel/testdata/PersonInOtherPackage.java @@ -0,0 +1,38 @@ +/* + * Copyright 2014 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 + * + * http://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.testdata; + +/** + * + * @author Andy Clement + * @since 4.1.2 + */ +public class PersonInOtherPackage { + + private int age; + + public PersonInOtherPackage(int age) { + this.age = age; + } + + public int getAge() { + return age; + } + + public void setAge(int age) { + this.age = age; + } +} \ No newline at end of file