From 3b1bb188bf87cdd281b379a49b099c26efff853b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mika=C3=ABl=20Francoeur?= Date: Mon, 22 Apr 2024 20:22:04 -0400 Subject: [PATCH] Fix array wrapping in varargs invocations. --- .../spel/support/ReflectionHelper.java | 17 +++- .../spel/SpelCompilationCoverageTests.java | 15 ++++ .../spel/support/ReflectionHelperTests.java | 77 +++++++++++++++++-- 3 files changed, 98 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 1e311e2ed261..306889e0fa8a 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 @@ -36,6 +36,8 @@ import org.springframework.util.CollectionUtils; import org.springframework.util.MethodInvoker; +import static org.springframework.util.ObjectUtils.isArray; + /** * Utility methods used by the reflection resolver code to discover the appropriate * methods, constructors, and fields that should be used in expressions. @@ -457,14 +459,18 @@ private static boolean isFirstEntryInArray(Object value, @Nullable Object possib * @return a repackaged array of arguments where any varargs setup has been done */ public static Object[] setupArgumentsForVarargsInvocation(Class[] requiredParameterTypes, Object... args) { - // Check if array already built for final argument int parameterCount = requiredParameterTypes.length; + Assert.notEmpty(requiredParameterTypes, "Required parameter types must not be empty"); + + Class lastRequiredParameterType = requiredParameterTypes[parameterCount - 1]; + Assert.isTrue(lastRequiredParameterType.isArray(), "Method must be varargs"); + int argumentCount = args.length; + Object lastArgument = argumentCount > 0 ? args[argumentCount - 1] : null; // Check if repackaging is needed... if (parameterCount != args.length || - requiredParameterTypes[parameterCount - 1] != - (args[argumentCount - 1] != null ? args[argumentCount - 1].getClass() : null)) { + (!isArray(lastArgument) && differentTypes(lastRequiredParameterType, lastArgument))) { // Create an array for the leading arguments plus the varargs array argument. Object[] newArgs = new Object[parameterCount]; @@ -477,7 +483,7 @@ public static Object[] setupArgumentsForVarargsInvocation(Class[] requiredPar if (argumentCount >= parameterCount) { varargsArraySize = argumentCount - (parameterCount - 1); } - Class componentType = requiredParameterTypes[parameterCount - 1].componentType(); + Class componentType = lastRequiredParameterType.componentType(); Object varargsArray = Array.newInstance(componentType, varargsArraySize); for (int i = 0; i < varargsArraySize; i++) { Array.set(varargsArray, i, args[parameterCount - 1 + i]); @@ -489,6 +495,9 @@ public static Object[] setupArgumentsForVarargsInvocation(Class[] requiredPar return args; } + private static boolean differentTypes(Class lastRequiredParameterType, @Nullable Object lastArgument) { + return lastArgument == null || lastRequiredParameterType != lastArgument.getClass(); + } /** * Arguments match kinds. diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java index 77da11dea144..9248a49e8d03 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java @@ -4648,6 +4648,17 @@ void methodReferenceVarargs() { assertThat(tc.s).isEqualTo("aaabbbccc"); tc.reset(); + expression = parser.parseExpression("sixteen(seventeen)"); + assertCannotCompile(expression); + expression.getValue(tc); + assertThat(tc.s).isEqualTo("aaabbbccc"); + assertCanCompile(expression); + tc.reset(); + // see TODO below + // expression.getValue(tc); + // assertThat(tc.s).isEqualTo("aaabbbccc"); + // tc.reset(); + // TODO Determine why the String[] is passed as the first element of the Object... varargs array instead of the entire varargs array. // expression = parser.parseExpression("sixteen(stringArray)"); // assertCantCompile(expression); @@ -6502,6 +6513,10 @@ public void sixteen(Object... vargs) { } } } + + public String[] seventeen() { + return new String[] { "aaa", "bbb", "ccc" }; + } } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/support/ReflectionHelperTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/support/ReflectionHelperTests.java index 1e990417dd83..1da97f75c270 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/support/ReflectionHelperTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/support/ReflectionHelperTests.java @@ -40,6 +40,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.InstanceOfAssertFactories.array; import static org.springframework.expression.spel.support.ReflectionHelper.ArgumentsMatchKind.CLOSE; import static org.springframework.expression.spel.support.ReflectionHelper.ArgumentsMatchKind.EXACT; import static org.springframework.expression.spel.support.ReflectionHelper.ArgumentsMatchKind.REQUIRES_CONVERSION; @@ -252,14 +254,75 @@ void convertAllArguments() throws Exception { @Test void setupArgumentsForVarargsInvocation() { - Object[] newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( - new Class[] {String[].class}, "a", "b", "c"); + Object[] newArray; - assertThat(newArray).hasSize(1); - Object firstParam = newArray[0]; - assertThat(firstParam.getClass().componentType()).isEqualTo(String.class); - Object[] firstParamArray = (Object[]) firstParam; - assertThat(firstParamArray).containsExactly("a", "b", "c"); + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] {String[].class}, "a", "b", "c"); + assertThat(newArray) + .singleElement() + .asInstanceOf(array(String[].class)) + .containsExactly("a", "b", "c"); + + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] { Object[].class }, "a", "b", "c"); + assertThat(newArray) + .singleElement() + .asInstanceOf(array(Object[].class)) + .containsExactly("a", "b", "c"); + + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] { Integer.class, Integer.class, String[].class }, 123, 456, "a", "b", "c"); + assertThat(newArray) + .satisfiesExactly( + i -> assertThat(i).isEqualTo(123), + i -> assertThat(i).isEqualTo(456), + i -> assertThat(i).asInstanceOf(array(String[].class)).containsExactly("a", "b", "c")); + + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] { String[].class }); + assertThat(newArray) + .singleElement() + .asInstanceOf(array(String[].class)) + .isEmpty(); + + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] { String[].class }, new Object[] { new String[] { "a", "b", "c" } }); + assertThat(newArray) + .singleElement() + .asInstanceOf(array(String[].class)) + .containsExactly("a", "b", "c"); + + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] { Object[].class }, new Object[] { new String[] { "a", "b", "c" } }); + assertThat(newArray) + .singleElement() + .asInstanceOf(array(Object[].class)) + .containsExactly("a", "b", "c"); + + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] { String[].class }, "a"); + assertThat(newArray) + .singleElement() + .asInstanceOf(array(String[].class)) + .containsExactly("a"); + + + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] { String[].class }, new Object[]{null}); + assertThat(newArray) + .singleElement() + .asInstanceOf(array(String[].class)) + .singleElement() + .isNull(); + + assertThatThrownBy(() -> ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] { Integer.class, Integer.class }, 123, 456)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Method must be varargs"); + + assertThatThrownBy(() -> ReflectionHelper.setupArgumentsForVarargsInvocation(new Class[] {}, "a", "b", "c")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Required parameter types must not be empty"); } @Test