From 50a0000ed7c03ebc72e38b2215864bb917ee4f17 Mon Sep 17 00:00:00 2001 From: ljmn3211 Date: Mon, 1 Feb 2021 10:50:39 +0800 Subject: [PATCH 1/7] Introduce IndexAccessor SPI to customize the SpEL Indexer See gh-26409 See gh-26478 --- spring-expression/spring-expression.gradle | 2 + .../expression/EvaluationContext.java | 5 + .../expression/IndexAccessor.java | 97 ++++++++++++++++ .../expression/spel/ast/Indexer.java | 67 ++++++++++- .../spel/support/SimpleEvaluationContext.java | 6 + .../support/StandardEvaluationContext.java | 30 +++++ .../expression/spel/PropertyAccessTests.java | 106 ++++++++++++++++++ 7 files changed, 312 insertions(+), 1 deletion(-) create mode 100644 spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java diff --git a/spring-expression/spring-expression.gradle b/spring-expression/spring-expression.gradle index 4b31d5b6f5b8..5bc6aadf2ee2 100644 --- a/spring-expression/spring-expression.gradle +++ b/spring-expression/spring-expression.gradle @@ -8,4 +8,6 @@ dependencies { testImplementation(testFixtures(project(":spring-core"))) testImplementation("org.jetbrains.kotlin:kotlin-reflect") testImplementation("org.jetbrains.kotlin:kotlin-stdlib") + testImplementation("com.fasterxml.jackson.core:jackson-databind") + testImplementation("com.fasterxml.jackson.core:jackson-core") } diff --git a/spring-expression/src/main/java/org/springframework/expression/EvaluationContext.java b/spring-expression/src/main/java/org/springframework/expression/EvaluationContext.java index 9dd23361cd0a..217b84fb1b24 100644 --- a/spring-expression/src/main/java/org/springframework/expression/EvaluationContext.java +++ b/spring-expression/src/main/java/org/springframework/expression/EvaluationContext.java @@ -56,6 +56,11 @@ public interface EvaluationContext { */ List getPropertyAccessors(); + /** + * Return a list of index accessors that will be asked in turn to read/write a property. + */ + List getIndexAccessors(); + /** * Return a list of resolvers that will be asked in turn to locate a constructor. */ diff --git a/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java b/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java new file mode 100644 index 000000000000..a5a76a9c769b --- /dev/null +++ b/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java @@ -0,0 +1,97 @@ +/* + * Copyright 2002-2019 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 org.springframework.expression; + +import org.springframework.expression.spel.ast.ValueRef; +import org.springframework.lang.Nullable; + +/** + * A index accessor is able to read from (and possibly write to) an array's elements. + * + *

This interface places no restrictions, and so implementors are free to access elements + * directly as fields or through getters or in any other way they see as appropriate. + * + *

A resolver can optionally specify an array of target classes for which it should be + * called. However, if it returns {@code null} from {@link #getSpecificTargetClasses()}, + * it will be called for all property references and given a chance to determine if it + * can read or write them. + * + *

Property resolvers are considered to be ordered, and each will be called in turn. + * The only rule that affects the call order is that any resolver naming the target + * class directly in {@link #getSpecificTargetClasses()} will be called first, before + * the general resolvers. + * + * @author jackmiking lee + * @since 3.0 + */ +public interface IndexAccessor { + /** + * Return an array of classes for which this resolver should be called. + *

Returning {@code null} indicates this is a general resolver that + * can be called in an attempt to resolve a property on any type. + * @return an array of classes that this resolver is suitable for + * (or {@code null} if a general resolver) + */ + @Nullable + Class[] getSpecificTargetClasses(); + + /** + * Called to determine if a resolver instance is able to access a specified property + * on a specified target object. + * @param context the evaluation context in which the access is being attempted + * @param target the target object upon which the property is being accessed + * @param index the index of the array being accessed + * @return true if this resolver is able to read the property + * @throws AccessException if there is any problem determining whether the property can be read + */ + boolean canRead(EvaluationContext context, @Nullable Object target, Object index) throws AccessException; + + /** + * Called to read a property from a specified target object. + * Should only succeed if {@link #canRead} also returns {@code true}. + * @param context the evaluation context in which the access is being attempted + * @param target the target object upon which the property is being accessed + * @param index the index of the array being accessed + * @return a TypedValue object wrapping the property value read and a type descriptor for it + * @throws AccessException if there is any problem accessing the property value + */ + ValueRef read(EvaluationContext context, @Nullable Object target,Object index) throws AccessException; + + /** + * Called to determine if a resolver instance is able to write to a specified + * property on a specified target object. + * @param context the evaluation context in which the access is being attempted + * @param target the target object upon which the property is being accessed + * @param index the index of the array being accessed + * @return true if this resolver is able to write to the property + * @throws AccessException if there is any problem determining whether the + * property can be written to + */ + boolean canWrite(EvaluationContext context, @Nullable Object target, Object index) throws AccessException; + + /** + * Called to write to a property on a specified target object. + * Should only succeed if {@link #canWrite} also returns {@code true}. + * @param context the evaluation context in which the access is being attempted + * @param target the target object upon which the property is being accessed + * @param index the index of the array being accessed + * @param newValue the new value for the property + * @throws AccessException if there is any problem writing to the property value + */ + void write(EvaluationContext context, @Nullable Object target, Object index, @Nullable Object newValue) + throws AccessException; +} 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 0e888782af5a..5a1c6f527dc1 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 @@ -17,9 +17,11 @@ package org.springframework.expression.spel.ast; import java.lang.reflect.Constructor; +import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.function.Supplier; import org.springframework.asm.Label; @@ -28,6 +30,7 @@ import org.springframework.expression.AccessException; import org.springframework.expression.EvaluationContext; import org.springframework.expression.EvaluationException; +import org.springframework.expression.IndexAccessor; import org.springframework.expression.PropertyAccessor; import org.springframework.expression.TypeConverter; import org.springframework.expression.TypedValue; @@ -246,11 +249,73 @@ else if (target instanceof Collection collection) { return new PropertyIndexingValueRef( target, (String) index, state.getEvaluationContext(), targetDescriptor); } - + Optional optional = tryIndexAccessor(state, index); + if (optional.isPresent()) { + return optional.get(); + } throw new SpelEvaluationException( getStartPosition(), SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, targetDescriptor); } + private Optional tryIndexAccessor(ExpressionState state, Object index) { + EvaluationContext context = state.getEvaluationContext(); + Object target = state.getActiveContextObject().getValue(); + if (context != null) { + List list = context.getIndexAccessors(); + if (list != null) { + List availableAccessors = getIndexAccessorsToTry(state.getActiveContextObject(), list); + try { + for (IndexAccessor indexAccessor : availableAccessors) { + if (indexAccessor.canRead(context, target, index)) { + ValueRef valueRef = indexAccessor.read(context, target, index); + return Optional.of(valueRef); + } + } + } + catch (Exception ex) { + } + } + } + return Optional.empty(); + } + + private List getIndexAccessorsToTry( + @Nullable Object contextObject, List propertyAccessors) { + + Class targetType; + if (contextObject instanceof TypedValue) { + targetType = ((TypedValue) contextObject).getTypeDescriptor().getObjectType(); + } + else { + targetType = (contextObject != null ? contextObject.getClass() : null); + } + + List specificAccessors = new ArrayList<>(); + List generalAccessors = new ArrayList<>(); + for (IndexAccessor resolver : propertyAccessors) { + Class[] targets = resolver.getSpecificTargetClasses(); + if (targets == null) { + // generic resolver that says it can be used for any type + generalAccessors.add(resolver); + } + else if (targetType != null) { + for (Class clazz : targets) { + if (clazz == targetType) { + specificAccessors.add(resolver); + break; + } + else if (clazz.isAssignableFrom(targetType)) { + generalAccessors.add(resolver); + } + } + } + } + List resolvers = new ArrayList<>(specificAccessors); + generalAccessors.removeAll(specificAccessors); + resolvers.addAll(generalAccessors); + return resolvers; + } + @Override public boolean isCompilable() { if (this.indexedType == IndexedType.ARRAY) { diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java index f8cd8a1cce5a..24ab07b9ca71 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java @@ -28,6 +28,7 @@ import org.springframework.expression.BeanResolver; import org.springframework.expression.ConstructorResolver; import org.springframework.expression.EvaluationContext; +import org.springframework.expression.IndexAccessor; import org.springframework.expression.MethodResolver; import org.springframework.expression.OperatorOverloader; import org.springframework.expression.PropertyAccessor; @@ -146,6 +147,11 @@ public List getPropertyAccessors() { return this.propertyAccessors; } + @Override + public List getIndexAccessors() { + return null; + } + /** * Return an empty list, always, since this context does not support the * use of type references. diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardEvaluationContext.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardEvaluationContext.java index 864395a8871b..ebe22a449a86 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardEvaluationContext.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardEvaluationContext.java @@ -27,6 +27,7 @@ import org.springframework.expression.BeanResolver; import org.springframework.expression.ConstructorResolver; import org.springframework.expression.EvaluationContext; +import org.springframework.expression.IndexAccessor; import org.springframework.expression.MethodFilter; import org.springframework.expression.MethodResolver; import org.springframework.expression.OperatorOverloader; @@ -83,6 +84,9 @@ public class StandardEvaluationContext implements EvaluationContext { @Nullable private volatile List propertyAccessors; + @Nullable + private volatile List indexAccessors; + @Nullable private volatile List constructorResolvers; @@ -142,6 +146,10 @@ public void setPropertyAccessors(List propertyAccessors) { this.propertyAccessors = propertyAccessors; } + public void setIndexAccessors(ListindexAccessors){ + this.indexAccessors=indexAccessors; + } + @Override public List getPropertyAccessors() { return initPropertyAccessors(); @@ -155,6 +163,14 @@ public boolean removePropertyAccessor(PropertyAccessor accessor) { return initPropertyAccessors().remove(accessor); } + public void addIndexAccessor(IndexAccessor accessor){ + initIndexAccessors().add(accessor); + } + + public boolean removeIndexAccessor(IndexAccessor indexAccessor){ + return initIndexAccessors().remove(indexAccessor); + } + public void setConstructorResolvers(List constructorResolvers) { this.constructorResolvers = constructorResolvers; } @@ -404,6 +420,15 @@ private List initPropertyAccessors() { return accessors; } + private ListinitIndexAccessors(){ + List accessors = this.indexAccessors; + if(accessors == null){ + accessors = new ArrayList<>(5); + this.indexAccessors = accessors; + } + return accessors; + } + private List initConstructorResolvers() { List resolvers = this.constructorResolvers; if (resolvers == null) { @@ -429,4 +454,9 @@ private static void addBeforeDefault(List list, T element) { list.add(list.size() - 1, element); } + @Override + public List getIndexAccessors() { + return initIndexAccessors(); + } + } 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 287644f84538..0557ab64cfc7 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,10 @@ import java.util.List; import java.util.Map; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.TextNode; import org.junit.jupiter.api.Test; import org.springframework.core.convert.TypeDescriptor; @@ -28,8 +32,10 @@ import org.springframework.expression.EvaluationContext; import org.springframework.expression.EvaluationException; 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.SpelExpression; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.SimpleEvaluationContext; @@ -144,6 +150,25 @@ void addingAndRemovingAccessors() { assertThat(ctx.getPropertyAccessors()).hasSize(2); } + @Test + public void testAddingRemovingIndexAccessors() { + StandardEvaluationContext ctx = new StandardEvaluationContext(); + List indexAccessors = ctx.getIndexAccessors(); + assertThat(indexAccessors.size()).isEqualTo(0); + JsonIndexAccessor jsonIndexAccessor=new JsonIndexAccessor(); + ctx.addIndexAccessor(jsonIndexAccessor); + assertThat(indexAccessors.size()).isEqualTo(1); + JsonIndexAccessor jsonIndexAccessor1=new JsonIndexAccessor(); + ctx.addIndexAccessor(jsonIndexAccessor1); + assertThat(indexAccessors.size()).isEqualTo(2); + List copy=new ArrayList(indexAccessors); + assertThat(ctx.removeIndexAccessor(jsonIndexAccessor)).isTrue(); + assertThat(ctx.removeIndexAccessor(jsonIndexAccessor)).isFalse(); + assertThat(indexAccessors.size()).isEqualTo(1); + ctx.setIndexAccessors(copy); + assertThat(ctx.getIndexAccessors().size()).isEqualTo(2); + } + @Test void accessingPropertyOfClass() { Expression expression = parser.parseExpression("name"); @@ -223,6 +248,24 @@ void propertyReadWrite() { assertThat(target.getName()).isEqualTo("p4"); assertThat(expr.getValue(context, target)).isEqualTo("p4"); } + @Test + public void indexReadWrite(){ + StandardEvaluationContext context=new StandardEvaluationContext(); + JsonIndexAccessor indexAccessor=new JsonIndexAccessor(); + context.addIndexAccessor(indexAccessor); + ArrayNode arrayNode=indexAccessor.objectMapper.createArrayNode(); + arrayNode.add(new TextNode("node0")); + arrayNode.add(new TextNode("node1")); + Expression expr=parser.parseExpression("[0]"); + assertThat(new TextNode("node0").equals(expr.getValue(context,arrayNode))).isTrue(); + expr.setValue(context,arrayNode,new TextNode("nodeUpdate")); + assertThat(new TextNode("nodeUpdate").equals(expr.getValue(context,arrayNode))).isTrue(); + Expression expr1=parser.parseExpression("[1]"); + assertThat(new TextNode("node1").equals(expr1.getValue(context,arrayNode))).isTrue(); + + + } + @Test void propertyReadWriteWithRootObject() { @@ -363,4 +406,67 @@ public void write(EvaluationContext context, Object target, String name, Object } } + private static class JsonIndexAccessor implements IndexAccessor { + ObjectMapper objectMapper=new ObjectMapper(); + public class ArrayValueRef implements ValueRef { + ArrayNode arrayNode; + Integer index; + + @Override + public TypedValue getValue() { + return new TypedValue(arrayNode.get(index)); + } + + @Override + public void setValue(Object newValue) { + arrayNode.set(index,objectMapper.convertValue(newValue, JsonNode.class)); + } + public void setArrayNode(ArrayNode arrayNode){ + this.arrayNode=arrayNode; + } + + public void setIndex(Object index) { + if (index instanceof Integer) { + this.index = (Integer) index; + } + } + + @Override + public boolean isWritable() { + return false; + } + } + + @Override + public Class[] getSpecificTargetClasses() { + return new Class[]{ + ArrayNode.class + }; + } + + @Override + public boolean canRead(EvaluationContext context, Object target, Object index) throws AccessException { + return true; + } + + @Override + public ValueRef read(EvaluationContext context, Object target, Object index) throws AccessException { + ArrayValueRef valueRef = new ArrayValueRef(); + valueRef.setArrayNode((ArrayNode) target); + valueRef.setIndex(index); + return valueRef; + } + + @Override + public boolean canWrite(EvaluationContext context, Object target, Object index) throws AccessException { + return true; + } + + @Override + public void write(EvaluationContext context, Object target, Object index, Object newValue) throws AccessException { + ValueRef valueRef=read(context,target,index); + valueRef.setValue(newValue); + } + } + } From d91277095a3eee690f3e6766257cec6e1c2510f8 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sun, 24 Mar 2024 18:07:20 +0100 Subject: [PATCH 2/7] Initial review and polish of IndexAccessor support in SpEL See gh-26409 See gh-26478 --- spring-expression/spring-expression.gradle | 1 - .../expression/EvaluationContext.java | 10 +- .../expression/IndexAccessor.java | 103 +++++++------ .../expression/PropertyAccessor.java | 15 +- .../expression/TargetedAccessor.java | 56 ++++++++ .../expression/spel/ast/AstUtils.java | 81 ++++++++--- .../expression/spel/ast/Indexer.java | 91 ++++-------- .../spel/support/SimpleEvaluationContext.java | 6 - .../support/StandardEvaluationContext.java | 62 ++++++-- .../expression/spel/IndexingTests.java | 136 ++++++++++++++++++ .../expression/spel/PropertyAccessTests.java | 106 -------------- 11 files changed, 405 insertions(+), 262 deletions(-) create mode 100644 spring-expression/src/main/java/org/springframework/expression/TargetedAccessor.java diff --git a/spring-expression/spring-expression.gradle b/spring-expression/spring-expression.gradle index 5bc6aadf2ee2..d141d5309ab7 100644 --- a/spring-expression/spring-expression.gradle +++ b/spring-expression/spring-expression.gradle @@ -9,5 +9,4 @@ dependencies { testImplementation("org.jetbrains.kotlin:kotlin-reflect") testImplementation("org.jetbrains.kotlin:kotlin-stdlib") testImplementation("com.fasterxml.jackson.core:jackson-databind") - testImplementation("com.fasterxml.jackson.core:jackson-core") } diff --git a/spring-expression/src/main/java/org/springframework/expression/EvaluationContext.java b/spring-expression/src/main/java/org/springframework/expression/EvaluationContext.java index 217b84fb1b24..2746352d1ac2 100644 --- a/spring-expression/src/main/java/org/springframework/expression/EvaluationContext.java +++ b/spring-expression/src/main/java/org/springframework/expression/EvaluationContext.java @@ -16,6 +16,7 @@ package org.springframework.expression; +import java.util.Collections; import java.util.List; import java.util.function.Supplier; @@ -57,9 +58,14 @@ public interface EvaluationContext { List getPropertyAccessors(); /** - * Return a list of index accessors that will be asked in turn to read/write a property. + * Return a list of index accessors that will be asked in turn to access or + * set an indexed value. + *

The default implementation returns an empty list. + * @since 6.2 */ - List getIndexAccessors(); + default List getIndexAccessors() { + return Collections.emptyList(); + } /** * Return a list of resolvers that will be asked in turn to locate a constructor. 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 a5a76a9c769b..e26136f6822c 100644 --- a/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * 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. @@ -20,78 +20,91 @@ import org.springframework.lang.Nullable; /** - * A index accessor is able to read from (and possibly write to) an array's elements. + * An index accessor is able to read from (and possibly write to) an indexed + * structure of an object. * - *

This interface places no restrictions, and so implementors are free to access elements - * directly as fields or through getters or in any other way they see as appropriate. + *

This interface places no restrictions on what constitutes an indexed + * structure. Implementors are therefore free to access indexed values any way + * they deem appropriate. * - *

A resolver can optionally specify an array of target classes for which it should be - * called. However, if it returns {@code null} from {@link #getSpecificTargetClasses()}, - * it will be called for all property references and given a chance to determine if it - * can read or write them. + *

An index accessor can optionally specify an array of target classes for + * which it should be called. However, if it returns {@code null} or an empty + * array from {@link #getSpecificTargetClasses()}, it will be called for all + * indexing operations and given a chance to determine if it can read from or + * write to the indexed structure. * - *

Property resolvers are considered to be ordered, and each will be called in turn. - * The only rule that affects the call order is that any resolver naming the target - * class directly in {@link #getSpecificTargetClasses()} will be called first, before - * the general resolvers. + *

Index accessors are considered to be ordered, and each will be called in + * turn. The only rule that affects the call order is that any index accessor + * which specifies explicit support for the target class via + * {@link #getSpecificTargetClasses()} will be called first, before other + * generic index accessors. * - * @author jackmiking lee - * @since 3.0 + * @author Jackmiking Lee + * @author Sam Brannen + * @since 6.2 + * @see PropertyAccessor */ -public interface IndexAccessor { +public interface IndexAccessor extends TargetedAccessor { + /** - * Return an array of classes for which this resolver should be called. - *

Returning {@code null} indicates this is a general resolver that - * can be called in an attempt to resolve a property on any type. - * @return an array of classes that this resolver is suitable for - * (or {@code null} if a general resolver) + * Get the set of classes for which this index accessor should be called. + *

Returning {@code null} or an empty array indicates this is a generic + * index accessor that can be called in an attempt to access an index on any + * type. + * @return an array of classes that this index accessor is suitable for + * (or {@code null} or an empty array if a generic index accessor) */ + @Override @Nullable Class[] getSpecificTargetClasses(); /** - * Called to determine if a resolver instance is able to access a specified property - * on a specified target object. + * Called to determine if this index accessor is able to read a specified + * index on a specified target object. * @param context the evaluation context in which the access is being attempted - * @param target the target object upon which the property is being accessed - * @param index the index of the array being accessed - * @return true if this resolver is able to read the property - * @throws AccessException if there is any problem determining whether the property can be read + * @param target the target object upon which the index is being accessed + * @param index the index being accessed + * @return true if this index accessor is able to read the index + * @throws AccessException if there is any problem determining whether the + * index can be read */ boolean canRead(EvaluationContext context, @Nullable Object target, Object index) throws AccessException; /** - * Called to read a property from a specified target object. - * Should only succeed if {@link #canRead} also returns {@code true}. + * Called to read an index from a specified target object. + *

Should only succeed if {@link #canRead} also returns {@code true}. * @param context the evaluation context in which the access is being attempted - * @param target the target object upon which the property is being accessed - * @param index the index of the array being accessed - * @return a TypedValue object wrapping the property value read and a type descriptor for it - * @throws AccessException if there is any problem accessing the property value + * @param target the target object upon which the index is being accessed + * @param index the index being accessed + * @return a TypedValue object wrapping the index value read and a type + * descriptor for it + * @throws AccessException if there is any problem reading the index value */ - ValueRef read(EvaluationContext context, @Nullable Object target,Object index) throws AccessException; + // TODO Change return type to TypedValue to avoid package cycle. + ValueRef read(EvaluationContext context, @Nullable Object target, Object index) throws AccessException; /** - * Called to determine if a resolver instance is able to write to a specified - * property on a specified target object. + * Called to determine if this index accessor is able to write to a specified + * index on a specified target object. * @param context the evaluation context in which the access is being attempted - * @param target the target object upon which the property is being accessed - * @param index the index of the array being accessed - * @return true if this resolver is able to write to the property + * @param target the target object upon which the index is being accessed + * @param index the index being accessed + * @return true if this index accessor is able to write to the index * @throws AccessException if there is any problem determining whether the - * property can be written to + * index can be written to */ boolean canWrite(EvaluationContext context, @Nullable Object target, Object index) throws AccessException; /** - * Called to write to a property on a specified target object. - * Should only succeed if {@link #canWrite} also returns {@code true}. + * Called to write to an index on a specified target object. + *

Should only succeed if {@link #canWrite} also returns {@code true}. * @param context the evaluation context in which the access is being attempted - * @param target the target object upon which the property is being accessed - * @param index the index of the array being accessed - * @param newValue the new value for the property - * @throws AccessException if there is any problem writing to the property value + * @param target the target object upon which the index is being accessed + * @param index the index being accessed + * @param newValue the new value for the index + * @throws AccessException if there is any problem writing to the index value */ void write(EvaluationContext context, @Nullable Object target, Object index, @Nullable Object newValue) throws AccessException; + } diff --git a/spring-expression/src/main/java/org/springframework/expression/PropertyAccessor.java b/spring-expression/src/main/java/org/springframework/expression/PropertyAccessor.java index c063750b1b12..a439046be2df 100644 --- a/spring-expression/src/main/java/org/springframework/expression/PropertyAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/PropertyAccessor.java @@ -34,21 +34,24 @@ *

Property accessors are considered to be ordered, and each will be called in * turn. The only rule that affects the call order is that any property accessor * which specifies explicit support for the target class via - * {@link #getSpecificTargetClasses()} will be called first, before the general + * {@link #getSpecificTargetClasses()} will be called first, before the generic * property accessors. * * @author Andy Clement * @since 3.0 + * @see IndexAccessor */ -public interface PropertyAccessor { +public interface PropertyAccessor extends TargetedAccessor { /** - * Return an array of classes for which this property accessor should be called. - *

Returning {@code null} indicates this is a general property accessor that - * can be called in an attempt to access a property on any type. + * Get the set of classes for which this property accessor should be called. + *

Returning {@code null} or an empty array indicates this is a generic + * property accessor that can be called in an attempt to access a property on + * any type. * @return an array of classes that this property accessor is suitable for - * (or {@code null} if a general property accessor) + * (or {@code null} if a generic property accessor) */ + @Override @Nullable Class[] getSpecificTargetClasses(); diff --git a/spring-expression/src/main/java/org/springframework/expression/TargetedAccessor.java b/spring-expression/src/main/java/org/springframework/expression/TargetedAccessor.java new file mode 100644 index 000000000000..88b91055f5da --- /dev/null +++ b/spring-expression/src/main/java/org/springframework/expression/TargetedAccessor.java @@ -0,0 +1,56 @@ +/* + * 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 org.springframework.expression; + +import org.springframework.lang.Nullable; + +/** + * Strategy for types that access elements of specific target classes. + * + *

This interface places no restrictions on what constitutes an element. + * + *

A targeted accessor can specify a set of target classes for which it should + * be called. However, if it returns {@code null} or an empty array from + * {@link #getSpecificTargetClasses()}, it will typically be called for all + * access operations and given a chance to determine if it supports a concrete + * access attempt. + * + *

Targeted accessors are considered to be ordered, and each will be called + * in turn. The only rule that affects the call order is that any accessor which + * specifies explicit support for a given target class via + * {@link #getSpecificTargetClasses()} will be called first, before other generic + * accessors that do not specify explicit support for the given target class. + * + * @author Sam Brannen + * @since 6.2 + * @see PropertyAccessor + * @see IndexAccessor + */ +public interface TargetedAccessor { + + /** + * Get the set of classes for which this accessor should be called. + *

Returning {@code null} or an empty array indicates this is a generic + * accessor that can be called in an attempt to access an element on any + * type. + * @return an array of classes that this accessor is suitable for + * (or {@code null} or an empty array if a generic accessor) + */ + @Nullable + Class[] getSpecificTargetClasses(); + +} diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/AstUtils.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/AstUtils.java index e4e710f7d611..665f1b6ee13c 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/AstUtils.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/AstUtils.java @@ -17,9 +17,11 @@ package org.springframework.expression.spel.ast; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import org.springframework.expression.PropertyAccessor; +import org.springframework.expression.TargetedAccessor; import org.springframework.lang.Nullable; import org.springframework.util.ObjectUtils; @@ -27,53 +29,92 @@ * Utility methods for use in the AST classes. * * @author Andy Clement + * @author Sam Brannen * @since 3.0.2 */ public abstract class AstUtils { /** - * Determine the set of property accessors that should be used to try to - * access a property on the specified target type. + * Determine the set of accessors that should be used to try to access an + * element on the specified target type. *

The accessors are considered to be in an ordered list; however, in the * returned list any accessors that are exact matches for the input target - * type (as opposed to 'general' accessors that could work for any type) are + * type (as opposed to 'generic' accessors that could work for any type) are * placed at the start of the list. In addition, if there are specific * accessors that exactly name the class in question and accessors that name * a specific class which is a supertype of the class in question, the latter * are put at the end of the specific accessors set and will be tried after * exactly matching accessors but before generic accessors. - * @param targetType the type upon which property access is being attempted - * @param propertyAccessors the list of property accessors to process - * @return a list of accessors that should be tried in order to access the property + * @param targetType the type upon which element access is being attempted + * @param accessors the list of element accessors to process + * @return a list of accessors that should be tried in order to access the + * element on the specified target type, or an empty list if no suitable + * accessor could be found + * @since 6.2 */ - public static List getPropertyAccessorsToTry( - @Nullable Class targetType, List propertyAccessors) { + public static List getAccessorsToTry( + @Nullable Class targetType, List accessors) { + + if (accessors.isEmpty()) { + return Collections.emptyList(); + } - List specificAccessors = new ArrayList<>(); - List generalAccessors = new ArrayList<>(); - for (PropertyAccessor accessor : propertyAccessors) { + List exactMatches = new ArrayList<>(); + List inexactMatches = new ArrayList<>(); + List genericMatches = new ArrayList<>(); + for (T accessor : accessors) { Class[] targets = accessor.getSpecificTargetClasses(); if (ObjectUtils.isEmpty(targets)) { // generic accessor that says it can be used for any type - generalAccessors.add(accessor); + genericMatches.add(accessor); } else if (targetType != null) { for (Class clazz : targets) { if (clazz == targetType) { - // add exact matches to the specificAccessors list - specificAccessors.add(accessor); + exactMatches.add(accessor); } else if (clazz.isAssignableFrom(targetType)) { - // add supertype matches to the front of the generalAccessors list - generalAccessors.add(0, accessor); + inexactMatches.add(accessor); } } } } - List accessors = new ArrayList<>(specificAccessors.size() + generalAccessors.size()); - accessors.addAll(specificAccessors); - accessors.addAll(generalAccessors); - return accessors; + + int size = exactMatches.size() + inexactMatches.size() + genericMatches.size(); + if (size == 0) { + return Collections.emptyList(); + } + else { + List result = new ArrayList<>(size); + result.addAll(exactMatches); + result.addAll(inexactMatches); + result.addAll(genericMatches); + return result; + } + } + + /** + * Determine the set of property accessors that should be used to try to + * access a property on the specified target type. + *

The accessors are considered to be in an ordered list; however, in the + * returned list any accessors that are exact matches for the input target + * type (as opposed to 'generic' accessors that could work for any type) are + * placed at the start of the list. In addition, if there are specific + * accessors that exactly name the class in question and accessors that name + * a specific class which is a supertype of the class in question, the latter + * are put at the end of the specific accessors set and will be tried after + * exactly matching accessors but before generic accessors. + * @param targetType the type upon which property access is being attempted + * @param propertyAccessors the list of property accessors to process + * @return a list of accessors that should be tried in order to access the + * property on the specified target type, or an empty list if no suitable + * accessor could be found + * @see #getAccessorsToTry(Class, List) + */ + public static List getPropertyAccessorsToTry( + @Nullable Class targetType, List propertyAccessors) { + + return getAccessorsToTry(targetType, propertyAccessors); } } 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 5a1c6f527dc1..88f534409e5e 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 @@ -17,11 +17,9 @@ package org.springframework.expression.spel.ast; import java.lang.reflect.Constructor; -import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.function.Supplier; import org.springframework.asm.Label; @@ -160,6 +158,7 @@ public TypedValue setValueInternal(ExpressionState state, Supplier 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. getValueRef(state).setValue(typedValue.getValue()); return typedValue; } @@ -249,71 +248,23 @@ else if (target instanceof Collection collection) { return new PropertyIndexingValueRef( target, (String) index, state.getEvaluationContext(), targetDescriptor); } - Optional optional = tryIndexAccessor(state, index); - if (optional.isPresent()) { - return optional.get(); - } - throw new SpelEvaluationException( - getStartPosition(), SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, targetDescriptor); - } - private Optional tryIndexAccessor(ExpressionState state, Object index) { - EvaluationContext context = state.getEvaluationContext(); - Object target = state.getActiveContextObject().getValue(); - if (context != null) { - List list = context.getIndexAccessors(); - if (list != null) { - List availableAccessors = getIndexAccessorsToTry(state.getActiveContextObject(), list); - try { - for (IndexAccessor indexAccessor : availableAccessors) { - if (indexAccessor.canRead(context, target, index)) { - ValueRef valueRef = indexAccessor.read(context, target, index); - return Optional.of(valueRef); - } - } - } - catch (Exception ex) { + EvaluationContext evalContext = state.getEvaluationContext(); + List accessorsToTry = getIndexAccessorsToTry(target, evalContext.getIndexAccessors()); + try { + for (IndexAccessor indexAccessor : accessorsToTry) { + if (indexAccessor.canRead(evalContext, target, index)) { + // TODO Introduce local IndexAccessorValueRef. + return indexAccessor.read(evalContext, target, index); } } } - return Optional.empty(); - } - - private List getIndexAccessorsToTry( - @Nullable Object contextObject, List propertyAccessors) { - - Class targetType; - if (contextObject instanceof TypedValue) { - targetType = ((TypedValue) contextObject).getTypeDescriptor().getObjectType(); + catch (Exception ex) { + // TODO throw SpelEvaluationException for "exception during index access" } - else { - targetType = (contextObject != null ? contextObject.getClass() : null); - } - - List specificAccessors = new ArrayList<>(); - List generalAccessors = new ArrayList<>(); - for (IndexAccessor resolver : propertyAccessors) { - Class[] targets = resolver.getSpecificTargetClasses(); - if (targets == null) { - // generic resolver that says it can be used for any type - generalAccessors.add(resolver); - } - else if (targetType != null) { - for (Class clazz : targets) { - if (clazz == targetType) { - specificAccessors.add(resolver); - break; - } - else if (clazz.isAssignableFrom(targetType)) { - generalAccessors.add(resolver); - } - } - } - } - List resolvers = new ArrayList<>(specificAccessors); - generalAccessors.removeAll(specificAccessors); - resolvers.addAll(generalAccessors); - return resolvers; + + throw new SpelEvaluationException( + getStartPosition(), SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, targetDescriptor); } @Override @@ -451,6 +402,22 @@ private void setExitTypeDescriptor(String descriptor) { } } + /** + * Determine the set of index accessors that should be used to try to access + * an index on the specified context object. + *

Delegates to {@link AstUtils#getAccessorsToTry(Class, List)}. + * @param targetObject the object upon which index access is being attempted + * @param indexAccessors the list of index accessors to process + * @return a list of accessors that should be tried in order to access the + * index, or an empty list if no suitable accessor could be found + */ + private static List getIndexAccessorsToTry( + @Nullable Object targetObject, List indexAccessors) { + + Class targetType = (targetObject != null ? targetObject.getClass() : null); + return AstUtils.getAccessorsToTry(targetType, indexAccessors); + } + private class ArrayIndexingValueRef implements ValueRef { diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java index 24ab07b9ca71..f8cd8a1cce5a 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/SimpleEvaluationContext.java @@ -28,7 +28,6 @@ import org.springframework.expression.BeanResolver; import org.springframework.expression.ConstructorResolver; import org.springframework.expression.EvaluationContext; -import org.springframework.expression.IndexAccessor; import org.springframework.expression.MethodResolver; import org.springframework.expression.OperatorOverloader; import org.springframework.expression.PropertyAccessor; @@ -147,11 +146,6 @@ public List getPropertyAccessors() { return this.propertyAccessors; } - @Override - public List getIndexAccessors() { - return null; - } - /** * Return an empty list, always, since this context does not support the * use of type references. diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardEvaluationContext.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardEvaluationContext.java index ebe22a449a86..5d196b7e930e 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardEvaluationContext.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardEvaluationContext.java @@ -146,10 +146,6 @@ public void setPropertyAccessors(List propertyAccessors) { this.propertyAccessors = propertyAccessors; } - public void setIndexAccessors(ListindexAccessors){ - this.indexAccessors=indexAccessors; - } - @Override public List getPropertyAccessors() { return initPropertyAccessors(); @@ -163,11 +159,53 @@ public boolean removePropertyAccessor(PropertyAccessor accessor) { return initPropertyAccessors().remove(accessor); } - public void addIndexAccessor(IndexAccessor accessor){ - initIndexAccessors().add(accessor); + /** + * Set the list of index accessors to use in this evaluation context. + *

Replaces any previously configured index accessors. + * @since 6.2 + * @see #getIndexAccessors() + * @see #addIndexAccessor(IndexAccessor) + * @see #removeIndexAccessor(IndexAccessor) + */ + public void setIndexAccessors(List indexAccessors) { + this.indexAccessors = indexAccessors; + } + + /** + * Get the list of index accessors configured in this evaluation context. + * @since 6.2 + * @see #setIndexAccessors(List) + * @see #addIndexAccessor(IndexAccessor) + * @see #removeIndexAccessor(IndexAccessor) + */ + @Override + public List getIndexAccessors() { + return initIndexAccessors(); } - public boolean removeIndexAccessor(IndexAccessor indexAccessor){ + /** + * Add the supplied index accessor to this evaluation context. + * @param indexAccessor the index accessor to add + * @since 6.2 + * @see #getIndexAccessors() + * @see #setIndexAccessors(List) + * @see #removeIndexAccessor(IndexAccessor) + */ + public void addIndexAccessor(IndexAccessor indexAccessor) { + initIndexAccessors().add(indexAccessor); + } + + /** + * Remove the supplied index accessor from this evaluation context. + * @param indexAccessor the index accessor to remove + * @return {@code true} if the index accessor was removed, {@code false} if + * the index accessor was not configured in this evaluation context + * @since 6.2 + * @see #getIndexAccessors() + * @see #setIndexAccessors(List) + * @see #addIndexAccessor(IndexAccessor) + */ + public boolean removeIndexAccessor(IndexAccessor indexAccessor) { return initIndexAccessors().remove(indexAccessor); } @@ -400,6 +438,7 @@ public void applyDelegatesTo(StandardEvaluationContext evaluationContext) { evaluationContext.setConstructorResolvers(new ArrayList<>(getConstructorResolvers())); evaluationContext.setMethodResolvers(new ArrayList<>(getMethodResolvers())); evaluationContext.setPropertyAccessors(new ArrayList<>(getPropertyAccessors())); + evaluationContext.setIndexAccessors(new ArrayList<>(getIndexAccessors())); evaluationContext.setTypeLocator(getTypeLocator()); evaluationContext.setTypeConverter(getTypeConverter()); @@ -420,9 +459,9 @@ private List initPropertyAccessors() { return accessors; } - private ListinitIndexAccessors(){ + private List initIndexAccessors() { List accessors = this.indexAccessors; - if(accessors == null){ + if (accessors == null) { accessors = new ArrayList<>(5); this.indexAccessors = accessors; } @@ -454,9 +493,4 @@ private static void addBeforeDefault(List list, T element) { list.add(list.size() - 1, element); } - @Override - public List getIndexAccessors() { - return initIndexAccessors(); - } - } 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 ab424b3b53a4..22e7717444fb 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 @@ -28,13 +28,20 @@ import java.util.Map; import java.util.Set; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +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.Nested; import org.junit.jupiter.api.Test; import org.springframework.expression.EvaluationContext; 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; @@ -449,6 +456,135 @@ static class RootContextWithIndexedProperties { } + @Nested + class IndexAccessorTests { // gh-26478 + + @Test + void addingAndRemovingIndexAccessors() { + ObjectMapper objectMapper = new ObjectMapper(); + IndexAccessor accessor1 = new JacksonArrayNodeIndexAccessor(objectMapper); + IndexAccessor accessor2 = new JacksonArrayNodeIndexAccessor(objectMapper); + + StandardEvaluationContext context = new StandardEvaluationContext(); + List indexAccessors = context.getIndexAccessors(); + assertThat(indexAccessors).isEmpty(); + + context.addIndexAccessor(accessor1); + assertThat(indexAccessors).containsExactly(accessor1); + + context.addIndexAccessor(accessor2); + assertThat(indexAccessors).containsExactly(accessor1, accessor2); + + List copy = new ArrayList<>(indexAccessors); + assertThat(context.removeIndexAccessor(accessor1)).isTrue(); + assertThat(context.removeIndexAccessor(accessor1)).isFalse(); + assertThat(indexAccessors).containsExactly(accessor2); + + context.setIndexAccessors(copy); + assertThat(context.getIndexAccessors()).containsExactly(accessor1, accessor2); + } + + @Test + void indexReadWrite() { + StandardEvaluationContext context = new StandardEvaluationContext(); + + ObjectMapper objectMapper = new ObjectMapper(); + context.addIndexAccessor(new JacksonArrayNodeIndexAccessor(objectMapper)); + + TextNode node0 = new TextNode("node0"); + TextNode node1 = new TextNode("node1"); + ArrayNode arrayNode = objectMapper.createArrayNode(); + arrayNode.addAll(List.of(node0, node1)); + + SpelExpressionParser parser = new SpelExpressionParser(); + Expression expr = parser.parseExpression("[0]"); + assertThat(expr.getValue(context, arrayNode)).isSameAs(node0); + + TextNode nodeX = new TextNode("nodeX"); + expr.setValue(context, arrayNode, nodeX); + // We use isEqualTo() instead of isSameAs(), since ObjectMapper.convertValue() + // converts the supplied TextNode to an equivalent JsonNode. + assertThat(expr.getValue(context, arrayNode)).isEqualTo(nodeX); + + NullNode nullNode = NullNode.getInstance(); + expr.setValue(context, arrayNode, nullNode); + assertThat(expr.getValue(context, arrayNode)).isSameAs(nullNode); + + expr = parser.parseExpression("[1]"); + assertThat(expr.getValue(context, arrayNode)).isSameAs(node1); + + expr = parser.parseExpression("[-1]"); + // Jackson's ArrayNode returns null for a non-existent index instead + // of throwing an ArrayIndexOutOfBoundsException or similar. + assertThat(expr.getValue(context, arrayNode)).isNull(); + } + + + /** + * {@link IndexAccessor} that knows how to read and write indexes in a + * Jackson {@link ArrayNode}. + */ + private static class JacksonArrayNodeIndexAccessor implements IndexAccessor { + + private final ObjectMapper objectMapper; + + JacksonArrayNodeIndexAccessor(ObjectMapper objectMapper) { + this.objectMapper = objectMapper; + } + + @Override + public Class[] getSpecificTargetClasses() { + return new Class[] { ArrayNode.class }; + } + + @Override + public boolean canRead(EvaluationContext context, Object target, Object index) { + return (target instanceof ArrayNode && index instanceof Integer); + } + + @Override + public ValueRef read(EvaluationContext context, Object target, Object index) { + return new ArrayNodeValueRef((ArrayNode) target, (Integer) index, this.objectMapper); + } + + @Override + public boolean canWrite(EvaluationContext context, Object target, Object index) { + return (target instanceof ArrayNode && index instanceof Integer); + } + + @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()); + } + + 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; + } + } + } + } + @Target({ElementType.FIELD}) @Retention(RetentionPolicy.RUNTIME) 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 0557ab64cfc7..287644f84538 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,10 +21,6 @@ import java.util.List; import java.util.Map; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.node.ArrayNode; -import com.fasterxml.jackson.databind.node.TextNode; import org.junit.jupiter.api.Test; import org.springframework.core.convert.TypeDescriptor; @@ -32,10 +28,8 @@ import org.springframework.expression.EvaluationContext; import org.springframework.expression.EvaluationException; 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.SpelExpression; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.SimpleEvaluationContext; @@ -150,25 +144,6 @@ void addingAndRemovingAccessors() { assertThat(ctx.getPropertyAccessors()).hasSize(2); } - @Test - public void testAddingRemovingIndexAccessors() { - StandardEvaluationContext ctx = new StandardEvaluationContext(); - List indexAccessors = ctx.getIndexAccessors(); - assertThat(indexAccessors.size()).isEqualTo(0); - JsonIndexAccessor jsonIndexAccessor=new JsonIndexAccessor(); - ctx.addIndexAccessor(jsonIndexAccessor); - assertThat(indexAccessors.size()).isEqualTo(1); - JsonIndexAccessor jsonIndexAccessor1=new JsonIndexAccessor(); - ctx.addIndexAccessor(jsonIndexAccessor1); - assertThat(indexAccessors.size()).isEqualTo(2); - List copy=new ArrayList(indexAccessors); - assertThat(ctx.removeIndexAccessor(jsonIndexAccessor)).isTrue(); - assertThat(ctx.removeIndexAccessor(jsonIndexAccessor)).isFalse(); - assertThat(indexAccessors.size()).isEqualTo(1); - ctx.setIndexAccessors(copy); - assertThat(ctx.getIndexAccessors().size()).isEqualTo(2); - } - @Test void accessingPropertyOfClass() { Expression expression = parser.parseExpression("name"); @@ -248,24 +223,6 @@ void propertyReadWrite() { assertThat(target.getName()).isEqualTo("p4"); assertThat(expr.getValue(context, target)).isEqualTo("p4"); } - @Test - public void indexReadWrite(){ - StandardEvaluationContext context=new StandardEvaluationContext(); - JsonIndexAccessor indexAccessor=new JsonIndexAccessor(); - context.addIndexAccessor(indexAccessor); - ArrayNode arrayNode=indexAccessor.objectMapper.createArrayNode(); - arrayNode.add(new TextNode("node0")); - arrayNode.add(new TextNode("node1")); - Expression expr=parser.parseExpression("[0]"); - assertThat(new TextNode("node0").equals(expr.getValue(context,arrayNode))).isTrue(); - expr.setValue(context,arrayNode,new TextNode("nodeUpdate")); - assertThat(new TextNode("nodeUpdate").equals(expr.getValue(context,arrayNode))).isTrue(); - Expression expr1=parser.parseExpression("[1]"); - assertThat(new TextNode("node1").equals(expr1.getValue(context,arrayNode))).isTrue(); - - - } - @Test void propertyReadWriteWithRootObject() { @@ -406,67 +363,4 @@ public void write(EvaluationContext context, Object target, String name, Object } } - private static class JsonIndexAccessor implements IndexAccessor { - ObjectMapper objectMapper=new ObjectMapper(); - public class ArrayValueRef implements ValueRef { - ArrayNode arrayNode; - Integer index; - - @Override - public TypedValue getValue() { - return new TypedValue(arrayNode.get(index)); - } - - @Override - public void setValue(Object newValue) { - arrayNode.set(index,objectMapper.convertValue(newValue, JsonNode.class)); - } - public void setArrayNode(ArrayNode arrayNode){ - this.arrayNode=arrayNode; - } - - public void setIndex(Object index) { - if (index instanceof Integer) { - this.index = (Integer) index; - } - } - - @Override - public boolean isWritable() { - return false; - } - } - - @Override - public Class[] getSpecificTargetClasses() { - return new Class[]{ - ArrayNode.class - }; - } - - @Override - public boolean canRead(EvaluationContext context, Object target, Object index) throws AccessException { - return true; - } - - @Override - public ValueRef read(EvaluationContext context, Object target, Object index) throws AccessException { - ArrayValueRef valueRef = new ArrayValueRef(); - valueRef.setArrayNode((ArrayNode) target); - valueRef.setIndex(index); - return valueRef; - } - - @Override - public boolean canWrite(EvaluationContext context, Object target, Object index) throws AccessException { - return true; - } - - @Override - public void write(EvaluationContext context, Object target, Object index, Object newValue) throws AccessException { - ValueRef valueRef=read(context,target,index); - valueRef.setValue(newValue); - } - } - } From b7c383373285dac2f6509b9acf5182836393961e Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 27 Mar 2024 10:24:56 +0100 Subject: [PATCH 3/7] Revise null-safety contracts in IndexAccessor SPI When indexing into an object, the target object can never be null. See gh-26409 See gh-26478 --- .../org/springframework/expression/IndexAccessor.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 e26136f6822c..b75ae537a03c 100644 --- a/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java @@ -68,7 +68,7 @@ public interface IndexAccessor extends TargetedAccessor { * @throws AccessException if there is any problem determining whether the * index can be read */ - boolean canRead(EvaluationContext context, @Nullable Object target, Object index) throws AccessException; + boolean canRead(EvaluationContext context, Object target, Object index) throws AccessException; /** * Called to read an index from a specified target object. @@ -81,7 +81,7 @@ public interface IndexAccessor extends TargetedAccessor { * @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, @Nullable Object target, Object index) throws AccessException; + ValueRef read(EvaluationContext context, Object target, Object index) throws AccessException; /** * Called to determine if this index accessor is able to write to a specified @@ -93,7 +93,7 @@ public interface IndexAccessor extends TargetedAccessor { * @throws AccessException if there is any problem determining whether the * index can be written to */ - boolean canWrite(EvaluationContext context, @Nullable Object target, Object index) throws AccessException; + boolean canWrite(EvaluationContext context, Object target, Object index) throws AccessException; /** * Called to write to an index on a specified target object. @@ -104,7 +104,7 @@ public interface IndexAccessor extends TargetedAccessor { * @param newValue the new value for the index * @throws AccessException if there is any problem writing to the index value */ - void write(EvaluationContext context, @Nullable Object target, Object index, @Nullable Object newValue) + void write(EvaluationContext context, Object target, Object index, @Nullable Object newValue) throws AccessException; } 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 4/7] 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 b75ae537a03c..9d33feeadc5a 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 88f534409e5e..cfd7d44986d7 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 TypedValue setValueInternal(ExpressionState state, Supplier 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 --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 22e7717444fb..c349765a1a7e 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.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; - } - } } } From 22bfe7da5ade3985847ef24a0ede21b4bc420165 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 27 Mar 2024 12:18:06 +0100 Subject: [PATCH 5/7] Introduce proper error handling for IndexAccessor support in SpEL See gh-26409 See gh-26478 --- .../expression/spel/SpelMessage.java | 11 +- .../expression/spel/ast/Indexer.java | 19 +-- .../expression/spel/IndexingTests.java | 120 +++++++++++++++++- 3 files changed, 140 insertions(+), 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 index 2356b6011c59..0ed9f8586b8b 100644 --- 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 index cfd7d44986d7..61dc309f283a 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 @@ -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 index c349765a1a7e..f49fe9b60bc6 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 @@ -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(); From 15511890bdd5cceaf37952e4b7886b42a43e0a46 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 8 Apr 2024 15:06:42 +0200 Subject: [PATCH 6/7] Revise Javadoc for IndexAccessor See gh-26409 See gh-26478 --- .../expression/IndexAccessor.java | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 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 9d33feeadc5a..e1719f54b5a0 100644 --- a/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/IndexAccessor.java @@ -58,49 +58,51 @@ public interface IndexAccessor extends TargetedAccessor { Class[] getSpecificTargetClasses(); /** - * Called to determine if this index accessor is able to read a specified - * index on a specified target object. + * Determine if this index accessor is able to read a specified index on a + * specified target object. * @param context the evaluation context in which the access is being attempted * @param target the target object upon which the index is being accessed * @param index the index being accessed - * @return true if this index accessor is able to read the index + * @return {@code true} if this index accessor is able to read the index * @throws AccessException if there is any problem determining whether the * index can be read */ boolean canRead(EvaluationContext context, Object target, Object index) throws AccessException; /** - * Called to read an index from a specified target object. - *

Should only succeed if {@link #canRead} also returns {@code true}. + * Read an index from a specified target object. + *

Should only be invoked if {@link #canRead} returns {@code true} for the + * same arguments. * @param context the evaluation context in which the access is being attempted * @param target the target object upon which the index is being accessed * @param index the index being accessed * @return a TypedValue object wrapping the index value read and a type - * descriptor for it - * @throws AccessException if there is any problem reading the index value + * descriptor for the value + * @throws AccessException if there is any problem reading the index */ TypedValue read(EvaluationContext context, Object target, Object index) throws AccessException; /** - * Called to determine if this index accessor is able to write to a specified - * index on a specified target object. + * Determine if this index accessor is able to write to a specified index on + * a specified target object. * @param context the evaluation context in which the access is being attempted * @param target the target object upon which the index is being accessed * @param index the index being accessed - * @return true if this index accessor is able to write to the index + * @return {@code true} if this index accessor is able to write to the index * @throws AccessException if there is any problem determining whether the * index can be written to */ boolean canWrite(EvaluationContext context, Object target, Object index) throws AccessException; /** - * Called to write to an index on a specified target object. - *

Should only succeed if {@link #canWrite} also returns {@code true}. + * Write to an index on a specified target object. + *

Should only be invoked if {@link #canWrite} returns {@code true} for the + * same arguments. * @param context the evaluation context in which the access is being attempted * @param target the target object upon which the index is being accessed * @param index the index being accessed * @param newValue the new value for the index - * @throws AccessException if there is any problem writing to the index value + * @throws AccessException if there is any problem writing to the index */ void write(EvaluationContext context, Object target, Object index, @Nullable Object newValue) throws AccessException; From 1c9cff668c4765f4dd676958a6386de90af8fd7e Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 10 Apr 2024 15:09:48 +0200 Subject: [PATCH 7/7] Honor IndexAccessor#canWrite() and cache accessors See gh-26409 See gh-26478 --- .../expression/spel/ast/Indexer.java | 315 ++++++++++++++---- .../expression/spel/IndexingTests.java | 25 +- 2 files changed, 264 insertions(+), 76 deletions(-) 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 61dc309f283a..16be5d19adb2 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 @@ -74,7 +74,24 @@ */ public class Indexer extends SpelNodeImpl { - private enum IndexedType {ARRAY, LIST, MAP, STRING, OBJECT} + private enum IndexedType {ARRAY, LIST, MAP, STRING, OBJECT, CUSTOM} + + private enum AccessMode { + + READ(true, false), + + WRITE(false, true), + + READ_WRITE(true, true); + + private final boolean supportsReads; + private final boolean supportsWrites; + + private AccessMode(boolean supportsReads, boolean supportsWrites) { + this.supportsReads = supportsReads; + this.supportsWrites = supportsWrites; + } + } private final boolean nullSafe; @@ -88,33 +105,81 @@ private enum IndexedType {ARRAY, LIST, MAP, STRING, OBJECT} @Nullable private volatile String arrayTypeDescriptor; - // These fields are used when the indexer is being used as a property read accessor. - // If the name and target type match these cached values then the cachedReadAccessor - // is used to read the property. If they do not match, the correct accessor is - // discovered and then cached for later use. + // These fields are used when the Indexer is being used as PropertyAccessor + // for reading. If the name and target type match these cached values, then + // the cachedPropertyReadAccessor is used to read the property. If they do + // not match, a suitable accessor is discovered and then cached for later use. @Nullable - private String cachedReadName; + private String cachedPropertyReadName; @Nullable - private Class cachedReadTargetType; + private Class cachedPropertyReadTargetType; @Nullable - private PropertyAccessor cachedReadAccessor; + private PropertyAccessor cachedPropertyReadAccessor; - // These fields are used when the indexer is being used as a property write accessor. - // If the name and target type match these cached values then the cachedWriteAccessor - // is used to write the property. If they do not match, the correct accessor is - // discovered and then cached for later use. + // These fields are used when the Indexer is being used as a PropertyAccessor + // for writing. If the name and target type match these cached values, then + // the cachedPropertyWriteAccessor is used to write the property. If they do + // not match, a suitable accessor is discovered and then cached for later use. + + @Nullable + private String cachedPropertyWriteName; @Nullable - private String cachedWriteName; + private Class cachedPropertyWriteTargetType; @Nullable - private Class cachedWriteTargetType; + private PropertyAccessor cachedPropertyWriteAccessor; + // These fields are used when the Indexer is being used as an IndexAccessor + // for reading. If the index value and target type match these cached values, + // then the cachedIndexReadAccessor is used to read the index. If they do not + // match, a suitable accessor is discovered and then cached for later use. + + /** + * The index value: the value inside the square brackets, such as the + * Integer 0 in [0], the String "name" in ['name'], etc. + */ @Nullable - private PropertyAccessor cachedWriteAccessor; + private Object cachedIndexReadIndex; + + /** + * The target type on which the index is being read. + */ + @Nullable + private Class cachedIndexReadTargetType; + + /** + * Cached {@link IndexAccessor} for reading. + */ + @Nullable + private IndexAccessor cachedIndexReadAccessor; + + // These fields are used when the Indexer is being used as an IndexAccessor + // for writing. If the name and target type match these cached values, then + // the cachedIndexWriteAccessor is used to read the property. If they do not + // match, a suitable accessor is discovered and then cached for later use. + + /** + * The index value: the value inside the square brackets, such as the + * Integer 0 in [0], the String "name" in ['name'], etc. + */ + @Nullable + private Object cachedIndexWriteIndex; + + /** + * The target type on which the index is being written. + */ + @Nullable + private Class cachedIndexWriteTargetType; + + /** + * Cached {@link IndexAccessor} for writing. + */ + @Nullable + private IndexAccessor cachedIndexWriteAccessor; /** @@ -150,7 +215,7 @@ public final boolean isNullSafe() { @Override public TypedValue getValueInternal(ExpressionState state) throws EvaluationException { - return getValueRef(state).getValue(); + return getValueRef(state, AccessMode.READ).getValue(); } @Override @@ -158,19 +223,21 @@ public TypedValue setValueInternal(ExpressionState state, Supplier v throws EvaluationException { TypedValue typedValue = valueSupplier.get(); - // TODO Query IndexAccessor's canWrite() method before invoking its write() method. - getValueRef(state).setValue(typedValue.getValue()); + getValueRef(state, AccessMode.WRITE).setValue(typedValue.getValue()); return typedValue; } @Override public boolean isWritable(ExpressionState expressionState) throws SpelEvaluationException { - return getValueRef(expressionState).isWritable(); + return getValueRef(expressionState, AccessMode.WRITE).isWritable(); } - @Override protected ValueRef getValueRef(ExpressionState state) throws EvaluationException { + return getValueRef(state, AccessMode.READ_WRITE); + } + + private ValueRef getValueRef(ExpressionState state, AccessMode accessMode) throws EvaluationException { TypedValue context = state.getActiveContextObject(); Object target = context.getValue(); @@ -241,27 +308,45 @@ else if (target instanceof Collection collection) { } } - // Try and treat the index value as a property of the context object + // Try to treat the index value as a property of the context object. TypeDescriptor valueType = indexValue.getTypeDescriptor(); if (valueType != null && String.class == valueType.getType()) { this.indexedType = IndexedType.OBJECT; - return new PropertyIndexingValueRef( + return new PropertyAccessorValueRef( target, (String) index, state.getEvaluationContext(), targetDescriptor); } EvaluationContext evalContext = state.getEvaluationContext(); List accessorsToTry = getIndexAccessorsToTry(target, evalContext.getIndexAccessors()); - try { - for (IndexAccessor indexAccessor : accessorsToTry) { - if (indexAccessor.canRead(evalContext, target, index)) { - return new IndexAccessorValueRef(indexAccessor, target, index, evalContext, targetDescriptor); + if (accessMode.supportsReads) { + try { + for (IndexAccessor indexAccessor : accessorsToTry) { + if (indexAccessor.canRead(evalContext, target, index)) { + this.indexedType = IndexedType.CUSTOM; + return new IndexAccessorValueRef(target, index, evalContext, targetDescriptor); + } } } + catch (Exception ex) { + throw new SpelEvaluationException( + getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_INDEX_READ, + index, target.getClass().getTypeName()); + } } - catch (Exception ex) { - throw new SpelEvaluationException( - getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_INDEX_READ, index, - target.getClass().getTypeName()); + if (accessMode.supportsWrites) { + try { + for (IndexAccessor indexAccessor : accessorsToTry) { + if (indexAccessor.canWrite(evalContext, target, index)) { + this.indexedType = IndexedType.CUSTOM; + return new IndexAccessorValueRef(target, index, evalContext, targetDescriptor); + } + } + } + catch (Exception ex) { + throw new SpelEvaluationException( + getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_INDEX_WRITE, + index, target.getClass().getTypeName()); + } } throw new SpelEvaluationException( @@ -284,8 +369,8 @@ else if (this.indexedType == IndexedType.OBJECT) { // If the string name is changing, the accessor is clearly going to change. // So compilation is only possible if the index expression is a StringLiteral. return (index instanceof StringLiteral && - this.cachedReadAccessor instanceof CompilablePropertyAccessor compilablePropertyAccessor && - compilablePropertyAccessor.isCompilable()); + this.cachedPropertyReadAccessor instanceof CompilablePropertyAccessor cpa && + cpa.isCompilable()); } return false; } @@ -364,7 +449,8 @@ else if (this.indexedType == IndexedType.OBJECT) { throw new IllegalStateException( "Index expression must be a StringLiteral, but was: " + index.getClass().getName()); } - CompilablePropertyAccessor compilablePropertyAccessor = (CompilablePropertyAccessor) this.cachedReadAccessor; + CompilablePropertyAccessor compilablePropertyAccessor = + (CompilablePropertyAccessor) this.cachedPropertyReadAccessor; Assert.state(compilablePropertyAccessor != null, "No cached read accessor"); String propertyName = (String) stringLiteral.getLiteralValue().getValue(); Assert.state(propertyName != null, "No property name"); @@ -403,6 +489,34 @@ private void setExitTypeDescriptor(String descriptor) { } } + private void updatePropertyReadState(@Nullable PropertyAccessor propertyAccessor, @Nullable String name, + @Nullable Class targetType) { + this.cachedPropertyReadAccessor = propertyAccessor; + this.cachedPropertyReadName = name; + this.cachedPropertyReadTargetType = targetType; + } + + private void updatePropertyWriteState(@Nullable PropertyAccessor propertyAccessor, @Nullable String name, + @Nullable Class targetType) { + this.cachedPropertyWriteAccessor = propertyAccessor; + this.cachedPropertyWriteName = name; + this.cachedPropertyWriteTargetType = targetType; + } + + private void updateIndexReadState(@Nullable IndexAccessor indexAccessor, @Nullable Object index, + @Nullable Class targetType) { + this.cachedIndexReadAccessor = indexAccessor; + this.cachedIndexReadIndex = index; + this.cachedIndexReadTargetType = targetType; + } + + private void updateIndexWriteState(@Nullable IndexAccessor indexAccessor, @Nullable Object index, + @Nullable Class targetType) { + this.cachedIndexWriteAccessor = indexAccessor; + this.cachedIndexWriteIndex = index; + this.cachedIndexWriteTargetType = targetType; + } + /** * Determine the set of index accessors that should be used to try to access * an index on the specified context object. @@ -637,7 +751,7 @@ public boolean isWritable() { } - private class PropertyIndexingValueRef implements ValueRef { + private class PropertyAccessorValueRef implements ValueRef { private final Object targetObject; @@ -647,7 +761,7 @@ private class PropertyIndexingValueRef implements ValueRef { private final TypeDescriptor targetObjectTypeDescriptor; - public PropertyIndexingValueRef(Object targetObject, String value, + public PropertyAccessorValueRef(Object targetObject, String value, EvaluationContext evaluationContext, TypeDescriptor targetObjectTypeDescriptor) { this.targetObject = targetObject; @@ -657,29 +771,27 @@ public PropertyIndexingValueRef(Object targetObject, String value, } @Override - @SuppressWarnings("NullAway") public TypedValue getValue() { - Class targetObjectRuntimeClass = getObjectClass(this.targetObject); + Class targetType = getObjectClass(this.targetObject); try { - if (Indexer.this.cachedReadName != null && Indexer.this.cachedReadName.equals(this.name) && - Indexer.this.cachedReadTargetType != null && - Indexer.this.cachedReadTargetType.equals(targetObjectRuntimeClass)) { + String cachedPropertyReadName = Indexer.this.cachedPropertyReadName; + Class cachedPropertyReadTargetType = Indexer.this.cachedPropertyReadTargetType; + if (cachedPropertyReadName != null && cachedPropertyReadName.equals(this.name) && + cachedPropertyReadTargetType != null && cachedPropertyReadTargetType.equals(targetType)) { // It is OK to use the cached accessor - PropertyAccessor accessor = Indexer.this.cachedReadAccessor; - Assert.state(accessor != null, "No cached read accessor"); + PropertyAccessor accessor = Indexer.this.cachedPropertyReadAccessor; + Assert.state(accessor != null, "No cached PropertyAccessor for reading"); return accessor.read(this.evaluationContext, this.targetObject, this.name); } List accessorsToTry = AstUtils.getPropertyAccessorsToTry( - targetObjectRuntimeClass, this.evaluationContext.getPropertyAccessors()); + targetType, this.evaluationContext.getPropertyAccessors()); for (PropertyAccessor accessor : accessorsToTry) { if (accessor.canRead(this.evaluationContext, this.targetObject, this.name)) { if (accessor instanceof ReflectivePropertyAccessor reflectivePropertyAccessor) { accessor = reflectivePropertyAccessor.createOptimalAccessor( this.evaluationContext, this.targetObject, this.name); } - Indexer.this.cachedReadAccessor = accessor; - Indexer.this.cachedReadName = this.name; - Indexer.this.cachedReadTargetType = targetObjectRuntimeClass; + updatePropertyReadState(accessor, this.name, targetType); if (accessor instanceof CompilablePropertyAccessor compilablePropertyAccessor) { setExitTypeDescriptor(CodeFlow.toDescriptor(compilablePropertyAccessor.getPropertyType())); } @@ -696,26 +808,24 @@ public TypedValue getValue() { } @Override - @SuppressWarnings("NullAway") public void setValue(@Nullable Object newValue) { - Class contextObjectClass = getObjectClass(this.targetObject); + Class targetType = getObjectClass(this.targetObject); try { - if (Indexer.this.cachedWriteName != null && Indexer.this.cachedWriteName.equals(this.name) && - Indexer.this.cachedWriteTargetType != null && - Indexer.this.cachedWriteTargetType.equals(contextObjectClass)) { + String cachedPropertyWriteName = Indexer.this.cachedPropertyWriteName; + Class cachedPropertyWriteTargetType = Indexer.this.cachedPropertyWriteTargetType; + if (cachedPropertyWriteName != null && cachedPropertyWriteName.equals(this.name) && + cachedPropertyWriteTargetType != null && cachedPropertyWriteTargetType.equals(targetType)) { // It is OK to use the cached accessor - PropertyAccessor accessor = Indexer.this.cachedWriteAccessor; - Assert.state(accessor != null, "No cached write accessor"); + PropertyAccessor accessor = Indexer.this.cachedPropertyWriteAccessor; + Assert.state(accessor != null, "No cached PropertyAccessor for writing"); accessor.write(this.evaluationContext, this.targetObject, this.name, newValue); return; } List accessorsToTry = AstUtils.getPropertyAccessorsToTry( - contextObjectClass, this.evaluationContext.getPropertyAccessors()); + targetType, this.evaluationContext.getPropertyAccessors()); for (PropertyAccessor accessor : accessorsToTry) { if (accessor.canWrite(this.evaluationContext, this.targetObject, this.name)) { - Indexer.this.cachedWriteName = this.name; - Indexer.this.cachedWriteTargetType = contextObjectClass; - Indexer.this.cachedWriteAccessor = accessor; + updatePropertyWriteState(accessor, this.name, targetType); accessor.write(this.evaluationContext, this.targetObject, this.name, newValue); return; } @@ -878,8 +988,6 @@ public boolean isWritable() { private class IndexAccessorValueRef implements ValueRef { - private final IndexAccessor indexAccessor; - private final Object target; private final Object index; @@ -889,10 +997,9 @@ private class IndexAccessorValueRef implements ValueRef { private final TypeDescriptor typeDescriptor; - IndexAccessorValueRef(IndexAccessor indexAccessor, Object target, Object index, - EvaluationContext evaluationContext, TypeDescriptor typeDescriptor) { + IndexAccessorValueRef(Object target, Object index, EvaluationContext evaluationContext, + TypeDescriptor typeDescriptor) { - this.indexAccessor = indexAccessor; this.target = target; this.index = index; this.evaluationContext = evaluationContext; @@ -902,26 +1009,102 @@ private class IndexAccessorValueRef implements ValueRef { @Override public TypedValue getValue() { + Class targetType = getObjectClass(this.target); + Exception exception = null; try { - return this.indexAccessor.read(this.evaluationContext, this.target, this.index); + Object cachedIndexReadIndex = Indexer.this.cachedIndexReadIndex; + Class cachedIndexReadTargetType = Indexer.this.cachedIndexReadTargetType; + // Is it OK to use the cached IndexAccessor? + if (cachedIndexReadIndex != null && cachedIndexReadIndex.equals(this.index) && + cachedIndexReadTargetType != null && cachedIndexReadTargetType.equals(targetType)) { + IndexAccessor accessor = Indexer.this.cachedIndexReadAccessor; + Assert.state(accessor != null, "No cached IndexAccessor for reading"); + if (this.evaluationContext.getIndexAccessors().contains(accessor)) { + try { + return accessor.read(this.evaluationContext, this.target, this.index); + } + catch (Exception ex) { + // This is OK: it may have gone stale due to a class change. + // So, we track the exception and try to find a new accessor + // before giving up... + exception = ex; + } + } + // If the above code block did not use a cached accessor, + // we need to reset our cached state. + updateIndexReadState(null, null, null); + } + List accessorsToTry = + getIndexAccessorsToTry(this.target, this.evaluationContext.getIndexAccessors()); + for (IndexAccessor indexAccessor : accessorsToTry) { + if (indexAccessor.canRead(this.evaluationContext, this.target, this.index)) { + updateIndexReadState(indexAccessor, this.index, targetType); + return indexAccessor.read(this.evaluationContext, this.target, this.index); + } + } } catch (Exception ex) { + exception = ex; + } + + if (exception != null) { throw new SpelEvaluationException( - getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_INDEX_READ, this.index, + getStartPosition(), exception, SpelMessage.EXCEPTION_DURING_INDEX_READ, this.index, this.typeDescriptor.toString()); } + throw new SpelEvaluationException(getStartPosition(), + SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, this.typeDescriptor.toString()); } @Override public void setValue(@Nullable Object newValue) { + Class targetType = getObjectClass(this.target); + Exception exception = null; try { - this.indexAccessor.write(this.evaluationContext, this.target, this.index, newValue); + Object cachedIndexWriteIndex = Indexer.this.cachedIndexWriteIndex; + Class cachedIndexWriteTargetType = Indexer.this.cachedIndexWriteTargetType; + // Is it OK to use the cached IndexAccessor? + if (cachedIndexWriteIndex != null && cachedIndexWriteIndex.equals(this.index) && + cachedIndexWriteTargetType != null && cachedIndexWriteTargetType.equals(targetType)) { + IndexAccessor accessor = Indexer.this.cachedIndexWriteAccessor; + Assert.state(accessor != null, "No cached IndexAccessor for writing"); + if (this.evaluationContext.getIndexAccessors().contains(accessor)) { + try { + accessor.write(this.evaluationContext, this.target, this.index, newValue); + return; + } + catch (Exception ex) { + // This is OK: it may have gone stale due to a class change. + // So, we track the exception and try to find a new accessor + // before giving up... + exception = ex; + } + } + // If the above code block did not use a cached accessor, + // we need to reset our cached state. + updateIndexWriteState(null, null, null); + } + List accessorsToTry = + getIndexAccessorsToTry(this.target, this.evaluationContext.getIndexAccessors()); + for (IndexAccessor indexAccessor : accessorsToTry) { + if (indexAccessor.canWrite(this.evaluationContext, this.target, this.index)) { + updateIndexWriteState(indexAccessor, this.index, targetType); + indexAccessor.write(this.evaluationContext, this.target, this.index, newValue); + return; + } + } } catch (Exception ex) { + exception = ex; + } + + if (exception != null) { throw new SpelEvaluationException( - getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_INDEX_WRITE, this.index, + getStartPosition(), exception, SpelMessage.EXCEPTION_DURING_INDEX_WRITE, this.index, this.typeDescriptor.toString()); } + throw new SpelEvaluationException(getStartPosition(), + SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, this.typeDescriptor.toString()); } @Override 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 f49fe9b60bc6..452a61badaf0 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 @@ -33,7 +33,6 @@ 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; @@ -56,6 +55,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; 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; @@ -513,6 +513,7 @@ void canReadThrowsException() throws Exception { RuntimeException exception = new RuntimeException("Boom!"); IndexAccessor mock = mock(); + given(mock.getSpecificTargetClasses()).willReturn(null); given(mock.canRead(any(), eq(this), any())).willThrow(exception); context.addIndexAccessor(mock); @@ -525,7 +526,9 @@ void canReadThrowsException() throws Exception { .withCause(exception) .extracting(SpelEvaluationException::getMessageCode).isEqualTo(EXCEPTION_DURING_INDEX_READ); + verify(mock, times(1)).getSpecificTargetClasses(); verify(mock, times(1)).canRead(any(), any(), any()); + verifyNoMoreInteractions(mock); } @Test @@ -534,6 +537,7 @@ void readThrowsException() throws Exception { RuntimeException exception = new RuntimeException("Boom!"); IndexAccessor mock = mock(); + given(mock.getSpecificTargetClasses()).willReturn(null); given(mock.canRead(any(), eq(this), any())).willReturn(true); given(mock.read(any(), eq(this), any())).willThrow(exception); context.addIndexAccessor(mock); @@ -547,20 +551,19 @@ void readThrowsException() throws Exception { .withCause(exception) .extracting(SpelEvaluationException::getMessageCode).isEqualTo(EXCEPTION_DURING_INDEX_READ); - verify(mock, times(1)).canRead(any(), any(), any()); + verify(mock, times(2)).getSpecificTargetClasses(); + verify(mock, times(2)).canRead(any(), any(), any()); verify(mock, times(1)).read(any(), any(), any()); + verifyNoMoreInteractions(mock); } - @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.getSpecificTargetClasses()).willReturn(null); given(mock.canWrite(eq(context), eq(this), eq(0))).willThrow(exception); context.addIndexAccessor(mock); @@ -573,7 +576,9 @@ void canWriteThrowsException() throws Exception { .withCause(exception) .extracting(SpelEvaluationException::getMessageCode).isEqualTo(EXCEPTION_DURING_INDEX_WRITE); + verify(mock, times(1)).getSpecificTargetClasses(); verify(mock, times(1)).canWrite(any(), any(), any()); + verifyNoMoreInteractions(mock); } @Test @@ -582,8 +587,7 @@ void writeThrowsException() throws Exception { 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.getSpecificTargetClasses()).willReturn(null); given(mock.canWrite(eq(context), eq(this), eq(0))).willReturn(true); doThrow(exception).when(mock).write(any(), any(), any(), any()); context.addIndexAccessor(mock); @@ -597,9 +601,10 @@ void writeThrowsException() throws Exception { .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(2)).getSpecificTargetClasses(); + verify(mock, times(2)).canWrite(any(), any(), any()); verify(mock, times(1)).write(any(), any(), any(), any()); + verifyNoMoreInteractions(mock); } @Test