From 079d53c8d69c17e58422303a9f6a80b952c27106 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Tue, 23 Apr 2024 11:34:18 +0300 Subject: [PATCH] Support compilation of array and list indexing with Integer in SpEL Prior to this commit, the Spring Expression Language (SpEL) failed to compile an expression that indexed into an array or list using an Integer. This commit adds support for compilation of such expressions by ensuring that an Integer is unboxed into an int in the compiled bytecode. See gh-32694 Closes gh-32908 (cherry picked from commit cda577d1aacf0f90225f13050da42506f95bd2c8) --- .../expression/spel/ast/Indexer.java | 23 +- .../spel/SpelCompilationCoverageTests.java | 443 +++++++++++++++++- 2 files changed, 455 insertions(+), 11 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 7e80a576c1..eabf26682e 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-2023 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. @@ -225,6 +225,8 @@ public class Indexer extends SpelNodeImpl { cf.loadTarget(mv); } + SpelNodeImpl index = this.children[0]; + if (this.indexedType == IndexedType.ARRAY) { int insn; if ("D".equals(this.exitTypeDescriptor)) { @@ -261,18 +263,14 @@ public class Indexer extends SpelNodeImpl { //depthPlusOne(exitTypeDescriptor)+"Ljava/lang/Object;"); insn = AALOAD; } - SpelNodeImpl index = this.children[0]; - cf.enterCompilationScope(); - index.generateCode(mv, cf); - cf.exitCompilationScope(); + + generateIndexCode(mv, cf, index, int.class); mv.visitInsn(insn); } else if (this.indexedType == IndexedType.LIST) { mv.visitTypeInsn(CHECKCAST, "java/util/List"); - cf.enterCompilationScope(); - this.children[0].generateCode(mv, cf); - cf.exitCompilationScope(); + generateIndexCode(mv, cf, index, int.class); mv.visitMethodInsn(INVOKEINTERFACE, "java/util/List", "get", "(I)Ljava/lang/Object;", true); } @@ -280,13 +278,13 @@ public class Indexer extends SpelNodeImpl { mv.visitTypeInsn(CHECKCAST, "java/util/Map"); // Special case when the key is an unquoted string literal that will be parsed as // a property/field reference - if ((this.children[0] instanceof PropertyOrFieldReference reference)) { + if (index instanceof PropertyOrFieldReference reference) { String mapKeyName = reference.getName(); mv.visitLdcInsn(mapKeyName); } else { cf.enterCompilationScope(); - this.children[0].generateCode(mv, cf); + index.generateCode(mv, cf); cf.exitCompilationScope(); } mv.visitMethodInsn( @@ -323,6 +321,11 @@ public class Indexer extends SpelNodeImpl { cf.pushDescriptor(this.exitTypeDescriptor); } + private void generateIndexCode(MethodVisitor mv, CodeFlow cf, SpelNodeImpl indexNode, Class indexType) { + String indexDesc = CodeFlow.toDescriptor(indexType); + generateCodeForArgument(mv, cf, indexNode, indexDesc); + } + @Override public String toStringAST() { StringJoiner sj = new StringJoiner(",", "[", "]"); diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java index c7fa4f887e..2c9772d288 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 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,6 +20,8 @@ import java.io.IOException; import java.lang.reflect.Field; import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -27,7 +29,10 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.StringTokenizer; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -132,6 +137,442 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { private SpelNodeImpl ast; + @Nested + class VariableReferenceTests { + + @Test + void userDefinedVariable() { + EvaluationContext ctx = new StandardEvaluationContext(); + ctx.setVariable("target", "abc"); + expression = parser.parseExpression("#target"); + assertThat(expression.getValue(ctx)).isEqualTo("abc"); + assertCanCompile(expression); + assertThat(expression.getValue(ctx)).isEqualTo("abc"); + ctx.setVariable("target", "123"); + assertThat(expression.getValue(ctx)).isEqualTo("123"); + + // Changing the variable type from String to Integer results in a + // ClassCastException in the compiled code. + ctx.setVariable("target", 42); + assertThatExceptionOfType(SpelEvaluationException.class) + .isThrownBy(() -> expression.getValue(ctx)) + .withCauseInstanceOf(ClassCastException.class); + + ctx.setVariable("target", "abc"); + expression = parser.parseExpression("#target.charAt(0)"); + assertThat(expression.getValue(ctx)).isEqualTo('a'); + assertCanCompile(expression); + assertThat(expression.getValue(ctx)).isEqualTo('a'); + ctx.setVariable("target", "1"); + assertThat(expression.getValue(ctx)).isEqualTo('1'); + + // Changing the variable type from String to Integer results in a + // ClassCastException in the compiled code. + ctx.setVariable("target", 42); + assertThatExceptionOfType(SpelEvaluationException.class) + .isThrownBy(() -> expression.getValue(ctx)) + .withCauseInstanceOf(ClassCastException.class); + } + + } + + @Nested + class IndexingTests { + + @Test + void indexIntoPrimitiveShortArray() { + short[] shorts = { (short) 33, (short) 44, (short) 55 }; + + expression = parser.parseExpression("[2]"); + + assertThat(expression.getValue(shorts)).isEqualTo((short) 55); + assertCanCompile(expression); + assertThat(expression.getValue(shorts)).isEqualTo((short) 55); + assertThat(getAst().getExitDescriptor()).isEqualTo("S"); + } + + @Test + void indexIntoPrimitiveByteArray() { + byte[] bytes = { (byte) 2, (byte) 3, (byte) 4 }; + + expression = parser.parseExpression("[2]"); + + assertThat(expression.getValue(bytes)).isEqualTo((byte) 4); + assertCanCompile(expression); + assertThat(expression.getValue(bytes)).isEqualTo((byte) 4); + assertThat(getAst().getExitDescriptor()).isEqualTo("B"); + } + + @Test + void indexIntoPrimitiveIntArray() { + int[] ints = { 8, 9, 10 }; + + expression = parser.parseExpression("[2]"); + + assertThat(expression.getValue(ints)).isEqualTo(10); + assertCanCompile(expression); + assertThat(expression.getValue(ints)).isEqualTo(10); + assertThat(getAst().getExitDescriptor()).isEqualTo("I"); + } + + @Test + void indexIntoPrimitiveLongArray() { + long[] longs = { 2L, 3L, 4L }; + + expression = parser.parseExpression("[0]"); + + assertThat(expression.getValue(longs)).isEqualTo(2L); + assertCanCompile(expression); + assertThat(expression.getValue(longs)).isEqualTo(2L); + assertThat(getAst().getExitDescriptor()).isEqualTo("J"); + } + + @Test + void indexIntoPrimitiveFloatArray() { + float[] floats = { 6.0f, 7.0f, 8.0f }; + + expression = parser.parseExpression("[0]"); + + assertThat(expression.getValue(floats)).isEqualTo(6.0f); + assertCanCompile(expression); + assertThat(expression.getValue(floats)).isEqualTo(6.0f); + assertThat(getAst().getExitDescriptor()).isEqualTo("F"); + } + + @Test + void indexIntoPrimitiveDoubleArray() { + double[] doubles = { 3.0d, 4.0d, 5.0d }; + + expression = parser.parseExpression("[1]"); + + assertThat(expression.getValue(doubles)).isEqualTo(4.0d); + assertCanCompile(expression); + assertThat(expression.getValue(doubles)).isEqualTo(4.0d); + assertThat(getAst().getExitDescriptor()).isEqualTo("D"); + } + + @Test + void indexIntoPrimitiveCharArray() { + char[] chars = { 'a', 'b', 'c' }; + + expression = parser.parseExpression("[1]"); + + assertThat(expression.getValue(chars)).isEqualTo('b'); + assertCanCompile(expression); + assertThat(expression.getValue(chars)).isEqualTo('b'); + assertThat(getAst().getExitDescriptor()).isEqualTo("C"); + } + + @Test + void indexIntoStringArray() { + String[] strings = { "a", "b", "c" }; + + expression = parser.parseExpression("[0]"); + + assertThat(expression.getValue(strings)).isEqualTo("a"); + assertCanCompile(expression); + assertThat(expression.getValue(strings)).isEqualTo("a"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/String"); + } + + @Test + void indexIntoNumberArray() { + Number[] numbers = { 2, 8, 9 }; + + expression = parser.parseExpression("[1]"); + + assertThat(expression.getValue(numbers)).isEqualTo(8); + assertCanCompile(expression); + assertThat(expression.getValue(numbers)).isEqualTo(8); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Number"); + } + + @Test + void indexInto2DPrimitiveIntArray() { + int[][] array = new int[][] { + { 1, 2, 3 }, + { 4, 5, 6 } + }; + + expression = parser.parseExpression("[1]"); + + assertThat(stringify(expression.getValue(array))).isEqualTo("4 5 6"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(array))).isEqualTo("4 5 6"); + assertThat(getAst().getExitDescriptor()).isEqualTo("[I"); + + expression = parser.parseExpression("[1][2]"); + + assertThat(stringify(expression.getValue(array))).isEqualTo("6"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(array))).isEqualTo("6"); + assertThat(getAst().getExitDescriptor()).isEqualTo("I"); + } + + @Test + void indexInto2DStringArray() { + String[][] array = new String[][] { + { "a", "b", "c" }, + { "d", "e", "f" } + }; + + expression = parser.parseExpression("[1]"); + + assertThat(stringify(expression.getValue(array))).isEqualTo("d e f"); + assertCanCompile(expression); + assertThat(getAst().getExitDescriptor()).isEqualTo("[Ljava/lang/String"); + assertThat(stringify(expression.getValue(array))).isEqualTo("d e f"); + assertThat(getAst().getExitDescriptor()).isEqualTo("[Ljava/lang/String"); + + expression = parser.parseExpression("[1][2]"); + + assertThat(stringify(expression.getValue(array))).isEqualTo("f"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(array))).isEqualTo("f"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/String"); + } + + @Test + @SuppressWarnings("unchecked") + void indexIntoArrayOfListOfString() { + List[] array = new List[] { + List.of("a", "b", "c"), + List.of("d", "e", "f") + }; + + expression = parser.parseExpression("[1]"); + + assertThat(stringify(expression.getValue(array))).isEqualTo("d e f"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(array))).isEqualTo("d e f"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/util/List"); + + expression = parser.parseExpression("[1][2]"); + + assertThat(stringify(expression.getValue(array))).isEqualTo("f"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(array))).isEqualTo("f"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + } + + @Test + @SuppressWarnings("unchecked") + void indexIntoArrayOfMap() { + Map[] array = new Map[] { Map.of("key", "value1") }; + + expression = parser.parseExpression("[0]"); + + assertThat(stringify(expression.getValue(array))).isEqualTo("{key=value1}"); + assertCanCompile(expression); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/util/Map"); + assertThat(stringify(expression.getValue(array))).isEqualTo("{key=value1}"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/util/Map"); + + expression = parser.parseExpression("[0]['key']"); + + assertThat(stringify(expression.getValue(array))).isEqualTo("value1"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(array))).isEqualTo("value1"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + } + + @Test + void indexIntoListOfString() { + List list = List.of("aaa", "bbb", "ccc"); + + expression = parser.parseExpression("[1]"); + + assertThat(expression.getValue(list)).isEqualTo("bbb"); + assertCanCompile(expression); + assertThat(expression.getValue(list)).isEqualTo("bbb"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + } + + @Test + void indexIntoListOfInteger() { + List list = List.of(123, 456, 789); + + expression = parser.parseExpression("[2]"); + + assertThat(expression.getValue(list)).isEqualTo(789); + assertCanCompile(expression); + assertThat(expression.getValue(list)).isEqualTo(789); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + } + + @Test + void indexIntoListOfStringArray() { + List list = List.of( + new String[] { "a", "b", "c" }, + new String[] { "d", "e", "f" } + ); + + expression = parser.parseExpression("[1]"); + + assertThat(stringify(expression.getValue(list))).isEqualTo("d e f"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(list))).isEqualTo("d e f"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + + expression = parser.parseExpression("[1][0]"); + + assertThat(stringify(expression.getValue(list))).isEqualTo("d"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(list))).isEqualTo("d"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/String"); + } + + @Test + void indexIntoListOfIntegerArray() { + List list = List.of( + new Integer[] { 1, 2, 3 }, + new Integer[] { 4, 5, 6 } + ); + + expression = parser.parseExpression("[0]"); + + assertThat(stringify(expression.getValue(list))).isEqualTo("1 2 3"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(list))).isEqualTo("1 2 3"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + + expression = parser.parseExpression("[0][1]"); + + assertThat(expression.getValue(list)).isEqualTo(2); + assertCanCompile(expression); + assertThat(expression.getValue(list)).isEqualTo(2); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Integer"); + } + + @Test + void indexIntoListOfListOfString() { + List> list = List.of( + List.of("a", "b", "c"), + List.of("d", "e", "f") + ); + + expression = parser.parseExpression("[1]"); + + assertThat(stringify(expression.getValue(list))).isEqualTo("d e f"); + assertCanCompile(expression); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + assertThat(stringify(expression.getValue(list))).isEqualTo("d e f"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + + expression = parser.parseExpression("[1][2]"); + + assertThat(stringify(expression.getValue(list))).isEqualTo("f"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(list))).isEqualTo("f"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + } + + @Test + void indexIntoMap() { + Map map = Map.of("aaa", 111); + + expression = parser.parseExpression("['aaa']"); + + assertThat(expression.getValue(map)).isEqualTo(111); + assertCanCompile(expression); + assertThat(expression.getValue(map)).isEqualTo(111); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + } + + @Test + void indexIntoMapOfListOfString() { + Map> map = Map.of("foo", List.of("a", "b", "c")); + + expression = parser.parseExpression("['foo']"); + + assertThat(stringify(expression.getValue(map))).isEqualTo("a b c"); + assertCanCompile(expression); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + assertThat(stringify(expression.getValue(map))).isEqualTo("a b c"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + + expression = parser.parseExpression("['foo'][2]"); + + assertThat(stringify(expression.getValue(map))).isEqualTo("c"); + assertCanCompile(expression); + assertThat(stringify(expression.getValue(map))).isEqualTo("c"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + } + + @Test + void indexIntoObject() { + TestClass6 tc = new TestClass6(); + + // field access + expression = parser.parseExpression("['orange']"); + + assertThat(expression.getValue(tc)).isEqualTo("value1"); + assertCanCompile(expression); + assertThat(expression.getValue(tc)).isEqualTo("value1"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/String"); + + // field access + expression = parser.parseExpression("['peach']"); + + assertThat(expression.getValue(tc)).isEqualTo(34L); + assertCanCompile(expression); + assertThat(expression.getValue(tc)).isEqualTo(34L); + assertThat(getAst().getExitDescriptor()).isEqualTo("J"); + + // property access (getter) + expression = parser.parseExpression("['banana']"); + + assertThat(expression.getValue(tc)).isEqualTo("value3"); + assertCanCompile(expression); + assertThat(expression.getValue(tc)).isEqualTo("value3"); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/String"); + } + + @Test // gh-32694, gh-32908 + void indexIntoArrayUsingIntegerWrapper() { + context.setVariable("array", new int[] {1, 2, 3, 4}); + context.setVariable("index", 2); + + expression = parser.parseExpression("#array[#index]"); + + assertThat(expression.getValue(context)).isEqualTo(3); + assertCanCompile(expression); + assertThat(expression.getValue(context)).isEqualTo(3); + assertThat(getAst().getExitDescriptor()).isEqualTo("I"); + } + + @Test // gh-32694, gh-32908 + void indexIntoListUsingIntegerWrapper() { + context.setVariable("list", List.of(1, 2, 3, 4)); + context.setVariable("index", 2); + + expression = parser.parseExpression("#list[#index]"); + + assertThat(expression.getValue(context)).isEqualTo(3); + assertCanCompile(expression); + assertThat(expression.getValue(context)).isEqualTo(3); + assertThat(getAst().getExitDescriptor()).isEqualTo("Ljava/lang/Object"); + } + + private String stringify(Object object) { + Stream stream; + if (object instanceof Collection collection) { + stream = collection.stream(); + } + else if (object instanceof Object[] objects) { + stream = Arrays.stream(objects); + } + else if (object instanceof int[] ints) { + stream = Arrays.stream(ints).mapToObj(Integer::valueOf); + } + else { + return String.valueOf(object); + } + return stream.map(Object::toString).collect(Collectors.joining(" ")); + } + + } + @Test void typeReference() { expression = parse("T(String)");