Polishing

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.
+	 * <p>This will speed up later getValue calls and reduce the amount of garbage
 	 * created.
 	 */
 	@Nullable

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.
+	 * <p>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<Object, Object> 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;

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"
+			);
 	}
 
 }