Revise null-safe index operator support in SpEL

See spring-projectsgh-29847
sbrannen · Mar 23, 2024 · 4d43317 · 4d43317
1 parent 9f4d46f
commit 4d43317
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 31 deletions.
diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java
@@ -68,6 +68,8 @@ public class Indexer extends SpelNodeImpl {
 	private enum IndexedType {ARRAY, LIST, MAP, STRING, OBJECT}
 
 
+	private final boolean nullSafe;
+
 	@Nullable
 	private IndexedType indexedType;
 
@@ -103,11 +105,24 @@ private enum IndexedType {ARRAY, LIST, MAP, STRING, OBJECT}
 	private PropertyAccessor cachedWriteAccessor;
 
 
-	private final boolean nullSafe;
-
+	/**
+	 * Create an {@code Indexer} with the given start position, end position, and
+	 * index expression.
+	 * @see #Indexer(boolean, int, int, SpelNodeImpl)
+	 * @deprecated as of Spring Framework 6.2, in favor of {@link #Indexer(boolean, int, int, SpelNodeImpl)}
+	 */
+	@Deprecated(since = "6.2", forRemoval = true)
+	public Indexer(int startPos, int endPos, SpelNodeImpl indexExpression) {
+		this(false, startPos, endPos, indexExpression);
+	}
 
-	public Indexer(boolean nullSafe, int startPos, int endPos, SpelNodeImpl expr) {
-		super(startPos, endPos, expr);
+	/**
+	 * Create an {@code Indexer} with the given null-safe flag, start position,
+	 * end position, and index expression.
+	 * @since 6.2
+	 */
+	public Indexer(boolean nullSafe, int startPos, int endPos, SpelNodeImpl indexExpression) {
+		super(startPos, endPos, indexExpression);
 		this.nullSafe = nullSafe;
 	}
 
@@ -136,6 +151,15 @@ public boolean isWritable(ExpressionState expressionState) throws SpelEvaluation
 	protected ValueRef getValueRef(ExpressionState state) throws EvaluationException {
 		TypedValue context = state.getActiveContextObject();
 		Object target = context.getValue();
+
+		if (target == null) {
+			if (this.nullSafe) {
+				return ValueRef.NullValueRef.INSTANCE;
+			}
+			// Raise a proper exception in case of a null target
+			throw new SpelEvaluationException(getStartPosition(), SpelMessage.CANNOT_INDEX_INTO_NULL_VALUE);
+		}
+
 		TypeDescriptor targetDescriptor = context.getTypeDescriptor();
 		TypedValue indexValue;
 		Object index;
@@ -159,14 +183,6 @@ protected ValueRef getValueRef(ExpressionState state) throws EvaluationException
 			}
 		}
 
-		// Raise a proper exception in case of a null target
-		if (target == null) {
-			if (this.nullSafe) {
-				return ValueRef.NullValueRef.INSTANCE;
-			}
-			throw new SpelEvaluationException(getStartPosition(), SpelMessage.CANNOT_INDEX_INTO_NULL_VALUE);
-		}
-
 		// At this point, we need a TypeDescriptor for a non-null target object
 		Assert.state(targetDescriptor != null, "No type descriptor");
 

diff --git a/.../main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java b/.../main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java
@@ -538,8 +538,7 @@ else if (maybeEatTypeReference() || maybeEatNullReference() || maybeEatConstruct
 		else if (maybeEatBeanReference()) {
 			return pop();
 		}
-		else if (maybeEatProjection(false) || maybeEatSelection(false) ||
-				maybeEatIndexer(false)) {
+		else if (maybeEatProjection(false) || maybeEatSelection(false) || maybeEatIndexer(false)) {
 			return pop();
 		}
 		else if (maybeEatInlineListOrMap()) {

diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/IndexingTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/IndexingTests.java
@@ -26,7 +26,9 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 
 import org.springframework.expression.EvaluationContext;
@@ -35,6 +37,7 @@
 import org.springframework.expression.TypedValue;
 import org.springframework.expression.spel.standard.SpelExpressionParser;
 import org.springframework.expression.spel.support.StandardEvaluationContext;
+import org.springframework.expression.spel.testresources.Person;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
@@ -376,18 +379,74 @@ void listOfMaps() {
 		assertThat(expression.getValue(this, String.class)).isEqualTo("apple");
 	}
 
-	@Test
-	void nullSafeIndex() {
-		ContextWithNullCollections testContext = new ContextWithNullCollections();
-		StandardEvaluationContext context = new StandardEvaluationContext(testContext);
-		Expression expr = new SpelExpressionParser().parseRaw("nullList?.[4]");
-		assertThat(expr.getValue(context)).isNull();
+	@Nested
+	class NullSafeIndexTests {  // gh-29847
+
+		private final RootContextWithIndexedProperties rootContext = new RootContextWithIndexedProperties();
+
+		private final StandardEvaluationContext context = new StandardEvaluationContext(rootContext);
 
-		expr = new SpelExpressionParser().parseRaw("nullArray?.[4]");
-		assertThat(expr.getValue(context)).isNull();
+		private final SpelExpressionParser parser = new SpelExpressionParser();
+
+		private Expression expression;
+
+		@Test
+		void nullSafeIndexIntoArray() {
+			expression = parser.parseExpression("array?.[0]");
+			assertThat(expression.getValue(context)).isNull();
+			rootContext.array = new int[] {42};
+			assertThat(expression.getValue(context)).isEqualTo(42);
+		}
+
+		@Test
+		void nullSafeIndexIntoList() {
+			expression = parser.parseExpression("list?.[0]");
+			assertThat(expression.getValue(context)).isNull();
+			rootContext.list = List.of(42);
+			assertThat(expression.getValue(context)).isEqualTo(42);
+		}
+
+		@Test
+		void nullSafeIndexIntoSet() {
+			expression = parser.parseExpression("set?.[0]");
+			assertThat(expression.getValue(context)).isNull();
+			rootContext.set = Set.of(42);
+			assertThat(expression.getValue(context)).isEqualTo(42);
+		}
+
+		@Test
+		void nullSafeIndexIntoString() {
+			expression = parser.parseExpression("string?.[0]");
+			assertThat(expression.getValue(context)).isNull();
+			rootContext.string = "XYZ";
+			assertThat(expression.getValue(context)).isEqualTo("X");
+		}
+
+		@Test
+		void nullSafeIndexIntoMap() {
+			expression = parser.parseExpression("map?.['enigma']");
+			assertThat(expression.getValue(context)).isNull();
+			rootContext.map = Map.of("enigma", 42);
+			assertThat(expression.getValue(context)).isEqualTo(42);
+		}
+
+		@Test
+		void nullSafeIndexIntoObject() {
+			expression = parser.parseExpression("person?.['name']");
+			assertThat(expression.getValue(context)).isNull();
+			rootContext.person = new Person("Jane");
+			assertThat(expression.getValue(context)).isEqualTo("Jane");
+		}
+
+		static class RootContextWithIndexedProperties {
+			public int[] array;
+			public List<Integer> list;
+			public Set<Integer> set;
+			public String string;
+			public Map<String, Integer> map;
+			public Person person;
+		}
 
-		expr = new SpelExpressionParser().parseRaw("nullMap:?.[4]");
-		assertThat(expr.getValue(context)).isNull();
 	}
 
 
@@ -450,11 +509,4 @@ public Class<?>[] getSpecificTargetClasses() {
 
 	}
 
-
-	static class ContextWithNullCollections {
-		public List nullList = null;
-		public String[] nullArray = null;
-		public Map nullMap = null;
-	}
-
 }