From fa670dd07d630e94ae1b0e7487906adf79f31749 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 1 Mar 2018 13:31:56 +0100 Subject: [PATCH] Indexer enforces target descriptor only after non-null target check Issue: SPR-16544 --- .../expression/spel/ast/Indexer.java | 95 ++++++----- .../expression/spel/SpelExceptionTests.java | 156 ++++++++++++++++++ 2 files changed, 206 insertions(+), 45 deletions(-) create mode 100644 spring-expression/src/test/java/org/springframework/expression/spel/SpelExceptionTests.java 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 c0d3d9aef2..4a26b4a27d 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2018 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. @@ -16,6 +16,7 @@ package org.springframework.expression.spel.ast; +import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Method; @@ -113,22 +114,20 @@ public class Indexer extends SpelNodeImpl { @Override protected ValueRef getValueRef(ExpressionState state) throws EvaluationException { TypedValue context = state.getActiveContextObject(); - Object targetObject = context.getValue(); + Object target = context.getValue(); TypeDescriptor targetDescriptor = context.getTypeDescriptor(); - Assert.state(targetDescriptor != null, "No type descriptor"); - TypedValue indexValue = null; - Object index = null; + TypedValue indexValue; + Object index; - // This first part of the if clause prevents a 'double dereference' of - // the property (SPR-5847) - if (targetObject instanceof Map && (this.children[0] instanceof PropertyOrFieldReference)) { + // This first part of the if clause prevents a 'double dereference' of the property (SPR-5847) + if (target instanceof Map && (this.children[0] instanceof PropertyOrFieldReference)) { PropertyOrFieldReference reference = (PropertyOrFieldReference) this.children[0]; index = reference.getName(); indexValue = new TypedValue(index); } else { - // In case the map key is unqualified, we want it evaluated against - // the root object so temporarily push that on whilst evaluating the key + // In case the map key is unqualified, we want it evaluated against the root object + // so temporarily push that on whilst evaluating the key try { state.pushActiveContextObject(state.getRootContextObject()); indexValue = this.children[0].getValueInternal(state); @@ -140,48 +139,52 @@ public class Indexer extends SpelNodeImpl { } } + // Raise a proper exception in case of a null target + if (target == null) { + throw new SpelEvaluationException(getStartPosition(), SpelMessage.CANNOT_INDEX_INTO_NULL_VALUE); + } + // At this point, we need a TypeDescriptor for a non-null target object + Assert.state(targetDescriptor != null, "No type descriptor"); + // Indexing into a Map - if (targetObject instanceof Map) { + if (target instanceof Map) { Object key = index; if (targetDescriptor.getMapKeyTypeDescriptor() != null) { key = state.convertValue(key, targetDescriptor.getMapKeyTypeDescriptor()); } this.indexedType = IndexedType.MAP; - return new MapIndexingValueRef(state.getTypeConverter(), (Map) targetObject, key, targetDescriptor); - } - - if (targetObject == null) { - throw new SpelEvaluationException(getStartPosition(), SpelMessage.CANNOT_INDEX_INTO_NULL_VALUE); + return new MapIndexingValueRef(state.getTypeConverter(), (Map) target, key, targetDescriptor); } - // if the object is something that looks indexable by an integer, + // If the object is something that looks indexable by an integer, // attempt to treat the index value as a number - if (targetObject.getClass().isArray() || targetObject instanceof Collection || targetObject instanceof String) { + if (target.getClass().isArray() || target instanceof Collection || target instanceof String) { int idx = (Integer) state.convertValue(index, TypeDescriptor.valueOf(Integer.class)); - if (targetObject.getClass().isArray()) { + if (target.getClass().isArray()) { this.indexedType = IndexedType.ARRAY; - return new ArrayIndexingValueRef(state.getTypeConverter(), targetObject, idx, targetDescriptor); + return new ArrayIndexingValueRef(state.getTypeConverter(), target, idx, targetDescriptor); } - else if (targetObject instanceof Collection) { - if (targetObject instanceof List) { + else if (target instanceof Collection) { + if (target instanceof List) { this.indexedType = IndexedType.LIST; } - return new CollectionIndexingValueRef((Collection) targetObject, idx, targetDescriptor, + return new CollectionIndexingValueRef((Collection) target, idx, targetDescriptor, state.getTypeConverter(), state.getConfiguration().isAutoGrowCollections(), state.getConfiguration().getMaximumAutoGrowSize()); } else { this.indexedType = IndexedType.STRING; - return new StringIndexingLValue((String) targetObject, idx, targetDescriptor); + return new StringIndexingLValue((String) target, idx, targetDescriptor); } } // Try and treat the index value as a property of the context object - // TODO could call the conversion service to convert the value to a String + // TODO: could call the conversion service to convert the value to a String TypeDescriptor valueType = indexValue.getTypeDescriptor(); if (valueType != null && String.class == valueType.getType()) { this.indexedType = IndexedType.OBJECT; - return new PropertyIndexingValueRef(targetObject, (String) index, state.getEvaluationContext(), targetDescriptor); + return new PropertyIndexingValueRef( + target, (String) index, state.getEvaluationContext(), targetDescriptor); } throw new SpelEvaluationException( @@ -200,12 +203,10 @@ public class Indexer extends SpelNodeImpl { return (this.children[0] instanceof PropertyOrFieldReference || this.children[0].isCompilable()); } else if (this.indexedType == IndexedType.OBJECT) { - // If the string name is changing the accessor is clearly going to change (so compilation is not possible) - if (this.cachedReadAccessor != null && - (this.cachedReadAccessor instanceof ReflectivePropertyAccessor.OptimalPropertyAccessor) && - (getChild(0) instanceof StringLiteral)) { - return true; - } + // If the string name is changing the accessor is clearly going to change (so no compilation possible) + return (this.cachedReadAccessor != null && + this.cachedReadAccessor instanceof ReflectivePropertyAccessor.OptimalPropertyAccessor && + getChild(0) instanceof StringLiteral); } return false; } @@ -283,7 +284,8 @@ public class Indexer extends SpelNodeImpl { this.children[0].generateCode(mv, cf); cf.exitCompilationScope(); } - mv.visitMethodInsn(INVOKEINTERFACE, "java/util/Map", "get", "(Ljava/lang/Object;)Ljava/lang/Object;", true); + mv.visitMethodInsn( + INVOKEINTERFACE, "java/util/Map", "get", "(Ljava/lang/Object;)Ljava/lang/Object;", true); } else if (this.indexedType == IndexedType.OBJECT) { @@ -592,11 +594,11 @@ public class Indexer extends SpelNodeImpl { } } catch (AccessException ex) { - throw new SpelEvaluationException(getStartPosition(), ex, SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, - this.targetObjectTypeDescriptor.toString()); + throw new SpelEvaluationException(getStartPosition(), ex, + SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, this.targetObjectTypeDescriptor.toString()); } - throw new SpelEvaluationException(getStartPosition(), SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, - this.targetObjectTypeDescriptor.toString()); + throw new SpelEvaluationException(getStartPosition(), + SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, this.targetObjectTypeDescriptor.toString()); } @Override @@ -612,8 +614,8 @@ public class Indexer extends SpelNodeImpl { accessor.write(this.evaluationContext, this.targetObject, this.name, newValue); return; } - List accessorsToTry = - AstUtils.getPropertyAccessorsToTry(contextObjectClass, this.evaluationContext.getPropertyAccessors()); + List accessorsToTry = AstUtils.getPropertyAccessorsToTry( + contextObjectClass, this.evaluationContext.getPropertyAccessors()); for (PropertyAccessor accessor : accessorsToTry) { if (accessor.canWrite(this.evaluationContext, this.targetObject, this.name)) { Indexer.this.cachedWriteName = this.name; @@ -625,8 +627,8 @@ public class Indexer extends SpelNodeImpl { } } catch (AccessException ex) { - throw new SpelEvaluationException(getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_PROPERTY_WRITE, - this.name, ex.getMessage()); + throw new SpelEvaluationException(getStartPosition(), ex, + SpelMessage.EXCEPTION_DURING_PROPERTY_WRITE, this.name, ex.getMessage()); } } @@ -652,11 +654,12 @@ public class Indexer extends SpelNodeImpl { private final int maximumSize; - public CollectionIndexingValueRef(Collection collection, int index, TypeDescriptor collectionEntryTypeDescriptor, + public CollectionIndexingValueRef(Collection collection, int index, TypeDescriptor collectionEntryDescriptor, TypeConverter typeConverter, boolean growCollection, int maximumSize) { + this.collection = collection; this.index = index; - this.collectionEntryDescriptor = collectionEntryTypeDescriptor; + this.collectionEntryDescriptor = collectionEntryDescriptor; this.typeConverter = typeConverter; this.growCollection = growCollection; this.maximumSize = maximumSize; @@ -707,13 +710,15 @@ public class Indexer extends SpelNodeImpl { throw new SpelEvaluationException(getStartPosition(), SpelMessage.UNABLE_TO_GROW_COLLECTION); } if (this.collectionEntryDescriptor.getElementTypeDescriptor() == null) { - throw new SpelEvaluationException(getStartPosition(), SpelMessage.UNABLE_TO_GROW_COLLECTION_UNKNOWN_ELEMENT_TYPE); + throw new SpelEvaluationException( + getStartPosition(), SpelMessage.UNABLE_TO_GROW_COLLECTION_UNKNOWN_ELEMENT_TYPE); } TypeDescriptor elementType = this.collectionEntryDescriptor.getElementTypeDescriptor(); try { + Constructor ctor = ReflectionUtils.accessibleConstructor(elementType.getType()); int newElements = this.index - this.collection.size(); while (newElements >= 0) { - (this.collection).add(ReflectionUtils.accessibleConstructor(elementType.getType()).newInstance()); + this.collection.add(ctor.newInstance()); newElements--; } } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelExceptionTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelExceptionTests.java new file mode 100644 index 0000000000..6f7b9ef47a --- /dev/null +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelExceptionTests.java @@ -0,0 +1,156 @@ +/* + * Copyright 2002-2018 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 + * + * http://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.spel; + +import java.util.ArrayList; +import java.util.HashMap; + +import org.junit.Test; + +import org.springframework.expression.Expression; +import org.springframework.expression.ExpressionParser; +import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.expression.spel.support.StandardEvaluationContext; + +import static org.junit.Assert.*; + +/** + * SpelEvaluationException tests (SPR-16544). + * + * @author Juergen Hoeller + * @author DJ Kulkarni + */ +public class SpelExceptionTests { + + @Test(expected = SpelEvaluationException.class) + public void spelExpressionMapNullVariables() { + ExpressionParser parser = new SpelExpressionParser(); + Expression spelExpression = parser.parseExpression("#aMap.containsKey('one')"); + spelExpression.getValue(); + } + + @Test(expected = SpelEvaluationException.class) + public void spelExpressionMapIndexAccessNullVariables() { + ExpressionParser parser = new SpelExpressionParser(); + Expression spelExpression = parser.parseExpression("#aMap['one'] eq 1"); + spelExpression.getValue(); + } + + @Test + @SuppressWarnings("serial") + public void spelExpressionMapWithVariables() { + ExpressionParser parser = new SpelExpressionParser(); + Expression spelExpression = parser.parseExpression("#aMap['one'] eq 1"); + StandardEvaluationContext ctx = new StandardEvaluationContext(); + ctx.setVariables(new HashMap() { + { + put("aMap", new HashMap() { + { + put("one", 1); + put("two", 2); + put("three", 3); + } + }); + + } + }); + boolean result = spelExpression.getValue(ctx, Boolean.class); + assertTrue(result); + + } + + @Test(expected = SpelEvaluationException.class) + public void spelExpressionListNullVariables() { + ExpressionParser parser = new SpelExpressionParser(); + Expression spelExpression = parser.parseExpression("#aList.contains('one')"); + spelExpression.getValue(); + } + + @Test(expected = SpelEvaluationException.class) + public void spelExpressionListIndexAccessNullVariables() { + ExpressionParser parser = new SpelExpressionParser(); + Expression spelExpression = parser.parseExpression("#aList[0] eq 'one'"); + spelExpression.getValue(); + } + + @Test + @SuppressWarnings("serial") + public void spelExpressionListWithVariables() { + ExpressionParser parser = new SpelExpressionParser(); + Expression spelExpression = parser.parseExpression("#aList.contains('one')"); + StandardEvaluationContext ctx = new StandardEvaluationContext(); + ctx.setVariables(new HashMap() { + { + put("aList", new ArrayList() { + { + add("one"); + add("two"); + add("three"); + } + }); + + } + }); + boolean result = spelExpression.getValue(ctx, Boolean.class); + assertTrue(result); + } + + @Test + @SuppressWarnings("serial") + public void spelExpressionListIndexAccessWithVariables() { + ExpressionParser parser = new SpelExpressionParser(); + Expression spelExpression = parser.parseExpression("#aList[0] eq 'one'"); + StandardEvaluationContext ctx = new StandardEvaluationContext(); + ctx.setVariables(new HashMap() { + { + put("aList", new ArrayList() { + { + add("one"); + add("two"); + add("three"); + } + }); + + } + }); + boolean result = spelExpression.getValue(ctx, Boolean.class); + assertTrue(result); + } + + @Test(expected = SpelEvaluationException.class) + public void spelExpressionArrayIndexAccessNullVariables() { + ExpressionParser parser = new SpelExpressionParser(); + Expression spelExpression = parser.parseExpression("#anArray[0] eq 1"); + spelExpression.getValue(); + } + + @Test + @SuppressWarnings("serial") + public void spelExpressionArrayWithVariables() { + ExpressionParser parser = new SpelExpressionParser(); + Expression spelExpression = parser.parseExpression("#anArray[0] eq 1"); + StandardEvaluationContext ctx = new StandardEvaluationContext(); + ctx.setVariables(new HashMap() { + { + put("anArray", new int[] {1,2,3}); + } + }); + boolean result = spelExpression.getValue(ctx, Boolean.class); + assertTrue(result); + } + +}