Skip to content

Commit

Permalink
Support compilation of null-safe index operations in SpEL
Browse files Browse the repository at this point in the history 
See gh-29847
  • Loading branch information
@sbrannen
sbrannen committed Mar 23, 2024
1 parent d2bd0d5 commit 38c473f
Show file tree
Hide file tree
Showing 2 changed files with 251 additions and 10 deletions.
58 changes: 48 additions & 10 deletions spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Map;
import java.util.function.Supplier;

import org.springframework.asm.Label;
import org.springframework.asm.MethodVisitor;
import org.springframework.core.convert.TypeDescriptor;
import org.springframework.expression.AccessException;
Expand Down Expand Up @@ -73,6 +74,9 @@ private enum IndexedType {ARRAY, LIST, MAP, STRING, OBJECT}
@Nullable
private IndexedType indexedType;

@Nullable
private String originalPrimitiveExitTypeDescriptor;

@Nullable
private volatile String arrayTypeDescriptor;

Expand Down Expand Up @@ -271,6 +275,17 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
cf.loadTarget(mv);
}

Label skipIfNull = null;
if (this.nullSafe) {
mv.visitInsn(DUP);
skipIfNull = new Label();
Label continueLabel = new Label();
mv.visitJumpInsn(IFNONNULL, continueLabel);
CodeFlow.insertCheckCast(mv, exitTypeDescriptor);
mv.visitJumpInsn(GOTO, skipIfNull);
mv.visitLabel(continueLabel);
}

SpelNodeImpl index = this.children[0];

if (this.indexedType == IndexedType.ARRAY) {
Expand Down Expand Up @@ -333,6 +348,16 @@ else if (this.indexedType == IndexedType.OBJECT) {
}

cf.pushDescriptor(exitTypeDescriptor);

if (skipIfNull != null) {
if (this.originalPrimitiveExitTypeDescriptor != null) {
// The output of the indexer is a primitive, but from the logic above it
// might be null. So, to have a common stack element type at the skipIfNull
// target, it is necessary to box the primitive.
CodeFlow.insertBoxIfNecessary(mv, this.originalPrimitiveExitTypeDescriptor);
}
mv.visitLabel(skipIfNull);
}
}

@Override
Expand Down Expand Up @@ -396,64 +421,64 @@ private Object accessArrayElement(Object ctx, int idx) throws SpelEvaluationExce
if (arrayComponentType == boolean.class) {
boolean[] array = (boolean[]) ctx;
checkAccess(array.length, idx);
this.exitTypeDescriptor = "Z";
setExitTypeDescriptor("Z");
this.arrayTypeDescriptor = "[Z";
return array[idx];
}
else if (arrayComponentType == byte.class) {
byte[] array = (byte[]) ctx;
checkAccess(array.length, idx);
this.exitTypeDescriptor = "B";
setExitTypeDescriptor("B");
this.arrayTypeDescriptor = "[B";
return array[idx];
}
else if (arrayComponentType == char.class) {
char[] array = (char[]) ctx;
checkAccess(array.length, idx);
this.exitTypeDescriptor = "C";
setExitTypeDescriptor("C");
this.arrayTypeDescriptor = "[C";
return array[idx];
}
else if (arrayComponentType == double.class) {
double[] array = (double[]) ctx;
checkAccess(array.length, idx);
this.exitTypeDescriptor = "D";
setExitTypeDescriptor("D");
this.arrayTypeDescriptor = "[D";
return array[idx];
}
else if (arrayComponentType == float.class) {
float[] array = (float[]) ctx;
checkAccess(array.length, idx);
this.exitTypeDescriptor = "F";
setExitTypeDescriptor("F");
this.arrayTypeDescriptor = "[F";
return array[idx];
}
else if (arrayComponentType == int.class) {
int[] array = (int[]) ctx;
checkAccess(array.length, idx);
this.exitTypeDescriptor = "I";
setExitTypeDescriptor("I");
this.arrayTypeDescriptor = "[I";
return array[idx];
}
else if (arrayComponentType == long.class) {
long[] array = (long[]) ctx;
checkAccess(array.length, idx);
this.exitTypeDescriptor = "J";
setExitTypeDescriptor("J");
this.arrayTypeDescriptor = "[J";
return array[idx];
}
else if (arrayComponentType == short.class) {
short[] array = (short[]) ctx;
checkAccess(array.length, idx);
this.exitTypeDescriptor = "S";
setExitTypeDescriptor("S");
this.arrayTypeDescriptor = "[S";
return array[idx];
}
else {
Object[] array = (Object[]) ctx;
checkAccess(array.length, idx);
Object retValue = array[idx];
this.exitTypeDescriptor = CodeFlow.toDescriptor(arrayComponentType);
setExitTypeDescriptor(CodeFlow.toDescriptor(arrayComponentType));
this.arrayTypeDescriptor = CodeFlow.toDescriptor(array.getClass());
return retValue;
}
Expand All @@ -466,6 +491,19 @@ private void checkAccess(int arrayLength, int index) throws SpelEvaluationExcept
}
}

