Skip to content

Commit 6f95467

Browse files
SammyVimessdeleuze
SammyVimes
authored and
sdeleuze
committed
Improve SpEL inline collection caching
This commit fixes SpEL inline collection caching with with negative keys or values. Closes gh-25921
1 parent 6548cee commit 6f95467

File tree

5 files changed

+218
-2
lines changed

5 files changed

+218
-2
lines changed

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.springframework.expression.spel.CodeFlow;
2828
import org.springframework.expression.spel.ExpressionState;
2929
import org.springframework.expression.spel.SpelNode;
30+
import org.springframework.expression.spel.support.StandardEvaluationContext;
3031
import org.springframework.lang.Nullable;
3132
import org.springframework.util.Assert;
3233

@@ -36,6 +37,7 @@
3637
* @author Andy Clement
3738
* @author Sam Brannen
3839
* @author Harry Yang
40+
* @author Semyon Danilov
3941
* @since 3.0.4
4042
*/
4143
public class InlineList extends SpelNodeImpl {
@@ -66,14 +68,15 @@ private TypedValue computeConstantValue() {
6668
return null;
6769
}
6870
}
69-
else {
71+
else if (!(child instanceof OpMinus) || !((OpMinus) child).isNegativeNumber()) {
7072
return null;
7173
}
7274
}
7375
}
7476

7577
List<Object> constantList = new ArrayList<>();
7678
int childcount = getChildCount();
79+
ExpressionState expressionState = new ExpressionState(new StandardEvaluationContext());
7780
for (int c = 0; c < childcount; c++) {
7881
SpelNode child = getChild(c);
7982
if (child instanceof Literal literal) {
@@ -82,6 +85,9 @@ private TypedValue computeConstantValue() {
8285
else if (child instanceof InlineList inlineList) {
8386
constantList.add(inlineList.getConstantValue());
8487
}
88+
else if (child instanceof OpMinus) {
89+
constantList.add(child.getValue(expressionState));
90+
}
8591
}
8692
return new TypedValue(Collections.unmodifiableList(constantList));
8793
}

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.springframework.expression.TypedValue;
2525
import org.springframework.expression.spel.ExpressionState;
2626
import org.springframework.expression.spel.SpelNode;
27+
import org.springframework.expression.spel.support.StandardEvaluationContext;
2728
import org.springframework.lang.Nullable;
2829
import org.springframework.util.Assert;
2930

@@ -33,6 +34,7 @@
3334
* @author Andy Clement
3435
* @author Sam Brannen
3536
* @author Harry Yang
37+
* @author Semyon Danilov
3638
* @since 4.1
3739
*/
3840
public class InlineMap extends SpelNodeImpl {
@@ -69,13 +71,16 @@ else if (child instanceof InlineMap inlineMap) {
6971
}
7072
}
7173
else if (!(c % 2 == 0 && child instanceof PropertyOrFieldReference)) {
72-
return null;
74+
if (!(child instanceof OpMinus) || !((OpMinus) child).isNegativeNumber()) {
75+
return null;
76+
}
7377
}
7478
}
7579
}
7680

