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 spring-projectsgh-26409
See spring-projectsgh-26478

Author: sbrannen

Diff for: 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 for: spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java

@@ -158,14 +158,14 @@ public TypedValue setValueInternal(ExpressionState state, Supplier<TypedValue> v
 			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 @@ else if (target instanceof Collection<?> collection) {
 		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 boolean isWritable() {
 		}
 	}
 
+
+	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 for: spring-expression/src/test/java/org/springframework/expression/spel/IndexingTests.java

@@ -41,10 +41,10 @@
 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 @@ public boolean canRead(EvaluationContext context, Object target, Object index) {
 			}
 
 			@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;
-				}
-			}
 		}
 	}