From 8afff3359bc201f228616275123e670a5ad8fd91 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 5 Aug 2024 15:01:41 +0300 Subject: [PATCH 1/2] Polishing --- .../spel/MethodInvocationTests.java | 72 ++++++++++--------- .../spel/testresources/Inventor.java | 14 ++-- 2 files changed, 42 insertions(+), 44 deletions(-) diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java index 8ea2cdf933..6782245d26 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java @@ -32,6 +32,7 @@ import org.springframework.expression.MethodResolver; import org.springframework.expression.spel.standard.SpelExpression; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.StandardEvaluationContext; +import org.springframework.expression.spel.testresources.Inventor; import org.springframework.expression.spel.testresources.PlaceOfBirth; import static org.assertj.core.api.Assertions.assertThat; @@ -48,12 +49,12 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; class MethodInvocationTests extends AbstractExpressionTests { @Test - void testSimpleAccess01() { + void simpleAccess() { evaluate("getPlaceOfBirth().getCity()", "Smiljan", String.class); } @Test - void testStringClass() { + void stringClass() { evaluate("new java.lang.String('hello').charAt(2)", 'l', Character.class); evaluate("new java.lang.String('hello').charAt(2).equals('l'.charAt(0))", true, Boolean.class); evaluate("'HELLO'.toLowerCase()", "hello", String.class); @@ -61,13 +62,13 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void testNonExistentMethods() { + void nonExistentMethods() { // name is ok but madeup() does not exist evaluateAndCheckError("name.madeup()", SpelMessage.METHOD_NOT_FOUND, 5); } @Test - void testWidening01() { + void widening() { // widening of int 3 to double 3 is OK evaluate("new Double(3.0d).compareTo(8)", -1, Integer.class); evaluate("new Double(3.0d).compareTo(3)", 0, Integer.class); @@ -75,18 +76,19 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void testArgumentConversion01() { + void argumentConversion() { // Rely on Double>String conversion for calling startsWith() evaluate("new String('hello 2.0 to you').startsWith(7.0d)", false, Boolean.class); evaluate("new String('7.0 foobar').startsWith(7.0d)", true, Boolean.class); } - @Test - void testMethodThrowingException_SPR6760() { + @Test // SPR-6760 + void methodThrowingException() { // Test method on inventor: throwException() - // On 1 it will throw an IllegalArgumentException - // On 2 it will throw a RuntimeException - // On 3 it will exit normally + // On 1 it will throw an IllegalArgumentException. + // On 2 it will throw a RuntimeException. + // On 4 it will throw a TestException. + // Otherwise, it will exit normally. // In each case it increments the Inventor field 'counter' when invoked SpelExpressionParser parser = new SpelExpressionParser(); @@ -115,7 +117,6 @@ class MethodInvocationTests extends AbstractExpressionTests { assertThat(o).isEqualTo(3); assertThat(parser.parseExpression("counter").getValue(eContext)).isEqualTo(2); - // Now cause it to throw an exception: eContext.setVariable("bar", 1); assertThatException() @@ -135,12 +136,13 @@ class MethodInvocationTests extends AbstractExpressionTests { /** * Check on first usage (when the cachedExecutor in MethodReference is null) that the exception is not wrapped. */ - @Test - void testMethodThrowingException_SPR6941() { + @Test // SPR-6941 + void methodThrowingRuntimeException() { // Test method on inventor: throwException() - // On 1 it will throw an IllegalArgumentException - // On 2 it will throw a RuntimeException - // On 3 it will exit normally + // On 1 it will throw an IllegalArgumentException. + // On 2 it will throw a RuntimeException. + // On 4 it will throw a TestException. + // Otherwise, it will exit normally. // In each case it increments the Inventor field 'counter' when invoked SpelExpressionParser parser = new SpelExpressionParser(); @@ -152,12 +154,13 @@ class MethodInvocationTests extends AbstractExpressionTests { .isNotInstanceOf(SpelEvaluationException.class); } - @Test - void testMethodThrowingException_SPR6941_2() { + @Test // SPR-6941 + void methodThrowingCustomException() { // Test method on inventor: throwException() - // On 1 it will throw an IllegalArgumentException - // On 2 it will throw a RuntimeException - // On 3 it will exit normally + // On 1 it will throw an IllegalArgumentException. + // On 2 it will throw a RuntimeException. + // On 4 it will throw a TestException. + // Otherwise, it will exit normally. // In each case it increments the Inventor field 'counter' when invoked SpelExpressionParser parser = new SpelExpressionParser(); @@ -166,12 +169,11 @@ class MethodInvocationTests extends AbstractExpressionTests { context.setVariable("bar", 4); assertThatExceptionOfType(ExpressionInvocationTargetException.class) .isThrownBy(() -> expr.getValue(context)) - .satisfies(ex -> assertThat(ex.getCause().getClass().getName()).isEqualTo( - "org.springframework.expression.spel.testresources.Inventor$TestException")); + .withCauseExactlyInstanceOf(Inventor.TestException.class); } - @Test - void testMethodFiltering_SPR6764() { + @Test // SPR-6764 + void methodFiltering() { SpelExpressionParser parser = new SpelExpressionParser(); StandardEvaluationContext context = new StandardEvaluationContext(); context.setRootObject(new TestObject()); @@ -211,7 +213,7 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void testAddingMethodResolvers() { + void addingMethodResolvers() { StandardEvaluationContext ctx = new StandardEvaluationContext(); // reflective method accessor is the only one by default @@ -235,7 +237,7 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void testVarargsInvocation01() { + void varargsInvocation01() { // Calling 'public String aVarargsMethod(String... strings)' evaluate("aVarargsMethod('a','b','c')", "[a, b, c]", String.class); evaluate("aVarargsMethod('a')", "[a]", String.class); @@ -252,7 +254,7 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void testVarargsInvocation02() { + void varargsInvocation02() { // Calling 'public String aVarargsMethod2(int i, String... strings)' evaluate("aVarargsMethod2(5,'a','b','c')", "5-[a, b, c]", String.class); evaluate("aVarargsMethod2(2,'a')", "2-[a]", String.class); @@ -267,7 +269,7 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void testVarargsInvocation03() { + void varargsInvocation03() { // Calling 'public int aVarargsMethod3(String str1, String... strings)' - returns all strings concatenated with "-" // No conversion necessary @@ -295,7 +297,7 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test // gh-33013 - void testVarargsWithObjectArrayType() { + void varargsWithObjectArrayType() { // Calling 'public String formatObjectVarargs(String format, Object... args)' -> String.format(format, args) // No var-args and no conversion necessary @@ -336,7 +338,7 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void testVarargsWithPrimitiveArrayType() { + void varargsWithPrimitiveArrayType() { // Calling 'public String formatPrimitiveVarargs(String format, int... nums)' -> effectively String.format(format, args) // No var-args and no conversion necessary @@ -357,13 +359,13 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void testVarargsWithPrimitiveArrayToObjectArrayConversion() { + void varargsWithPrimitiveArrayToObjectArrayConversion() { evaluate("formatObjectVarargs('x -> %s %s %s', new short[]{1, 2, 3})", "x -> 1 2 3", String.class); // short[] to Object[] evaluate("formatObjectVarargs('x -> %s %s %s', new int[]{1, 2, 3})", "x -> 1 2 3", String.class); // int[] to Object[] } @Test - void testVarargsOptionalInvocation() { + void varargsOptionalInvocation() { // Calling 'public String optionalVarargsMethod(Optional... values)' evaluate("optionalVarargsMethod()", "[]", String.class); evaluate("optionalVarargsMethod(new String[0])", "[]", String.class); @@ -379,12 +381,12 @@ class MethodInvocationTests extends AbstractExpressionTests { } @Test - void testInvocationOnNullContextObject() { + void invocationOnNullContextObject() { evaluateAndCheckError("null.toString()",SpelMessage.METHOD_CALL_ON_NULL_OBJECT_NOT_ALLOWED); } @Test - void testMethodOfClass() { + void methodOfClass() { Expression expression = parser.parseExpression("getName()"); Object value = expression.getValue(new StandardEvaluationContext(String.class)); assertThat(value).isEqualTo("java.lang.String"); diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java b/spring-expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java index 99c5eaa4cc..37c454d02b 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java @@ -119,20 +119,16 @@ public class Inventor { public int throwException(int valueIn) throws Exception { counter++; - if (valueIn==1) { - throw new IllegalArgumentException("IllegalArgumentException for 1"); - } - if (valueIn==2) { - throw new RuntimeException("RuntimeException for 2"); - } - if (valueIn==4) { - throw new TestException(); + switch (valueIn) { + case 1 -> throw new IllegalArgumentException("IllegalArgumentException for 1"); + case 2 -> throw new RuntimeException("RuntimeException for 2"); + case 4 -> throw new TestException(); } return valueIn; } @SuppressWarnings("serial") - static class TestException extends Exception {} + public static class TestException extends Exception {} public String throwException(PlaceOfBirth pob) { return pob.getCity(); From fcc99a67b6e9ffe34c170a7f817353408ec567da Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 5 Aug 2024 16:27:55 +0300 Subject: [PATCH 2/2] Support lists for varargs invocations in SpEL The changes made in conjunction with gh-33013 resulted in a regression for varargs support in SpEL expressions. Specifically, before gh-33013 one could supply a list -- for example, an "inline list" -- as the varargs array when invoking a varargs constructor, method, or function within a SpEL expression. However, after gh-33013 an inline list (or collection in general) is no longer converted to an array for varargs invocations. Instead, the list is supplied as a single argument of the resulting varargs array which breaks applications that depend on the previous behavior. Although it was never intended that one could supply a collection as the set of varargs, we concede that this is a regression in existing behavior, and this commit therefore restores support for supplying a java.util.List as the varargs "array". In addition, this commit introduces the same "list to array" conversion support for MethodHandle-based functions that accept varargs. Note, however, that this commit does not restore support for converting arbitrary single objects to an array for varargs invocations if the single object is already an instance of the varargs array's component type. See gh-33013 Closes gh-33315 --- .../spel/support/ReflectionHelper.java | 18 ++++++++---- .../spel/MethodInvocationTests.java | 16 +++++++++++ .../spel/VariableAndFunctionTests.java | 28 +++++++++++++++++++ 3 files changed, 56 insertions(+), 6 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java index b0f2943b6f..10854120d5 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java @@ -314,11 +314,14 @@ public abstract class ReflectionHelper { // convert it or wrap it in an array. For example, using StringToArrayConverter to convert // a String containing a comma would result in the String being split and repackaged in an // array when it should be used as-is. Similarly, if the argument is an array that is - // assignable to the varargs array type, there is no need to convert it. + // assignable to the varargs array type, there is no need to convert it. However, if the + // argument is a java.util.List, we let the TypeConverter convert the list to an array. else if (!sourceType.isAssignableTo(componentTypeDesc) || - (sourceType.isArray() && !sourceType.isAssignableTo(targetType))) { + (sourceType.isArray() && !sourceType.isAssignableTo(targetType)) || + (argument instanceof List)) { - TypeDescriptor targetTypeToUse = (sourceType.isArray() ? targetType : componentTypeDesc); + TypeDescriptor targetTypeToUse = + (sourceType.isArray() || argument instanceof List ? targetType : componentTypeDesc); arguments[varargsPosition] = converter.convertValue(argument, sourceType, targetTypeToUse); } // Possible outcomes of the above if-else block: @@ -413,11 +416,14 @@ public abstract class ReflectionHelper { // convert it. For example, using StringToArrayConverter to convert a String containing a // comma would result in the String being split and repackaged in an array when it should // be used as-is. Similarly, if the argument is an array that is assignable to the varargs - // array type, there is no need to convert it. + // array type, there is no need to convert it. However, if the argument is a java.util.List, + // we let the TypeConverter convert the list to an array. else if (!sourceType.isAssignableTo(varargsComponentType) || - (sourceType.isArray() && !sourceType.isAssignableTo(varargsArrayType))) { + (sourceType.isArray() && !sourceType.isAssignableTo(varargsArrayType)) || + (argument instanceof List)) { - TypeDescriptor targetTypeToUse = (sourceType.isArray() ? varargsArrayType : varargsComponentType); + TypeDescriptor targetTypeToUse = + (sourceType.isArray() || argument instanceof List ? varargsArrayType : varargsComponentType); arguments[varargsPosition] = converter.convertValue(argument, sourceType, targetTypeToUse); } // Possible outcomes of the above if-else block: diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java index 6782245d26..3da7456189 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java @@ -32,6 +32,7 @@ import org.springframework.expression.MethodResolver; import org.springframework.expression.spel.standard.SpelExpression; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.StandardEvaluationContext; +import org.springframework.expression.spel.support.StandardTypeLocator; import org.springframework.expression.spel.testresources.Inventor; import org.springframework.expression.spel.testresources.PlaceOfBirth; @@ -364,6 +365,21 @@ class MethodInvocationTests extends AbstractExpressionTests { evaluate("formatObjectVarargs('x -> %s %s %s', new int[]{1, 2, 3})", "x -> 1 2 3", String.class); // int[] to Object[] } + @Test // gh-33315 + void varargsWithListConvertedToVarargsArray() { + ((StandardTypeLocator) context.getTypeLocator()).registerImport("java.util"); + + // Calling 'public String aVarargsMethod(String... strings)' -> Arrays.toString(strings) + String expected = "[a, b, c]"; + evaluate("aVarargsMethod(T(List).of('a', 'b', 'c'))", expected, String.class); + evaluate("aVarargsMethod({'a', 'b', 'c'})", expected, String.class); + + // Calling 'public String formatObjectVarargs(String format, Object... args)' -> String.format(format, args) + expected = "x -> a b c"; + evaluate("formatObjectVarargs('x -> %s %s %s', T(List).of('a', 'b', 'c'))", expected, String.class); + evaluate("formatObjectVarargs('x -> %s %s %s', {'a', 'b', 'c'})", expected, String.class); + } + @Test void varargsOptionalInvocation() { // Calling 'public String optionalVarargsMethod(Optional... values)' diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/VariableAndFunctionTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/VariableAndFunctionTests.java index c4af0e4ad8..29a4255254 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/VariableAndFunctionTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/VariableAndFunctionTests.java @@ -20,6 +20,7 @@ import org.junit.jupiter.api.Test; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.StandardEvaluationContext; +import org.springframework.expression.spel.support.StandardTypeLocator; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; @@ -211,6 +212,33 @@ class VariableAndFunctionTests extends AbstractExpressionTests { evaluate("#formatObjectVarargs('x -> %s %s %s', new int[]{1, 2, 3})", "x -> 1 2 3", String.class); // int[] to Object[] } + @Test // gh-33315 + void functionFromMethodWithListConvertedToVarargsArray() { + ((StandardTypeLocator) context.getTypeLocator()).registerImport("java.util"); + String expected = "[a, b, c]"; + + evaluate("#varargsFunction(T(List).of('a', 'b', 'c'))", expected, String.class); + evaluate("#varargsFunction({'a', 'b', 'c'})", expected, String.class); + + // Calling 'public String formatObjectVarargs(String format, Object... args)' -> String.format(format, args) + evaluate("#varargsObjectFunction(T(List).of('a', 'b', 'c'))", expected, String.class); + evaluate("#varargsObjectFunction({'a', 'b', 'c'})", expected, String.class); + } + + @Test // gh-33315 + void functionFromMethodHandleWithListConvertedToVarargsArray() { + ((StandardTypeLocator) context.getTypeLocator()).registerImport("java.util"); + String expected = "x -> a b c"; + + // Calling 'public static String message(String template, String... args)' -> template.formatted((Object[]) args) + evaluate("#message('x -> %s %s %s', T(List).of('a', 'b', 'c'))", expected, String.class); + evaluate("#message('x -> %s %s %s', {'a', 'b', 'c'})", expected, String.class); + + // Calling 'public static String formatObjectVarargs(String format, Object... args)' -> String.format(format, args) + evaluate("#formatObjectVarargs('x -> %s %s %s', T(List).of('a', 'b', 'c'))", expected, String.class); + evaluate("#formatObjectVarargs('x -> %s %s %s', {'a', 'b', 'c'})", expected, String.class); + } + @Test void functionMethodMustBeStatic() throws Exception { SpelExpressionParser parser = new SpelExpressionParser();