From c55c64427c49f32472004f082402a56be056882d Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 10 Jul 2024 11:51:55 +0200 Subject: [PATCH] Support single String argument for varargs invocations in SpEL Prior to this commit, the Spring Expression Language (SpEL) incorrectly split single String arguments by comma for Object... varargs method and constructor invocations. This commit addresses this by checking if the single argument type is already "assignable" to the varargs component type instead of "equal" to the varargs component type. Closes gh-33013 --- .../spel/support/ReflectionHelper.java | 10 ++-- .../spel/MethodInvocationTests.java | 40 +++++++++++++++ .../expression/spel/SpelReproTests.java | 18 +++++-- .../expression/spel/TestScenarioCreator.java | 11 ++++- .../spel/VariableAndFunctionTests.java | 49 +++++++++++++++++++ .../spel/testresources/Inventor.java | 5 ++ 6 files changed, 122 insertions(+), 11 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 5fd242044fb..31bd84389a2 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 @@ -308,11 +308,11 @@ public abstract class ReflectionHelper { conversionOccurred = true; } } - // If the argument type is equal to the varargs component type, there is no need to + // If the argument type is assignable to the varargs component type, there is no need to // 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. - else if (!sourceType.equals(componentTypeDesc)) { + else if (!sourceType.isAssignableTo(componentTypeDesc)) { arguments[varargsPosition] = converter.convertValue(argument, sourceType, targetType); } // Possible outcomes of the above if-else block: @@ -384,7 +384,7 @@ public abstract class ReflectionHelper { ResolvableType varArgResolvableType = ResolvableType.forClass(varArgClass); TypeDescriptor varArgComponentType = new TypeDescriptor(varArgResolvableType, varArgClass, null); TypeDescriptor componentTypeDesc = varArgComponentType.getElementTypeDescriptor(); - // TODO Determine why componentTypeDesc is null. + // TODO Determine why componentTypeDesc can be null. // Assert.state(componentTypeDesc != null, "Component type must not be null for a varargs array"); // If the target is varargs and there is just one more argument, then convert it here. @@ -398,11 +398,11 @@ public abstract class ReflectionHelper { conversionOccurred = true; } } - // If the argument type is equal to the varargs component type, there is no need to + // If the argument type is assignable to the varargs component type, there is no need to // 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. - else if (!sourceType.equals(componentTypeDesc)) { + else if (componentTypeDesc != null && !sourceType.isAssignableTo(componentTypeDesc)) { arguments[varargsPosition] = converter.convertValue(argument, sourceType, varArgComponentType); } // 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 626db90a46b..b84a57498ee 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 @@ -293,6 +293,46 @@ class MethodInvocationTests extends AbstractExpressionTests { evaluate("aVarargsMethod3('foo', 'bar,baz')", "foo-bar,baz", String.class); } + @Test // gh-33013 + void testVarargsWithObjectArrayType() { + // Calling 'public String formatObjectVarargs(String format, Object... args)' -> String.format(format, args) + + // No var-args and no conversion necessary + evaluate("formatObjectVarargs('x')", "x", String.class); + + // No var-args but conversion necessary + evaluate("formatObjectVarargs(9)", "9", String.class); + + // No conversion necessary + evaluate("formatObjectVarargs('x -> %s', '')", "x -> ", String.class); + evaluate("formatObjectVarargs('x -> %s', ' ')", "x -> ", String.class); + evaluate("formatObjectVarargs('x -> %s', 'a')", "x -> a", String.class); + evaluate("formatObjectVarargs('x -> %s %s %s', 'a', 'b', 'c')", "x -> a b c", String.class); + evaluate("formatObjectVarargs('x -> %s', new Object[]{''})", "x -> ", String.class); + evaluate("formatObjectVarargs('x -> %s', new String[]{''})", "x -> ", String.class); + evaluate("formatObjectVarargs('x -> %s', new Object[]{' '})", "x -> ", String.class); + evaluate("formatObjectVarargs('x -> %s', new String[]{' '})", "x -> ", String.class); + evaluate("formatObjectVarargs('x -> %s', new Object[]{'a'})", "x -> a", String.class); + evaluate("formatObjectVarargs('x -> %s', new String[]{'a'})", "x -> a", String.class); + evaluate("formatObjectVarargs('x -> %s %s %s', new Object[]{'a', 'b', 'c'})", "x -> a b c", String.class); + evaluate("formatObjectVarargs('x -> %s %s %s', new String[]{'a', 'b', 'c'})", "x -> a b c", String.class); + + // Conversion necessary + evaluate("formatObjectVarargs('x -> %s %s', 2, 3)", "x -> 2 3", String.class); + evaluate("formatObjectVarargs('x -> %s %s', 'a', 3.0d)", "x -> a 3.0", String.class); + + // Individual string contains a comma with multiple varargs arguments + evaluate("formatObjectVarargs('foo -> %s %s', ',', 'baz')", "foo -> , baz", String.class); + evaluate("formatObjectVarargs('foo -> %s %s', 'bar', ',baz')", "foo -> bar ,baz", String.class); + evaluate("formatObjectVarargs('foo -> %s %s', 'bar,', 'baz')", "foo -> bar, baz", String.class); + + // Individual string contains a comma with single varargs argument. + evaluate("formatObjectVarargs('foo -> %s', ',')", "foo -> ,", String.class); + evaluate("formatObjectVarargs('foo -> %s', ',bar')", "foo -> ,bar", String.class); + evaluate("formatObjectVarargs('foo -> %s', 'bar,')", "foo -> bar,", String.class); + evaluate("formatObjectVarargs('foo -> %s', 'bar,baz')", "foo -> bar,baz", String.class); + } + @Test void testVarargsOptionalInvocation() { // Calling 'public String optionalVarargsMethod(Optional... values)' diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java index 60d0a14a8f2..f8491daae4e 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java @@ -69,6 +69,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatException; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; +import static org.assertj.core.api.InstanceOfAssertFactories.list; /** * Reproduction tests cornering various reported SpEL issues. @@ -1436,13 +1437,20 @@ class SpelReproTests extends AbstractExpressionTests { assertThat(expression.getValue(new NamedUser())).isEqualTo(NamedUser.class.getName()); } - @Test - void SPR12522() { + @Test // gh-17127, SPR-12522 + void arraysAsListWithNoArguments() { + SpelExpressionParser parser = new SpelExpressionParser(); + Expression expression = parser.parseExpression("T(java.util.Arrays).asList()"); + List value = expression.getValue(List.class); + assertThat(value).isEmpty(); + } + + @Test // gh-33013 + void arraysAsListWithSingleEmptyStringArgument() { SpelExpressionParser parser = new SpelExpressionParser(); Expression expression = parser.parseExpression("T(java.util.Arrays).asList('')"); - Object value = expression.getValue(); - assertThat(value).isInstanceOf(List.class); - assertThat(((List) value)).isEmpty(); + List value = expression.getValue(List.class); + assertThat(value).asInstanceOf(list(String.class)).containsExactly(""); } @Test diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/TestScenarioCreator.java b/spring-expression/src/test/java/org/springframework/expression/spel/TestScenarioCreator.java index 614fcf431d2..b2aec5c02cc 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/TestScenarioCreator.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/TestScenarioCreator.java @@ -100,7 +100,12 @@ class TestScenarioCreator { MethodHandle messageStaticFullyBound = messageStaticPartiallyBound .bindTo(new String[] { "prerecorded", "3", "Oh Hello World", "ignored"}); testContext.registerFunction("messageStaticBound", messageStaticFullyBound); - } + + // #formatObjectVarargs(format, args...) + MethodHandle formatObjectVarargs = MethodHandles.lookup().findStatic(TestScenarioCreator.class, + "formatObjectVarargs", MethodType.methodType(String.class, String.class, Object[].class)); + testContext.registerFunction("formatObjectVarargs", formatObjectVarargs); +} /** * Register some variables that can be referenced from the tests @@ -154,4 +159,8 @@ class TestScenarioCreator { return template.formatted((Object[]) args); } + public static String formatObjectVarargs(String format, Object... args) { + return String.format(format, args); + } + } 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 0dfa472aea6..fbfb7cc0447 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 @@ -16,6 +16,7 @@ package org.springframework.expression.spel; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.springframework.expression.spel.standard.SpelExpressionParser; @@ -101,6 +102,54 @@ class VariableAndFunctionTests extends AbstractExpressionTests { evaluate("#varargsFunction2(9,'a',null,'b')", "9-[a, null, b]", String.class); } + @Disabled("Disabled until bugs are reported and fixed") + @Test + void functionWithVarargsViaMethodHandle_CurrentlyFailing() { + // Calling 'public static String formatObjectVarargs(String format, Object... args)' -> String.format(format, args) + + // No var-args and no conversion necessary + evaluate("#formatObjectVarargs('x')", "x", String.class); + + // No var-args but conversion necessary + evaluate("#formatObjectVarargs(9)", "9", String.class); + + // No conversion necessary + evaluate("#formatObjectVarargs('x -> %s', new Object[]{''})", "x -> ", String.class); + evaluate("#formatObjectVarargs('x -> %s', new String[]{''})", "x -> ", String.class); + evaluate("#formatObjectVarargs('x -> %s', new Object[]{' '})", "x -> ", String.class); + evaluate("#formatObjectVarargs('x -> %s', new String[]{' '})", "x -> ", String.class); + evaluate("#formatObjectVarargs('x -> %s', new Object[]{'a'})", "x -> a", String.class); + evaluate("#formatObjectVarargs('x -> %s', new String[]{'a'})", "x -> a", String.class); + evaluate("#formatObjectVarargs('x -> %s %s %s', new Object[]{'a', 'b', 'c'})", "x -> a b c", String.class); + evaluate("#formatObjectVarargs('x -> %s %s %s', new String[]{'a', 'b', 'c'})", "x -> a b c", String.class); + } + + @Test // gh-33013 + void functionWithVarargsViaMethodHandle() { + // Calling 'public static String formatObjectVarargs(String format, Object... args)' -> String.format(format, args) + + // No conversion necessary + evaluate("#formatObjectVarargs('x -> %s', '')", "x -> ", String.class); + evaluate("#formatObjectVarargs('x -> %s', ' ')", "x -> ", String.class); + evaluate("#formatObjectVarargs('x -> %s', 'a')", "x -> a", String.class); + evaluate("#formatObjectVarargs('x -> %s %s %s', 'a', 'b', 'c')", "x -> a b c", String.class); + + // Conversion necessary + evaluate("#formatObjectVarargs('x -> %s %s', 2, 3)", "x -> 2 3", String.class); + evaluate("#formatObjectVarargs('x -> %s %s', 'a', 3.0d)", "x -> a 3.0", String.class); + + // Individual string contains a comma with multiple varargs arguments + evaluate("#formatObjectVarargs('foo -> %s %s', ',', 'baz')", "foo -> , baz", String.class); + evaluate("#formatObjectVarargs('foo -> %s %s', 'bar', ',baz')", "foo -> bar ,baz", String.class); + evaluate("#formatObjectVarargs('foo -> %s %s', 'bar,', 'baz')", "foo -> bar, baz", String.class); + + // Individual string contains a comma with single varargs argument. + evaluate("#formatObjectVarargs('foo -> %s', ',')", "foo -> ,", String.class); + evaluate("#formatObjectVarargs('foo -> %s', ',bar')", "foo -> ,bar", String.class); + evaluate("#formatObjectVarargs('foo -> %s', 'bar,')", "foo -> bar,", String.class); + evaluate("#formatObjectVarargs('foo -> %s', 'bar,baz')", "foo -> bar,baz", String.class); + } + @Test void functionMethodMustBeStatic() throws Exception { SpelExpressionParser parser = new SpelExpressionParser(); 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 1ef7a29cd37..a40106d2124 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 @@ -217,6 +217,11 @@ public class Inventor { return str1 + "-" + String.join("-", strings); } + public String formatObjectVarargs(String format, Object... args) { + return String.format(format, args); + } + + public Inventor(String... strings) { if (strings.length > 0) { this.name = strings[0];