Polishing

Author: sbrannen

spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java

@@ -19,8 +19,8 @@
 import org.springframework.lang.Nullable;
 
 /**
- * An index accessor is able to read from (and possibly write to) an indexed
- * structure of an object.
+ * An index accessor is able to read from and possibly write to an indexed
+ * structure of a target object.
  *
  * <p>This interface places no restrictions on what constitutes an indexed
  * structure. Implementors are therefore free to access indexed values any way

spring-expression/src/test/java/example/FruitMap.java

@@ -0,0 +1,50 @@
+/*
+ * Copyright 2002-2024 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.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package example;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Type that can be indexed by the {@link Color} enum (i.e., something other
+ * than an int, Integer, or String) and whose indexed values are Strings.
+ */
+public class FruitMap {
+
+	private final Map<Color, String> map = new HashMap<>();
+
+	public FruitMap() {
+		this.map.put(Color.RED, "cherry");
+		this.map.put(Color.ORANGE, "orange");
+		this.map.put(Color.YELLOW, "banana");
+		this.map.put(Color.GREEN, "kiwi");
+		this.map.put(Color.BLUE, "blueberry");
+		// We don't map PURPLE so that we can test for an unsupported color.
+	}
+
+	public String getFruit(Color color) {
+		if (!this.map.containsKey(color)) {
+			throw new IllegalArgumentException("No fruit for color " + color);
+		}
+		return this.map.get(color);
+	}
+
+	public void setFruit(Color color, String fruit) {
+		this.map.put(color, fruit);
+	}
+
+}

spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java

@@ -33,6 +33,7 @@
 import java.util.stream.Stream;
 
 import example.Color;
+import example.FruitMap;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -900,22 +901,22 @@ void indexWithReferenceIndexTypeAndPrimitiveValueType() {
 			}
 
 			@ParameterizedTest(name = "{0}")
-			@MethodSource("fruitsIndexAccessors")
+			@MethodSource("fruitMapIndexAccessors")
 			void indexWithReferenceIndexTypeAndReferenceValueType(IndexAccessor indexAccessor) {
 				String exitTypeDescriptor = CodeFlow.toDescriptor(String.class);
 
 				StandardEvaluationContext context = new StandardEvaluationContext();
 				context.addIndexAccessor(indexAccessor);
-				context.setVariable("list", List.of(new Fruits()));
+				context.setVariable("list", List.of(new FruitMap()));
 
 				expression = parser.parseExpression("#list.get(0)[T(example.Color).PURPLE]");
 				assertCannotCompile(expression);
 
 				assertThatExceptionOfType(SpelEvaluationException.class)
 						.isThrownBy(() -> expression.getValue(context))
 						.withMessageEndingWith("A problem occurred while attempting to read index '%s' in '%s'",
-								Color.PURPLE, Fruits.class.getName())
-						.withCauseInstanceOf(IndexOutOfBoundsException.class)
+								Color.PURPLE, FruitMap.class.getName())
+						.withCauseInstanceOf(IllegalArgumentException.class)
 						.extracting(SpelEvaluationException::getMessageCode).isEqualTo(EXCEPTION_DURING_INDEX_READ);
 				assertCannotCompile(expression);
 
@@ -943,12 +944,21 @@ void indexWithReferenceIndexTypeAndReferenceValueType(IndexAccessor indexAccesso
 				assertCanCompile(expression);
 				assertThat(expression.getValue(context)).isEqualTo("blueberry");
 				assertThat(getAst().getExitDescriptor()).isEqualTo(exitTypeDescriptor);
+
+				// Set fruit for purple
+				context.setVariable("color", Color.PURPLE);
+				expression.setValue(context, "plum");
+				assertCanCompile(expression);
+				assertThat(expression.getValue(context)).isEqualTo("plum");
+				assertThat(getAst().getExitDescriptor()).isEqualTo(exitTypeDescriptor);
 			}
 
