Ensure cast correctly included for OpPlus compilation

When the plus operator is used between strings in a SpEL
expression and that expression is compiled, it is
important to include a cast if computation of any of
the operands isn't obviously leaving strings on the
stack. Likewise if the stack contents are known to
be strings, a cast should not be included.

Issue: SPR-12426
(cherry picked from commit 58756b0)

spring-expression/src/main/java/org/springframework/expression/spel/ast/OpPlus.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.
@@ -206,7 +206,7 @@ public class OpPlus extends Operator {
 		else {
 			cf.enterCompilationScope();
 			operand.generateCode(mv,cf);
-			if (cf.lastDescriptor() != "Ljava/lang/String") {
+			if (!"Ljava/lang/String".equals(cf.lastDescriptor())) {
 				mv.visitTypeInsn(CHECKCAST, "java/lang/String");
 			}
 			cf.exitCompilationScope();

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 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.
@@ -1041,7 +1041,8 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		try {
 			assertEquals(42,expression.getValue(ctx));
 			fail();
-		} catch (SpelEvaluationException see) {
+		}
+		catch (SpelEvaluationException see) {
 			assertTrue(see.getCause() instanceof ClassCastException);
 		}
 
@@ -1056,7 +1057,8 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		try {
 			assertEquals('4',expression.getValue(ctx));
 			fail();
-		} catch (SpelEvaluationException see) {
+		}
+		catch (SpelEvaluationException see) {
 			assertTrue(see.getCause() instanceof ClassCastException);
 		}
 	}
@@ -1170,7 +1172,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertFalse((Boolean)expression.getValue());
 	}
 
-	
 	@Test
 	public void opGt() throws Exception {
 		expression = parse("3.0d > 4.0d");
@@ -1317,7 +1318,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertCanCompile(expression);
 		assertTrue((Boolean)expression.getValue(tc7));
 
-		
 		expression = parse("3.0d == 4.0d");
 		assertCanCompile(expression);
 		assertFalse((Boolean)expression.getValue());
@@ -1723,16 +1723,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertEquals("objectobject",expression.getValue(new Greeter()));
 	}
 
-	public static class Greeter {
-		public String getWorld() {
-			return "world"; 
-		}
-		
-		public Object getObject() {
-			return "object";
-		}
-	}
-
 	@Test
 	public void opMinus() throws Exception {
 		expression = parse("2-2");
@@ -1819,7 +1809,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertEquals(6L,expression.getValue());
 	}
 
-	
 	@Test
 	public void opMultiply() throws Exception {
 		expression = parse("2*2");
@@ -2006,17 +1995,17 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 
 	@Test
 	public void failsWhenSettingContextForExpression_SPR12326() {
-	    SpelExpressionParser parser = new SpelExpressionParser(new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, this
+		SpelExpressionParser parser = new SpelExpressionParser(
-	            .getClass().getClassLoader()));
+				new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, getClass().getClassLoader()));
-	    Person3 person = new Person3("foo", 1);
+		Person3 person = new Person3("foo", 1);
-	    SpelExpression expression = parser.parseRaw("#it?.age?.equals([0])");
+		SpelExpression expression = parser.parseRaw("#it?.age?.equals([0])");
-	    StandardEvaluationContext context = new StandardEvaluationContext(new Object[] { 1 });
+		StandardEvaluationContext context = new StandardEvaluationContext(new Object[] { 1 });
-	    context.setVariable("it", person);
+		context.setVariable("it", person);
-	    expression.setEvaluationContext(context);
+		expression.setEvaluationContext(context);
-	    assertTrue(expression.getValue(Boolean.class));
+		assertTrue(expression.getValue(Boolean.class));
-	    assertTrue(expression.getValue(Boolean.class));	     
+		assertTrue(expression.getValue(Boolean.class));
-	    assertCanCompile(expression);
+		assertCanCompile(expression);
-	    assertTrue(expression.getValue(Boolean.class));
+		assertTrue(expression.getValue(Boolean.class));
 	}
 
 	@Test
