Use `ConversionService` to convert POJO to array for SpEL varargs invocations

[Nephery]

We have a scenario where we have a POJO that is "like a list/array", but actually isn't. Which we registered to the ConversionService as being convertible to Collection<Object> and Object[].

However, even though we've registered this POJO as being convertible to Collection and Object[], function references aren't able to take advantage of that, and instead attempts to convert the POJO to the element type instead of the varargs container type.

Reproduction

Spring Framework: 6.2.2

Here's a simple test class to reproduce the issue:

package test;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.core.convert.TypeDescriptor;
import org.springframework.core.convert.converter.GenericConverter;
import org.springframework.core.convert.support.DefaultConversionService;
import org.springframework.expression.Expression;
import org.springframework.expression.spel.standard.SpelExpressionParser;
import org.springframework.expression.spel.support.StandardEvaluationContext;
import org.springframework.expression.spel.support.StandardTypeConverter;
import org.springframework.lang.Nullable;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.util.Set;

public class SpELBugReproTest {
	private final StandardEvaluationContext context = new StandardEvaluationContext();
	private final SpelExpressionParser parser = new SpelExpressionParser();

	@BeforeEach
	void setUp() throws Exception {
		// configure the type converter to convert LikeAList to Object[]
		DefaultConversionService conversionService = new DefaultConversionService();
		conversionService.addConverter(new LikeAListConverter());
		context.setTypeConverter(new StandardTypeConverter(conversionService));

		// register functions
		MethodHandle varArgsMethodHandle = MethodHandles.lookup().findStatic(SpELBugReproTest.class,
				"varArgsFunction", MethodType.methodType(String.class, String[].class));
		MethodHandle notVarArgsFunction = MethodHandles.lookup().findStatic(SpELBugReproTest.class,
				"notVarArgsFunction", MethodType.methodType(String.class, String[].class, String.class));
		context.registerFunction("varArgsFunction", varArgsMethodHandle);
		context.registerFunction("notVarArgsFunction", notVarArgsFunction);
	}

	// This test is just to show that type converter works correctly when converting LikeAList to Object[]
	// for a parameter that is NOT a varargs parameter
	@Test
	void testNotVarArgsFunction() {
		Expression expression = parser.parseExpression("#notVarArgsFunction(#root, 'foo')");
		LikeAList source = new LikeAList("a", "b", "c");
		Assertions.assertEquals("a,b,c", expression.getValue(context, source));
	}

	// This test is to show that type converter does not work correctly when converting LikeAList to Object[]
	// for a parameter that IS a varargs parameter
	@Test
	void testVarArgsFunction() {
		Expression expression = parser.parseExpression("#varArgsFunction(#root)");
		LikeAList source = new LikeAList("a", "b", "c");
		Assertions.assertEquals("a,b,c", expression.getValue(context, source));
	}

	// This is a simplified version of a class that is "like a collection/array, but not really"
	private record LikeAList(String... blah) {}

	// Type converter to tell SpEL to generally treat LikeAList as an Object[]
	private static class LikeAListConverter implements GenericConverter {

		@Nullable
		@Override
		public Set<ConvertiblePair> getConvertibleTypes() {
			return Set.of(new ConvertiblePair(LikeAList.class, Object[].class));
		}

		@Nullable
		@Override
		public Object convert(@Nullable Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
			return ((LikeAList) source).blah();
		}
	}

	// extra parameter so that SpEL doesn't treat the 'input' parameter as a varargs parameter
	public static String notVarArgsFunction(String[] input, String extra) {
		return String.join(",", input);
	}

	public static String varArgsFunction(String... input) {
		return String.join(",", input);
	}
}

Now running the testVarArgsFunction test will throw the following exception:

org.springframework.expression.spel.SpelEvaluationException: EL1001E: Type conversion problem, cannot convert from test.SpELBugReproTest$LikeAList to java.lang.String

	at org.springframework.expression.spel.support.StandardTypeConverter.convertValue(StandardTypeConverter.java:87)
	at org.springframework.expression.spel.support.ReflectionHelper.convertAllMethodHandleArguments(ReflectionHelper.java:425)
	at org.springframework.expression.spel.ast.FunctionReference.executeFunctionViaMethodHandle(FunctionReference.java:229)
	at org.springframework.expression.spel.ast.FunctionReference.getValueInternal(FunctionReference.java:98)
	at org.springframework.expression.spel.ast.SpelNodeImpl.getValue(SpelNodeImpl.java:116)
	at org.springframework.expression.spel.standard.SpelExpression.getValue(SpelExpression.java:338)
	at test.SpELBugReproTest.testVarArgsFunction(SpELBugReproTest.java:55)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: org.springframework.core.convert.ConverterNotFoundException: No converter found capable of converting from type [test.SpELBugReproTest$LikeAList] to type [java.lang.String]
	at org.springframework.core.convert.support.GenericConversionService.handleConverterNotFound(GenericConversionService.java:294)
	at org.springframework.core.convert.support.GenericConversionService.convert(GenericConversionService.java:185)
	at org.springframework.expression.spel.support.StandardTypeConverter.convertValue(StandardTypeConverter.java:82)
	... 9 more

Potential Fix

Looking at ReflectionHelper, it seems that the detection for whether the last argument should be converted to the varargsArrayType or varargsComponentType depends on if the argument itself is a literal List or an array implementation.

Looking at the comments above and below this else-if block, maybe the fix for this is to change:

sourceType.isArray() || argument instanceof List ? varargsArrayType : varargsComponentType

to something like:

sourceType.isArray() || argument instanceof List || !converter.canConvert(sourceType, varargsComponentType) ? varargsArrayType : varargsComponentType

That way you'd still preserve the use case where you put priority on converting to the varargsComponentType over the varargsArrayType, which (I think) would preserve avoiding edge cases like accidentally converting String to String[] via StringToArrayConverter.

[sbrannen]

Hi @Nephery,

Thanks for raising the issue and making a proposal to address it.

I think converter.canConvert(sourceType, varargsArrayType) would make more sense than !converter.canConvert(sourceType, varargsComponentType) in that scenario, and that appears to do the job.

So, I'll investigate it further.

In addition, we need to make sure that this scenario works for a SpEL function reference registered from a java.lang.reflect.Method in addition to MethodHandle registrations.

Cheers,

Sam