-			static Stream<Arguments> fruitsIndexAccessors() {
+			static Stream<Arguments> fruitMapIndexAccessors() {
 				return Stream.of(
-					arguments(named("FruitsIndexAccessor", new FruitsIndexAccessor())),
-					arguments(named("ReflectiveIndexAccessor", new ReflectiveIndexAccessor(Fruits.class, Color.class, "get")))
+					arguments(named("FruitMapIndexAccessor",
+							new FruitMapIndexAccessor())),
+					arguments(named("ReflectiveIndexAccessor",
+							new ReflectiveIndexAccessor(FruitMap.class, Color.class, "getFruit", "setFruit")))
 				);
 			}
 		}
@@ -1185,26 +1195,26 @@ void nullSafeIndexWithReferenceIndexTypeAndReferenceValueType() {
 				String exitTypeDescriptor = CodeFlow.toDescriptor(String.class);
 
 				StandardEvaluationContext context = new StandardEvaluationContext();
-				context.addIndexAccessor(new FruitsIndexAccessor());
+				context.addIndexAccessor(new FruitMapIndexAccessor());
 				context.setVariable("color", Color.RED);
 
-				expression = parser.parseExpression("#fruits?.[#color]");
+				expression = parser.parseExpression("#fruitMap?.[#color]");
 
 				// Cannot compile before the indexed value type is known.
 				assertThat(expression.getValue(context)).isNull();
 				assertCannotCompile(expression);
 				assertThat(expression.getValue(context)).isNull();
 				assertThat(getAst().getExitDescriptor()).isNull();
 
-				context.setVariable("fruits", new Fruits());
+				context.setVariable("fruitMap", new FruitMap());
 
 				assertThat(expression.getValue(context)).isEqualTo("cherry");
 				assertCanCompile(expression);
 				assertThat(expression.getValue(context)).isEqualTo("cherry");
 				assertThat(getAst().getExitDescriptor()).isEqualTo(exitTypeDescriptor);
 
 				// Null-safe support should have been compiled once the indexed value type is known.
-				context.setVariable("fruits", null);
+				context.setVariable("fruitMap", null);
 				assertThat(expression.getValue(context)).isNull();
 				assertCanCompile(expression);
 				assertThat(expression.getValue(context)).isNull();
@@ -7249,18 +7259,34 @@ static class ReflectiveIndexAccessor implements CompilableIndexAccessor {
 
 		private final Method readMethodToInvoke;
 
+		@Nullable
+		private final Method writeMethodToInvoke;
+
 		private final String targetTypeDesc;
 
 		private final String methodDescr;
 
 
-		public ReflectiveIndexAccessor(Class<?> targetType, Class<?> indexType, String readMethodName) {
+		public ReflectiveIndexAccessor(Class<?> targetType, Class<?> indexType, String readMethodName,
+				@Nullable String writeMethodName) {
+
 			this.targetType = targetType;
 			this.indexType = indexType;
 			this.readMethod = ReflectionUtils.findMethod(targetType, readMethodName, indexType);
-			Assert.notNull(this.readMethod, () -> "Failed to find method '%s(%s)' in class '%s'."
+			Assert.notNull(this.readMethod, () -> "Failed to find read-method '%s(%s)' in class '%s'."
 					.formatted(readMethodName, indexType.getTypeName(), targetType.getTypeName()));
 			this.readMethodToInvoke = ClassUtils.getInterfaceMethodIfPossible(this.readMethod, targetType);
+			if (writeMethodName != null) {
+				Class<?> indexedValueType = this.readMethod.getReturnType();
+				Method writeMethod = ReflectionUtils.findMethod(targetType, writeMethodName, indexType, indexedValueType);
+				Assert.notNull(writeMethod, () -> "Failed to find write-method '%s(%s, %s)' in class '%s'."
+						.formatted(writeMethodName, indexType.getTypeName(), indexedValueType.getTypeName(),
+								targetType.getTypeName()));
+				this.writeMethodToInvoke = ClassUtils.getInterfaceMethodIfPossible(writeMethod, targetType);
+			}
+			else {
+				this.writeMethodToInvoke = null;
+			}
 			this.targetTypeDesc = CodeFlow.toDescriptor(targetType);
 			this.methodDescr = CodeFlow.createSignatureDescriptor(this.readMethod);
 		}
@@ -7286,12 +7312,13 @@ public TypedValue read(EvaluationContext context, Object target, Object index) {
 
 		@Override
 		public boolean canWrite(EvaluationContext context, Object target, Object index) {
-			return false;
+			return (this.writeMethodToInvoke != null && canRead(context, target, index));
 		}
 
 		@Override
 		public void write(EvaluationContext context, Object target, Object index, @Nullable Object newValue) {
-			throw new UnsupportedOperationException();
+			Assert.state(this.writeMethodToInvoke != null, "Write-method cannot be null");
+			ReflectionUtils.invokeMethod(this.writeMethodToInvoke, target, index, newValue);
 		}
 
 		@Override
@@ -7355,7 +7382,7 @@ public Color get(int index) {
 	private static class ColorsIndexAccessor extends ReflectiveIndexAccessor {
 
 		ColorsIndexAccessor() {
-			super(Colors.class, int.class, "get");
+			super(Colors.class, int.class, "get", null);
 		}
 	}
 
@@ -7376,41 +7403,21 @@ public int get(Color color) {
 	private static class ColorOrdinalsIndexAccessor extends ReflectiveIndexAccessor {
 
 		ColorOrdinalsIndexAccessor() {
-			super(ColorOrdinals.class, Color.class, "get");
-		}
-	}
-
-	/**
-	 * Type that can be indexed by the {@link Color} enum (i.e., something other
-	 * than an int, Integer, or String) and whose indexed values are Strings.
-	 */
-	public static class Fruits {
-
-		public String get(Color color) {
-			return switch (color) {
-				case RED -> "cherry";
-				case ORANGE -> "orange";
-				case YELLOW -> "banana";
-				case GREEN -> "kiwi";
-				case BLUE -> "blueberry";
-				// We don't map PURPLE so that we can test for IndexOutOfBoundsException.
-				// case PURPLE -> "plum";
-				default -> throw new IndexOutOfBoundsException("color " + color + " is not supported");
-			};
+			super(ColorOrdinals.class, Color.class, "get", null);
 		}
 	}
 
 	/**
 	 * Manually implemented {@link CompilableIndexAccessor} that knows how to
-	 * index into {@link Fruits}.
+	 * index into {@link FruitMap}.
 	 */
-	private static class FruitsIndexAccessor implements CompilableIndexAccessor {
+	private static class FruitMapIndexAccessor implements CompilableIndexAccessor {
 
-		private final Class<?> targetType = Fruits.class;
+		private final Class<?> targetType = FruitMap.class;
 
 		private final Class<?> indexType = Color.class;
 
-		private final Method method = ReflectionUtils.findMethod(this.targetType, "get", this.indexType);
+		private final Method method = ReflectionUtils.findMethod(this.targetType, "getFruit", this.indexType);
 
 		private final String targetTypeDesc = CodeFlow.toDescriptor(this.targetType);
 
@@ -7431,19 +7438,22 @@ public boolean canRead(EvaluationContext context, Object target, Object index) {
 
 		@Override
 		public TypedValue read(EvaluationContext context, Object target, Object index) {
-			Fruits fruits = (Fruits) target;
+			FruitMap fruitMap = (FruitMap) target;
 			Color color = (Color) index;
-			return new TypedValue(fruits.get(color));
+			return new TypedValue(fruitMap.getFruit(color));
 		}
 
 		@Override
 		public boolean canWrite(EvaluationContext context, Object target, Object index) {
-			return false;
+			return canRead(context, target, index);
 		}
 
 		@Override
 		public void write(EvaluationContext context, Object target, Object index, @Nullable Object newValue) {
-			throw new UnsupportedOperationException();
+			FruitMap fruitMap = (FruitMap) target;
+			Color color = (Color) index;
+			String fruit = String.valueOf(newValue);
+			fruitMap.setFruit(color, fruit);
 		}
 
 		@Override
@@ -7465,7 +7475,7 @@ public void generateCode(SpelNode index, MethodVisitor mv, CodeFlow cf) {
 			}
 			// Push the index onto the stack.
 			cf.generateCodeForArgument(mv, index, Color.class);
-			// Invoke the read-index method.
+			// Invoke the read-method.
 			mv.visitMethodInsn(INVOKEVIRTUAL, this.classDesc, this.method.getName(), this.methodDescr, false);
 		}