From e7cc19537d8a0c1eda07e3d7db144faaef5e98fd Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 11 Mar 2015 18:58:39 +0100 Subject: [PATCH] ReflectiveMethodResolver reliably prefers direct matches over vararg methods Issue: SPR-12803 --- .../spel/support/ReflectionHelper.java | 76 +++++++++++-------- .../support/ReflectiveMethodResolver.java | 44 +++++++---- .../expression/spel/SpelReproTests.java | 23 +++++- 3 files changed, 95 insertions(+), 48 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 b6f2f7dcdb4..bb1fcec0c0d 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-2013 the original author or authors. + * Copyright 2002-2015 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. @@ -42,13 +42,14 @@ import org.springframework.util.MethodInvoker; public class ReflectionHelper { /** - * Compare argument arrays and return information about whether they match. A supplied - * type converter and conversionAllowed flag allow for matches to take into account - * that a type may be transformed into a different type by the converter. - * @param expectedArgTypes the array of types the method/constructor is expecting - * @param suppliedArgTypes the array of types that are being supplied at the point of invocation + * Compare argument arrays and return information about whether they match. + * A supplied type converter and conversionAllowed flag allow for matches to take + * into account that a type may be transformed into a different type by the converter. + * @param expectedArgTypes the types the method/constructor is expecting + * @param suppliedArgTypes the types that are being supplied at the point of invocation * @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 + * @return a MatchInfo object indicating what kind of match it was, + * or {@code null} if it was not a match */ static ArgumentsMatchInfo compareArguments( List expectedArgTypes, List suppliedArgTypes, TypeConverter typeConverter) { @@ -111,7 +112,7 @@ public class ReflectionHelper { int result = 0; for (int i = 0; i < paramTypes.size(); i++) { TypeDescriptor paramType = paramTypes.get(i); - TypeDescriptor argType = argTypes.get(i); + TypeDescriptor argType = (i < argTypes.size() ? argTypes.get(i) : null); if (argType == null) { if (paramType.isPrimitive()) { return Integer.MAX_VALUE; @@ -148,13 +149,15 @@ public class ReflectionHelper { } /** - * Compare argument arrays and return information about whether they match. A supplied type converter and - * conversionAllowed flag allow for matches to take into account that a type may be transformed into a different - * type by the converter. This variant of compareArguments also allows for a varargs match. - * @param expectedArgTypes the array of types the method/constructor is expecting - * @param suppliedArgTypes the array of types that are being supplied at the point of invocation + * Compare argument arrays and return information about whether they match. + * A supplied type converter and conversionAllowed flag allow for matches to + * take into account that a type may be transformed into a different type by the + * converter. This variant of compareArguments also allows for a varargs match. + * @param expectedArgTypes the types the method/constructor is expecting + * @param suppliedArgTypes the types that are being supplied at the point of invocation * @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 + * @return a MatchInfo object indicating what kind of match it was, + * or {@code null} if it was not a match */ static ArgumentsMatchInfo compareArgumentsVarargs( List expectedArgTypes, List suppliedArgTypes, TypeConverter typeConverter) { @@ -264,14 +267,15 @@ public class ReflectionHelper { } /** - * 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. + * Takes an input set of argument values and converts them to the types specified as the + * required parameter types. The arguments are converted 'in-place' in the input array. * @param converter the type converter to use for attempting conversions * @param arguments the actual arguments that need conversion * @param methodOrCtor the target Method or Constructor - * @param argumentsRequiringConversion details which of the input arguments for sure need conversion + * @param argumentsRequiringConversion details which of the input arguments need conversion * @param varargsPosition the known position of the varargs argument, if any + * ({@code null} if not varargs) + * @return {@code true} if some kind of conversion occurred on an argument * @throws EvaluationException if a problem occurs during conversion */ static void convertArguments(TypeConverter converter, Object[] arguments, Object methodOrCtor, @@ -285,6 +289,7 @@ public class ReflectionHelper { } } else { + // Convert everything up to the varargs position for (int i = 0; i < varargsPosition; i++) { TypeDescriptor targetType = new TypeDescriptor(MethodParameter.forMethodOrConstructor(methodOrCtor, i)); Object argument = arguments[i]; @@ -292,11 +297,13 @@ public class ReflectionHelper { } MethodParameter methodParam = MethodParameter.forMethodOrConstructor(methodOrCtor, varargsPosition); 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]; arguments[varargsPosition] = converter.convertValue(argument, TypeDescriptor.forObject(argument), targetType); } else { + // Convert remaining arguments to the varargs element type TypeDescriptor targetType = TypeDescriptor.nested(methodParam, 1); for (int i = varargsPosition; i < arguments.length; i++) { Object argument = arguments[i]; @@ -307,12 +314,14 @@ public class ReflectionHelper { } /** - * 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 + * 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 {@code {Integer, String[]}} and the input arguments + * are {@code {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 converter the converter to use for type conversions * @param arguments the arguments to convert to the requested parameter types * @param method the target Method @@ -359,10 +368,10 @@ public class ReflectionHelper { } /** - * Package up the arguments so that they correctly match what is expected in parameterTypes. For example, if - * 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. + * 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 + * 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 * @param args the arguments to be setup ready for the invocation * @return a repackaged array of arguments where any varargs setup has been done @@ -372,7 +381,7 @@ public class ReflectionHelper { int parameterCount = requiredParameterTypes.length; int argumentCount = args.length; - // Check if repackaging is needed: + // Check if repackaging is needed... if (parameterCount != args.length || requiredParameterTypes[parameterCount - 1] != (args[argumentCount - 1] != null ? args[argumentCount - 1].getClass() : null)) { @@ -421,10 +430,11 @@ public class ReflectionHelper { /** - * An instance of ArgumentsMatchInfo describes what kind of match was achieved between two sets of arguments - - * the set that a method/constructor is expecting and the set that are being supplied at the point of invocation. - * If the kind indicates that conversion is required for some of the arguments then the arguments that require - * conversion are listed in the argsRequiringConversion array. + * An instance of ArgumentsMatchInfo describes what kind of match was achieved + * between two sets of arguments - the set that a method/constructor is expecting + * and the set that are being supplied at the point of invocation. If the kind + * indicates that conversion is required for some of the arguments then the arguments + * that require conversion are listed in the argsRequiringConversion array. */ public static class ArgumentsMatchInfo { diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java index 5108c7265b2..4c30ab8e099 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2015 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. @@ -66,13 +66,14 @@ public class ReflectiveMethodResolver implements MethodResolver { } /** - * This constructors allows the ReflectiveMethodResolver to be configured such that it will - * use a distance computation to check which is the better of two close matches (when there - * are multiple matches). Using the distance computation is intended to ensure matches - * are more closely representative of what a Java compiler would do when taking into - * account boxing/unboxing and whether the method candidates are declared to handle a - * supertype of the type (of the argument) being passed in. - * @param useDistance true if distance computation should be used when calculating matches + * This constructor allows the ReflectiveMethodResolver to be configured such that it + * will use a distance computation to check which is the better of two close matches + * (when there are multiple matches). Using the distance computation is intended to + * ensure matches are more closely representative of what a Java compiler would do + * when taking into account boxing/unboxing and whether the method candidates are + * declared to handle a supertype of the type (of the argument) being passed in. + * @param useDistance {@code true} if distance computation should be used when + * calculating matches; {@code false} otherwise */ public ReflectiveMethodResolver(boolean useDistance) { this.useDistance = useDistance; @@ -122,7 +123,19 @@ public class ReflectiveMethodResolver implements MethodResolver { public int compare(Method m1, Method m2) { int m1pl = m1.getParameterTypes().length; int m2pl = m2.getParameterTypes().length; - return (new Integer(m1pl)).compareTo(m2pl); + // varargs methods go last + if (m1pl == m2pl) { + if (!m1.isVarArgs() && m2.isVarArgs()) { + return -1; + } + else if (m1.isVarArgs() && !m2.isVarArgs()) { + return 1; + } + else { + return 0; + } + } + return (m1pl < m2pl ? -1 : (m1pl > m2pl ? 1 : 0)); } }); } @@ -162,14 +175,17 @@ public class ReflectiveMethodResolver implements MethodResolver { return new ReflectiveMethodExecutor(method, null); } else if (matchInfo.isCloseMatch()) { - if (!this.useDistance) { - closeMatch = method; - } - else { + if (this.useDistance) { int matchDistance = ReflectionHelper.getTypeDifferenceWeight(paramDescriptors, argumentTypes); - if (matchDistance < closeMatchDistance) { + if (closeMatch == null || matchDistance < closeMatchDistance) { // This is a better match... + closeMatch = method; closeMatchDistance = matchDistance; + } + } + else { + // Take this as a close match if there isn't one already + if (closeMatch == null) { closeMatch = method; } } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java index 418ab3155d8..5b9351839b5 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2015 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. @@ -1843,6 +1843,15 @@ public class SpelReproTests extends ExpressionTestCase { assertEquals(NamedUser.class.getName(), expression.getValue(new NamedUser())); } + @Test + public void SPR12803() throws Exception { + StandardEvaluationContext sec = new StandardEvaluationContext(); + sec.setVariable("iterable", Collections.emptyList()); + SpelExpressionParser parser = new SpelExpressionParser(); + Expression expression = parser.parseExpression("T(org.springframework.expression.spel.SpelReproTests.GuavaLists).newArrayList(#iterable)"); + assertTrue(expression.getValue(sec) instanceof ArrayList); + } + private static enum ABC { A, B, C } @@ -1996,4 +2005,16 @@ public class SpelReproTests extends ExpressionTestCase { } } + + public static class GuavaLists { + + public static List newArrayList(Iterable iterable) { + return new ArrayList(); + } + + public static List newArrayList(Object... elements) { + throw new UnsupportedOperationException(); + } + } + }