From 9af11ad5ce137e8fe71f0ce9e980745e4c840818 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 22 Oct 2021 11:08:33 +0200 Subject: [PATCH 1/3] Fix Javadoc formatting issues --- .../core/convert/TypeDescriptor.java | 14 +++++++------- .../springframework/expression/TypeConverter.java | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/convert/TypeDescriptor.java b/spring-core/src/main/java/org/springframework/core/convert/TypeDescriptor.java index 1bc8cb281b..f6e25824e0 100644 --- a/spring-core/src/main/java/org/springframework/core/convert/TypeDescriptor.java +++ b/spring-core/src/main/java/org/springframework/core/convert/TypeDescriptor.java @@ -37,7 +37,7 @@ import org.springframework.util.ObjectUtils; /** * Contextual descriptor about a type to convert from or to. - * Capable of representing arrays and generic collection types. + *

Capable of representing arrays and generic collection types. * * @author Keith Donald * @author Andy Clement @@ -345,9 +345,9 @@ public class TypeDescriptor implements Serializable { * from the provided collection or array element. *

Narrows the {@link #getElementTypeDescriptor() elementType} property to the class * of the provided collection or array element. For example, if this describes a - * {@code java.util.List<java.lang.Number<} and the element argument is an + * {@code java.util.List} and the element argument is a * {@code java.lang.Integer}, the returned TypeDescriptor will be {@code java.lang.Integer}. - * If this describes a {@code java.util.List<?>} and the element argument is an + * If this describes a {@code java.util.List} and the element argument is a * {@code java.lang.Integer}, the returned TypeDescriptor will be {@code java.lang.Integer} * as well. *

Annotation and nested type context will be preserved in the narrowed @@ -388,9 +388,9 @@ public class TypeDescriptor implements Serializable { * from the provided map key. *

Narrows the {@link #getMapKeyTypeDescriptor() mapKeyType} property * to the class of the provided map key. For example, if this describes a - * {@code java.util.Map<java.lang.Number, java.lang.String<} and the key + * {@code java.util.Map} and the key * argument is a {@code java.lang.Integer}, the returned TypeDescriptor will be - * {@code java.lang.Integer}. If this describes a {@code java.util.Map<?, ?>} + * {@code java.lang.Integer}. If this describes a {@code java.util.Map} * and the key argument is a {@code java.lang.Integer}, the returned * TypeDescriptor will be {@code java.lang.Integer} as well. *

Annotation and nested type context will be preserved in the narrowed @@ -425,9 +425,9 @@ public class TypeDescriptor implements Serializable { * from the provided map value. *

Narrows the {@link #getMapValueTypeDescriptor() mapValueType} property * to the class of the provided map value. For example, if this describes a - * {@code java.util.Map<java.lang.String, java.lang.Number<} and the value + * {@code java.util.Map} and the value * argument is a {@code java.lang.Integer}, the returned TypeDescriptor will be - * {@code java.lang.Integer}. If this describes a {@code java.util.Map<?, ?>} + * {@code java.lang.Integer}. If this describes a {@code java.util.Map} * and the value argument is a {@code java.lang.Integer}, the returned * TypeDescriptor will be {@code java.lang.Integer} as well. *

Annotation and nested type context will be preserved in the narrowed diff --git a/spring-expression/src/main/java/org/springframework/expression/TypeConverter.java b/spring-expression/src/main/java/org/springframework/expression/TypeConverter.java index f2f67c0965..6974d6b874 100644 --- a/spring-expression/src/main/java/org/springframework/expression/TypeConverter.java +++ b/spring-expression/src/main/java/org/springframework/expression/TypeConverter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -44,7 +44,7 @@ public interface TypeConverter { * Convert (or coerce) a value from one type to another, for example from a * {@code boolean} to a {@code String}. *

The {@link TypeDescriptor} parameters enable support for typed collections: - * A caller may prefer a {@code List<Integer>}, for example, rather than + * A caller may prefer a {@code List}, for example, rather than * simply any {@code List}. * @param value the value to be converted * @param sourceType a type descriptor that supplies extra information about the From 9b967775ede2d885746d734347a356abb8b3d1db Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 22 Oct 2021 10:45:34 +0200 Subject: [PATCH 2/3] Polish Spel's ReflectionHelper.setupArgumentsForVarargsInvocation() --- .../spel/support/ReflectionHelper.java | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 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 d8af41170d..65e3cff83a 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 @@ -332,8 +332,8 @@ public abstract class ReflectionHelper { } /** - * Package up the arguments so that they correctly match what is expected in parameterTypes. - * For example, if parameterTypes is {@code (int, String[])} because the second parameter + * 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. * @param requiredParameterTypes the types of the parameters for the invocation @@ -350,23 +350,24 @@ public abstract class ReflectionHelper { requiredParameterTypes[parameterCount - 1] != (args[argumentCount - 1] != null ? args[argumentCount - 1].getClass() : null)) { - int arraySize = 0; // zero size array if nothing to pass as the varargs parameter - if (argumentCount >= parameterCount) { - arraySize = argumentCount - (parameterCount - 1); - } - - // Create an array for the varargs arguments + // 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. System.arraycopy(args, 0, newArgs, 0, newArgs.length - 1); // Now sort out the final argument, which is the varargs one. Before entering this method, // the arguments should have been converted to the box form of the required type. - Class componentType = requiredParameterTypes[parameterCount - 1].getComponentType(); - Object repackagedArgs = Array.newInstance(componentType, arraySize); - for (int i = 0; i < arraySize; i++) { - Array.set(repackagedArgs, i, args[parameterCount - 1 + i]); + int varargsArraySize = 0; // zero size array if nothing to pass as the varargs parameter + if (argumentCount >= parameterCount) { + varargsArraySize = argumentCount - (parameterCount - 1); } - newArgs[newArgs.length - 1] = repackagedArgs; + Class componentType = requiredParameterTypes[parameterCount - 1].getComponentType(); + Object varargsArray = Array.newInstance(componentType, varargsArraySize); + for (int i = 0; i < varargsArraySize; i++) { + Array.set(varargsArray, i, args[parameterCount - 1 + i]); + } + // Finally, add the varargs array to the new arguments array. + newArgs[newArgs.length - 1] = varargsArray; return newArgs; } return args; From bc657eb4d5babccf5065ef26fca8c9723675a083 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Thu, 21 Oct 2021 14:50:34 +0200 Subject: [PATCH 3/3] Fix SpEL vararg method invocation for strings containing commas Prior to this commit, if a SpEL expression invoked a method or registered function that declares a String varargs argument, there were sometimes issues with converting the input arguments into the varargs array argument. Specifically, if the expression supplied a single String argument containing a comma for the varargs (such as "a,b"), SpEL's ReflectionHelper.convertArguments() method incorrectly converted that single String to an array via the ConversionService, which indirectly converted that String using the StringToArrayConverter, which converts a comma-delimited String to an array. Thus, "a,b" effectively got converted to a two-dimensional array ["a", "b"] instead of simply ["a,b"]. This commit fixes this bug by avoiding use of the TypeConverter and ConversionService for single arguments supplied as varargs when the single argument's type matches the varargs array component type. Closes gh-27582 --- .../spel/support/ReflectionHelper.java | 28 +++++++----- .../spel/MethodInvocationTests.java | 43 ++++++++++++++++--- .../spel/testresources/Inventor.java | 11 ++++- 3 files changed, 63 insertions(+), 19 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 65e3cff83a..821c0b246a 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -38,6 +38,7 @@ import org.springframework.util.MethodInvoker; * * @author Andy Clement * @author Juergen Hoeller + * @author Sam Brannen * @since 3.0 */ public abstract class ReflectionHelper { @@ -281,25 +282,32 @@ public abstract class ReflectionHelper { arguments[i] = converter.convertValue(argument, TypeDescriptor.forObject(argument), targetType); conversionOccurred |= (argument != arguments[i]); } + MethodParameter methodParam = MethodParameter.forExecutable(executable, varargsPosition); + + // If the target is varargs and there is just one more argument, then convert it here. if (varargsPosition == arguments.length - 1) { - // If the target is varargs and there is just one more argument - // then convert it here - TypeDescriptor targetType = new TypeDescriptor(methodParam); Object argument = arguments[varargsPosition]; + TypeDescriptor targetType = new TypeDescriptor(methodParam); TypeDescriptor sourceType = TypeDescriptor.forObject(argument); - arguments[varargsPosition] = converter.convertValue(argument, sourceType, targetType); - // Three outcomes of that previous line: - // 1) the input argument was already compatible (ie. array of valid type) and nothing was done - // 2) the input argument was correct type but not in an array so it was made into an array - // 3) the input argument was the wrong type and got converted and put into an array + // If the argument type is equal to the varargs element type, there is no need + // to convert it or wrap it in an array. For example, using StringToArrayConverter + // to 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. + if (!sourceType.equals(targetType.getElementTypeDescriptor())) { + arguments[varargsPosition] = converter.convertValue(argument, sourceType, targetType); + } + // Three outcomes of the above if-block: + // 1) the input argument was correct type but not wrapped in an array, and nothing was done. + // 2) the input argument was already compatible (i.e., array of valid type), and nothing was done. + // 3) the input argument was the wrong type and got converted and wrapped in an array. if (argument != arguments[varargsPosition] && !isFirstEntryInArray(argument, arguments[varargsPosition])) { conversionOccurred = true; // case 3 } } + // Otherwise, convert remaining arguments to the varargs element type. else { - // Convert remaining arguments to the varargs element type TypeDescriptor targetType = new TypeDescriptor(methodParam).getElementTypeDescriptor(); Assert.state(targetType != null, "No element type"); for (int i = varargsPosition; i < arguments.length; i++) { diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java index 0a025acf72..b2cde1f10f 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -46,6 +46,7 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; * * @author Andy Clement * @author Phillip Webb + * @author Sam Brannen */ public class MethodInvocationTests extends AbstractExpressionTests { @@ -233,26 +234,54 @@ public class MethodInvocationTests extends AbstractExpressionTests { @Test public void testVarargsInvocation01() { - // Calling 'public int aVarargsMethod(String... strings)' - //evaluate("aVarargsMethod('a','b','c')", 3, Integer.class); - //evaluate("aVarargsMethod('a')", 1, Integer.class); + // Calling 'public int aVarargsMethod(String... strings)' - returns number of arguments + evaluate("aVarargsMethod('a','b','c')", 3, Integer.class); + evaluate("aVarargsMethod('a')", 1, Integer.class); evaluate("aVarargsMethod()", 0, Integer.class); evaluate("aVarargsMethod(1,2,3)", 3, Integer.class); // all need converting to strings evaluate("aVarargsMethod(1)", 1, Integer.class); // needs string conversion evaluate("aVarargsMethod(1,'a',3.0d)", 3, Integer.class); // first and last need conversion - // evaluate("aVarargsMethod(new String[]{'a','b','c'})", 3, Integer.class); + evaluate("aVarargsMethod(new String[]{'a','b','c'})", 3, Integer.class); } @Test public void testVarargsInvocation02() { - // Calling 'public int aVarargsMethod2(int i, String... strings)' - returns int+length_of_strings + // Calling 'public int aVarargsMethod2(int i, String... strings)' - returns int + length_of_strings evaluate("aVarargsMethod2(5,'a','b','c')", 8, Integer.class); evaluate("aVarargsMethod2(2,'a')", 3, Integer.class); evaluate("aVarargsMethod2(4)", 4, Integer.class); evaluate("aVarargsMethod2(8,2,3)", 10, Integer.class); evaluate("aVarargsMethod2(9)", 9, Integer.class); evaluate("aVarargsMethod2(2,'a',3.0d)", 4, Integer.class); - // evaluate("aVarargsMethod2(8,new String[]{'a','b','c'})", 11, Integer.class); + evaluate("aVarargsMethod2(8,new String[]{'a','b','c'})", 11, Integer.class); + } + + @Test + public void testVarargsInvocation03() { + // Calling 'public int aVarargsMethod3(String str1, String... strings)' - returns all strings concatenated with "-" + + // No conversion necessary + evaluate("aVarargsMethod3('x')", "x", String.class); + evaluate("aVarargsMethod3('x', 'a')", "x-a", String.class); + evaluate("aVarargsMethod3('x', 'a', 'b', 'c')", "x-a-b-c", String.class); + + // Conversion necessary + evaluate("aVarargsMethod3(9)", "9", String.class); + evaluate("aVarargsMethod3(8,2,3)", "8-2-3", String.class); + evaluate("aVarargsMethod3('2','a',3.0d)", "2-a-3.0", String.class); + evaluate("aVarargsMethod3('8',new String[]{'a','b','c'})", "8-a-b-c", String.class); + + // Individual string contains a comma with multiple varargs arguments + evaluate("aVarargsMethod3('foo', ',', 'baz')", "foo-,-baz", String.class); + evaluate("aVarargsMethod3('foo', 'bar', ',baz')", "foo-bar-,baz", String.class); + evaluate("aVarargsMethod3('foo', 'bar,', 'baz')", "foo-bar,-baz", String.class); + + // Individual string contains a comma with single varargs argument. + // Reproduces https://github.com/spring-projects/spring-framework/issues/27582 + evaluate("aVarargsMethod3('foo', ',')", "foo-,", String.class); + evaluate("aVarargsMethod3('foo', ',bar')", "foo-,bar", String.class); + evaluate("aVarargsMethod3('foo', 'bar,')", "foo-bar,", String.class); + evaluate("aVarargsMethod3('foo', 'bar,baz')", "foo-bar,baz", String.class); } @Test diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java b/spring-expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java index bdf6d79c1d..34960d982f 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -28,6 +28,7 @@ import org.springframework.util.ObjectUtils; ///CLOVER:OFF @SuppressWarnings("unused") public class Inventor { + private String name; public String _name; public String _name_; @@ -202,8 +203,14 @@ public class Inventor { return strings.length + i; } - public Inventor(String... strings) { + public String aVarargsMethod3(String str1, String... strings) { + if (ObjectUtils.isEmpty(strings)) { + return str1; + } + return str1 + "-" + String.join("-", strings); + } + public Inventor(String... strings) { } public boolean getSomeProperty() {