From 8ec23a0fcc65a6b00b415bc7d0bf9a08e7463034 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 5 Aug 2024 14:15:39 +0300 Subject: [PATCH 1/2] Polishing --- .../expression/spel/PropertyAccessTests.java | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java index 287644f8453..aa07ea8fc25 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import org.assertj.core.api.ThrowableTypeAssert; import org.junit.jupiter.api.Test; import org.springframework.core.convert.TypeDescriptor; @@ -83,11 +84,11 @@ class PropertyAccessTests extends AbstractExpressionTests { void accessingOnNullObject() { SpelExpression expr = (SpelExpression) parser.parseExpression("madeup"); EvaluationContext context = new StandardEvaluationContext(null); - assertThatExceptionOfType(SpelEvaluationException.class) + assertThatSpelEvaluationException() .isThrownBy(() -> expr.getValue(context)) .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.PROPERTY_OR_FIELD_NOT_READABLE_ON_NULL); assertThat(expr.isWritable(context)).isFalse(); - assertThatExceptionOfType(SpelEvaluationException.class) + assertThatSpelEvaluationException() .isThrownBy(() -> expr.setValue(context, "abc")) .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.PROPERTY_OR_FIELD_NOT_WRITABLE_ON_NULL); } @@ -117,8 +118,7 @@ class PropertyAccessTests extends AbstractExpressionTests { assertThat((int) i).isEqualTo(99); // Cannot set it to a string value - assertThatExceptionOfType(EvaluationException.class).isThrownBy(() -> - flibbleexpr.setValue(ctx, "not allowed")); + assertThatSpelEvaluationException().isThrownBy(() -> flibbleexpr.setValue(ctx, "not allowed")); // message will be: EL1063E:(pos 20): A problem occurred whilst attempting to set the property // 'flibbles': 'Cannot set flibbles to an object of type 'class java.lang.String'' // System.out.println(e.getMessage()); @@ -173,8 +173,7 @@ class PropertyAccessTests extends AbstractExpressionTests { @Test void noGetClassAccess() { EvaluationContext context = SimpleEvaluationContext.forReadOnlyDataBinding().build(); - assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() -> - parser.parseExpression("'a'.class.name").getValue(context)); + assertThatSpelEvaluationException().isThrownBy(() -> parser.parseExpression("'a'.class.name").getValue(context)); } @Test @@ -187,8 +186,9 @@ class PropertyAccessTests extends AbstractExpressionTests { target.setName("p2"); assertThat(expr.getValue(context, target)).isEqualTo("p2"); - assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() -> - parser.parseExpression("name='p3'").getValue(context, target)); + assertThatSpelEvaluationException() + .isThrownBy(() -> parser.parseExpression("name='p3'").getValue(context, target)) + .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.PROPERTY_OR_FIELD_NOT_WRITABLE); } @Test @@ -201,8 +201,9 @@ class PropertyAccessTests extends AbstractExpressionTests { RecordPerson target2 = new RecordPerson("p2"); assertThat(expr.getValue(context, target2)).isEqualTo("p2"); - assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() -> - parser.parseExpression("name='p3'").getValue(context, target2)); + assertThatSpelEvaluationException() + .isThrownBy(() -> parser.parseExpression("name='p3'").getValue(context, target2)) + .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.PROPERTY_OR_FIELD_NOT_WRITABLE); } @Test @@ -248,7 +249,7 @@ class PropertyAccessTests extends AbstractExpressionTests { void propertyAccessWithoutMethodResolver() { EvaluationContext context = SimpleEvaluationContext.forReadOnlyDataBinding().build(); Person target = new Person("p1"); - assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() -> + assertThatSpelEvaluationException().isThrownBy(() -> parser.parseExpression("name.substring(1)").getValue(context, target)); } @@ -274,12 +275,17 @@ class PropertyAccessTests extends AbstractExpressionTests { void propertyAccessWithArrayIndexOutOfBounds() { EvaluationContext context = SimpleEvaluationContext.forReadOnlyDataBinding().build(); Expression expression = parser.parseExpression("stringArrayOfThreeItems[3]"); - assertThatExceptionOfType(SpelEvaluationException.class) + assertThatSpelEvaluationException() .isThrownBy(() -> expression.getValue(context, new Inventor())) .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.ARRAY_INDEX_OUT_OF_BOUNDS); } + private ThrowableTypeAssert assertThatSpelEvaluationException() { + return assertThatExceptionOfType(SpelEvaluationException.class); + } + + // This can resolve the property 'flibbles' on any String (very useful...) private static class StringyPropertyAccessor implements PropertyAccessor { From c57c2272a1d2a214bbbacac166a0845f287bbc73 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 5 Aug 2024 14:15:55 +0300 Subject: [PATCH 2/2] Throw exception for failure to set property as index in SpEL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, the Indexer in the Spring Expression Language (SpEL) silently ignored a failure to set a property via the indexed property syntax ([''] = ) – for example, if property write access was disabled in the EvaluationContext. This commit addresses this issue by properly throwing a SpelEvaluationException in PropertyIndexingValueRef.setValue(Object) if the property could not be set. Closes gh-33310 --- .../java/org/springframework/expression/spel/ast/Indexer.java | 2 ++ .../springframework/expression/spel/PropertyAccessTests.java | 4 ++++ 2 files changed, 6 insertions(+) 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 e6505d0abbf..4b4b3160265 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 @@ -630,6 +630,8 @@ public class Indexer extends SpelNodeImpl { throw new SpelEvaluationException(getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_PROPERTY_WRITE, this.name, ex.getMessage()); } + throw new SpelEvaluationException(getStartPosition(), + SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, this.targetObjectTypeDescriptor.toString()); } @Override diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java index aa07ea8fc25..d0ef7f889f0 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java @@ -189,6 +189,10 @@ class PropertyAccessTests extends AbstractExpressionTests { assertThatSpelEvaluationException() .isThrownBy(() -> parser.parseExpression("name='p3'").getValue(context, target)) .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.PROPERTY_OR_FIELD_NOT_WRITABLE); + + assertThatSpelEvaluationException() + .isThrownBy(() -> parser.parseExpression("['name']='p4'").getValue(context, target)) + .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE); } @Test