From d41d28f8cef5355e0cc36e51049577fb113f3b6f Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Mon, 6 Feb 2017 09:43:10 -0800 Subject: [PATCH] Fix compilation of SpEL elvis/ternary expressions Without this fix the compiled version of elvis actual behaved differently to the interpreted version if the value being queried was an empty string. This is now fixed. It also now correctly handles the query value being a primitive and addresses the findings of SPR-15192 where some type inferencing logic was trying to be too clever, that code has been deleted. Issue: SPR-15192 --- .../expression/spel/ast/Elvis.java | 17 +- .../expression/spel/ast/Ternary.java | 8 +- .../spel/SpelCompilationCoverageTests.java | 184 ++++++++++++++++++ 3 files changed, 194 insertions(+), 15 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Elvis.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Elvis.java index 77f342ec17..12ec3243f1 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Elvis.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Elvis.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2016 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. @@ -49,6 +49,7 @@ public class Elvis extends SpelNodeImpl { @Override public TypedValue getValueInternal(ExpressionState state) throws EvaluationException { TypedValue value = this.children[0].getValueInternal(state); + // If this check is changed, the generateCode method will need changing too if (!StringUtils.isEmpty(value.getValue())) { return value; } @@ -77,11 +78,17 @@ public class Elvis extends SpelNodeImpl { // exit type descriptor can be null if both components are literal expressions computeExitTypeDescriptor(); this.children[0].generateCode(mv, cf); + CodeFlow.insertBoxIfNecessary(mv, cf.lastDescriptor().charAt(0)); Label elseTarget = new Label(); Label endOfIf = new Label(); mv.visitInsn(DUP); mv.visitJumpInsn(IFNULL, elseTarget); - mv.visitJumpInsn(GOTO, endOfIf); + // Also check if empty string, as per the code in the interpreted version + mv.visitInsn(DUP); + mv.visitLdcInsn(""); + mv.visitInsn(SWAP); + mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/String", "equals", "(Ljava/lang/Object;)Z",false); + mv.visitJumpInsn(IFEQ, endOfIf); // If not empty, drop through to elseTarget mv.visitLabel(elseTarget); mv.visitInsn(POP); this.children[1].generateCode(mv, cf); @@ -100,12 +107,6 @@ public class Elvis extends SpelNodeImpl { if (conditionDescriptor.equals(ifNullValueDescriptor)) { this.exitTypeDescriptor = conditionDescriptor; } - else if (conditionDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(ifNullValueDescriptor)) { - this.exitTypeDescriptor = ifNullValueDescriptor; - } - else if (ifNullValueDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(conditionDescriptor)) { - this.exitTypeDescriptor = conditionDescriptor; - } else { // Use the easiest to compute common super type this.exitTypeDescriptor = "Ljava/lang/Object"; diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Ternary.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Ternary.java index 6855d59c4a..33668128f1 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Ternary.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Ternary.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2016 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. @@ -71,12 +71,6 @@ public class Ternary extends SpelNodeImpl { if (leftDescriptor.equals(rightDescriptor)) { this.exitTypeDescriptor = leftDescriptor; } - else if (leftDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(rightDescriptor)) { - this.exitTypeDescriptor = rightDescriptor; - } - else if (rightDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(leftDescriptor)) { - this.exitTypeDescriptor = leftDescriptor; - } else { // Use the easiest to compute common super type this.exitTypeDescriptor = "Ljava/lang/Object"; 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 9aef460db0..77dbd7bdbd 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 @@ -17,8 +17,10 @@ package org.springframework.expression.spel; import java.io.IOException; +import java.lang.reflect.Field; import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -4626,6 +4628,188 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertEquals(3, expression.getValue(root)); assertEquals(3, expression.getValue(root)); } + + @Test + public void elvisOperator_SPR15192() { + SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, null); + Expression exp; + + exp = new SpelExpressionParser(configuration).parseExpression("bar()"); + assertEquals("BAR", exp.getValue(new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("BAR", exp.getValue(new Foo(), String.class)); + assertIsCompiled(exp); + + exp = new SpelExpressionParser(configuration).parseExpression("bar('baz')"); + assertEquals("BAZ", exp.getValue(new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("BAZ", exp.getValue(new Foo(), String.class)); + assertIsCompiled(exp); + + StandardEvaluationContext context = new StandardEvaluationContext(); + context.setVariable("map", Collections.singletonMap("foo", "qux")); + + exp = new SpelExpressionParser(configuration).parseExpression("bar(#map['foo'])"); + assertEquals("QUX", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("QUX", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + exp = new SpelExpressionParser(configuration).parseExpression("bar(#map['foo'] ?: 'qux')"); + assertEquals("QUX", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("QUX", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // When the condition is a primitive + exp = new SpelExpressionParser(configuration).parseExpression("3?:'foo'"); + assertEquals("3", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("3", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // When the condition is a double slot primitive + exp = new SpelExpressionParser(configuration).parseExpression("3L?:'foo'"); + assertEquals("3", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("3", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // When the condition is an empty string + exp = new SpelExpressionParser(configuration).parseExpression("''?:4L"); + assertEquals("4", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("4", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // null condition + exp = new SpelExpressionParser(configuration).parseExpression("null?:4L"); + assertEquals("4", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("4", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // variable access returning primitive + exp = new SpelExpressionParser(configuration).parseExpression("#x?:'foo'"); + context.setVariable("x",50); + assertEquals("50", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("50", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + exp = new SpelExpressionParser(configuration).parseExpression("#x?:'foo'"); + context.setVariable("x",null); + assertEquals("foo", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("foo", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // variable access returning array + exp = new SpelExpressionParser(configuration).parseExpression("#x?:'foo'"); + context.setVariable("x",new int[]{1,2,3}); + assertEquals("1,2,3", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("1,2,3", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + } + + @Test + public void ternaryOperator_SPR15192() { + SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, null); + Expression exp; + StandardEvaluationContext context = new StandardEvaluationContext(); + context.setVariable("map", Collections.singletonMap("foo", "qux")); + + exp = new SpelExpressionParser(configuration).parseExpression("bar(#map['foo'] != null ? #map['foo'] : 'qux')"); + assertEquals("QUX", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("QUX", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + exp = new SpelExpressionParser(configuration).parseExpression("3==3?3:'foo'"); + assertEquals("3", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("3", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + exp = new SpelExpressionParser(configuration).parseExpression("3!=3?3:'foo'"); + assertEquals("foo", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("foo", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // When the condition is a double slot primitive + exp = new SpelExpressionParser(configuration).parseExpression("3==3?3L:'foo'"); + assertEquals("3", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("3", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + exp = new SpelExpressionParser(configuration).parseExpression("3!=3?3L:'foo'"); + assertEquals("foo", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("foo", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // When the condition is an empty string + exp = new SpelExpressionParser(configuration).parseExpression("''==''?'abc':4L"); + assertEquals("abc", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("abc", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // null condition + exp = new SpelExpressionParser(configuration).parseExpression("3==3?null:4L"); + assertEquals(null, exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals(null, exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // variable access returning primitive + exp = new SpelExpressionParser(configuration).parseExpression("#x==#x?50:'foo'"); + context.setVariable("x",50); + assertEquals("50", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("50", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + exp = new SpelExpressionParser(configuration).parseExpression("#x!=#x?50:'foo'"); + context.setVariable("x",null); + assertEquals("foo", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("foo", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + + // variable access returning array + exp = new SpelExpressionParser(configuration).parseExpression("#x==#x?'1,2,3':'foo'"); + context.setVariable("x",new int[]{1,2,3}); + assertEquals("1,2,3", exp.getValue(context, new Foo(), String.class)); + assertCanCompile(exp); + assertEquals("1,2,3", exp.getValue(context, new Foo(), String.class)); + assertIsCompiled(exp); + } + + private void assertIsCompiled(Expression ex) { + try { + Field f = SpelExpression.class.getDeclaredField("compiledAst"); + f.setAccessible(true); + Object object = f.get(ex); + assertNotNull(object); + } catch (Exception e) { + fail(e.toString()); + } + } + + public static class Foo { + + public String bar() { + return "BAR"; + } + + public String bar(String arg) { + return arg.toUpperCase(); + } + + } + // helper methods