more tests - about 82% coverage now. focused on testing reflection helper since it is rather complex. some asserts also added.

org.springframework.expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java

@@ -95,7 +95,7 @@ public class FunctionReference extends SpelNodeImpl {
 		// Convert arguments if necessary and remap them for varargs if required
 		if (functionArgs != null) {
 			TypeConverter converter = state.getEvaluationContext().getTypeConverter();
-			ReflectionHelper.convertArguments(m.getParameterTypes(), m.isVarArgs(), converter, functionArgs);
+			ReflectionHelper.convertAllArguments(m.getParameterTypes(), m.isVarArgs(), converter, functionArgs);
 		}
 		if (m.isVarArgs()) {
 			functionArgs = ReflectionHelper.setupArgumentsForVarargsInvocation(m.getParameterTypes(), functionArgs);

org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java

@@ -24,6 +24,7 @@ import org.springframework.expression.EvaluationException;
 import org.springframework.expression.TypeConverter;
 import org.springframework.expression.spel.SpelException;
 import org.springframework.expression.spel.SpelMessages;
+import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
 
 /**
@@ -45,9 +46,11 @@ public class ReflectionHelper {
 	 * @param typeConverter a registered type converter
 	 * @return a MatchInfo object indicating what kind of match it was or null if it was not a match
 	 */
-	public static ArgumentsMatchInfo compareArguments(
+	public static ArgumentsMatchInfo compareArguments( 
 			Class[] expectedArgTypes, Class[] suppliedArgTypes, TypeConverter typeConverter) {
 
+		Assert.isTrue(expectedArgTypes.length==suppliedArgTypes.length, "Expected argument types and supplied argument types should be arrays of same length");
+
 		ArgsMatchKind match = ArgsMatchKind.EXACT;
 		List<Integer> argsRequiringConversion = null;
 		for (int i = 0; i < expectedArgTypes.length && match != null; i++) {
@@ -94,9 +97,12 @@ public class ReflectionHelper {
 	 * @param typeConverter a registered type converter 
 	 * @return a MatchInfo object indicating what kind of match it was or null if it was not a match
 	 */
-	static ArgumentsMatchInfo compareArgumentsVarargs(
+	public static ArgumentsMatchInfo compareArgumentsVarargs(
 			Class[] expectedArgTypes, Class[] suppliedArgTypes, TypeConverter typeConverter) {
  
+		Assert.isTrue(expectedArgTypes!=null && expectedArgTypes.length>0, "Expected arguments must at least include one array (the vargargs parameter)");
+		Assert.isTrue(expectedArgTypes[expectedArgTypes.length-1].isArray(), "Final expected argument should be array type (the varargs parameter)");
+		
 		ArgsMatchKind match = ArgsMatchKind.EXACT;
 		List<Integer> argsRequiringConversion = null;
 
@@ -145,8 +151,7 @@ public class ReflectionHelper {
 						if (match != ArgsMatchKind.REQUIRES_CONVERSION) {
 							match = ArgsMatchKind.CLOSE;
 						}
-					}
-					else if (typeConverter.canConvert(suppliedArg, varargsParameterType)) {
+					} else if (typeConverter.canConvert(suppliedArg, varargsParameterType)) {
 						if (argsRequiringConversion == null) {
 							argsRequiringConversion = new ArrayList<Integer>();
 						}
@@ -177,34 +182,60 @@ public class ReflectionHelper {
 		}
 	}
 
-	public static void convertArguments(Class[] parameterTypes, boolean isVarargs, TypeConverter converter,
-			int[] argsRequiringConversion, Object... arguments) throws EvaluationException {
+	/**
+	 * Takes an input set of argument values and, following the positions specified in the int array, it converts
+	 * them to the types specified as the required parameter types.  The arguments are converted 'in-place' in the
+	 * input array.
+	 * @param requiredParameterTypes the types that the caller would like to have
+	 * @param isVarargs whether the requiredParameterTypes is a varargs list
+	 * @param converter the type converter to use for attempting conversions
+	 * @param argumentsRequiringConversion details which of the input arguments need conversion
+	 * @param arguments the actual arguments that need conversion
+	 * @throws EvaluationException if a problem occurs during conversion
+	 */
+	public static void convertArguments(Class[] requiredParameterTypes, boolean isVarargs, TypeConverter converter,
+			int[] argumentsRequiringConversion, Object[] arguments) throws EvaluationException {
+		 
+		Assert.notNull(argumentsRequiringConversion,"should not be called if no conversions required");
+		Assert.notNull(arguments,"should not be called if no conversions required");
+		
 		Class varargsType = null;
 		if (isVarargs) {
-			varargsType = parameterTypes[parameterTypes.length - 1].getComponentType();
+			Assert.isTrue(requiredParameterTypes[requiredParameterTypes.length-1].isArray(),"if varargs then last parameter type must be array");
+			varargsType = requiredParameterTypes[requiredParameterTypes.length - 1].getComponentType();
 		}
-		for (Integer argPosition : argsRequiringConversion) {
+		for (Integer argPosition : argumentsRequiringConversion) {
 			Class<?> targetType = null;
-			if (isVarargs && argPosition >= (parameterTypes.length - 1)) {
+			if (isVarargs && argPosition >= (requiredParameterTypes.length - 1)) {
 				targetType = varargsType;
+			} else {
+				targetType = requiredParameterTypes[argPosition];
 			}
-			else {
-				targetType = parameterTypes[argPosition];
-			}
-			// try {
 			arguments[argPosition] = converter.convertValue(arguments[argPosition], targetType);
-			// } catch (EvaluationException e) {
-			// throw new SpelException(e, SpelMessages.PROBLEM_DURING_TYPE_CONVERSION, "Converter failed to convert '"
-			// + arguments[argPosition] + " to type '" + targetType + "'");
-			// }
 		}
 	}
 
-	public static void convertArguments(Class[] parameterTypes, boolean isVarargs, TypeConverter converter,
-			Object... arguments) throws EvaluationException {
+	/**
+	 * Convert a supplied set of arguments into the requested types.  If the parameterTypes are related to 
+	 * a varargs method then the final entry in the parameterTypes array is going to be an array itself whose
+	 * component type should be used as the conversion target for extraneous arguments. (For example, if the
+	 * parameterTypes are {Integer, String[]} and the input arguments are {Integer, boolean, float} then both
+	 * the boolean and float must be converted to strings).  This method does not repackage the arguments
+	 * into a form suitable for the varargs invocation
+	 * @param parameterTypes the types to be converted to
+	 * @param isVarargs whether parameterTypes relates to a varargs method
+	 * @param converter the converter to use for type conversions
+	 * @param arguments the arguments to convert to the requested parameter types
+	 * @throws SpelException if there is a problem with conversion
+	 */
+	public static void convertAllArguments(Class[] parameterTypes, boolean isVarargs, TypeConverter converter,
+			Object[] arguments) throws SpelException {
 
+		Assert.notNull(arguments,"should not be called if nothing to convert");
+		
 		Class varargsType = null;
 		if (isVarargs) {
+			Assert.isTrue(parameterTypes[parameterTypes.length-1].isArray(),"if varargs then last parameter type must be array");
 			varargsType = parameterTypes[parameterTypes.length - 1].getComponentType();
 		}
 		for (int i = 0; i < arguments.length; i++) {
@@ -214,23 +245,19 @@ public class ReflectionHelper {
 			} else {
 				targetType = parameterTypes[i];
 			}
-			if (converter == null) {
-				throw new SpelException(SpelMessages.PROBLEM_DURING_TYPE_CONVERSION,
-						"No converter available to convert '" + arguments[i] + " to type '" + targetType + "'");
-			}
 			try {
 				if (arguments[i] != null && arguments[i].getClass() != targetType) {
+					if (converter == null) {
+						throw new SpelException(SpelMessages.TYPE_CONVERSION_ERROR, arguments[i].getClass().getName(),targetType);
+					}
 					arguments[i] = converter.convertValue(arguments[i], targetType);
 				}
-			}
-			catch (EvaluationException ex) {
+			} catch (EvaluationException ex) {
 				// allows for another type converter throwing a different kind of EvaluationException
 				if (ex instanceof SpelException) {
-					throw ex;
-				}
-				else {
-					throw new SpelException(ex, SpelMessages.PROBLEM_DURING_TYPE_CONVERSION,
-							"Converter failed to convert '" + arguments[i].getClass().getName() + "' to type '" + targetType + "'");
+					throw (SpelException)ex;
+				} else {
+					throw new SpelException(ex, SpelMessages.TYPE_CONVERSION_ERROR,arguments[i].getClass().getName(),targetType);
 				}
 			}
 		}
@@ -241,30 +268,30 @@ public class ReflectionHelper {
 	 * parameterTypes is (int, String[]) because the second parameter was declared String... then if arguments is
 	 * [1,"a","b"] then it must be repackaged as [1,new String[]{"a","b"}] in order to match the expected
 	 * parameterTypes.
-	 * @param paramTypes the types of the parameters for the invocation
+	 * @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
 	 */
-	public static Object[] setupArgumentsForVarargsInvocation(Class[] paramTypes, Object... args) {
+	public static Object[] setupArgumentsForVarargsInvocation(Class[] requiredParameterTypes, Object... args) {
 		// Check if array already built for final argument
-		int nParams = paramTypes.length;
-		int nArgs = args.length;
+		int parameterCount = requiredParameterTypes.length;
+		int argumentCount = args.length;
 
 		// Check if repackaging is needed:
-		if (nParams != args.length || paramTypes[nParams - 1] != (args[nArgs - 1] == null ? null : args[nArgs - 1].getClass())) {
+		if (parameterCount != args.length || requiredParameterTypes[parameterCount - 1] != (args[argumentCount - 1] == null ? null : args[argumentCount - 1].getClass())) {
 			int arraySize = 0; // zero size array if nothing to pass as the varargs parameter
-			if (nArgs >= nParams) {
-				arraySize = nArgs - (nParams - 1);
+			if (argumentCount >= parameterCount) {
+				arraySize = argumentCount - (parameterCount - 1);
 			}
-			Object[] repackagedArguments = (Object[]) Array.newInstance(paramTypes[nParams - 1].getComponentType(),
+			Object[] repackagedArguments = (Object[]) Array.newInstance(requiredParameterTypes[parameterCount - 1].getComponentType(),
 					arraySize);
 
 			// Copy all but the varargs arguments
 			for (int i = 0; i < arraySize; i++) {
-				repackagedArguments[i] = args[nParams + i - 1];
+				repackagedArguments[i] = args[parameterCount + i - 1];
 			}
 			// Create an array for the varargs arguments
-			Object[] newArgs = new Object[nParams];
+			Object[] newArgs = new Object[parameterCount];
 			for (int i = 0; i < newArgs.length - 1; i++) {
 				newArgs[i] = args[i];
 			}
@@ -276,7 +303,12 @@ public class ReflectionHelper {
 
 
 	public static enum ArgsMatchKind {
-		EXACT, CLOSE, REQUIRES_CONVERSION
+		// An exact match is where the parameter types exactly match what the method/constructor being invoked is expecting
+		EXACT, 
+		// A close match is where the parameter types either exactly match or are assignment compatible with the method/constructor being invoked
+		CLOSE, 
+		// A conversion match is where the type converter must be used to transform some of the parameter types
+		REQUIRES_CONVERSION
 	}
 
 

org.springframework.expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java

@@ -62,8 +62,7 @@ public class ReflectiveMethodResolver implements MethodResolver {
 					if (method.isVarArgs() && argumentTypes.length >= (method.getParameterTypes().length - 1)) {
 						// *sigh* complicated
 						matchInfo = ReflectionHelper.compareArgumentsVarargs(method.getParameterTypes(), argumentTypes, typeConverter);
-					}
-					else if (method.getParameterTypes().length == argumentTypes.length) {
+					} else if (method.getParameterTypes().length == argumentTypes.length) {
 						// name and parameter number match, check the arguments
 						matchInfo = ReflectionHelper.compareArguments(method.getParameterTypes(), argumentTypes, typeConverter);
 					}

org.springframework.expression/src/test/java/org/springframework/expression/spel/ExpressionStateTests.java

@@ -115,6 +115,10 @@ public class ExpressionStateTests extends ExpressionTestCase {
 		
 		state = new ExpressionState(new StandardEvaluationContext());
 		assertEquals(TypedValue.NULL_TYPED_VALUE,state.getRootContextObject());
+		
+
+		((StandardEvaluationContext)state.getEvaluationContext()).setRootObject(null,TypeDescriptor.NULL_TYPE_DESCRIPTOR);
+		assertEquals(null,state.getRootContextObject().getValue());
 	}
 	
 	public void testActiveContextObject() {

org.springframework.expression/src/test/java/org/springframework/expression/spel/HelperTests.java

@@ -20,6 +20,7 @@ import java.io.PrintStream;
 import java.util.ArrayList;
 import java.util.List;
 
+import org.springframework.expression.EvaluationException;
 import org.springframework.expression.ParseException;
 import org.springframework.expression.TypedValue;
 import org.springframework.expression.spel.ast.FormatHelper;
@@ -94,13 +95,6 @@ public class HelperTests extends ExpressionTestCase {
 		checkMatch(new Class[]{String.class,Integer.class},new Class[]{String.class,Integer.class},typeConverter,ArgsMatchKind.EXACT);
 	}
 	
-	public void testReflectionHelperCompareArguments_Varargs_ExactMatching() {
-		StandardTypeConverter tc = new StandardTypeConverter();
-		
-		// Calling foo(String) with (String) is exact match
-		checkMatch(new Class[]{String.class},new Class[]{String.class},tc,ArgsMatchKind.EXACT);
-	}
-	
 	public void testReflectionHelperCompareArguments_CloseMatching() {
 		StandardTypeConverter typeConverter = new StandardTypeConverter();
 		
@@ -136,6 +130,131 @@ public class HelperTests extends ExpressionTestCase {
 		// Passing (Super,String) on call to foo(Sub,String) is not a match
 		checkMatch(new Class[]{Super.class,String.class},new Class[]{Sub.class,String.class},typeConverter,null);
 	}
+
+	public void testReflectionHelperCompareArguments_Varargs_ExactMatching() {
+		StandardTypeConverter tc = new StandardTypeConverter();
+		Class<?> stringArrayClass = new String[0].getClass();
+		Class<?> integerArrayClass = new Integer[0].getClass();
+				
+		// Passing (String[]) on call to (String[]) is exact match
+		checkMatch2(new Class[]{stringArrayClass},new Class[]{stringArrayClass},tc,ArgsMatchKind.EXACT);
+		
+		// Passing (Integer, String[]) on call to (Integer, String[]) is exact match
+		checkMatch2(new Class[]{Integer.class,stringArrayClass},new Class[]{Integer.class,stringArrayClass},tc,ArgsMatchKind.EXACT);
+
+		// Passing (String, Integer, String[]) on call to (String, String, String[]) is exact match
+		checkMatch2(new Class[]{String.class,Integer.class,stringArrayClass},new Class[]{String.class,Integer.class,stringArrayClass},tc,ArgsMatchKind.EXACT);
+		
+		// Passing (Sub, String[]) on call to (Super, String[]) is exact match
+		checkMatch2(new Class[]{Sub.class,stringArrayClass},new Class[]{Super.class,stringArrayClass},tc,ArgsMatchKind.CLOSE);
+
+		// Passing (Integer, String[]) on call to (String, String[]) is exact match
+		checkMatch2(new Class[]{Integer.class,stringArrayClass},new Class[]{String.class,stringArrayClass},tc,ArgsMatchKind.REQUIRES_CONVERSION,0);
+
+		// Passing (Integer, Sub, String[]) on call to (String, Super, String[]) is exact match
+		checkMatch2(new Class[]{Integer.class,Sub.class,String[].class},new Class[]{String.class,Super.class,String[].class},tc,ArgsMatchKind.REQUIRES_CONVERSION,0);
+		
+		// Passing (String) on call to (String[]) is exact match
+		checkMatch2(new Class[]{String.class},new Class[]{stringArrayClass},tc,ArgsMatchKind.EXACT);
+		
+		// Passing (Integer,String) on call to (Integer,String[]) is exact match
+		checkMatch2(new Class[]{Integer.class,String.class},new Class[]{Integer.class,stringArrayClass},tc,ArgsMatchKind.EXACT);
+
+		// Passing (String) on call to (Integer[]) is conversion match (String to Integer)
+		checkMatch2(new Class[]{String.class},new Class[]{integerArrayClass},tc,ArgsMatchKind.REQUIRES_CONVERSION,0);
+
+		// Passing (Sub) on call to (Super[]) is close match
+		checkMatch2(new Class[]{Sub.class},new Class[]{new Super[0].getClass()},tc,ArgsMatchKind.CLOSE);
+		
+		// Passing (Super) on call to (Sub[]) is not a match
+		checkMatch2(new Class[]{Super.class},new Class[]{new Sub[0].getClass()},tc,null);
+
+		checkMatch2(new Class[]{Unconvertable.class,String.class},new Class[]{Sub.class,Super[].class},tc,null);
+
+		checkMatch2(new Class[]{Integer.class,Integer.class,String.class},new Class[]{String.class,String.class,Super[].class},tc,null);
+
+		checkMatch2(new Class[]{Unconvertable.class,String.class},new Class[]{Sub.class,Super[].class},tc,null);
+
+		checkMatch2(new Class[]{Integer.class,Integer.class,String.class},new Class[]{String.class,String.class,Super[].class},tc,null);
+
+		checkMatch2(new Class[]{Integer.class,Integer.class,Sub.class},new Class[]{String.class,String.class,Super[].class},tc,ArgsMatchKind.REQUIRES_CONVERSION,0,1);
+
+		checkMatch2(new Class[]{Integer.class,Integer.class,Integer.class},new Class[]{Integer.class,String[].class},tc,ArgsMatchKind.REQUIRES_CONVERSION,1,2);
+		// what happens on (Integer,String) passed to (Integer[]) ?
+	}
+	
+	public void testConvertArguments() throws Exception {
+		StandardTypeConverter tc = new StandardTypeConverter();
+
+		// basic conversion int>String
+		Object[] args = new Object[]{3};
+		ReflectionHelper.convertArguments(new Class[]{String.class}, false, tc, new int[]{0}, args);
+		checkArguments(args, "3");
+
+		// varargs but nothing to convert
+		args = new Object[]{3};
+		ReflectionHelper.convertArguments(new Class[]{String.class,String[].class}, false, tc, new int[]{0}, args);
+		checkArguments(args, "3");
+
+		// varargs with nothing needing conversion
+		args = new Object[]{3,"abc","abc"};
+		ReflectionHelper.convertArguments(new Class[]{String.class,String[].class}, true, tc, new int[]{0,1,2}, args);
+		checkArguments(args, "3","abc","abc");
+
+		// varargs with conversion required
+		args = new Object[]{3,false,3.0d};
+		ReflectionHelper.convertArguments(new Class[]{String.class,String[].class}, true, tc, new int[]{0,1,2}, args);
+		checkArguments(args, "3","false","3.0");
+}
+
+	public void testConvertArguments2() throws EvaluationException {
+		StandardTypeConverter tc = new StandardTypeConverter();
+
+		// Simple conversion: int to string
+		Object[] args = new Object[]{3};
+		ReflectionHelper.convertAllArguments(new Class[]{String.class}, false, tc, args);
+		checkArguments(args,"3");
+
+		// varargs conversion
+		args = new Object[]{3,false,3.0f};
+		ReflectionHelper.convertAllArguments(new Class[]{String.class,String[].class}, true, tc, args);
+		checkArguments(args,"3","false","3.0");
+
+		// varargs conversion but no varargs
+		args = new Object[]{3};
+		ReflectionHelper.convertAllArguments(new Class[]{String.class,String[].class}, true, tc, args);
+		checkArguments(args,"3");
+
+		// missing converter
+		args = new Object[]{3,false,3.0f};
+		try {
+			ReflectionHelper.convertAllArguments(new Class[]{String.class,String[].class}, true, null, args);
+			fail("Should have failed because no converter supplied");
+		} catch (SpelException se) {
+			assertEquals(SpelMessages.TYPE_CONVERSION_ERROR,se.getMessageUnformatted());
+		}
+		
+		// null value
+		args = new Object[]{3,null,3.0f};
+		ReflectionHelper.convertAllArguments(new Class[]{String.class,String[].class}, true, tc, args);
+		checkArguments(args,"3",null,"3.0");
+	}
+	
+	
+	public void testSetupArguments() {
+		Object[] newArray = ReflectionHelper.setupArgumentsForVarargsInvocation(new Class[]{new String[0].getClass()},"a","b","c");
+		
+		assertEquals(1,newArray.length);
+		Object firstParam = newArray[0];
+		assertEquals(String.class,firstParam.getClass().getComponentType());
+		Object[] firstParamArray = (Object[])firstParam;
+		assertEquals(3,firstParamArray.length);
+		assertEquals("a",firstParamArray[0]);
+		assertEquals("b",firstParamArray[1]);
+		assertEquals("c",firstParamArray[2]);
+	}
+
+	// test classes
 	
 	static class Super {
 	}
@@ -143,6 +262,8 @@ public class HelperTests extends ExpressionTestCase {
 	static class Sub extends Super {
 	}
 	
+	static class Unconvertable {}
+	
 	// ---
 	
 	/**
@@ -173,4 +294,44 @@ public class HelperTests extends ExpressionTestCase {
 			}
 		}
 	}
+
+	/**
+	 * Used to validate the match returned from a compareArguments call.
+	 */
+	private void checkMatch2(Class[] inputTypes, Class[] expectedTypes, StandardTypeConverter typeConverter,ArgsMatchKind expectedMatchKind,int... argsForConversion) {
+		ReflectionHelper.ArgumentsMatchInfo matchInfo = ReflectionHelper.compareArgumentsVarargs(expectedTypes, inputTypes, typeConverter);
+		if (expectedMatchKind==null) {
+			assertNull("Did not expect them to match in any way: "+matchInfo, matchInfo);
+		} else {
+			assertNotNull("Should not be a null match", matchInfo);
+		}
+
+		if (expectedMatchKind==ArgsMatchKind.EXACT) {
+			assertTrue(matchInfo.isExactMatch());
+			assertNull(matchInfo.argsRequiringConversion);		
+		} else if (expectedMatchKind==ArgsMatchKind.CLOSE) {
+			assertTrue(matchInfo.isCloseMatch());
+			assertNull(matchInfo.argsRequiringConversion);		
+		} else if (expectedMatchKind==ArgsMatchKind.REQUIRES_CONVERSION) {
+			assertTrue("expected to be a match requiring conversion, but was "+matchInfo,matchInfo.isMatchRequiringConversion());
+			if (argsForConversion==null) {
+				fail("there are arguments that need conversion");
+			}
+			assertEquals("The array of args that need conversion is different length to that expected",argsForConversion.length, matchInfo.argsRequiringConversion.length);
+			for (int a=0;a<argsForConversion.length;a++) {
+				assertEquals(argsForConversion[a],matchInfo.argsRequiringConversion[a]);
+			}
+		}
+	}
+
+	private void checkArguments(Object[] args, Object... expected) {
+		assertEquals(expected.length,args.length);
+		for (int i=0;i<expected.length;i++) {
+			checkArgument(expected[i],args[i]);
+		}
+	}
+	
+	private void checkArgument(Object expected, Object actual) {
+		assertEquals(expected,actual);
+	}
 }

org.springframework.expression/src/test/java/org/springframework/expression/spel/ScenariosForSpringSecurity.java

@@ -278,7 +278,7 @@ public class ScenariosForSpringSecurity extends ExpressionTestCase {
 					Method m = HasRoleExecutor.class.getMethod("hasRole", String[].class);
 					Object[] args = arguments;
 					if (args != null) {
-						ReflectionHelper.convertArguments(m.getParameterTypes(), m.isVarArgs(), tc, args);
+						ReflectionHelper.convertAllArguments(m.getParameterTypes(), m.isVarArgs(), tc, args);
 					}
 					if (m.isVarArgs()) {
 						args = ReflectionHelper.setupArgumentsForVarargsInvocation(m.getParameterTypes(), args);