Skip to content

Commit

Permalink
Support single String argument for varargs invocations in SpEL
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sbrannen committed Jul 10, 2024
1 parent a4fcd30 commit c55c644
Show file tree
Hide file tree
Showing 6 changed files with 122 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -308,11 +308,11 @@ static boolean convertArguments(TypeConverter converter, Object[] arguments, Exe
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:
Expand Down Expand Up @@ -384,7 +384,7 @@ public static boolean convertAllMethodHandleArguments(TypeConverter converter, O
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.
Expand All @@ -398,11 +398,11 @@ public static boolean convertAllMethodHandleArguments(TypeConverter converter, O
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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,46 @@ void testVarargsInvocation03() {
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<String>... values)'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
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.
Expand Down Expand Up @@ -1436,13 +1437,20 @@ void SPR12502() {
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,12 @@ private static void populateMethodHandles(StandardEvaluationContext testContext)
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
Expand Down Expand Up @@ -154,4 +159,8 @@ public static String message(String template, String... args) {
return template.formatted((Object[]) args);
}

public static String formatObjectVarargs(String format, Object... args) {
return String.format(format, args);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -101,6 +102,54 @@ void functionWithVarargs() {
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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,11 @@ public String aVarargsMethod3(String str1, String... strings) {
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];
Expand Down

0 comments on commit c55c644

Please sign in to comment.