@@ -2124,40 +2113,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		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;
-	    }
-	}
-	
-	public class Person3 {
-		 
-	    private int age;
-	 
-	    public Person3(String name, 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
@@ -2680,7 +2635,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertCantCompile(expression); // method takes a string and we are passing an Integer
 	}
 
-	
 	@Test
 	public void errorHandling() throws Exception {
 		TestClass5 tc = new TestClass5();
@@ -2698,7 +2652,8 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		try {
 			assertEquals(2,expression.getValue(strings));
 			fail();
-		} catch (SpelEvaluationException see) {
+		}
+		catch (SpelEvaluationException see) {
 			assertTrue(see.getCause() instanceof ClassCastException);
 		}
 		SpelCompiler.revertToInterpreted(expression);
@@ -2729,7 +2684,8 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		try {
 			expression.getValue(tc);
 			fail();
-		} catch (SpelEvaluationException see) {
+		}
+		catch (SpelEvaluationException see) {
 			assertTrue(see.getCause() instanceof ClassCastException);
 		}
 
@@ -2741,7 +2697,8 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		try {
 			expression.getValue(new Integer(42));
 			fail();
-		} catch (SpelEvaluationException see) {
+		}
+		catch (SpelEvaluationException see) {
 			// java.lang.Integer cannot be cast to java.lang.String
 			assertTrue(see.getCause() instanceof ClassCastException);
 		}
@@ -2844,7 +2801,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertEquals("D",getAst().getExitDescriptor());
 	}
 
-	
 	@Test
 	public void mixingItUp_indexerOpEqTernary() throws Exception {
 		Map<String, String> m = new HashMap<String,String>();
@@ -3210,6 +3166,21 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertEquals("Ljava/lang/Object",getAst().getExitDescriptor());
 	}
 
+	@Test
+	public void plusNeedingCheckcast_SPR12426() {
+		expression = parser.parseExpression("object + ' world'");
+		Object v = expression.getValue(new FooObject());
+		assertEquals("hello world",v);
+		assertCanCompile(expression);
+		assertEquals("hello world",v);
+
+		expression = parser.parseExpression("object + ' world'");
+		v = expression.getValue(new FooString());
+		assertEquals("hello world",v);
+		assertCanCompile(expression);
+		assertEquals("hello world",v);
+	}
+
 	@Test
 	public void mixingItUp_propertyAccessIndexerOpLtTernaryRootNull() throws Exception {
 		Payload payload = new Payload();
@@ -3489,23 +3460,90 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertEquals(3,expression.getValue(root));
 	}
 
-	// ---
 
-	public static interface Message<T> {
+	// helper methods
+
+	private SpelNodeImpl getAst() {
+		SpelExpression spelExpression = (SpelExpression)expression;
+		SpelNode ast = spelExpression.getAST();
+		return (SpelNodeImpl)ast;
+	}
+
+	private String stringify(Object object) {
+		StringBuilder s = new StringBuilder();
+		if (object instanceof List) {
+			List<?> ls = (List<?>)object;
+			for (Object l: ls) {
+				s.append(l);
+				s.append(" ");
+			}
+		}
+		else if (object instanceof Object[]) {
+			Object[] os = (Object[])object;
+			for (Object o: os) {
+				s.append(o);
+				s.append(" ");
+			}
+		}
+		else if (object instanceof int[]) {
+			int[] is = (int[])object;
+			for (int i: is) {
+				s.append(i);
+				s.append(" ");
+			}
+		}
+		else {
+			s.append(object.toString());
+		}
+		return s.toString().trim();
+	}
+
+	private void assertCanCompile(Expression expression) {
+		assertTrue(SpelCompiler.compile(expression));
+	}
+
+	private void assertCantCompile(Expression expression) {
+		assertFalse(SpelCompiler.compile(expression));
+	}
+
+	private Expression parse(String expression) {
+		return parser.parseExpression(expression);
+	}
+
+	private void assertGetValueFail(Expression expression) {
+		try {
+			Object o = expression.getValue();
+			fail("Calling getValue on the expression should have failed but returned "+o);
+		}
+		catch (Exception ex) {
+			// success!
+		}
+	}
+
+
+	// helper classes
+
+	public interface Message<T> {
+
 		MessageHeaders getHeaders();
+
 		@SuppressWarnings("rawtypes")
 		List getList();
+
 		int[] getIa();
 	}
 
 	public static class MyMessage implements Message<String> {
+
 		public MessageHeaders getHeaders() {
 			MessageHeaders mh = new MessageHeaders();
 			mh.put("command", "wibble");
 			mh.put("command2", "wobble");
 			return mh;
 		}
+
 		public int[] getIa() { return new int[]{5,3}; }
+
 		@SuppressWarnings({ "rawtypes", "unchecked" })
 		public List getList() {
 			List l = new ArrayList();
@@ -3524,9 +3562,11 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	@SuppressWarnings("serial")
-	public static class MessageHeaders extends HashMap<String,Object> {	}
+	public static class MessageHeaders extends HashMap<String,Object> {
+	}
 
 	public static class GenericMessageTestHelper<T> {
+
 		private T payload;
 
 		GenericMessageTestHelper(T value) {
@@ -3540,6 +3580,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 
 	// This test helper has a bound on the type variable
 	public static class GenericMessageTestHelper2<T extends Number> {
+
 		private T payload;
 
 		GenericMessageTestHelper2(T value) {
@@ -3608,7 +3649,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		}
 	}
 
-
 	static class CompilableMapAccessor implements CompilablePropertyAccessor {
 
 		@Override
@@ -3662,27 +3702,9 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 			}
 			mv.visitLdcInsn(propertyName);
 			mv.visitMethodInsn(INVOKEINTERFACE, "java/util/Map", "get","(Ljava/lang/Object;)Ljava/lang/Object;",true);
-
-//			if (method == null) {
-//				try {
-//					method = Payload2.class.getDeclaredMethod("getField", String.class);
-//				} catch (Exception e) {}
-//			}
-//			String descriptor = codeflow.lastDescriptor();
-//			String memberDeclaringClassSlashedDescriptor = method.getDeclaringClass().getName().replace('.','/');
-//			if (descriptor == null) {
-//				codeflow.loadTarget(mv);
-//			}
-//			if (descriptor == null || !memberDeclaringClassSlashedDescriptor.equals(descriptor.substring(1))) {
-//				mv.visitTypeInsn(CHECKCAST, memberDeclaringClassSlashedDescriptor);
-//			}
-//			mv.visitLdcInsn(propertyReference.getName());
-//			mv.visitMethodInsn(INVOKEVIRTUAL, memberDeclaringClassSlashedDescriptor, method.getName(),CodeFlow.createDescriptor(method));
-//			   6:	invokeinterface	#6,  2; //InterfaceMethod java/util/Map.get:(Ljava/lang/Object;)Ljava/lang/Object;
 		}
 	}
 
-
 	/**
 	 * Exception thrown from {@code read} in order to reset a cached
 	 * PropertyAccessor, allowing other accessors to have a try.
@@ -3704,68 +3726,33 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 
-	// helpers
-
-	private SpelNodeImpl getAst() {
-		SpelExpression spelExpression = (SpelExpression)expression;
-		SpelNode ast = spelExpression.getAST();
-		return (SpelNodeImpl)ast;
-	}
-
-	private String stringify(Object object) {
-		StringBuilder s = new StringBuilder();
-		if (object instanceof List) {
-			List<?> ls = (List<?>)object;
-			for (Object l: ls) {
-				s.append(l);
-				s.append(" ");
-			}
-		}
-		else if (object instanceof Object[]) {
-			Object[] os = (Object[])object;
-			for (Object o: os) {
-				s.append(o);
-				s.append(" ");
-			}
-		}
-		else if (object instanceof int[]) {
-			int[] is = (int[])object;
-			for (int i: is) {
-				s.append(i);
-				s.append(" ");
-			}
-		}
-		else {
-			s.append(object.toString());
-		}
-		return s.toString().trim();
-	}
-	
-	private void assertCanCompile(Expression expression) {
-		assertTrue(SpelCompiler.compile(expression));
-	}
-	
-	private void assertCantCompile(Expression expression) {
-		assertFalse(SpelCompiler.compile(expression));
-	}
-	
-	private Expression parse(String expression) {
-		return parser.parseExpression(expression);
-	}
-	
-	private void assertGetValueFail(Expression expression) {
-		try {
-			Object o = expression.getValue();
-			fail("Calling getValue on the expression should have failed but returned "+o);
-		} catch (Exception ex) {
-			// success!
-		}
-	}
-	
 	// test classes
 
+	public static class Greeter {
+
+		public String getWorld() {
+			return "world";
+		}
+
+		public Object getObject() {
+			return "object";
+		}
+	}
+
+	public static class FooObject {
+
+		public Object getObject() { return "hello"; }
+	}
+
+	public static class FooString {
+
+		public String getObject() { return "hello"; }
+	}
+
 	public static class Payload {
+
 		Two[] DR = new Two[]{new Two()};
+
 		public Two holder = new Two();
 
 		public Two[] getDR() {
@@ -3774,12 +3761,15 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	public static class Payload2 {
+
 		String var1 = "abc";
 		String var2 = "def";
+
 		public Object getField(String name) {
 			if (name.equals("var1")) {
 				return var1;
-			} else if (name.equals("var2")) {
+			}
+			else if (name.equals("var2")) {
 				return var2;
 			}
 			return null;
@@ -3787,11 +3777,48 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	public static class Payload2Holder {
+
 		public Payload2 payload2 = new Payload2();
 	}
 
+	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;
+		}
+	}
+
+	public class Person3 {
+
+		private int age;
+
+		public Person3(String name, int age) {
+			this.age = age;
+		}
+
+		public int getAge() {
+			return age;
+		}
+
+		public void setAge(int age) {
+			this.age = age;
+		}
+	}
+
 	public static class Two {
+
 		Three three = new Three();
+
 		public Three getThree() {
 			return three;
 		}
@@ -3801,19 +3828,23 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	public static class Three {
+
 		double four = 0.04d;
+
 		public double getFour() {
 			return four;
 		}
 	}
 
 	public static class TestClass1 {
+
 		public int index1 = 1;
 		public int index2 = 3;
 		public String word = "abcd";
 	}
 
 	public static class TestClass4 {
+
 		public boolean a,b;
 		public boolean gettrue() { return true; }
 		public boolean getfalse() { return false; }
@@ -3822,6 +3853,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	public static class TestClass10 {
+
 		public String s = null;
 
 		public void reset() {
@@ -3862,6 +3894,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	public static class TestClass5 {
+
 		public int i = 0;
 		public String s = null;
 		public static int _i = 0;
@@ -4071,10 +4104,10 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 				}
 			}
 		}
-		
 	}
 
 	public static class TestClass6 {
+
 		public String orange = "value1";
 		public static String apple = "value2";
 
@@ -4090,7 +4123,9 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	public static class TestClass7 {
+
 		public static String property;
+
 		static {
 			String s = "UK 123";
 			StringTokenizer st = new StringTokenizer(s);
@@ -4106,6 +4141,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	public static class TestClass8 {
+
 		public int i;
 		public String s;
 		public double d;
@@ -4132,68 +4168,69 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		}
 	}
 
-    public static class Obj {
+	public static class Obj {
 
-        private final String param1;
+		private final String param1;
 
-        public Obj(String param1){
+		public Obj(String param1){
-            this.param1 = param1;
+			this.param1 = param1;
-        }
+		}
-    }
+	}
 
-    public static class Obj2 {
+	public static class Obj2 {
 
-        public final String output;
+		public final String output;
 
-        public Obj2(String... params){
+		public Obj2(String... params){
-        	StringBuilder b = new StringBuilder();
+			StringBuilder b = new StringBuilder();
-        	for (String param: params) {
+			for (String param: params) {
-        		b.append(param);
+				b.append(param);
-        	}
+			}
-        	output = b.toString();
+			output = b.toString();
-        }
+		}
-    }
+	}
 
-    public static class Obj3 {
+	public static class Obj3 {
 
-        public final String output;
+		public final String output;
 
-        public Obj3(int... params) {
+		public Obj3(int... params) {
-        	StringBuilder b = new StringBuilder();
+			StringBuilder b = new StringBuilder();
-        	for (int param: params) {
+			for (int param: params) {
-        		b.append(Integer.toString(param));
+				b.append(Integer.toString(param));
-        	}
+			}
-        	output = b.toString();
+			output = b.toString();
-        }
+		}
 
-        public Obj3(String s, Float f, int... ints) {
+		public Obj3(String s, Float f, int... ints) {
-        	StringBuilder b = new StringBuilder();
+			StringBuilder b = new StringBuilder();
-        	b.append(s);
+			b.append(s);
-        	b.append(":");
+			b.append(":");
-        	b.append(Float.toString(f));
+			b.append(Float.toString(f));
-        	b.append(":");
+			b.append(":");
-        	for (int param: ints) {
+			for (int param: ints) {
-        		b.append(Integer.toString(param));
+				b.append(Integer.toString(param));
-        	}
+			}
-        	output = b.toString();
+			output = b.toString();
-        }
+		}
-    }
+	}
 
-    public static class Obj4 {
+	public static class Obj4 {
 
-        public final String output;
+		public final String output;
 
-        public Obj4(int[] params) {
+		public Obj4(int[] params) {
-        	StringBuilder b = new StringBuilder();
+			StringBuilder b = new StringBuilder();
-        	for (int param: params) {
+			for (int param: params) {
-        		b.append(Integer.toString(param));
+				b.append(Integer.toString(param));
-        	}
+			}
-        	output = b.toString();
+			output = b.toString();
-        }
+		}
-    }
+	}
 
 	@SuppressWarnings("unused")
 	private static class TestClass9 {
+
 		public TestClass9(int i) {}
 	}
 
@@ -4202,13 +4239,14 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	// final class HttpServlet3RequestFactory implements HttpServletRequestFactory
 	static class HttpServlet3RequestFactory {
 
-	  static Servlet3SecurityContextHolderAwareRequestWrapper getOne() {
+		static Servlet3SecurityContextHolderAwareRequestWrapper getOne() {
-		  HttpServlet3RequestFactory outer = new HttpServlet3RequestFactory();
+			HttpServlet3RequestFactory outer = new HttpServlet3RequestFactory();
-		  return outer.new Servlet3SecurityContextHolderAwareRequestWrapper();
+			return outer.new Servlet3SecurityContextHolderAwareRequestWrapper();
-	  }
+		}
-	  // private class Servlet3SecurityContextHolderAwareRequestWrapper extends SecurityContextHolderAwareRequestWrapper
+
-	  private class Servlet3SecurityContextHolderAwareRequestWrapper extends SecurityContextHolderAwareRequestWrapper {
+		// private class Servlet3SecurityContextHolderAwareRequestWrapper extends SecurityContextHolderAwareRequestWrapper
-	  }
+		private class Servlet3SecurityContextHolderAwareRequestWrapper extends SecurityContextHolderAwareRequestWrapper {
+		}
 	}
 
 	// public class SecurityContextHolderAwareRequestWrapper extends HttpServletRequestWrapper
@@ -4216,6 +4254,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	public static class HttpServletRequestWrapper {
+
 		public String getServletPath() {
 			return "wibble";
 		}
@@ -4236,6 +4275,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	}
 
 	public static class SomeCompareMethod2 {
+
 		public static int negate(int i1) {
 			return -i1;
 		}
@@ -4304,10 +4344,10 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 			}
 			return total;
 		}
-
 	}
 
 	public static class DelegatingStringFormat {
+
 		public static String format(String s, Object... args) {
 			return String.format(s, args);
 		}