From 1d2b5a15c3b4200e1873c5cbd35a7055e1753670 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Fri, 3 May 2024 17:02:50 +0300 Subject: [PATCH 1/3] Polishing --- .../expression/spel/support/ReflectionHelper.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 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 947566167a9..6d59ff937d8 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 @@ -440,13 +440,15 @@ public abstract class ReflectionHelper { } /** - * Package up the arguments so that they correctly match what is expected in requiredParameterTypes. - *

For example, if requiredParameterTypes is {@code (int, String[])} because the second parameter - * was declared {@code String...}, then if arguments is {@code [1,"a","b"]} then it must be - * repackaged as {@code [1,new String[]{"a","b"}]} in order to match the expected types. + * Package up the supplied {@code args} so that they correctly match what is + * expected in {@code requiredParameterTypes}. + *

For example, if {@code requiredParameterTypes} is {@code (int, String[])} + * because the second parameter was declared as {@code String...}, then if + * {@code args} is {@code [1, "a", "b"]} it must be repackaged as + * {@code [1, new String[] {"a", "b"}]} in order to match the expected types. * @param requiredParameterTypes the types of the parameters for the invocation - * @param args the arguments to be setup ready for the invocation - * @return a repackaged array of arguments where any varargs setup has been done + * @param args the arguments to be set up for the invocation + * @return a repackaged array of arguments where any varargs setup has performed */ public static Object[] setupArgumentsForVarargsInvocation(Class[] requiredParameterTypes, Object... args) { // Check if array already built for final argument From f51be0a17ebcfcf256722affcff5f7ad20a8b877 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 2/3] Support varargs invocations in SpEL for varargs array subtype Closes gh-32704 --- .../spel/support/ReflectionHelper.java | 17 +++- .../spel/SpelCompilationCoverageTests.java | 35 ++++++--- .../spel/support/ReflectionHelperTests.java | 77 +++++++++++++++++-- 3 files changed, 108 insertions(+), 21 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 6d59ff937d8..8353323466c 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.ClassUtils; 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. @@ -451,14 +453,18 @@ public abstract class ReflectionHelper { * @return a repackaged array of arguments where any varargs setup has performed */ 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]; @@ -471,7 +477,7 @@ public abstract class ReflectionHelper { 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]); @@ -483,6 +489,9 @@ public abstract class ReflectionHelper { 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 41c22b88a3d..51388a64087 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 @@ -4221,16 +4221,27 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertThat(tc.s).isEqualTo("aaabbbccc"); tc.reset(); - // TODO Fails related to conversion service converting a String[] to satisfy Object... -// expression = parser.parseExpression("sixteen(stringArray)"); -// assertCantCompile(expression); -// expression.getValue(tc); -// assertEquals("aaabbbccc", tc.s); -// assertCanCompile(expression); -// tc.reset(); -// expression.getValue(tc); -// assertEquals("aaabbbccc", tc.s); -// tc.reset(); + expression = parser.parseExpression("sixteen(seventeen)"); + assertCantCompile(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); + // expression.getValue(tc); + // assertThat(tc.s).isEqualTo("aaabbbccc"); + // assertCanCompile(expression); + // tc.reset(); + // expression.getValue(tc); + // assertThat(tc.s).isEqualTo("aaabbbccc"); + // tc.reset(); // varargs int expression = parser.parseExpression("twelve(1,2,3)"); @@ -6089,6 +6100,10 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { } } } + + 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 1e990417dd8..1da97f75c27 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 org.springframework.expression.spel.support.ReflectionHelper.ArgumentsMat 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 @@ class ReflectionHelperTests extends AbstractExpressionTests { @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 From aebc48ee8d475b0db93405461d65f6098da17196 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Fri, 3 May 2024 17:03:48 +0300 Subject: [PATCH 3/3] Revise contribution See gh-32704 --- .../spel/support/ReflectionHelper.java | 19 +++----- .../spel/support/ReflectionHelperTests.java | 48 +++++++++---------- 2 files changed, 29 insertions(+), 38 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 8353323466c..b103ba0d24e 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,8 +36,6 @@ import org.springframework.util.ClassUtils; 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. @@ -453,19 +451,18 @@ public abstract class ReflectionHelper { * @return a repackaged array of arguments where any varargs setup has performed */ public static Object[] setupArgumentsForVarargsInvocation(Class[] requiredParameterTypes, Object... args) { - int parameterCount = requiredParameterTypes.length; - Assert.notEmpty(requiredParameterTypes, "Required parameter types must not be empty"); + Assert.notEmpty(requiredParameterTypes, "Required parameter types array must not be empty"); + int parameterCount = requiredParameterTypes.length; Class lastRequiredParameterType = requiredParameterTypes[parameterCount - 1]; - Assert.isTrue(lastRequiredParameterType.isArray(), "Method must be varargs"); + Assert.isTrue(lastRequiredParameterType.isArray(), + "The last required parameter type must be an array to support varargs invocation"); int argumentCount = args.length; - Object lastArgument = argumentCount > 0 ? args[argumentCount - 1] : null; + Object lastArgument = (argumentCount > 0 ? args[argumentCount - 1] : null); // Check if repackaging is needed... - if (parameterCount != args.length || - (!isArray(lastArgument) && differentTypes(lastRequiredParameterType, lastArgument))) { - + if (parameterCount != argumentCount || !lastRequiredParameterType.isInstance(lastArgument)) { // Create an array for the leading arguments plus the varargs array argument. Object[] newArgs = new Object[parameterCount]; // Copy all leading arguments to the new array, omitting the varargs array argument. @@ -486,12 +483,10 @@ public abstract class ReflectionHelper { newArgs[newArgs.length - 1] = varargsArray; return newArgs; } + 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/support/ReflectionHelperTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/support/ReflectionHelperTests.java index 1da97f75c27..8a2884e8f11 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,7 +40,7 @@ import org.springframework.expression.spel.support.ReflectionHelper.ArgumentsMat 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.Assertions.assertThatIllegalArgumentException; 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; @@ -252,19 +252,29 @@ class ReflectionHelperTests extends AbstractExpressionTests { checkArguments(args, "3", null, "3.0"); } + @Test + void setupArgumentsForVarargsInvocationPreconditions() { + assertThatIllegalArgumentException() + .isThrownBy(() -> ReflectionHelper.setupArgumentsForVarargsInvocation(new Class[] {}, "a")) + .withMessage("Required parameter types array must not be empty"); + + assertThatIllegalArgumentException() + .isThrownBy(() -> ReflectionHelper.setupArgumentsForVarargsInvocation( + new Class[] { Integer.class, Integer.class }, 123)) + .withMessage("The last required parameter type must be an array to support varargs invocation"); + } + @Test void setupArgumentsForVarargsInvocation() { Object[] newArray; - newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( - new Class[] {String[].class}, "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"); + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation(new Class[] { Object[].class }, "a", "b", "c"); assertThat(newArray) .singleElement() .asInstanceOf(array(Object[].class)) @@ -272,14 +282,12 @@ class ReflectionHelperTests extends AbstractExpressionTests { 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")); + assertThat(newArray).satisfiesExactly( + one -> assertThat(one).isEqualTo(123), + two -> assertThat(two).isEqualTo(456), + three -> assertThat(three).asInstanceOf(array(String[].class)).containsExactly("a", "b", "c")); - newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( - new Class[] { String[].class }); + newArray = ReflectionHelper.setupArgumentsForVarargsInvocation(new Class[] { String[].class }); assertThat(newArray) .singleElement() .asInstanceOf(array(String[].class)) @@ -299,30 +307,18 @@ class ReflectionHelperTests extends AbstractExpressionTests { .asInstanceOf(array(Object[].class)) .containsExactly("a", "b", "c"); - newArray = ReflectionHelper.setupArgumentsForVarargsInvocation( - new Class[] { String[].class }, "a"); + 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}); + 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