From 6f9546722abb3a9a7072803629a354d549be3832 Mon Sep 17 00:00:00 2001 From: Semyon Danilov Date: Mon, 11 May 2020 14:59:23 +0300 Subject: [PATCH] Improve SpEL inline collection caching This commit fixes SpEL inline collection caching with with negative keys or values. Closes gh-25921 --- .../expression/spel/ast/InlineList.java | 8 +- .../expression/spel/ast/InlineMap.java | 13 +- .../expression/spel/ast/Literal.java | 12 ++ .../expression/spel/ast/OpMinus.java | 12 ++ .../spel/ast/InlineCollectionTests.java | 175 ++++++++++++++++++ 5 files changed, 218 insertions(+), 2 deletions(-) create mode 100644 spring-expression/src/test/java/org/springframework/expression/spel/ast/InlineCollectionTests.java diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineList.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineList.java index 0821c5a260..6aa2c4dd6a 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineList.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineList.java @@ -27,6 +27,7 @@ import org.springframework.expression.TypedValue; import org.springframework.expression.spel.CodeFlow; import org.springframework.expression.spel.ExpressionState; import org.springframework.expression.spel.SpelNode; +import org.springframework.expression.spel.support.StandardEvaluationContext; import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -36,6 +37,7 @@ import org.springframework.util.Assert; * @author Andy Clement * @author Sam Brannen * @author Harry Yang + * @author Semyon Danilov * @since 3.0.4 */ public class InlineList extends SpelNodeImpl { @@ -66,7 +68,7 @@ public class InlineList extends SpelNodeImpl { return null; } } - else { + else if (!(child instanceof OpMinus) || !((OpMinus) child).isNegativeNumber()) { return null; } } @@ -74,6 +76,7 @@ public class InlineList extends SpelNodeImpl { List constantList = new ArrayList<>(); int childcount = getChildCount(); + ExpressionState expressionState = new ExpressionState(new StandardEvaluationContext()); for (int c = 0; c < childcount; c++) { SpelNode child = getChild(c); if (child instanceof Literal literal) { @@ -82,6 +85,9 @@ public class InlineList extends SpelNodeImpl { else if (child instanceof InlineList inlineList) { constantList.add(inlineList.getConstantValue()); } + else if (child instanceof OpMinus) { + constantList.add(child.getValue(expressionState)); + } } return new TypedValue(Collections.unmodifiableList(constantList)); } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineMap.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineMap.java index 7c5001d0bb..1b95fc311a 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineMap.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineMap.java @@ -24,6 +24,7 @@ import org.springframework.expression.EvaluationException; import org.springframework.expression.TypedValue; import org.springframework.expression.spel.ExpressionState; import org.springframework.expression.spel.SpelNode; +import org.springframework.expression.spel.support.StandardEvaluationContext; import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -33,6 +34,7 @@ import org.springframework.util.Assert; * @author Andy Clement * @author Sam Brannen * @author Harry Yang + * @author Semyon Danilov * @since 4.1 */ public class InlineMap extends SpelNodeImpl { @@ -69,13 +71,16 @@ public class InlineMap extends SpelNodeImpl { } } else if (!(c % 2 == 0 && child instanceof PropertyOrFieldReference)) { - return null; + if (!(child instanceof OpMinus) || !((OpMinus) child).isNegativeNumber()) { + return null; + } } } } Map constantMap = new LinkedHashMap<>(); int childCount = getChildCount(); + ExpressionState expressionState = new ExpressionState(new StandardEvaluationContext()); for (int c = 0; c < childCount; c++) { SpelNode keyChild = getChild(c++); Object key; @@ -85,6 +90,9 @@ public class InlineMap extends SpelNodeImpl { else if (keyChild instanceof PropertyOrFieldReference propertyOrFieldReference) { key = propertyOrFieldReference.getName(); } + else if (keyChild instanceof OpMinus) { + key = keyChild.getValue(expressionState); + } else { return null; } @@ -100,6 +108,9 @@ public class InlineMap extends SpelNodeImpl { else if (valueChild instanceof InlineMap inlineMap) { value = inlineMap.getConstantValue(); } + else if (valueChild instanceof OpMinus) { + value = valueChild.getValue(expressionState); + } constantMap.put(key, value); } return new TypedValue(Collections.unmodifiableMap(constantMap)); diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Literal.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Literal.java index 6455f3a9d9..4eb859dad2 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Literal.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Literal.java @@ -29,6 +29,7 @@ import org.springframework.lang.Nullable; * * @author Andy Clement * @author Juergen Hoeller + * @author Semyon Danilov */ public abstract class Literal extends SpelNodeImpl { @@ -110,4 +111,15 @@ public abstract class Literal extends SpelNodeImpl { } } + /** + * Check whether this literal is a number. + * @return true if this is a number + */ + public boolean isNumberLiteral() { + return this instanceof IntLiteral || + this instanceof LongLiteral || + this instanceof FloatLiteral || + this instanceof RealLiteral; + } + } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpMinus.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpMinus.java index 1098e15897..748f1de532 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpMinus.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpMinus.java @@ -44,6 +44,7 @@ import org.springframework.util.NumberUtils; * @author Juergen Hoeller * @author Giovanni Dall'Oglio Risso * @author Sam Brannen + * @author Semyon Danilov * @since 3.0 */ public class OpMinus extends Operator { @@ -205,4 +206,15 @@ public class OpMinus extends Operator { cf.pushDescriptor(this.exitTypeDescriptor); } + /** + * Check whether this operator is an unary minus and it's child is a number. + * @return true if it is a negative number + */ + public boolean isNegativeNumber() { + if (children.length == 1 && children[0] instanceof Literal) { + return ((Literal) children[0]).isNumberLiteral(); + } + return false; + } + } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/ast/InlineCollectionTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/ast/InlineCollectionTests.java new file mode 100644 index 0000000000..24187c06b8 --- /dev/null +++ b/spring-expression/src/test/java/org/springframework/expression/spel/ast/InlineCollectionTests.java @@ -0,0 +1,175 @@ +/* + * Copyright 2002-2020 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 + * + * https://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.ast; + +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import org.springframework.expression.ExpressionParser; +import org.springframework.expression.spel.ExpressionState; +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 static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Semyon Danilov + */ +public class InlineCollectionTests { + + @Test + public void testListCached() { + InlineList list = parseList("{1, -2, 3, 4}"); + assertThat(list.isConstant()).isTrue(); + assertThat(list.getConstantValue()).isEqualTo(Arrays.asList(1, -2, 3, 4)); + } + + @Test + public void testDynamicListNotCached() { + InlineList list = parseList("{1, 5-2, 3, 4}"); + assertThat(list.isConstant()).isFalse(); + assertThat(list.getValue(null)).isEqualTo(Arrays.asList(1, 3, 3, 4)); + } + + @Test + public void testListWithVariableNotCached() { + InlineList list = parseList("{1, -a, 3, 4}"); + assertThat(list.isConstant()).isFalse(); + final StandardEvaluationContext standardEvaluationContext = new StandardEvaluationContext(new AHolder()); + standardEvaluationContext.setVariable("a", 2); + assertThat(list.getValue(new ExpressionState(standardEvaluationContext))).isEqualTo(Arrays.asList(1, -2, 3, 4)); + } + + @Test + public void testListCanBeCompiled() { + SpelExpression listExpression = parseExpression("{1, -2, 3, 4}"); + assertThat(((SpelNodeImpl) listExpression.getAST()).isCompilable()).isTrue(); + assertThat(SpelCompiler.compile(listExpression)).isTrue(); + } + + @Test + public void testDynamicListCantBeCompiled() { + SpelExpression listExpression = parseExpression("{1, 5-2, 3, 4}"); + assertThat(((SpelNodeImpl) listExpression.getAST()).isCompilable()).isFalse(); + assertThat(SpelCompiler.compile(listExpression)).isFalse(); + } + + @Test + public void testMapCached() { + InlineMap map = parseMap("{1 : 2, 3 : 4}"); + assertThat(map.isConstant()).isTrue(); + final Map expected = new HashMap<>(); + expected.put(1, 2); + expected.put(3, 4); + assertThat(map.getValue(null)).isEqualTo(expected); + } + + @Test + public void testMapWithNegativeKeyCached() { + InlineMap map = parseMap("{-1 : 2, -3 : 4}"); + assertThat(map.isConstant()).isTrue(); + final Map expected = new HashMap<>(); + expected.put(-1, 2); + expected.put(-3, 4); + assertThat(map.getValue(null)).isEqualTo(expected); + } + + @Test + public void testMapWithNegativeValueCached() { + InlineMap map = parseMap("{1 : -2, 3 : -4}"); + assertThat(map.isConstant()).isTrue(); + final Map expected = new HashMap<>(); + expected.put(1, -2); + expected.put(3, -4); + assertThat(map.getValue(null)).isEqualTo(expected); + } + + @Test + public void testMapWithNegativeLongTypesCached() { + InlineMap map = parseMap("{1L : -2L, 3L : -4L}"); + assertThat(map.isConstant()).isTrue(); + final Map expected = new HashMap<>(); + expected.put(1L, -2L); + expected.put(3L, -4L); + assertThat(map.getValue(null)).isEqualTo(expected); + } + + @Test + public void testMapWithNegativeFloatTypesCached() { + InlineMap map = parseMap("{-1.0f : -2.0f, -3.0f : -4.0f}"); + assertThat(map.isConstant()).isTrue(); + final Map expected = new HashMap<>(); + expected.put(-1.0f, -2.0f); + expected.put(-3.0f, -4.0f); + assertThat(map.getValue(null)).isEqualTo(expected); + } + + @Test + public void testMapWithNegativeRealTypesCached() { + InlineMap map = parseMap("{-1.0 : -2.0, -3.0 : -4.0}"); + assertThat(map.isConstant()).isTrue(); + final Map expected = new HashMap<>(); + expected.put(-1.0, -2.0); + expected.put(-3.0, -4.0); + assertThat(map.getValue(null)).isEqualTo(expected); + } + + @Test + public void testMapWithNegativeKeyAndValueCached() { + InlineMap map = parseMap("{-1 : -2, -3 : -4}"); + assertThat(map.isConstant()).isTrue(); + final Map expected = new HashMap<>(); + expected.put(-1, -2); + expected.put(-3, -4); + assertThat(map.getValue(null)).isEqualTo(expected); + } + + @Test + public void testMapWithDynamicNotCached() { + InlineMap map = parseMap("{-1 : 2, -3+1 : -4}"); + assertThat(map.isConstant()).isFalse(); + final Map expected = new HashMap<>(); + expected.put(-1, 2); + expected.put(-2, -4); + assertThat(map.getValue(null)).isEqualTo(expected); + } + + private InlineMap parseMap(String s) { + SpelExpression expression = parseExpression(s); + return (InlineMap) expression.getAST(); + } + + private InlineList parseList(String s) { + SpelExpression expression = parseExpression(s); + return (InlineList) expression.getAST(); + } + + private SpelExpression parseExpression(final String s) { + ExpressionParser parser = new SpelExpressionParser(); + return (SpelExpression) parser.parseExpression(s); + } + + private static class AHolder { + public int a = 2; + } + +}