From bc657eb4d5babccf5065ef26fca8c9723675a083 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Thu, 21 Oct 2021 14:50:34 +0200 Subject: [PATCH] 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 65e3cff83a86..821c0b246ae6 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 @@ * * @author Andy Clement * @author Juergen Hoeller + * @author Sam Brannen * @since 3.0 */ public abstract class ReflectionHelper { @@ -281,25 +282,32 @@ static boolean convertArguments(TypeConverter converter, Object[] arguments, Exe 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 0a025acf723f..b2cde1f10ff3 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 @@ * * @author Andy Clement * @author Phillip Webb + * @author Sam Brannen */ public class MethodInvocationTests extends AbstractExpressionTests { @@ -233,26 +234,54 @@ public void testAddingMethodResolvers() { @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 bdf6d79c1ddc..34960d982fe3 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 @@ ///CLOVER:OFF @SuppressWarnings("unused") public class Inventor { + private String name; public String _name; public String _name_; @@ -202,8 +203,14 @@ public int aVarargsMethod2(int i, String... strings) { 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() {