SpEL incorrectly splits string arguments by comma for Object... varargs method

[weiw005]

Affects

4.1.2.RELEASE or higher

Summary

In SpEL, a registered varargs function incorrectly splits string arguments by comma when varargs type is Object / Any.

Overview

This is related to previously reported #27582. The issue was likely not fully fixed yet.

I created a unit test here to demo the bug: https://github.com/weiw005/playground/blob/e6f1ae9336b8ac5e294758a90d2639c6a075df5b/spel-vararg/src/test/kotlin/SpelVarargTest.kt

It seems that when a registered function has vararg args: Any as the argument type, SpEL would have inconsistent behavior when there is single vs. multiple argument(s) of string type:

• Single argument: SpEL will split the string by comma, and take the first part only
• Multiple arguments: SpEL will take all arguments as is (expected behavior)

This bug doesn't manifest when the function signature declares vararg args: String instead (all arguments would be taken as is correctly).

I have also confirmed that version 4.1.1.RELEASE or lower did not have this bug.

Related Issues

• SpEL vararg method invocation fails if string literal contains a comma #27582
• Support varargs invocations in SpEL for varargs array subtype #32704

[weiw005]

@manoharank reported the same observation here: #27582 (comment)

[sbrannen]

Hi @weiw005,

Congratulations on submitting your first issue for the Spring Framework! 👍

Thanks for the Demo!

Though, please submit sample applications using Java (unless Kotlin is required to reproduce the issue).

I've reduced the demo to the following standalone Java test class.

import java.lang.reflect.Method;
import org.junit.jupiter.api.Test;
import org.springframework.expression.Expression;
import org.springframework.expression.spel.standard.SpelExpressionParser;
import org.springframework.expression.spel.support.StandardEvaluationContext;
import org.springframework.util.ReflectionUtils;
import static org.assertj.core.api.Assertions.assertThat;

class SpelVarargsTests {

	@Test
	void formatStringVarargs_EmptyString() {
		assertThat(evaluateExpression("#formatStringVarargs('Hello %s', '')"))
				.isEqualTo("Hello ");
	}

	@Test
	void formatObjectVarargs_EmptyString() {
		assertThat(evaluateExpression("#formatObjectVarargs('Hello %s', '')"))
				.isEqualTo("Hello ");
	}

	@Test
	void formatStringVarargs_SingleArgumentWithComma() {
		assertThat(evaluateExpression("#formatStringVarargs('Hello %s', 'a, b')"))
				.isEqualTo("Hello a, b");
	}

	@Test
	void formatObjectVarargs_SingleArgumentWithComma() {
		assertThat(evaluateExpression("#formatObjectVarargs('Hello %s', 'a, b')"))
				.isEqualTo("Hello a, b");
	}

	@Test
	void formatStringVarargs_MultiArgumentWithComma() {
		assertThat(evaluateExpression("#formatStringVarargs('Hello %s; %s', 'a, b', 'c, d')"))
				.isEqualTo("Hello a, b; c, d");
	}

	@Test
	void formatObjectVarargs_MultiArgumentWithComma() {
		assertThat(evaluateExpression("#formatObjectVarargs('Hello %s; %s', 'a, b', 'c, d')"))
				.isEqualTo("Hello a, b; c, d");
	}


	private static final Method formatObjectVarargs =
			ReflectionUtils.findMethod(SpelVarargsTests.class, "formatObjectVarargs", String.class, Object[].class);

	private static final Method formatStringVarargs =
			ReflectionUtils.findMethod(SpelVarargsTests.class, "formatStringVarargs", String.class, String[].class);

	private static Object evaluateExpression(String expression) {
		SpelExpressionParser parser = new SpelExpressionParser();
		Expression exp = parser.parseExpression(expression);
		StandardEvaluationContext context = new StandardEvaluationContext();
		context.registerFunction("formatObjectVarargs", formatObjectVarargs);
		context.registerFunction("formatStringVarargs", formatStringVarargs);
		return exp.getValue(context);
	}

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

	static String formatStringVarargs(String format, String... args) {
		return String.format(format, (Object[]) args);
	}

}

When running against main, formatObjectVarargs_EmptyString() and formatObjectVarargs_SingleArgumentWithComma() fail.

We'll look into fixing this.

Cheers,

Sam

[sbrannen]

While investigating this, I noticed that (in theory) the same bug must exist for a function registered as a MethodHandle, since that code is effectively a copy of the code for Method and Constructor invocations.

[sbrannen]

This has been fixed in 6.1.x and main and will be available in the 6.1.11 and 6.2.0-M5 releases later this week.

This will also be backported to 5.3.38 and 6.0.23.

[weiw005]

Thanks, @sbrannen

I did some testing in our project with the latest release (6.1.11) and things are working properly now AFAICT.

As part of the verification, I found another behavior also changed in terms of how array arguments work as vararg. The change probably makes sense, but just wanted to share the observation FWIW:

• Previously, if the expression code passed arguments as an array, e.g. {'hello', 123}, it worked not only for functions that take an array argument, but also functions that accept vararg in which case the array elements would be flattened.
• After this fix, the array would be treated as a single argument for vararg functions.

The new behavior is probably more clear and consistent IMHO.

Thanks again for the fix!

[sbrannen]

Hi @weiw005,

I did some testing in our project with the latest release (6.1.11) and things are working properly now AFAICT.

Great! Glad to hear that resolved your issues.

• Previously, if the expression code passed arguments as an array, e.g. {'hello', 123}, it worked not only for functions that take an array argument, but also functions that accept vararg in which case the array elements would be flattened.
• After this fix, the array would be treated as a single argument for vararg functions.

The new behavior is probably more clear and consistent IMHO.

Perhaps, but just to be certain... would you mind creating a new issue pointing out what used to work and what now doesn't (with simple, concrete examples that we can run (preferably in Java))?

That would at least allow us to assess if the change in behavior is intentional or acceptable.

Cheers,

Sam

[sbrannen]

@weiw005,

• Previously, if the expression code passed arguments as an array, e.g. {'hello', 123}, it worked not only for functions that take an array argument, but also functions that accept vararg in which case the array elements would be flattened.
• After this fix, the array would be treated as a single argument for vararg functions.

In org.springframework.expression.spel.MethodInvocationTests.testVarargsWithObjectArrayType(), we have following test.

evaluate("formatObjectVarargs('x -> %s %s %s', new String[]{'a', 'b', 'c'})", "x -> a b c", String.class);

That passes as expected.

Thus, I assume you mean you are supplying an inline list as follows.

evaluate("formatObjectVarargs('x -> %s %s %s', {'hello', 123, 456})", "x -> hello 123 456", String.class);

That fails, because the inline list (an instance of java.util.Collections$UnmodifiableRandomAccessList) is supplied as the first argument in the Object[] varargs array passed to the formatObjectVarargs() method/function, and that's the expected behavior.

Thus, perhaps you are saying that a List was previously converted to an array in such scenarios, but if that's the case, I believe that was merely coincidental (and accidental).

Can you please confirm that the above scenario is the change in behavior you experienced?

Thanks,

Sam

[weiw005]

Hi @sbrannen

Yes, it's about inline list. Here is a self-contained unit test to demonstrate the difference:

import org.junit.jupiter.api.Test;
import org.springframework.expression.Expression;
import org.springframework.expression.spel.standard.SpelExpressionParser;
import org.springframework.expression.spel.support.StandardEvaluationContext;
import org.springframework.util.ReflectionUtils;

import static org.assertj.core.api.Assertions.assertThat;

import java.lang.reflect.Method;

public class SpelVarargArrayTest {
    @Test
    public void arrayFlattenedAsVararg_Before_6_1_10() {
        assertThat(evaluateExpression("#objectVarargDemo({'World', 123})"))
                .isEqualTo("There are 2 argument(s): World 123");
    }

    @Test
    public void arrayFlattenedAsVararg_After_6_1_11() {
        assertThat(evaluateExpression("#objectVarargDemo({'World', 123})"))
                .isEqualTo("There are 1 argument(s): [World, 123]");
    }

    private static final Method demoMethod =
            ReflectionUtils.findMethod(SpelVarargArrayTest.class, "objectVarargDemo", Object[].class);

    private static Object evaluateExpression(String expression) {
        SpelExpressionParser parser = new SpelExpressionParser();
        Expression exp = parser.parseExpression(expression);
        StandardEvaluationContext context = new StandardEvaluationContext();
        context.registerFunction("objectVarargDemo", demoMethod);
        return exp.getValue(context);
    }

    static String objectVarargDemo(Object... args) {
        StringBuilder sb = new StringBuilder();
        sb.append(String.format("There are %d argument(s): ", args.length));
        for (Object arg : args) {
            sb.append(arg).append(" ");
        }
        return sb.toString().trim();
    }
}

Code link: https://github.com/weiw005/playground/blob/fe749cf911c0212e7f0892fdf06ee29250699aa7/spel-vararg/src/test/java/SpelVarargArrayTest.java

You can see that earlier versions would flatten the inline list as separate elements for vararg functions, and the new version simply treat the whole list as a single argument.

[sbrannen]

Thanks for the feedback and example, @weiw005.

You can see that earlier versions would flatten the inline list as separate elements for vararg functions, and the new version simply treat the whole list as a single argument.

OK. I can see how it used to do that.

As I mentioned before, I believe that behavior was accidental and that recent bug fixes in the varargs processing now implement the correct behavior.

Of course, if the recent changes result in major regressions within the community, we may potentially attempt to support the list (or potentially only inline list) to array conversion there.

Having said that, however, I think we should aim to stick with the current behavior since a list is in fact not an array, and the current behavior in SpEL aligns with the standard Java behavior for varargs invocations.

[sbrannen]

This will also be backported to 5.3.38 and 6.0.23.

Due to #33315, we have decided to revert the backports to 5.3.38 and 6.0.23.