From 27f3feea1a2488df17c3b6fbcbbf416768e7a5b3 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 11 Nov 2022 15:35:54 +0100 Subject: [PATCH] Ensure SpEL ternary and Elvis expressions are enclosed in parentheses in toStringAST() Prior to this commit, ternary and Elvis expressions enclosed in parentheses (to account for operator precedence) were properly parsed and evaluated; however, the corresponding toStringAST() implementations did not enclose the results in parentheses. Consequently, the string representation of the ASTs did not reflect the original semantics of such expressions. For example, given "(4 % 2 == 0 ? 1 : 0) * 10" as the expression to parse and evaluate, the result of toStringAST() was previously "(((4 % 2) == 0) ? 1 : 0 * 10)" instead of "((((4 % 2) == 0) ? 1 : 0) * 10)", implying that 0 should be multiplied by 10 instead of multiplying the result of the ternary expression by 10. This commit addresses this by ensuring that SpEL ternary and Elvis expressions are enclosed in parentheses in toStringAST(). Closes gh-29463 --- .../expression/spel/ast/Elvis.java | 5 +++-- .../expression/spel/ast/Ternary.java | 5 +++-- .../expression/spel/EvaluationTests.java | 22 +++++++++++++++++++ .../expression/spel/ParsingTests.java | 12 +++++++--- 4 files changed, 37 insertions(+), 7 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 818f43fca6..55e2267c4c 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-2019 the original author or authors. + * Copyright 2002-2022 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. @@ -32,6 +32,7 @@ import org.springframework.util.ObjectUtils; * * @author Andy Clement * @author Juergen Hoeller + * @author Sam Brannen * @since 3.0 */ public class Elvis extends SpelNodeImpl { @@ -64,7 +65,7 @@ public class Elvis extends SpelNodeImpl { @Override public String toStringAST() { - return getChild(0).toStringAST() + " ?: " + getChild(1).toStringAST(); + return "(" + getChild(0).toStringAST() + " ?: " + getChild(1).toStringAST() + ")"; } @Override 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 2889f57942..f536ec8924 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-2019 the original author or authors. + * Copyright 2002-2022 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. @@ -32,6 +32,7 @@ import org.springframework.util.ObjectUtils; * * @author Andy Clement * @author Juergen Hoeller + * @author Sam Brannen * @since 3.0 */ public class Ternary extends SpelNodeImpl { @@ -62,7 +63,7 @@ public class Ternary extends SpelNodeImpl { @Override public String toStringAST() { - return getChild(0).toStringAST() + " ? " + getChild(1).toStringAST() + " : " + getChild(2).toStringAST(); + return "(" + getChild(0).toStringAST() + " ? " + getChild(1).toStringAST() + " : " + getChild(2).toStringAST() + ")"; } private void computeExitTypeDescriptor() { diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java index febb1f4412..166a46783e 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java @@ -116,6 +116,10 @@ class EvaluationTests extends AbstractExpressionTests { void elvisOperator() { evaluate("'Andy'?:'Dave'", "Andy", String.class); evaluate("null?:'Dave'", "Dave", String.class); + evaluate("3?:1", 3, Integer.class); + evaluate("(2*3)?:1*10", 6, Integer.class); + evaluate("null?:2*10", 20, Integer.class); + evaluate("(null?:1)*10", 10, Integer.class); } @Test @@ -624,6 +628,24 @@ class EvaluationTests extends AbstractExpressionTests { evaluate("2>4?(3>2?true:false):(5<3?true:false)", false, Boolean.class); } + @Test + void ternaryOperator06() { + evaluate("3?:#var=5", 3, Integer.class); + evaluate("null?:#var=5", 5, Integer.class); + evaluate("2>4?(3>2?true:false):(5<3?true:false)", false, Boolean.class); + } + + @Test + void ternaryExpressionWithImplicitGrouping() { + evaluate("4 % 2 == 0 ? 2 : 3 * 10", 2, Integer.class); + evaluate("4 % 2 == 1 ? 2 : 3 * 10", 30, Integer.class); + } + + @Test + void ternaryExpressionWithExplicitGrouping() { + evaluate("((4 % 2 == 0) ? 2 : 1) * 10", 20, Integer.class); + } + @Test void ternaryOperatorWithNullValue() { assertThatExceptionOfType(EvaluationException.class).isThrownBy( diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/ParsingTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/ParsingTests.java index 9656c8a3fe..075688ea84 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/ParsingTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/ParsingTests.java @@ -162,7 +162,9 @@ class ParsingTests { @Test void elvis() { - parseCheck("3?:1", "3 ?: 1"); + parseCheck("3?:1", "(3 ?: 1)"); + parseCheck("(2*3)?:1*10", "((2 * 3) ?: (1 * 10))"); + parseCheck("((2*3)?:1)*10", "(((2 * 3) ?: 1) * 10)"); } // void relOperatorsIn01() { @@ -404,13 +406,17 @@ class ParsingTests { @Test void ternaryOperator01() { - parseCheck("1>2?3:4", "(1 > 2) ? 3 : 4"); + parseCheck("1>2?3:4", "((1 > 2) ? 3 : 4)"); + parseCheck("(a ? 1 : 0) * 10", "((a ? 1 : 0) * 10)"); + parseCheck("(a?1:0)*10", "((a ? 1 : 0) * 10)"); + parseCheck("(4 % 2 == 0 ? 1 : 0) * 10", "((((4 % 2) == 0) ? 1 : 0) * 10)"); + parseCheck("((4 % 2 == 0) ? 1 : 0) * 10", "((((4 % 2) == 0) ? 1 : 0) * 10)"); } @Test void ternaryOperator02() { parseCheck("{1}.#isEven(#this) == 'y'?'it is even':'it is odd'", - "({1}.#isEven(#this) == 'y') ? 'it is even' : 'it is odd'"); + "(({1}.#isEven(#this) == 'y') ? 'it is even' : 'it is odd')"); } //