From 62fa3f11c1673d5393434d0b82c088a6f5b1a8ce Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sun, 28 Jan 2024 18:21:53 +0100 Subject: [PATCH] Correctly request primitive conversion in SpEL's Indexer Prior to this commit, SpEL's Indexer incorrectly requested conversion to wrappers instead of primitives when setting an element in a primitive array. This commit addresses this by requesting primitive conversion -- for example, conversion to `int.class` instead of `Integer.class` when setting a value in an `int[]` array. For greater clarity, this commit also switches from using `TYPE` constants in wrapper classes to primitive class literals -- for example, from `Integer.TYPE` to `int.class`. Closes gh-32147 --- .../expression/spel/ast/Indexer.java | 34 +++++----- .../expression/spel/SetValueTests.java | 67 ++++++++++++++----- 2 files changed, 68 insertions(+), 33 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 438e582130..e93e151aa0 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. @@ -336,45 +336,45 @@ public class Indexer extends SpelNodeImpl { private void setArrayElement(TypeConverter converter, Object ctx, int idx, @Nullable Object newValue, Class arrayComponentType) throws EvaluationException { - if (arrayComponentType == Boolean.TYPE) { + if (arrayComponentType == boolean.class) { boolean[] array = (boolean[]) ctx; checkAccess(array.length, idx); - array[idx] = convertValue(converter, newValue, Boolean.class); + array[idx] = convertValue(converter, newValue, boolean.class); } - else if (arrayComponentType == Byte.TYPE) { + else if (arrayComponentType == byte.class) { byte[] array = (byte[]) ctx; checkAccess(array.length, idx); - array[idx] = convertValue(converter, newValue, Byte.class); + array[idx] = convertValue(converter, newValue, byte.class); } - else if (arrayComponentType == Character.TYPE) { + else if (arrayComponentType == char.class) { char[] array = (char[]) ctx; checkAccess(array.length, idx); - array[idx] = convertValue(converter, newValue, Character.class); + array[idx] = convertValue(converter, newValue, char.class); } - else if (arrayComponentType == Double.TYPE) { + else if (arrayComponentType == double.class) { double[] array = (double[]) ctx; checkAccess(array.length, idx); - array[idx] = convertValue(converter, newValue, Double.class); + array[idx] = convertValue(converter, newValue, double.class); } - else if (arrayComponentType == Float.TYPE) { + else if (arrayComponentType == float.class) { float[] array = (float[]) ctx; checkAccess(array.length, idx); - array[idx] = convertValue(converter, newValue, Float.class); + array[idx] = convertValue(converter, newValue, float.class); } - else if (arrayComponentType == Integer.TYPE) { + else if (arrayComponentType == int.class) { int[] array = (int[]) ctx; checkAccess(array.length, idx); - array[idx] = convertValue(converter, newValue, Integer.class); + array[idx] = convertValue(converter, newValue, int.class); } - else if (arrayComponentType == Long.TYPE) { + else if (arrayComponentType == long.class) { long[] array = (long[]) ctx; checkAccess(array.length, idx); - array[idx] = convertValue(converter, newValue, Long.class); + array[idx] = convertValue(converter, newValue, long.class); } - else if (arrayComponentType == Short.TYPE) { + else if (arrayComponentType == short.class) { short[] array = (short[]) ctx; checkAccess(array.length, idx); - array[idx] = convertValue(converter, newValue, Short.class); + array[idx] = convertValue(converter, newValue, short.class); } else { Object[] array = (Object[]) ctx; diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SetValueTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SetValueTests.java index 8bbc64d91a..ff1c300b82 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SetValueTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SetValueTests.java @@ -21,16 +21,19 @@ import java.util.List; import java.util.Set; import org.assertj.core.api.ThrowableTypeAssert; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.expression.EvaluationException; import org.springframework.expression.Expression; import org.springframework.expression.ParseException; import org.springframework.expression.spel.testresources.PlaceOfBirth; +import org.springframework.util.ObjectUtils; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.springframework.expression.spel.SpelMessage.TYPE_CONVERSION_ERROR; /** * Tests for assignment, setValue(), and isWritable() expressions. @@ -164,21 +167,19 @@ class SetValueTests extends AbstractExpressionTests { setValue("arrayContainer.doubles[1]", List.of(42D), 42D); } - @Disabled("Disabled due to bug in Indexer.setArrayElement() regarding primitive/wrapper types") - @Test - void setArrayElementToPrimitiveFromEmptyCollectionFails() { - List emptyList = List.of(); - // TODO These fail because CollectionToObjectConverter returns null. - // It currently throws: java.lang.IllegalStateException: Null conversion result for index [[]]. - // Whereas, it should throw a SpelEvaluationException. - setValueAndExpectError("arrayContainer.booleans[1]", emptyList); - setValueAndExpectError("arrayContainer.chars[1]", emptyList); - setValueAndExpectError("arrayContainer.shorts[1]", emptyList); - setValueAndExpectError("arrayContainer.bytes[1]", emptyList); - setValueAndExpectError("arrayContainer.ints[1]", emptyList); - setValueAndExpectError("arrayContainer.longs[1]", emptyList); - setValueAndExpectError("arrayContainer.floats[1]", emptyList); - setValueAndExpectError("arrayContainer.doubles[1]", emptyList); + @ParameterizedTest + @ValueSource(strings = { + "arrayContainer.booleans[1]", + "arrayContainer.chars[1]", + "arrayContainer.shorts[1]", + "arrayContainer.bytes[1]", + "arrayContainer.ints[1]", + "arrayContainer.longs[1]", + "arrayContainer.floats[1]", + "arrayContainer.doubles[1]" + }) + void setArrayElementToPrimitiveFromEmptyCollectionFailsWithTypeConversionError(String expression) { + setValueAndExpectError(expression, List.of(), TYPE_CONVERSION_ERROR); } @Test @@ -272,6 +273,40 @@ class SetValueTests extends AbstractExpressionTests { assertThatSpelEvaluationException().isThrownBy(() -> e.setValue(context, value)); } + /** + * Call setValue() but expect it to fail. + * @see #evaluateAndCheckError(org.springframework.expression.ExpressionParser, String, Class, SpelMessage, Object...) + */ + private void setValueAndExpectError(String expression, Object value, SpelMessage expectedMessage, + Object... otherProperties) { + + Expression expr = parser.parseExpression(expression); + assertThat(expr).as("expression").isNotNull(); + + if (DEBUG) { + SpelUtilities.printAbstractSyntaxTree(System.out, expr); + } + + assertThatSpelEvaluationException() + .isThrownBy(() -> expr.setValue(context, value)) + .satisfies(ex -> { + assertThat(ex.getMessageCode()).isEqualTo(expectedMessage); + if (!ObjectUtils.isEmpty(otherProperties)) { + // first one is expected position of the error within the string + int pos = (Integer) otherProperties[0]; + assertThat(ex.getPosition()).as("position").isEqualTo(pos); + if (otherProperties.length > 1) { + // Check inserts match + Object[] inserts = ex.getInserts(); + assertThat(inserts).as("inserts").hasSizeGreaterThanOrEqualTo(otherProperties.length - 1); + Object[] expectedInserts = new Object[inserts.length]; + System.arraycopy(otherProperties, 1, expectedInserts, 0, expectedInserts.length); + assertThat(inserts).as("inserts").containsExactly(expectedInserts); + } + } + }); + } + private void setValue(String expression, Object value) { Class expectedType = value.getClass(); try {