From 1e3099759e2d823b6dd1c0c43895abcbe3e02a12 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Sun, 20 Aug 2023 18:39:45 +0200 Subject: [PATCH] Polishing --- .../expression/spel/ast/InlineList.java | 8 +- .../expression/spel/ast/InlineMap.java | 14 +- .../expression/spel/MapTests.java | 142 +++++++++--------- 3 files changed, 78 insertions(+), 86 deletions(-) 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 816697f4831..0821c5a2604 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 @@ -40,7 +40,6 @@ import org.springframework.util.Assert; */ public class InlineList extends SpelNodeImpl { - // If the list is purely literals, it is a constant value and can be computed and cached @Nullable private final TypedValue constant; @@ -52,10 +51,9 @@ public class InlineList extends SpelNodeImpl { /** - * If all the components of the list are constants, or lists - * that themselves contain constants, then a constant list - * can be built to represent this node. This will speed up - * later getValue calls and reduce the amount of garbage + * If all the components of the list are constants, or lists that themselves + * contain constants, then a constant list can be built to represent this node. + *

This will speed up later getValue calls and reduce the amount of garbage * created. */ @Nullable 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 5bb7043cd26..7c5001d0bbc 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 @@ -37,21 +37,21 @@ import org.springframework.util.Assert; */ public class InlineMap extends SpelNodeImpl { - // If the map is purely literals, it is a constant value and can be computed and cached @Nullable private final TypedValue constant; public InlineMap(int startPos, int endPos, SpelNodeImpl... args) { super(startPos, endPos, args); - this.constant = computeConstantValue(); + this.constant = computeConstantValue(); } /** * If all the components of the map are constants, or lists/maps that themselves * contain constants, then a constant list can be built to represent this node. - * This will speed up later getValue calls and reduce the amount of garbage created. + *

This will speed up later getValue calls and reduce the amount of garbage + * created. */ @Nullable private TypedValue computeConstantValue() { @@ -78,9 +78,7 @@ public class InlineMap extends SpelNodeImpl { int childCount = getChildCount(); for (int c = 0; c < childCount; c++) { SpelNode keyChild = getChild(c++); - SpelNode valueChild = getChild(c); Object key; - Object value = null; if (keyChild instanceof Literal literal) { key = literal.getLiteralValue().getValue(); } @@ -90,6 +88,9 @@ public class InlineMap extends SpelNodeImpl { else { return null; } + + SpelNode valueChild = getChild(c); + Object value = null; if (valueChild instanceof Literal literal) { value = literal.getLiteralValue().getValue(); } @@ -113,7 +114,6 @@ public class InlineMap extends SpelNodeImpl { Map returnValue = new LinkedHashMap<>(); int childcount = getChildCount(); for (int c = 0; c < childcount; c++) { - // TODO allow for key being PropertyOrFieldReference like Indexer on maps SpelNode keyChild = getChild(c++); Object key = null; if (keyChild instanceof PropertyOrFieldReference reference) { @@ -146,7 +146,7 @@ public class InlineMap extends SpelNodeImpl { } /** - * Return whether this list is a constant value. + * Return whether this map is a constant value. */ public boolean isConstant() { return this.constant != null; diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/MapTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/MapTests.java index 66b7079de3a..e78736e1496 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/MapTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/MapTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2023 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. @@ -24,6 +24,7 @@ import java.util.Map; import org.junit.jupiter.api.Test; +import org.springframework.expression.Expression; import org.springframework.expression.spel.ast.InlineMap; import org.springframework.expression.spel.standard.SpelExpression; import org.springframework.expression.spel.standard.SpelExpressionParser; @@ -35,33 +36,26 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; * Test usage of inline maps. * * @author Andy Clement + * @author Sam Brannen * @since 4.1 */ -public class MapTests extends AbstractExpressionTests { +class MapTests extends AbstractExpressionTests { - // if the list is full of literals then it will be of the type unmodifiableClass + // if the list is full of literals then it will be of the type unmodifiableMapClass // rather than HashMap (or similar) - Class unmodifiableClass = Collections.unmodifiableMap(new LinkedHashMap<>()).getClass(); + private static final Class unmodifiableMapClass = Collections.unmodifiableMap(Map.of()).getClass(); @Test - public void testInlineMapCreation01() { - evaluate("{'a':1, 'b':2, 'c':3, 'd':4, 'e':5}", "{a=1, b=2, c=3, d=4, e=5}", unmodifiableClass); - evaluate("{'a':1}", "{a=1}", unmodifiableClass); + void inlineMapCreationForLiterals() { + evaluate("{'a':1, 'b':2, 'c':3, 'd':4, 'e':5}", "{a=1, b=2, c=3, d=4, e=5}", unmodifiableMapClass); + evaluate("{'a':1}", "{a=1}", unmodifiableMapClass); + evaluate("{'abc':'def', 'uvw':'xyz'}", "{abc=def, uvw=xyz}", unmodifiableMapClass); + evaluate("{:}", "{}", unmodifiableMapClass); } @Test - public void testInlineMapCreation02() { - evaluate("{'abc':'def', 'uvw':'xyz'}", "{abc=def, uvw=xyz}", unmodifiableClass); - } - - @Test - public void testInlineMapCreation03() { - evaluate("{:}", "{}", unmodifiableClass); - } - - @Test - public void testInlineMapCreation04() { + void inlineMapCreationForNonLiterals() { evaluate("{'key':'abc'=='xyz'}", "{key=false}", LinkedHashMap.class); evaluate("{key:'abc'=='xyz'}", "{key=false}", LinkedHashMap.class); evaluate("{key:'abc'=='xyz',key2:true}[key]", "false", Boolean.class); @@ -70,43 +64,48 @@ public class MapTests extends AbstractExpressionTests { } @Test - public void testInlineMapAndNesting() { - evaluate("{a:{a:1,b:2,c:3},b:{d:4,e:5,f:6}}", "{a={a=1, b=2, c=3}, b={d=4, e=5, f=6}}", unmodifiableClass); - evaluate("{a:{x:1,y:'2',z:3},b:{u:4,v:{'a','b'},w:5,x:6}}", "{a={x=1, y=2, z=3}, b={u=4, v=[a, b], w=5, x=6}}", unmodifiableClass); - evaluate("{a:{1,2,3},b:{4,5,6}}", "{a=[1, 2, 3], b=[4, 5, 6]}", unmodifiableClass); + void inlineMapAndNesting() { + evaluate("{a:{a:1,b:2,c:3},b:{d:4,e:5,f:6}}", "{a={a=1, b=2, c=3}, b={d=4, e=5, f=6}}", unmodifiableMapClass); + evaluate("{a:{x:1,y:'2',z:3},b:{u:4,v:{'a','b'},w:5,x:6}}", "{a={x=1, y=2, z=3}, b={u=4, v=[a, b], w=5, x=6}}", unmodifiableMapClass); + evaluate("{a:{1,2,3},b:{4,5,6}}", "{a=[1, 2, 3], b=[4, 5, 6]}", unmodifiableMapClass); } @Test - public void testInlineMapWithFunkyKeys() { - evaluate("{#root.name:true}","{Nikola Tesla=true}",LinkedHashMap.class); + void inlineMapWithFunkyKeys() { + evaluate("{#root.name:true}", "{Nikola Tesla=true}", LinkedHashMap.class); } @Test - public void testInlineMapError() { + void inlineMapSyntaxError() { parseAndCheckError("{key:'abc'", SpelMessage.OOD); } @Test - public void testRelOperatorsIs02() { - evaluate("{a:1, b:2, c:3, d:4, e:5} instanceof T(java.util.Map)", "true", Boolean.class); + void inelineMapIsInstanceOfMap() { + evaluate("{a:1, b:2} instanceof T(java.util.Map)", "true", Boolean.class); } @Test - public void testInlineMapAndProjectionSelection() { - evaluate("{a:1,b:2,c:3,d:4,e:5,f:6}.![value>3]", "[false, false, false, true, true, true]", ArrayList.class); - evaluate("{a:1,b:2,c:3,d:4,e:5,f:6}.?[value>3]", "{d=4, e=5, f=6}", HashMap.class); - evaluate("{a:1,b:2,c:3,d:4,e:5,f:6,g:7,h:8,i:9,j:10}.?[value%2==0]", "{b=2, d=4, f=6, h=8, j=10}", HashMap.class); - // TODO this looks like a serious issue (but not a new one): the context object against which arguments are evaluated seems wrong: -// evaluate("{a:1,b:2,c:3,d:4,e:5,f:6,g:7,h:8,i:9,j:10}.?[isEven(value) == 'y']", "[2, 4, 6, 8, 10]", ArrayList.class); + void inlineMapProjection() { + evaluate("{a:1,b:2,c:3,d:4}.![value > 2]", "[false, false, true, true]", ArrayList.class); + evaluate("{a:1,b:2,c:3,d:4}.![value % 2 == 0]", "[false, true, false, true]", ArrayList.class); + evaluate("{a:1,b:2,c:3,d:4}.![#isEven(value) == 'y']", "[false, true, false, true]", ArrayList.class); } @Test - public void testSetConstruction01() { - evaluate("new java.util.HashMap().putAll({a:'a',b:'b',c:'c'})", null, Object.class); + void inlineMapSelection() { + evaluate("{a:1,b:2,c:3,d:4}.?[value > 2]", "{c=3, d=4}", HashMap.class); + evaluate("{a:1,b:2,c:3,d:4,e:5,f:6}.?[value % 2 == 0]", "{b=2, d=4, f=6}", HashMap.class); + evaluate("{a:1,b:2,c:3,d:4,e:5,f:6}.?[#isEven(value) == 'y']", "{b=2, d=4, f=6}", HashMap.class); } @Test - public void testConstantRepresentation1() { + void mapConstruction() { + evaluate("new java.util.HashMap().putAll({a:'a',b:'b'})", null, Object.class); + } + + @Test + void constantMaps() { checkConstantMap("{f:{'a','b','c'}}", true); checkConstantMap("{'a':1,'b':2,'c':3,'d':4,'e':5}", true); checkConstantMap("{aaa:'abc'}", true); @@ -117,8 +116,7 @@ public class MapTests extends AbstractExpressionTests { checkConstantMap("{#root.name:true}",false); checkConstantMap("{a:1,b:2,c:{d:true,e:false}}", true); checkConstantMap("{a:1,b:2,c:{d:{1,2,3},e:{4,5,6},f:{'a','b','c'}}}", true); - // for nested InlineMap - checkConstantMap("{a:{k:#d}}", false); + checkConstantMap("{a:{k:#d}}", false); // nested InlineMap checkConstantMap("{@bean:@bean}", false); } @@ -126,34 +124,29 @@ public class MapTests extends AbstractExpressionTests { SpelExpressionParser parser = new SpelExpressionParser(); SpelExpression expression = (SpelExpression) parser.parseExpression(expressionText); SpelNode node = expression.getAST(); - boolean condition = node instanceof InlineMap; - assertThat(condition).isTrue(); - InlineMap inlineMap = (InlineMap) node; - if (expectedToBeConstant) { - assertThat(inlineMap.isConstant()).isTrue(); - } - else { - assertThat(inlineMap.isConstant()).isFalse(); - } + assertThat(node).isInstanceOfSatisfying(InlineMap.class, inlineMap -> { + if (expectedToBeConstant) { + assertThat(inlineMap.isConstant()).isTrue(); + } + else { + assertThat(inlineMap.isConstant()).isFalse(); + } + }); } @Test - public void testInlineMapWriting() { - // list should be unmodifiable - assertThatExceptionOfType(UnsupportedOperationException.class).isThrownBy(() -> - evaluate("{a:1, b:2, c:3, d:4, e:5}[a]=6", "[a:1,b: 2,c: 3,d: 4,e: 5]", unmodifiableClass)); + void inlineMapIsUnmodifiable() { + Expression expr = parser.parseExpression("{a:1}[a] = 6"); + assertThatExceptionOfType(UnsupportedOperationException.class) + .isThrownBy(() -> expr.getValue(context)); } @Test - public void testMapKeysThatAreAlsoSpELKeywords() { + void mapKeysThatAreAlsoSpELKeywords() { SpelExpressionParser parser = new SpelExpressionParser(); SpelExpression expression = null; Object o = null; - // expression = (SpelExpression) parser.parseExpression("foo['NEW']"); - // o = expression.getValue(new MapHolder()); - // assertEquals("VALUE",o); - expression = (SpelExpression) parser.parseExpression("foo[T]"); o = expression.getValue(new MapHolder()); assertThat(o).isEqualTo("TV"); @@ -162,6 +155,10 @@ public class MapTests extends AbstractExpressionTests { o = expression.getValue(new MapHolder()); assertThat(o).isEqualTo("tv"); + expression = (SpelExpression) parser.parseExpression("foo['NEW']"); + o = expression.getValue(new MapHolder()); + assertThat(o).isEqualTo("VALUE"); + expression = (SpelExpression) parser.parseExpression("foo[NEW]"); o = expression.getValue(new MapHolder()); assertThat(o).isEqualTo("VALUE"); @@ -174,35 +171,32 @@ public class MapTests extends AbstractExpressionTests { o = expression.getValue(new MapHolder()); assertThat(o).isEqualTo("value"); - expression = (SpelExpression)parser.parseExpression("foo[foo[NEW]]"); + expression = (SpelExpression) parser.parseExpression("foo[foo[NEW]]"); o = expression.getValue(new MapHolder()); assertThat(o).isEqualTo("37"); - expression = (SpelExpression)parser.parseExpression("foo[foo[new]]"); + expression = (SpelExpression) parser.parseExpression("foo[foo[new]]"); o = expression.getValue(new MapHolder()); assertThat(o).isEqualTo("38"); - expression = (SpelExpression)parser.parseExpression("foo[foo[foo[T]]]"); + expression = (SpelExpression) parser.parseExpression("foo[foo[foo[T]]]"); o = expression.getValue(new MapHolder()); assertThat(o).isEqualTo("value"); } @SuppressWarnings({ "rawtypes", "unchecked" }) - public static class MapHolder { - - public Map foo; - - public MapHolder() { - foo = new HashMap(); - foo.put("NEW", "VALUE"); - foo.put("new", "value"); - foo.put("T", "TV"); - foo.put("t", "tv"); - foo.put("abc.def", "value"); - foo.put("VALUE","37"); - foo.put("value","38"); - foo.put("TV","new"); - } + static class MapHolder { + + public Map foo = Map.of( + "NEW", "VALUE", + "new", "value", + "T", "TV", + "t", "tv", + "abc.def", "value", + "VALUE","37", + "value","38", + "TV","new" + ); } }