From dc16f3cffb32ea6574f5d33c6d2c6d280b37671a Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 10 Jul 2024 16:01:58 +0200 Subject: [PATCH 1/2] Polish SpEL function invocation support --- .../expression/spel/ast/FunctionReference.java | 2 +- .../spel/support/ReflectionHelper.java | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java index 9f715e29870b..22206f4bcb28 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java @@ -217,7 +217,7 @@ else if (spelParamCount != declaredParamCount) { } } - // more complex case, we need to look at conversion and vararg repacking + // more complex case, we need to look at conversion and varargs repackaging Integer varArgPosition = null; if (isSuspectedVarargs) { varArgPosition = declaredParamCount - 1; 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 31bd84389a29..44426fcb317b 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 @@ -356,10 +356,10 @@ public static boolean convertAllMethodHandleArguments(TypeConverter converter, O MethodHandle methodHandle, @Nullable Integer varargsPosition) throws EvaluationException { boolean conversionOccurred = false; - MethodType methodHandleArgumentTypes = methodHandle.type(); + MethodType methodHandleType = methodHandle.type(); if (varargsPosition == null) { for (int i = 0; i < arguments.length; i++) { - Class argumentClass = methodHandleArgumentTypes.parameterType(i); + Class argumentClass = methodHandleType.parameterType(i); ResolvableType resolvableType = ResolvableType.forClass(argumentClass); TypeDescriptor targetType = new TypeDescriptor(resolvableType, argumentClass, null); @@ -371,7 +371,7 @@ public static boolean convertAllMethodHandleArguments(TypeConverter converter, O else { // Convert everything up to the varargs position for (int i = 0; i < varargsPosition; i++) { - Class argumentClass = methodHandleArgumentTypes.parameterType(i); + Class argumentClass = methodHandleType.parameterType(i); ResolvableType resolvableType = ResolvableType.forClass(argumentClass); TypeDescriptor targetType = new TypeDescriptor(resolvableType, argumentClass, null); @@ -380,10 +380,10 @@ public static boolean convertAllMethodHandleArguments(TypeConverter converter, O conversionOccurred |= (argument != arguments[i]); } - Class varArgClass = methodHandleArgumentTypes.lastParameterType().componentType(); + Class varArgClass = methodHandleType.lastParameterType().componentType(); ResolvableType varArgResolvableType = ResolvableType.forClass(varArgClass); - TypeDescriptor varArgComponentType = new TypeDescriptor(varArgResolvableType, varArgClass, null); - TypeDescriptor componentTypeDesc = varArgComponentType.getElementTypeDescriptor(); + TypeDescriptor targetType = new TypeDescriptor(varArgResolvableType, varArgClass, null); + TypeDescriptor componentTypeDesc = targetType.getElementTypeDescriptor(); // TODO Determine why componentTypeDesc can be null. // Assert.state(componentTypeDesc != null, "Component type must not be null for a varargs array"); @@ -403,7 +403,7 @@ public static boolean convertAllMethodHandleArguments(TypeConverter converter, O // 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 (componentTypeDesc != null && !sourceType.isAssignableTo(componentTypeDesc)) { - arguments[varargsPosition] = converter.convertValue(argument, sourceType, varArgComponentType); + arguments[varargsPosition] = converter.convertValue(argument, sourceType, targetType); } // Possible outcomes of the above if-else block: // 1) the input argument was null, and nothing was done. @@ -420,7 +420,7 @@ else if (componentTypeDesc != null && !sourceType.isAssignableTo(componentTypeDe else { for (int i = varargsPosition; i < arguments.length; i++) { Object argument = arguments[i]; - arguments[i] = converter.convertValue(argument, TypeDescriptor.forObject(argument), varArgComponentType); + arguments[i] = converter.convertValue(argument, TypeDescriptor.forObject(argument), targetType); conversionOccurred |= (argument != arguments[i]); } } From 83ca2c0cff43f28fc4ddba3f229636ea1f7e628e Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 10 Jul 2024 16:05:43 +0200 Subject: [PATCH 2/2] Support MethodHandle function invocation with varargs array in SpEL Prior to this commit, the Spring Expression Language (SpEL) could not invoke a varargs MethodHandle function with an array containing the variable arguments, although that is supported for a varargs Method function. Attempting to do so resulted in the array being supplied as a single argument to the MethodHandle. This commit addresses this by updating the executeFunctionViaMethodHandle(...) method in FunctionReference as follows when the user supplies the varargs already packaged in an array. - Creates a new array large enough to hold the non-varargs arguments and the unpackaged varargs arguments. - Adds the non-varargs arguments to the beginning of that array and adds the unpackaged varargs arguments to the end of that array. - Invokes the MethodHandle with the new arguments array. Closes gh-33191 --- .../spel/ast/FunctionReference.java | 27 ++++++++++++++++--- .../spel/VariableAndFunctionTests.java | 25 ++++++----------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java index 22206f4bcb28..a798891f35d4 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java @@ -225,10 +225,29 @@ else if (spelParamCount != declaredParamCount) { TypeConverter converter = state.getEvaluationContext().getTypeConverter(); ReflectionHelper.convertAllMethodHandleArguments(converter, functionArgs, methodHandle, varArgPosition); - if (isSuspectedVarargs && declaredParamCount == 1) { - // we only repack the varargs if it is the ONLY argument - functionArgs = ReflectionHelper.setupArgumentsForVarargsInvocation( - methodHandle.type().parameterArray(), functionArgs); + if (isSuspectedVarargs) { + if (declaredParamCount == 1) { + // We only repackage the varargs if it is the ONLY argument -- for example, + // when we are dealing with a bound MethodHandle. + functionArgs = ReflectionHelper.setupArgumentsForVarargsInvocation( + methodHandle.type().parameterArray(), functionArgs); + } + else if (spelParamCount == declaredParamCount) { + // If the varargs were supplied already packaged in an array, we have to create + // a new array, add the non-varargs arguments to the beginning of that array, + // and add the unpackaged varargs arguments to the end of that array. The reason + // is that MethodHandle.invokeWithArguments(Object...) does not expect varargs + // to be packaged in an array, in contrast to how method invocation works with + // reflection. + int actualVarargsIndex = functionArgs.length - 1; + if (actualVarargsIndex >= 0 && functionArgs[actualVarargsIndex].getClass().isArray()) { + Object[] argsToUnpack = (Object[]) functionArgs[actualVarargsIndex]; + Object[] newArgs = new Object[actualVarargsIndex + argsToUnpack.length]; + System.arraycopy(functionArgs, 0, newArgs, 0, actualVarargsIndex); + System.arraycopy(argsToUnpack, 0, newArgs, actualVarargsIndex, argsToUnpack.length); + functionArgs = newArgs; + } + } } try { 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 ea543ad26ccb..a725b95306e8 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,7 +16,6 @@ package org.springframework.expression.spel; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.springframework.expression.spel.standard.SpelExpressionParser; @@ -106,22 +105,6 @@ 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 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) @@ -138,6 +121,14 @@ void functionWithVarargsViaMethodHandle() { 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("#add('2', 5.0)", 7, Integer.class);