From 4d433174eb23b34f514350422b348147b75a314a Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 21 Feb 2024 17:28:50 +0100 Subject: [PATCH] Revise null-safe index operator support in SpEL See gh-29847 --- .../expression/spel/ast/Indexer.java | 40 ++++++--- .../InternalSpelExpressionParser.java | 3 +- .../expression/spel/IndexingTests.java | 86 +++++++++++++++---- 3 files changed, 98 insertions(+), 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 index 1477724b1b81..59c4fdaf21a7 100644 --- 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/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java b/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java index c998c350f1a4..167700561049 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java +++ b/spring-expression/src/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 index af57312a9b1d..ab424b3b53a4 100644 --- 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 list; + public Set set; + public String string; + public Map 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; - } - }