Introduce proper error handling for IndexAccessor support in SpEL

See gh-26409 See gh-26478
spring-projects · Apr 10, 2024 · 22bfe7d · 22bfe7d
1 parent 5c6b82a
commit 22bfe7d
Show file tree

Hide file tree

Showing 3 changed files with 140 additions and 10 deletions.
diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java b/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java
@@ -291,7 +291,16 @@ public enum SpelMessage {
 
 	/** @since 6.0.13 */
 	NEGATIVE_REPEATED_TEXT_COUNT(Kind.ERROR, 1081,
-			"Repeat count ''{0}'' must not be negative");
+			"Repeat count ''{0}'' must not be negative"),
+
+	/** @since 6.2 */
+	EXCEPTION_DURING_INDEX_READ(Kind.ERROR, 1082,
+			"A problem occurred while attempting to read index ''{0}'' in ''{1}''"),
+
+	/** @since 6.2 */
+	EXCEPTION_DURING_INDEX_WRITE(Kind.ERROR, 1083,
+			"A problem occurred while attempting to write index ''{0}'' in ''{1}''");
+
 
 
 	private final Kind kind;

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
@@ -259,8 +259,9 @@ else if (target instanceof Collection<?> collection) {
 			}
 		}
 		catch (Exception ex) {
-			// TODO throw SpelEvaluationException for "exception during index access",
-			// analogous to SpelMessage.EXCEPTION_DURING_PROPERTY_READ.
+			throw new SpelEvaluationException(
+					getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_INDEX_READ, index,
+					target.getClass().getTypeName());
 		}
 
 		throw new SpelEvaluationException(
@@ -904,9 +905,10 @@ 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());
+			catch (Exception ex) {
+				throw new SpelEvaluationException(
+						getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_INDEX_READ, this.index,
+						this.typeDescriptor.toString());
 			}
 		}
 
@@ -915,9 +917,10 @@ 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());
+			catch (Exception ex) {
+				throw new SpelEvaluationException(
+						getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_INDEX_WRITE, this.index,
+						this.typeDescriptor.toString());
 			}
 		}
 

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
@@ -33,6 +33,7 @@
 import com.fasterxml.jackson.databind.node.ArrayNode;
 import com.fasterxml.jackson.databind.node.NullNode;
 import com.fasterxml.jackson.databind.node.TextNode;
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 
@@ -48,6 +49,15 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.BDDMockito.doThrow;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.springframework.expression.spel.SpelMessage.EXCEPTION_DURING_INDEX_READ;
+import static org.springframework.expression.spel.SpelMessage.EXCEPTION_DURING_INDEX_WRITE;
 import static org.springframework.expression.spel.SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE;
 import static org.springframework.expression.spel.SpelMessage.UNABLE_TO_GROW_COLLECTION_UNKNOWN_ELEMENT_TYPE;
 
@@ -485,7 +495,115 @@ void addingAndRemovingIndexAccessors() {
 		}
 
 		@Test
