Polish contribution

See spring-projectsgh-32704
sbrannen · May 3, 2024 · 0fc37e0 · 0fc37e0
1 parent 89d4b6f
commit 0fc37e0
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 48 deletions.
diff --git a/...xpression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java b/...xpression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java
@@ -36,8 +36,6 @@
 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.
@@ -450,28 +448,29 @@ private static boolean isFirstEntryInArray(Object value, @Nullable Object possib
 	}
 
 	/**
-	 * Package up the arguments so that they correctly match what is expected in requiredParameterTypes.
-	 * <p>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}.
+	 * <p>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) {
-		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.
@@ -492,12 +491,10 @@ public static Object[] setupArgumentsForVarargsInvocation(Class<?>[] requiredPar
 			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/...ssion/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java b/...ssion/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java
@@ -6631,10 +6631,6 @@ public void concat2(Object... vargs) {
 				}
 			}
 		}
-
-		public String[] seventeen() {
-			return new String[] { "aaa", "bbb", "ccc" };
-		}
 	}
 
 
@@ -6863,6 +6859,10 @@ public void sixteen(Object... vargs) {
 				}
 			}
 		}
+
+		public String[] seventeen() {
+			return new String[] { "aaa", "bbb", "ccc" };
+		}
 	}
 
 

diff --git a/...sion/src/test/java/org/springframework/expression/spel/support/ReflectionHelperTests.java b/...sion/src/test/java/org/springframework/expression/spel/support/ReflectionHelperTests.java
@@ -40,7 +40,7 @@
 
 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,34 +252,42 @@ void convertAllArguments() throws Exception {
 		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))
 				.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"));
+		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 @@ void setupArgumentsForVarargsInvocation() {
 				.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