From 5c6b82a9479522546161df3921368da812b2746c Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 27 Mar 2024 10:45:21 +0100 Subject: [PATCH] Change IndexAccessor#read()'s return type to TypedValue Prior to this commit, the read() method in the IndexAccessor SPI declared a return type of ValueRef which introduced a package cycle. This commit addresses this by aligning with the PropertyAccess SPI and returning TypedValue from IndexAccessor's read() method. This commit also reworks the internals of Indexer based on a new, local IndexAccessorValueRef implementation. See gh-26409 See gh-26478 --- .../expression/IndexAccessor.java | 4 +- .../expression/spel/ast/Indexer.java | 63 +++++++++++++++++-- .../expression/spel/IndexingTests.java | 40 +++--------- 3 files changed, 68 insertions(+), 39 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java b/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java index b75ae537a0..9d33feeadc 100644 --- a/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java @@ -16,7 +16,6 @@ package org.springframework.expression; -import org.springframework.expression.spel.ast.ValueRef; import org.springframework.lang.Nullable; /** @@ -80,8 +79,7 @@ public interface IndexAccessor extends TargetedAccessor { * descriptor for it * @throws AccessException if there is any problem reading the index value */ - // TODO Change return type to TypedValue to avoid package cycle. - ValueRef read(EvaluationContext context, Object target, Object index) throws AccessException; + TypedValue read(EvaluationContext context, Object target, Object index) throws AccessException; /** * Called to determine if this index accessor is able to write to a specified 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 88f534409e..cfd7d44986 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 @@ -158,14 +158,14 @@ public class Indexer extends SpelNodeImpl { throws EvaluationException { TypedValue typedValue = valueSupplier.get(); - // TODO Set value for IndexAccessor via its write() method, NOT via the ValueRef returned from its read() method. + // TODO Query IndexAccessor's canWrite() method before invoking its write() method. getValueRef(state).setValue(typedValue.getValue()); return typedValue; } @Override public boolean isWritable(ExpressionState expressionState) throws SpelEvaluationException { - return true; + return getValueRef(expressionState).isWritable(); } @@ -254,13 +254,13 @@ public class Indexer extends SpelNodeImpl { try { for (IndexAccessor indexAccessor : accessorsToTry) { if (indexAccessor.canRead(evalContext, target, index)) { - // TODO Introduce local IndexAccessorValueRef. - return indexAccessor.read(evalContext, target, index); + return new IndexAccessorValueRef(indexAccessor, target, index, evalContext, targetDescriptor); } } } catch (Exception ex) { - // TODO throw SpelEvaluationException for "exception during index access" + // TODO throw SpelEvaluationException for "exception during index access", + // analogous to SpelMessage.EXCEPTION_DURING_PROPERTY_READ. } throw new SpelEvaluationException( @@ -874,4 +874,57 @@ public class Indexer extends SpelNodeImpl { } } + + private class IndexAccessorValueRef implements ValueRef { + + private final IndexAccessor indexAccessor; + + private final Object target; + + private final Object index; + + private final EvaluationContext evaluationContext; + + private final TypeDescriptor typeDescriptor; + + + IndexAccessorValueRef(IndexAccessor indexAccessor, Object target, Object index, + EvaluationContext evaluationContext, TypeDescriptor typeDescriptor) { + + this.indexAccessor = indexAccessor; + this.target = target; + this.index = index; + this.evaluationContext = evaluationContext; + this.typeDescriptor = typeDescriptor; + } + + + @Override + public TypedValue getValue() { + try { + return this.indexAccessor.read(this.evaluationContext, this.target, this.index); + } + catch (AccessException ex) { + throw new SpelEvaluationException(getStartPosition(), ex, + SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, this.typeDescriptor.toString()); + } + } + + @Override + public void setValue(@Nullable Object newValue) { + try { + this.indexAccessor.write(this.evaluationContext, this.target, this.index, newValue); + } + catch (AccessException ex) { + throw new SpelEvaluationException(getStartPosition(), ex, + SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, this.typeDescriptor.toString()); + } + } + + @Override + public boolean isWritable() { + return true; + } + } + } 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 22e7717444..c349765a1a 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 @@ -41,10 +41,10 @@ import org.springframework.expression.Expression; import org.springframework.expression.IndexAccessor; import org.springframework.expression.PropertyAccessor; import org.springframework.expression.TypedValue; -import org.springframework.expression.spel.ast.ValueRef; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.StandardEvaluationContext; import org.springframework.expression.spel.testresources.Person; +import org.springframework.lang.Nullable; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; @@ -543,45 +543,23 @@ class IndexingTests { } @Override - public ValueRef read(EvaluationContext context, Object target, Object index) { - return new ArrayNodeValueRef((ArrayNode) target, (Integer) index, this.objectMapper); + public TypedValue read(EvaluationContext context, Object target, Object index) { + ArrayNode arrayNode = (ArrayNode) target; + Integer intIndex = (Integer) index; + return new TypedValue(arrayNode.get(intIndex)); } @Override public boolean canWrite(EvaluationContext context, Object target, Object index) { - return (target instanceof ArrayNode && index instanceof Integer); + return canRead(context, target, index); } @Override - public void write(EvaluationContext context, Object target, Object index, Object newValue) { - if (!(target instanceof ArrayNode arrayNode)) { - throw new IllegalStateException("target must be an ArrayNode: " + target.getClass().getName()); - } - if (!(index instanceof Integer intIndex)) { - throw new IllegalStateException("index must be an integer: " + target.getClass().getName()); - } - + public void write(EvaluationContext context, Object target, Object index, @Nullable Object newValue) { + ArrayNode arrayNode = (ArrayNode) target; + Integer intIndex = (Integer) index; arrayNode.set(intIndex, this.objectMapper.convertValue(newValue, JsonNode.class)); } - - private record ArrayNodeValueRef(ArrayNode arrayNode, int index, ObjectMapper objectMapper) implements ValueRef { - - @Override - public TypedValue getValue() { - return new TypedValue(this.arrayNode.get(this.index)); - } - - @Override - public void setValue(Object newValue) { - // TODO throw new UnsupportedOperationException("setValue() is not supported"); - this.arrayNode.set(index, this.objectMapper.convertValue(newValue, JsonNode.class)); - } - - @Override - public boolean isWritable() { - return true; - } - } } }