-		void indexReadWrite() {
+		void noSuitableIndexAccessorResultsInException() {
+			StandardEvaluationContext context = new StandardEvaluationContext();
+			assertThat(context.getIndexAccessors()).isEmpty();
+
+			SpelExpressionParser parser = new SpelExpressionParser();
+			Expression expr = parser.parseExpression("[0]");
+			assertThatExceptionOfType(SpelEvaluationException.class)
+					.isThrownBy(() -> expr.getValue(context, this))
+					.withMessageEndingWith("Indexing into type '%s' is not supported", getClass().getName())
+					.extracting(SpelEvaluationException::getMessageCode).isEqualTo(INDEXING_NOT_SUPPORTED_FOR_TYPE);
+		}
+
+		@Test
+		void canReadThrowsException() throws Exception {
+			StandardEvaluationContext context = new StandardEvaluationContext();
+			RuntimeException exception = new RuntimeException("Boom!");
+
+			IndexAccessor mock = mock();
+			given(mock.canRead(any(), eq(this), any())).willThrow(exception);
+			context.addIndexAccessor(mock);
+
+			SpelExpressionParser parser = new SpelExpressionParser();
+			Expression expr = parser.parseExpression("[0]");
+			assertThatExceptionOfType(SpelEvaluationException.class)
+					.isThrownBy(() -> expr.getValue(context, this))
+					.withMessageEndingWith("A problem occurred while attempting to read index '%d' in '%s'",
+							0, getClass().getName())
+					.withCause(exception)
+					.extracting(SpelEvaluationException::getMessageCode).isEqualTo(EXCEPTION_DURING_INDEX_READ);
+
+			verify(mock, times(1)).canRead(any(), any(), any());
+		}
+
+		@Test
+		void readThrowsException() throws Exception {
+			StandardEvaluationContext context = new StandardEvaluationContext();
+			RuntimeException exception = new RuntimeException("Boom!");
+
+			IndexAccessor mock = mock();
+			given(mock.canRead(any(), eq(this), any())).willReturn(true);
+			given(mock.read(any(), eq(this), any())).willThrow(exception);
+			context.addIndexAccessor(mock);
+
+			SpelExpressionParser parser = new SpelExpressionParser();
+			Expression expr = parser.parseExpression("[0]");
+			assertThatExceptionOfType(SpelEvaluationException.class)
+					.isThrownBy(() -> expr.getValue(context, this))
+					.withMessageEndingWith("A problem occurred while attempting to read index '%d' in '%s'",
+							0, getClass().getName())
+					.withCause(exception)
+					.extracting(SpelEvaluationException::getMessageCode).isEqualTo(EXCEPTION_DURING_INDEX_READ);
+
+			verify(mock, times(1)).canRead(any(), any(), any());
+			verify(mock, times(1)).read(any(), any(), any());
+		}
+
+		@Disabled("Disabled until IndexAccessor#canWrite is honored")
+		@Test
+		void canWriteThrowsException() throws Exception {
+			StandardEvaluationContext context = new StandardEvaluationContext();
+			RuntimeException exception = new RuntimeException("Boom!");
+
+			IndexAccessor mock = mock();
+			// TODO canRead() should not be invoked for this use case.
+			given(mock.canRead(any(), eq(this), any())).willReturn(true);
+			// TODO canWrite() should be invoked for this use case, but it is currently not.
+			given(mock.canWrite(eq(context), eq(this), eq(0))).willThrow(exception);
+			context.addIndexAccessor(mock);
+
+			SpelExpressionParser parser = new SpelExpressionParser();
+			Expression expr = parser.parseExpression("[0]");
+			assertThatExceptionOfType(SpelEvaluationException.class)
+					.isThrownBy(() -> expr.setValue(context, this, 999))
+					.withMessageEndingWith("A problem occurred while attempting to write index '%d' in '%s'",
+							0, getClass().getName())
+					.withCause(exception)
+					.extracting(SpelEvaluationException::getMessageCode).isEqualTo(EXCEPTION_DURING_INDEX_WRITE);
+
+			verify(mock, times(1)).canWrite(any(), any(), any());
+		}
+
+		@Test
+		void writeThrowsException() throws Exception {
+			StandardEvaluationContext context = new StandardEvaluationContext();
+			RuntimeException exception = new RuntimeException("Boom!");
+
+			IndexAccessor mock = mock();
+			// TODO canRead() should not be invoked for this use case.
+			given(mock.canRead(any(), eq(this), any())).willReturn(true);
+			given(mock.canWrite(eq(context), eq(this), eq(0))).willReturn(true);
+			doThrow(exception).when(mock).write(any(), any(), any(), any());
+			context.addIndexAccessor(mock);
+
+			SpelExpressionParser parser = new SpelExpressionParser();
+			Expression expr = parser.parseExpression("[0]");
+			assertThatExceptionOfType(SpelEvaluationException.class)
+					.isThrownBy(() -> expr.setValue(context, this, 999))
+					.withMessageEndingWith("A problem occurred while attempting to write index '%d' in '%s'",
+							0, getClass().getName())
+					.withCause(exception)
+					.extracting(SpelEvaluationException::getMessageCode).isEqualTo(EXCEPTION_DURING_INDEX_WRITE);
+
+			// TODO canWrite() should be invoked for this use case, but it is currently not.
+			// verify(mock, times(1)).canWrite(any(), any(), any());
+			verify(mock, times(1)).write(any(), any(), any(), any());
+		}
+
+		@Test
+		void readAndWriteIndex() {
 			StandardEvaluationContext context = new StandardEvaluationContext();
 
 			ObjectMapper objectMapper = new ObjectMapper();