Skip to content

Commit fa670dd

Browse files
jhoellerjhoeller
committed
Indexer enforces target descriptor only after non-null target check
Issue: SPR-16544
1 parent bfddbbe commit fa670dd

File tree

2 files changed

+206
-45
lines changed

2 files changed

+206
-45
lines changed

‎spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java

Lines changed: 50 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2017 the original author or authors.
2+
* Copyright 2002-2018 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -16,6 +16,7 @@
1616

1717
package org.springframework.expression.spel.ast;
1818

19+
import java.lang.reflect.Constructor;
1920
import java.lang.reflect.Field;
2021
import java.lang.reflect.Member;
2122
import java.lang.reflect.Method;
@@ -113,22 +114,20 @@ public boolean isWritable(ExpressionState expressionState) throws SpelEvaluation
113114
@Override
114115
protected ValueRef getValueRef(ExpressionState state) throws EvaluationException {
115116
TypedValue context = state.getActiveContextObject();
116-
Object targetObject = context.getValue();
117+
Object target = context.getValue();
117118
TypeDescriptor targetDescriptor = context.getTypeDescriptor();
118-
Assert.state(targetDescriptor != null, "No type descriptor");
119-
TypedValue indexValue = null;
120-
Object index = null;
119+
TypedValue indexValue;
120+
Object index;
121121

122-
// This first part of the if clause prevents a 'double dereference' of
123-
// the property (SPR-5847)
124-
if (targetObject instanceof Map && (this.children[0] instanceof PropertyOrFieldReference)) {
122+
// This first part of the if clause prevents a 'double dereference' of the property (SPR-5847)
123+
if (target instanceof Map && (this.children[0] instanceof PropertyOrFieldReference)) {
125124
PropertyOrFieldReference reference = (PropertyOrFieldReference) this.children[0];
126125
index = reference.getName();
127126
indexValue = new TypedValue(index);
128127
}
129128
else {
130-
// In case the map key is unqualified, we want it evaluated against
131-
// the root object so temporarily push that on whilst evaluating the key
129+
// In case the map key is unqualified, we want it evaluated against the root object
130+
// so temporarily push that on whilst evaluating the key
132131
try {
133132
state.pushActiveContextObject(state.getRootContextObject());
134133
indexValue = this.children[0].getValueInternal(state);
@@ -140,48 +139,52 @@ protected ValueRef getValueRef(ExpressionState state) throws EvaluationException
140139
}
141140
}
142141

142+
// Raise a proper exception in case of a null target
143+
if (target == null) {
144+
throw new SpelEvaluationException(getStartPosition(), SpelMessage.CANNOT_INDEX_INTO_NULL_VALUE);
145+
}
146+
// At this point, we need a TypeDescriptor for a non-null target object
147+
Assert.state(targetDescriptor != null, "No type descriptor");
148+
143149
// Indexing into a Map
144-
if (targetObject instanceof Map) {
150+
if (target instanceof Map) {
145151
Object key = index;
146152
if (targetDescriptor.getMapKeyTypeDescriptor() != null) {
147153
key = state.convertValue(key, targetDescriptor.getMapKeyTypeDescriptor());
148154
}
149155
this.indexedType = IndexedType.MAP;
150-
return new MapIndexingValueRef(state.getTypeConverter(), (Map<?, ?>) targetObject, key, targetDescriptor);
151-
}
152-
153-
if (targetObject == null) {
154-
throw new SpelEvaluationException(getStartPosition(), SpelMessage.CANNOT_INDEX_INTO_NULL_VALUE);
156+
return new MapIndexingValueRef(state.getTypeConverter(), (Map<?, ?>) target, key, targetDescriptor);
155157
}
156158

157-
// if the object is something that looks indexable by an integer,
159+
// If the object is something that looks indexable by an integer,
158160
// attempt to treat the index value as a number
159-
if (targetObject.getClass().isArray() || targetObject instanceof Collection || targetObject instanceof String) {
161+
if (target.getClass().isArray() || target instanceof Collection || target instanceof String) {
160162
int idx = (Integer) state.convertValue(index, TypeDescriptor.valueOf(Integer.class));
161-
if (targetObject.getClass().isArray()) {
163+
if (target.getClass().isArray()) {
162164
this.indexedType = IndexedType.ARRAY;
163-
return new ArrayIndexingValueRef(state.getTypeConverter(), targetObject, idx, targetDescriptor);
165+
return new ArrayIndexingValueRef(state.getTypeConverter(), target, idx, targetDescriptor);
164166
}
165-
else if (targetObject instanceof Collection) {
166-
if (targetObject instanceof List) {
167+
else if (target instanceof Collection) {
168+
if (target instanceof List) {
167169
this.indexedType = IndexedType.LIST;
168170
}
169-
return new CollectionIndexingValueRef((Collection<?>) targetObject, idx, targetDescriptor,
171+
return new CollectionIndexingValueRef((Collection<?>) target, idx, targetDescriptor,
170172
state.getTypeConverter(), state.getConfiguration().isAutoGrowCollections(),
171173
state.getConfiguration().getMaximumAutoGrowSize());
172174
}
173175
else {
174176
this.indexedType = IndexedType.STRING;
175-
return new StringIndexingLValue((String) targetObject, idx, targetDescriptor);
177+
return new StringIndexingLValue((String) target, idx, targetDescriptor);
176178
}
177179
}
178180

179181
// Try and treat the index value as a property of the context object
180-
// TODO could call the conversion service to convert the value to a String
182+
// TODO: could call the conversion service to convert the value to a String
181183
TypeDescriptor valueType = indexValue.getTypeDescriptor();
182184
if (valueType != null && String.class == valueType.getType()) {
183185
this.indexedType = IndexedType.OBJECT;
184-
return new PropertyIndexingValueRef(targetObject, (String) index, state.getEvaluationContext(), targetDescriptor);
186+
return new PropertyIndexingValueRef(
187+
target, (String) index, state.getEvaluationContext(), targetDescriptor);
185188
}
186189

187190
throw new SpelEvaluationException(
@@ -200,12 +203,10 @@ else if (this.indexedType == IndexedType.MAP) {
200203
return (this.children[0] instanceof PropertyOrFieldReference || this.children[0].isCompilable());
201204
}
202205
else if (this.indexedType == IndexedType.OBJECT) {
203-
// If the string name is changing the accessor is clearly going to change (so compilation is not possible)
204-
if (this.cachedReadAccessor != null &&
205-
(this.cachedReadAccessor instanceof ReflectivePropertyAccessor.OptimalPropertyAccessor) &&
206-
(getChild(0) instanceof StringLiteral)) {
207-
return true;
208-
}
206+
// If the string name is changing the accessor is clearly going to change (so no compilation possible)
207+
return (this.cachedReadAccessor != null &&
208+
this.cachedReadAccessor instanceof ReflectivePropertyAccessor.OptimalPropertyAccessor &&
209+
getChild(0) instanceof StringLiteral);
209210
}
210211
return false;
211212
}
@@ -283,7 +284,8 @@ else if (this.indexedType == IndexedType.MAP) {
283284
this.children[0].generateCode(mv, cf);
284285
cf.exitCompilationScope();
285286
}
286-
mv.visitMethodInsn(INVOKEINTERFACE, "java/util/Map", "get", "(Ljava/lang/Object;)Ljava/lang/Object;", true);
287+
mv.visitMethodInsn(
288+
INVOKEINTERFACE, "java/util/Map", "get", "(Ljava/lang/Object;)Ljava/lang/Object;", true);
287289
}
288290

289291
else if (this.indexedType == IndexedType.OBJECT) {
@@ -592,11 +594,11 @@ public TypedValue getValue() {
592594
}
593595
}
594596
catch (AccessException ex) {
595-
throw new SpelEvaluationException(getStartPosition(), ex, SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE,
596-
this.targetObjectTypeDescriptor.toString());
597+
throw new SpelEvaluationException(getStartPosition(), ex,
598+
SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, this.targetObjectTypeDescriptor.toString());
597599
}
598-
throw new SpelEvaluationException(getStartPosition(), SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE,
599-
this.targetObjectTypeDescriptor.toString());
600+
throw new SpelEvaluationException(getStartPosition(),
601+
SpelMessage.INDEXING_NOT_SUPPORTED_FOR_TYPE, this.targetObjectTypeDescriptor.toString());
600602
}
601603

602604
@Override
@@ -612,8 +614,8 @@ public void setValue(@Nullable Object newValue) {
612614
accessor.write(this.evaluationContext, this.targetObject, this.name, newValue);
613615
return;
614616
}
615-
List<PropertyAccessor> accessorsToTry =
616-
AstUtils.getPropertyAccessorsToTry(contextObjectClass, this.evaluationContext.getPropertyAccessors());
617+
List<PropertyAccessor> accessorsToTry = AstUtils.getPropertyAccessorsToTry(
618+
contextObjectClass, this.evaluationContext.getPropertyAccessors());
617619
for (PropertyAccessor accessor : accessorsToTry) {
618620
if (accessor.canWrite(this.evaluationContext, this.targetObject, this.name)) {
619621
Indexer.this.cachedWriteName = this.name;
@@ -625,8 +627,8 @@ public void setValue(@Nullable Object newValue) {
625627
}
626628
}
627629
catch (AccessException ex) {
628-
throw new SpelEvaluationException(getStartPosition(), ex, SpelMessage.EXCEPTION_DURING_PROPERTY_WRITE,
629-
this.name, ex.getMessage());
630+
throw new SpelEvaluationException(getStartPosition(), ex,
631+
SpelMessage.EXCEPTION_DURING_PROPERTY_WRITE, this.name, ex.getMessage());
630632
}
631633
}
632634

@@ -652,11 +654,12 @@ private class CollectionIndexingValueRef implements ValueRef {
652654

653655
private final int maximumSize;
654656

655-
public CollectionIndexingValueRef(Collection collection, int index, TypeDescriptor collectionEntryTypeDescriptor,
657+
public CollectionIndexingValueRef(Collection collection, int index, TypeDescriptor collectionEntryDescriptor,
656658
TypeConverter typeConverter, boolean growCollection, int maximumSize) {
659+
657660
this.collection = collection;
658661
this.index = index;
659-
this.collectionEntryDescriptor = collectionEntryTypeDescriptor;
662+
this.collectionEntryDescriptor = collectionEntryDescriptor;
660663
this.typeConverter = typeConverter;
661664
this.growCollection = growCollection;
662665
this.maximumSize = maximumSize;
@@ -707,13 +710,15 @@ private void growCollectionIfNecessary() {
707710
throw new SpelEvaluationException(getStartPosition(), SpelMessage.UNABLE_TO_GROW_COLLECTION);
708711
}
709712
if (this.collectionEntryDescriptor.getElementTypeDescriptor() == null) {
710-
throw new SpelEvaluationException(getStartPosition(), SpelMessage.UNABLE_TO_GROW_COLLECTION_UNKNOWN_ELEMENT_TYPE);
713+
throw new SpelEvaluationException(
714+
getStartPosition(), SpelMessage.UNABLE_TO_GROW_COLLECTION_UNKNOWN_ELEMENT_TYPE);
711715
}
712716
TypeDescriptor elementType = this.collectionEntryDescriptor.getElementTypeDescriptor();
713717
try {
718+
Constructor<?> ctor = ReflectionUtils.accessibleConstructor(elementType.getType());
714719
int newElements = this.index - this.collection.size();
715720
while (newElements >= 0) {
716-
(this.collection).add(ReflectionUtils.accessibleConstructor(elementType.getType()).newInstance());
721+
this.collection.add(ctor.newInstance());
717722
newElements--;
718723
}
719724
}

‎spring-expression/src/test/java/org/springframework/expression/spel/SpelExceptionTests.java

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
/*
2+
* Copyright 2002-2018 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.expression.spel;
18+
19+
import java.util.ArrayList;
20+
import java.util.HashMap;
21+
22+
import org.junit.Test;
23+
24+
import org.springframework.expression.Expression;
25+
import org.springframework.expression.ExpressionParser;
26+
import org.springframework.expression.spel.standard.SpelExpressionParser;
27+
import org.springframework.expression.spel.support.StandardEvaluationContext;
28+
29+
import static org.junit.Assert.*;
30+
31+
/**
32+
* SpelEvaluationException tests (SPR-16544).
33+
*
34+
* @author Juergen Hoeller
35+
* @author DJ Kulkarni
36+
*/
37+
public class SpelExceptionTests {
38+
39+
@Test(expected = SpelEvaluationException.class)
40+
public void spelExpressionMapNullVariables() {
41+
ExpressionParser parser = new SpelExpressionParser();
42+
Expression spelExpression = parser.parseExpression("#aMap.containsKey('one')");
43+
spelExpression.getValue();
44+
}
45+
46+
@Test(expected = SpelEvaluationException.class)
47+
public void spelExpressionMapIndexAccessNullVariables() {
48+
ExpressionParser parser = new SpelExpressionParser();
49+
Expression spelExpression = parser.parseExpression("#aMap['one'] eq 1");
50+
spelExpression.getValue();
51+
}
52+
53+
@Test
54+
@SuppressWarnings("serial")
55+
public void spelExpressionMapWithVariables() {
56+
ExpressionParser parser = new SpelExpressionParser();
57+
Expression spelExpression = parser.parseExpression("#aMap['one'] eq 1");
58+
StandardEvaluationContext ctx = new StandardEvaluationContext();
59+
ctx.setVariables(new HashMap<String, Object>() {
60+
{
61+
put("aMap", new HashMap<String, Integer>() {
62+
{
63+
put("one", 1);
64+
put("two", 2);
65+
put("three", 3);
66+
}
67+
});
68+
69+
}
70+
});
71+
boolean result = spelExpression.getValue(ctx, Boolean.class);
72+
assertTrue(result);
73+
74+
}
75+
76+
@Test(expected = SpelEvaluationException.class)
77+
public void spelExpressionListNullVariables() {
78+
ExpressionParser parser = new SpelExpressionParser();
79+
Expression spelExpression = parser.parseExpression("#aList.contains('one')");
80+
spelExpression.getValue();
81+
}
82+
83+
@Test(expected = SpelEvaluationException.class)
84+
public void spelExpressionListIndexAccessNullVariables() {
85+
ExpressionParser parser = new SpelExpressionParser();
86+
Expression spelExpression = parser.parseExpression("#aList[0] eq 'one'");
87+
spelExpression.getValue();
88+
}
89+
90+
@Test
91+
@SuppressWarnings("serial")
92+
public void spelExpressionListWithVariables() {
93+
ExpressionParser parser = new SpelExpressionParser();
94+
Expression spelExpression = parser.parseExpression("#aList.contains('one')");
95+
StandardEvaluationContext ctx = new StandardEvaluationContext();
96+
ctx.setVariables(new HashMap<String, Object>() {
97+
{
98+
put("aList", new ArrayList<String>() {
99+
{
100+
add("one");
101+
add("two");
102+
add("three");
103+
}
104+
});
105+
106+
}
107+
});
108+
boolean result = spelExpression.getValue(ctx, Boolean.class);
109+
assertTrue(result);
110+
}
111+
112+
@Test
113+
@SuppressWarnings("serial")
114+
public void spelExpressionListIndexAccessWithVariables() {
115+
ExpressionParser parser = new SpelExpressionParser();
116+
Expression spelExpression = parser.parseExpression("#aList[0] eq 'one'");
117+
StandardEvaluationContext ctx = new StandardEvaluationContext();
118+
ctx.setVariables(new HashMap<String, Object>() {
119+
{
120+
put("aList", new ArrayList<String>() {
121+
{
122+
add("one");
123+
add("two");
124+
add("three");
125+
}
126+
});
127+
128+
}
129+
});
130+
boolean result = spelExpression.getValue(ctx, Boolean.class);
131+
assertTrue(result);
132+
}
133+
134+
@Test(expected = SpelEvaluationException.class)
135+
public void spelExpressionArrayIndexAccessNullVariables() {
136+
ExpressionParser parser = new SpelExpressionParser();
137+
Expression spelExpression = parser.parseExpression("#anArray[0] eq 1");
138+
spelExpression.getValue();
139+
}
140+
141+
@Test
142+
@SuppressWarnings("serial")
143+
public void spelExpressionArrayWithVariables() {
144+
ExpressionParser parser = new SpelExpressionParser();
145+
Expression spelExpression = parser.parseExpression("#anArray[0] eq 1");
146+
StandardEvaluationContext ctx = new StandardEvaluationContext();
147+
ctx.setVariables(new HashMap<String, Object>() {
148+
{
149+
put("anArray", new int[] {1,2,3});
150+
}
151+
});
152+
boolean result = spelExpression.getValue(ctx, Boolean.class);
153+
assertTrue(result);
154+
}
155+
156+
}

0 commit comments

Comments
 (0)