private void setExitTypeDescriptor(String descriptor) {
// If this indexer would return a primitive - and yet it is also marked
// null-safe - then the exit type descriptor must be promoted to the box
// type to allow a null value to be passed on.
if (this.nullSafe && CodeFlow.isPrimitive(descriptor)) {
this.originalPrimitiveExitTypeDescriptor = descriptor;
this.exitTypeDescriptor = CodeFlow.toBoxedDescriptor(descriptor);
}
else {
this.exitTypeDescriptor = descriptor;
}
}

@SuppressWarnings("unchecked")
private <T> T convertValue(TypeConverter converter, @Nullable Object value, Class<T> targetType) {
T result = (T) converter.convertValue(
Expand Down Expand Up @@ -602,7 +640,7 @@ public TypedValue getValue() {
Indexer.this.cachedReadName = this.name;
Indexer.this.cachedReadTargetType = targetObjectRuntimeClass;
if (accessor instanceof CompilablePropertyAccessor compilablePropertyAccessor) {
Indexer.this.exitTypeDescriptor = CodeFlow.toDescriptor(compilablePropertyAccessor.getPropertyType());
setExitTypeDescriptor(CodeFlow.toDescriptor(compilablePropertyAccessor.getPropertyType()));
}
return accessor.read(this.evaluationContext, this.targetObject, this.name);
}
Expand Down
203 changes: 203 additions & 0 deletions ...ssion/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,198 @@ else if (object instanceof int[] ints) {

}

@Nested
class NullSafeIndexTests { // gh-29847

private final RootContextWithIndexedProperties rootContext = new RootContextWithIndexedProperties();

private final StandardEvaluationContext context = new StandardEvaluationContext(rootContext);

@Test
void nullSafeIndexIntoPrimitiveIntArray() {
expression = parser.parseExpression("intArray?.[0]");

// Cannot compile before the array type is known.
assertThat(expression.getValue(context)).isNull();
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isNull();

rootContext.intArray = new int[] { 8, 9, 10 };
assertThat(expression.getValue(context)).isEqualTo(8);
assertCanCompile(expression);
assertThat(expression.getValue(context)).isEqualTo(8);
// Normally we would expect the exit type descriptor to be "I" for an
// element of an int[]. However, with null-safe indexing support the
// only way for it to evaluate to null is to box the 'int' to an 'Integer'.
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Integer");

// Null-safe support should have been compiled once the array type is known.
rootContext.intArray = null;
assertThat(expression.getValue(context)).isNull();
assertCanCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Integer");
}

@Test
void nullSafeIndexIntoNumberArray() {
expression = parser.parseExpression("numberArray?.[0]");

// Cannot compile before the array type is known.
assertThat(expression.getValue(context)).isNull();
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isNull();

rootContext.numberArray = new Number[] { 8, 9, 10 };
assertThat(expression.getValue(context)).isEqualTo(8);
assertCanCompile(expression);
assertThat(expression.getValue(context)).isEqualTo(8);
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Number");

// Null-safe support should have been compiled once the array type is known.
rootContext.numberArray = null;
assertThat(expression.getValue(context)).isNull();
assertCanCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Number");
}

@Test
void nullSafeIndexIntoList() {
expression = parser.parseExpression("list?.[0]");

// Cannot compile before the list type is known.
assertThat(expression.getValue(context)).isNull();
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isNull();

rootContext.list = List.of(42);
assertThat(expression.getValue(context)).isEqualTo(42);
assertCanCompile(expression);
assertThat(expression.getValue(context)).isEqualTo(42);
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object");

// Null-safe support should have been compiled once the list type is known.
rootContext.list = null;
assertThat(expression.getValue(context)).isNull();
assertCanCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object");
}

@Test
void nullSafeIndexIntoSetCannotBeCompiled() {
expression = parser.parseExpression("set?.[0]");

assertThat(expression.getValue(context)).isNull();
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isNull();

rootContext.set = Set.of(42);
assertThat(expression.getValue(context)).isEqualTo(42);
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isEqualTo(42);
assertThat(getAst().getExitDescriptor()).isNull();
}

@Test
void nullSafeIndexIntoStringCannotBeCompiled() {
expression = parser.parseExpression("string?.[0]");

assertThat(expression.getValue(context)).isNull();
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isNull();

rootContext.string = "XYZ";
assertThat(expression.getValue(context)).isEqualTo("X");
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isEqualTo("X");
assertThat(getAst().getExitDescriptor()).isNull();
}

@Test
void nullSafeIndexIntoMap() {
expression = parser.parseExpression("map?.['enigma']");

// Cannot compile before the map type is known.
assertThat(expression.getValue(context)).isNull();
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isNull();

rootContext.map = Map.of("enigma", 42);
assertThat(expression.getValue(context)).isEqualTo(42);
assertCanCompile(expression);
assertThat(expression.getValue(context)).isEqualTo(42);
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object");

// Null-safe support should have been compiled once the map type is known.
rootContext.map = null;
assertThat(expression.getValue(context)).isNull();
assertCanCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object");
}

@Test
void nullSafeIndexIntoObjectViaPrimitiveProperty() {
expression = parser.parseExpression("person?.['age']");

// Cannot compile before the Person type is known.
assertThat(expression.getValue(context)).isNull();
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isNull();

rootContext.person = new Person("Jane");
rootContext.person.setAge(42);
assertThat(expression.getValue(context)).isEqualTo(42);
assertCanCompile(expression);
assertThat(expression.getValue(context)).isEqualTo(42);
// Normally we would expect the exit type descriptor to be "I" for
// an int. However, with null-safe indexing support the only way
// for it to evaluate to null is to box the 'int' to an 'Integer'.
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Integer");

// Null-safe support should have been compiled once the Person type is known.
rootContext.person = null;
assertThat(expression.getValue(context)).isNull();
assertCanCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Integer");
}

@Test
void nullSafeIndexIntoObjectViaStringProperty() {
expression = parser.parseExpression("person?.['name']");

// Cannot compile before the Person type is known.
assertThat(expression.getValue(context)).isNull();
assertCannotCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isNull();

rootContext.person = new Person("Jane");
assertThat(expression.getValue(context)).isEqualTo("Jane");
assertCanCompile(expression);
assertThat(expression.getValue(context)).isEqualTo("Jane");
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/String");

// Null-safe support should have been compiled once the Person type is known.
rootContext.person = null;
assertThat(expression.getValue(context)).isNull();
assertCanCompile(expression);
assertThat(expression.getValue(context)).isNull();
assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/String");
}

}

@Nested
class PropertyVisibilityTests {

Expand Down Expand Up @@ -6736,4 +6928,15 @@ public void setValue2(Integer value) {
}
}

// Must be public with public fields/properties.
public static class RootContextWithIndexedProperties {
public int[] intArray;
public Number[] numberArray;
public List<Integer> list;
public Set<Integer> set;
public String string;
public Map<String, Integer> map;
public Person person;
}

}

0 comments on commit 38c473f

Please sign in to comment.