7781
Map<Object, Object> constantMap = new LinkedHashMap<>();
7882
int childCount = getChildCount();
83+
ExpressionState expressionState = new ExpressionState(new StandardEvaluationContext());
7984
for (int c = 0; c < childCount; c++) {
8085
SpelNode keyChild = getChild(c++);
8186
Object key;
@@ -85,6 +90,9 @@ else if (!(c % 2 == 0 && child instanceof PropertyOrFieldReference)) {
8590
else if (keyChild instanceof PropertyOrFieldReference propertyOrFieldReference) {
8691
key = propertyOrFieldReference.getName();
8792
}
93+
else if (keyChild instanceof OpMinus) {
94+
key = keyChild.getValue(expressionState);
95+
}
8896
else {
8997
return null;
9098
}
@@ -100,6 +108,9 @@ else if (valueChild instanceof InlineList inlineList) {
100108
else if (valueChild instanceof InlineMap inlineMap) {
101109
value = inlineMap.getConstantValue();
102110
}
111+
else if (valueChild instanceof OpMinus) {
112+
value = valueChild.getValue(expressionState);
113+
}
103114
constantMap.put(key, value);
104115
}
105116
return new TypedValue(Collections.unmodifiableMap(constantMap));

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
*
3030
* @author Andy Clement
3131
* @author Juergen Hoeller
32+
* @author Semyon Danilov
3233
*/
3334
public abstract class Literal extends SpelNodeImpl {
3435

@@ -110,4 +111,15 @@ public static Literal getRealLiteral(String numberToken, int startPos, int endPo
110111
}
111112
}
112113

114+
/**
115+
* Check whether this literal is a number.
116+
* @return true if this is a number
117+
*/
118+
public boolean isNumberLiteral() {
119+
return this instanceof IntLiteral ||
120+
this instanceof LongLiteral ||
121+
this instanceof FloatLiteral ||
122+
this instanceof RealLiteral;
123+
}
124+
113125
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
* @author Juergen Hoeller
4545
* @author Giovanni Dall'Oglio Risso
4646
* @author Sam Brannen
47+
* @author Semyon Danilov
4748
* @since 3.0
4849
*/
4950
public class OpMinus extends Operator {
@@ -205,4 +206,15 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
205206
cf.pushDescriptor(this.exitTypeDescriptor);
206207
}
207208

209+
/**
210+
* Check whether this operator is an unary minus and it's child is a number.
211+
* @return true if it is a negative number
212+
*/
213+
public boolean isNegativeNumber() {
214+
if (children.length == 1 && children[0] instanceof Literal) {
215+
return ((Literal) children[0]).isNumberLiteral();
216+
}
217+
return false;
218+
}
219+
208220
}

‎spring-expression/src/test/java/org/springframework/expression/spel/ast/InlineCollectionTests.java

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
/*
2+
* Copyright 2002-2020 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.expression.spel.ast;
18+
19+
import java.util.Arrays;
20+
import java.util.HashMap;
21+
import java.util.Map;
22+
23+
import org.junit.jupiter.api.Test;
24+
25+
import org.springframework.expression.ExpressionParser;
26+
import org.springframework.expression.spel.ExpressionState;
27+
import org.springframework.expression.spel.standard.SpelCompiler;
28+
import org.springframework.expression.spel.standard.SpelExpression;
29+
import org.springframework.expression.spel.standard.SpelExpressionParser;
30+
import org.springframework.expression.spel.support.StandardEvaluationContext;
31+
32+
import static org.assertj.core.api.Assertions.assertThat;
33+
34+
/**
35+
* @author Semyon Danilov
36+
*/
37+
public class InlineCollectionTests {
38+
39+
@Test
40+
public void testListCached() {
41+
InlineList list = parseList("{1, -2, 3, 4}");
42+
assertThat(list.isConstant()).isTrue();
43+
assertThat(list.getConstantValue()).isEqualTo(Arrays.asList(1, -2, 3, 4));
44+
}
45+
46+
@Test
47+
public void testDynamicListNotCached() {
48+
InlineList list = parseList("{1, 5-2, 3, 4}");
49+
assertThat(list.isConstant()).isFalse();
50+
assertThat(list.getValue(null)).isEqualTo(Arrays.asList(1, 3, 3, 4));
51+
}
52+
53+
@Test
54+
public void testListWithVariableNotCached() {
55+
InlineList list = parseList("{1, -a, 3, 4}");
56+
assertThat(list.isConstant()).isFalse();
57+
final StandardEvaluationContext standardEvaluationContext = new StandardEvaluationContext(new AHolder());
58+
standardEvaluationContext.setVariable("a", 2);
59+
assertThat(list.getValue(new ExpressionState(standardEvaluationContext))).isEqualTo(Arrays.asList(1, -2, 3, 4));
60+
}
61+
62+
@Test
63+
public void testListCanBeCompiled() {
64+
SpelExpression listExpression = parseExpression("{1, -2, 3, 4}");
65+
assertThat(((SpelNodeImpl) listExpression.getAST()).isCompilable()).isTrue();
66+
assertThat(SpelCompiler.compile(listExpression)).isTrue();
67+
}
68+
69+
@Test
70+
public void testDynamicListCantBeCompiled() {
71+
SpelExpression listExpression = parseExpression("{1, 5-2, 3, 4}");
72+
assertThat(((SpelNodeImpl) listExpression.getAST()).isCompilable()).isFalse();
73+
assertThat(SpelCompiler.compile(listExpression)).isFalse();
74+
}
75+
76+
@Test
77+
public void testMapCached() {
78+
InlineMap map = parseMap("{1 : 2, 3 : 4}");
79+
assertThat(map.isConstant()).isTrue();
80+
final Map<Integer, Integer> expected = new HashMap<>();
81+
expected.put(1, 2);
82+
expected.put(3, 4);
83+
assertThat(map.getValue(null)).isEqualTo(expected);
84+
}
85+
86+
@Test
87+
public void testMapWithNegativeKeyCached() {
88+
InlineMap map = parseMap("{-1 : 2, -3 : 4}");
89+
assertThat(map.isConstant()).isTrue();
90+
final Map<Integer, Integer> expected = new HashMap<>();
91+
expected.put(-1, 2);
92+
expected.put(-3, 4);
93+
assertThat(map.getValue(null)).isEqualTo(expected);
94+
}
95+
96+
@Test
97+
public void testMapWithNegativeValueCached() {
98+
InlineMap map = parseMap("{1 : -2, 3 : -4}");
99+
assertThat(map.isConstant()).isTrue();
100+
final Map<Integer, Integer> expected = new HashMap<>();
101+
expected.put(1, -2);
102+
expected.put(3, -4);
103+
assertThat(map.getValue(null)).isEqualTo(expected);
104+
}
105+
106+
@Test
107+
public void testMapWithNegativeLongTypesCached() {
108+
InlineMap map = parseMap("{1L : -2L, 3L : -4L}");
109+
assertThat(map.isConstant()).isTrue();
110+
final Map<Long, Long> expected = new HashMap<>();
111+
expected.put(1L, -2L);
112+
expected.put(3L, -4L);
113+
assertThat(map.getValue(null)).isEqualTo(expected);
114+
}
115+
116+
@Test
117+
public void testMapWithNegativeFloatTypesCached() {
118+
InlineMap map = parseMap("{-1.0f : -2.0f, -3.0f : -4.0f}");
119+
assertThat(map.isConstant()).isTrue();
120+
final Map<Float, Float> expected = new HashMap<>();
121+
expected.put(-1.0f, -2.0f);
122+
expected.put(-3.0f, -4.0f);
123+
assertThat(map.getValue(null)).isEqualTo(expected);
124+
}
125+
126+
@Test
127+
public void testMapWithNegativeRealTypesCached() {
128+
InlineMap map = parseMap("{-1.0 : -2.0, -3.0 : -4.0}");
129+
assertThat(map.isConstant()).isTrue();
130+
final Map<Double, Double> expected = new HashMap<>();
131+
expected.put(-1.0, -2.0);
132+
expected.put(-3.0, -4.0);
133+
assertThat(map.getValue(null)).isEqualTo(expected);
134+
}
135+
136+
@Test
137+
public void testMapWithNegativeKeyAndValueCached() {
138+
InlineMap map = parseMap("{-1 : -2, -3 : -4}");
139+
assertThat(map.isConstant()).isTrue();
140+
final Map<Integer, Integer> expected = new HashMap<>();
141+
expected.put(-1, -2);
142+
expected.put(-3, -4);
143+
assertThat(map.getValue(null)).isEqualTo(expected);
144+
}
145+
146+
@Test
147+
public void testMapWithDynamicNotCached() {
148+
InlineMap map = parseMap("{-1 : 2, -3+1 : -4}");
149+
assertThat(map.isConstant()).isFalse();
150+
final Map<Integer, Integer> expected = new HashMap<>();
151+
expected.put(-1, 2);
152+
expected.put(-2, -4);
153+
assertThat(map.getValue(null)).isEqualTo(expected);
154+
}
155+
156+
private InlineMap parseMap(String s) {
157+
SpelExpression expression = parseExpression(s);
158+
return (InlineMap) expression.getAST();
159+
}
160+
161+
private InlineList parseList(String s) {
162+
SpelExpression expression = parseExpression(s);
163+
return (InlineList) expression.getAST();
164+
}
165+
166+
private SpelExpression parseExpression(final String s) {
167+
ExpressionParser parser = new SpelExpressionParser();
168+
return (SpelExpression) parser.parseExpression(s);
169+
}
170+
171+
private static class AHolder {
172+
public int a = 2;
173+
}
174+
175+
}

0 commit comments

Comments
 (0)