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

Author: aclement

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 static void insertNewArrayCode(MethodVisitor mv, int size, String arrayty
 	 * 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

spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java

@@ -437,29 +437,21 @@ public boolean isCompilable() {
 
 		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, "<init>",
 				CodeFlow.createSignatureDescriptor(constructor), false);
 		cf.pushDescriptor(this.exitTypeDescriptor);

spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java

@@ -295,6 +295,10 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
 		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;

spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java

@@ -221,7 +221,8 @@ public Object getValue(EvaluationContext context) throws EvaluationException {
 		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 Object getValue(EvaluationContext context, Object rootObject) throws Eval
 	public <T> T getValue(EvaluationContext context, Class<T> 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);
 				}

spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java

@@ -38,6 +38,7 @@
 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 void opModulus_12041() throws Exception {
 		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 @@ private TestClass8(String a, String b) {
 			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 {

spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationPerformanceTests.java

@@ -156,7 +156,7 @@ public void stringConcatenation() throws Exception {
 		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);

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