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 1067eddc0ced..ca80b4624bf7 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 @@ -303,7 +303,11 @@ public enum SpelMessage { /** @since 6.2 */ EXCEPTION_DURING_INDEX_WRITE(Kind.ERROR, 1084, - "A problem occurred while attempting to write index ''{0}'' in ''{1}''"); + "A problem occurred while attempting to write index ''{0}'' in ''{1}''"), + + /** @since 7.1 */ + COMPETING_ACCESSORS(Kind.ERROR, 1085, + "Found competing read and write accessors for ''{0}'' in ''{1}''"); 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 464a73f07e92..05f5a25ff8ed 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 @@ -256,11 +256,19 @@ private ValueRef getValueRef(ExpressionState state, AccessMode accessMode) throw try { for (IndexAccessor indexAccessor : accessorsToTry) { if (indexAccessor.canRead(evalContext, target, index)) { + if (accessMode.supportsWrites && !indexAccessor.canWrite(evalContext, target, index) && + hasCompetingWriteAccessor(accessorsToTry, indexAccessor, evalContext, target, index)) { + throw new SpelEvaluationException(getStartPosition(), SpelMessage.COMPETING_ACCESSORS, + index, target.getClass().getTypeName()); + } this.indexedType = IndexedType.CUSTOM; return new IndexAccessorValueRef(target, index, evalContext, targetDescriptor); } } } + catch (SpelEvaluationException ex) { + throw ex; + } catch (Exception ex) { throw new SpelEvaluationException( getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_INDEX_READ, @@ -472,6 +480,17 @@ private static Class getObjectType(Object obj) { return (obj instanceof Class clazz ? clazz : obj.getClass()); } + private static boolean hasCompetingWriteAccessor(List accessors, IndexAccessor readAccessor, + EvaluationContext evalContext, Object target, Object index) throws AccessException { + + for (IndexAccessor accessor : accessors) { + if (accessor != readAccessor && accessor.canWrite(evalContext, target, index)) { + return true; + } + } + return false; + } + /** * Tracks state when the {@code Indexer} is being used as a {@link PropertyAccessor}. diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java index b5f5ff27f694..6289a0fc1036 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java @@ -128,7 +128,13 @@ public TypedValue getValueInternal(ExpressionState state) throws EvaluationExcep private TypedValue getValueInternal(TypedValue contextObject, EvaluationContext evalContext, boolean isAutoGrowNullReferences) throws EvaluationException { - TypedValue result = readProperty(contextObject, evalContext, this.name); + return getValueInternal(contextObject, evalContext, isAutoGrowNullReferences, false); + } + + private TypedValue getValueInternal(TypedValue contextObject, EvaluationContext evalContext, + boolean isAutoGrowNullReferences, boolean readWriteRequired) throws EvaluationException { + + TypedValue result = readProperty(contextObject, evalContext, this.name, readWriteRequired); // Dynamically create the objects if the user has requested that optional behavior if (result.getValue() == null && isAutoGrowNullReferences && @@ -140,14 +146,14 @@ private TypedValue getValueInternal(TypedValue contextObject, EvaluationContext if (isWritableProperty(this.name, contextObject, evalContext)) { List newList = new ArrayList<>(); writeProperty(contextObject, evalContext, this.name, newList); - result = readProperty(contextObject, evalContext, this.name); + result = readProperty(contextObject, evalContext, this.name, false); } } else if (Map.class == resultDescriptor.getType()) { if (isWritableProperty(this.name,contextObject, evalContext)) { Map newMap = new HashMap<>(); writeProperty(contextObject, evalContext, this.name, newMap); - result = readProperty(contextObject, evalContext, this.name); + result = readProperty(contextObject, evalContext, this.name, false); } } else { @@ -157,7 +163,7 @@ else if (Map.class == resultDescriptor.getType()) { Class clazz = resultDescriptor.getType(); Object newObject = ReflectionUtils.accessibleConstructor(clazz).newInstance(); writeProperty(contextObject, evalContext, this.name, newObject); - result = readProperty(contextObject, evalContext, this.name); + result = readProperty(contextObject, evalContext, this.name, false); } } catch (InvocationTargetException ex) { @@ -197,7 +203,8 @@ public String toStringAST() { * @return the value of the property * @throws EvaluationException if any problem accessing the property, or if it cannot be found */ - private TypedValue readProperty(TypedValue contextObject, EvaluationContext evalContext, String name) + private TypedValue readProperty(TypedValue contextObject, EvaluationContext evalContext, String name, + boolean readWriteRequired) throws EvaluationException { final Object originalTarget = contextObject.getValue(); @@ -245,6 +252,11 @@ private TypedValue readProperty(TypedValue contextObject, EvaluationContext eval for (PropertyAccessor accessor : accessorsToTry) { // First, attempt to find the property on the target object. if (accessor.canRead(evalContext, target, name)) { + if (readWriteRequired && !accessor.canWrite(evalContext, target, name) && + hasCompetingWriteAccessor(accessorsToTry, accessor, evalContext, target, name)) { + throw new SpelEvaluationException(getStartPosition(), SpelMessage.COMPETING_ACCESSORS, + name, FormatHelper.formatClassNameForMessage(getObjectClass(target))); + } if (accessor instanceof ReflectivePropertyAccessor reflectivePropertyAccessor) { accessor = reflectivePropertyAccessor.createOptimalAccessor( evalContext, target, name); @@ -254,6 +266,12 @@ private TypedValue readProperty(TypedValue contextObject, EvaluationContext eval } // Second, attempt to find the property on the original Optional instance. else if (fallbackOptionalTarget != null && accessor.canRead(evalContext, fallbackOptionalTarget, name)) { + if (readWriteRequired && !accessor.canWrite(evalContext, fallbackOptionalTarget, name) && + hasCompetingWriteAccessor( + accessorsToTry, accessor, evalContext, fallbackOptionalTarget, name)) { + throw new SpelEvaluationException(getStartPosition(), SpelMessage.COMPETING_ACCESSORS, + name, FormatHelper.formatClassNameForMessage(getObjectClass(fallbackOptionalTarget))); + } if (accessor instanceof ReflectivePropertyAccessor reflectivePropertyAccessor) { accessor = reflectivePropertyAccessor.createOptimalAccessor( evalContext, fallbackOptionalTarget, name); @@ -266,6 +284,9 @@ else if (fallbackOptionalTarget != null && accessor.canRead(evalContext, fallbac } } } + catch (SpelEvaluationException ex) { + throw ex; + } catch (Exception ex) { throw new SpelEvaluationException(ex, SpelMessage.EXCEPTION_DURING_PROPERTY_READ, name, ex.getMessage()); } @@ -286,6 +307,17 @@ else if (fallbackOptionalTarget != null && accessor.canRead(evalContext, fallbac } } + private static boolean hasCompetingWriteAccessor(List accessors, PropertyAccessor readAccessor, + EvaluationContext evalContext, @Nullable Object target, String name) throws AccessException { + + for (PropertyAccessor accessor : accessors) { + if (accessor != readAccessor && accessor.canWrite(evalContext, target, name)) { + return true; + } + } + return false; + } + private void writeProperty( TypedValue contextObject, EvaluationContext evalContext, String name, @Nullable Object newValue) throws EvaluationException { @@ -432,7 +464,7 @@ public AccessorValueRef(PropertyOrFieldReference propertyOrFieldReference, Typed @Override public TypedValue getValue() { TypedValue value = - this.ref.getValueInternal(this.contextObject, this.evalContext, this.autoGrowNullReferences); + this.ref.getValueInternal(this.contextObject, this.evalContext, this.autoGrowNullReferences, true); if (this.ref.cachedReadAccessor instanceof CompilablePropertyAccessor compilablePropertyAccessor) { this.ref.setExitTypeDescriptor(CodeFlow.toDescriptor(compilablePropertyAccessor.getPropertyType())); } 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 4d2aacf34ba6..e49cc78a2f4d 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 @@ -63,6 +63,7 @@ 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.COMPETING_ACCESSORS; 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; @@ -716,6 +717,18 @@ void readAndWriteIndexWithSimpleEvaluationContext() { assertThat(expr.getValue(context, arrayNode)).isSameAs(node1); } + @Test // gh-32737 + void competingReadAndWriteIndexAccessors() { + context.addIndexAccessor(new ReadOnlyIndexAccessor()); + context.addIndexAccessor(new WriteOnlyIndexAccessor()); + + Expression expr = parser.parseExpression("[0]++"); + + assertThatExceptionOfType(SpelEvaluationException.class) + .isThrownBy(() -> expr.getValue(context, this)) + .extracting(SpelEvaluationException::getMessageCode).isEqualTo(COMPETING_ACCESSORS); + } + @Test // gh-32706 void readIndexWithStringIndexType() { BirdNameToColorMappings birdNameMappings = new BirdNameToColorMappings(); @@ -847,6 +860,62 @@ public void write(EvaluationContext context, Object target, Object index, @Nulla arrayNode.set(intIndex, this.objectMapper.convertValue(newValue, JsonNode.class)); } } + + + private static class ReadOnlyIndexAccessor implements IndexAccessor { + + @Override + public Class[] getSpecificTargetClasses() { + return null; + } + + @Override + public boolean canRead(EvaluationContext context, Object target, Object index) { + return true; + } + + @Override + public TypedValue read(EvaluationContext context, Object target, Object index) { + return new TypedValue(1); + } + + @Override + public boolean canWrite(EvaluationContext context, Object target, Object index) { + return false; + } + + @Override + public void write(EvaluationContext context, Object target, Object index, @Nullable Object newValue) { + } + } + + + private static class WriteOnlyIndexAccessor implements IndexAccessor { + + @Override + public Class[] getSpecificTargetClasses() { + return null; + } + + @Override + public boolean canRead(EvaluationContext context, Object target, Object index) { + return false; + } + + @Override + public TypedValue read(EvaluationContext context, Object target, Object index) { + return TypedValue.NULL; + } + + @Override + public boolean canWrite(EvaluationContext context, Object target, Object index) { + return true; + } + + @Override + public void write(EvaluationContext context, Object target, Object index, @Nullable Object newValue) { + } + } } 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 a5ab11cbb517..4c2075555b59 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 @@ -300,6 +300,19 @@ void propertyAccessWithArrayIndexOutOfBounds() { .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.ARRAY_INDEX_OUT_OF_BOUNDS); } + @Test // gh-32737 + void competingReadAndWritePropertyAccessors() { + StandardEvaluationContext context = new StandardEvaluationContext(new Object()); + context.addPropertyAccessor(new ConfigurablePropertyAccessor(Map.of("counter", 1))); + context.addPropertyAccessor(new WriteOnlyPropertyAccessor()); + + Expression expression = parser.parseExpression("counter++"); + + assertThatSpelEvaluationException() + .isThrownBy(() -> expression.getValue(context)) + .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.COMPETING_ACCESSORS); + } + private ThrowableTypeAssert assertThatSpelEvaluationException() { return assertThatExceptionOfType(SpelEvaluationException.class); @@ -389,4 +402,32 @@ public void write(EvaluationContext context, Object target, String name, Object } } + + private static class WriteOnlyPropertyAccessor implements PropertyAccessor { + + @Override + public Class[] getSpecificTargetClasses() { + return null; + } + + @Override + public boolean canRead(EvaluationContext context, Object target, String name) { + return false; + } + + @Override + public TypedValue read(EvaluationContext context, Object target, String name) { + return TypedValue.NULL; + } + + @Override + public boolean canWrite(EvaluationContext context, Object target, String name) { + return "counter".equals(name); + } + + @Override + public void write(EvaluationContext context, Object target, String name, Object newValue) { + } + } + }