From 71291e9af9935e20f65d682b3df294f6a0f2ecfc Mon Sep 17 00:00:00 2001 From: Take Weiland Date: Fri, 16 Mar 2018 00:25:36 +0100 Subject: [PATCH 1/2] Support Kotlin parameter default values in handler methods Allows using default parameter values for any method parameters resolved via AbstractNamedValueMethodArgumentResolver, similar to defaultValue parameter of e.g. @RequestParam annotation, but skipping the converter chain. Issues: SPR-16598 --- .../springframework/core/MethodParameter.java | 65 +++++++++++++++---- .../core/KotlinMethodParameterTests.kt | 17 +++++ ...tractNamedValueMethodArgumentResolver.java | 12 ++-- .../support/InvocableHandlerMethod.java | 65 +++++++++++++++++++ ...tParamMethodArgumentResolverKotlinTests.kt | 24 ++++++- 5 files changed, 163 insertions(+), 20 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/MethodParameter.java b/spring-core/src/main/java/org/springframework/core/MethodParameter.java index ad3b190f629c..62f85adffa17 100644 --- a/spring-core/src/main/java/org/springframework/core/MethodParameter.java +++ b/spring-core/src/main/java/org/springframework/core/MethodParameter.java @@ -342,6 +342,14 @@ public boolean isOptional() { (KotlinDetector.isKotlinType(getContainingClass()) && KotlinDelegate.isOptional(this))); } + /** + * Return whether this parameter declares a language-level default value, + * such as a default parameter in Kotlin. + */ + public boolean hasDefaultValue() { + return KotlinDetector.isKotlinType(getContainingClass()) && KotlinDelegate.hasDefaultValue(this); + } + /** * Check whether this method parameter is annotated with any variant of a * {@code Nullable} annotation, e.g. {@code javax.annotation.Nullable} or @@ -745,6 +753,28 @@ private static int validateIndex(Executable executable, int parameterIndex) { */ private static class KotlinDelegate { + private static KParameter getKotlinParameter(KFunction function, int index) { + List parameters = function.getParameters(); + return parameters + .stream() + .filter(p -> KParameter.Kind.VALUE.equals(p.getKind())) + .collect(Collectors.toList()) + .get(index); + } + + @Nullable + private static KFunction getKotlinFunction(@Nullable Method method, @Nullable Constructor ctor) { + if (method != null) { + return ReflectJvmMapping.getKotlinFunction(method); + } + else if (ctor != null) { + return ReflectJvmMapping.getKotlinFunction(ctor); + } + else { + return null; + } + } + /** * Check whether the specified {@link MethodParameter} represents a nullable Kotlin type * or an optional parameter (with a default value in the Kotlin declaration). @@ -758,25 +788,34 @@ public static boolean isOptional(MethodParameter param) { return (function != null && function.getReturnType().isMarkedNullable()); } else { - KFunction function = null; - if (method != null) { - function = ReflectJvmMapping.getKotlinFunction(method); - } - else if (ctor != null) { - function = ReflectJvmMapping.getKotlinFunction(ctor); - } + KFunction function = getKotlinFunction(method, ctor); if (function != null) { - List parameters = function.getParameters(); - KParameter parameter = parameters - .stream() - .filter(p -> KParameter.Kind.VALUE.equals(p.getKind())) - .collect(Collectors.toList()) - .get(index); + KParameter parameter = getKotlinParameter(function, index); return (parameter.getType().isMarkedNullable() || parameter.isOptional()); } } return false; } + + /** + * Check whether the specified {@link MethodParameter} has a default value in it's Kotlin declaration. + */ + public static boolean hasDefaultValue(MethodParameter param) { + int index = param.getParameterIndex(); + if (index == -1) { // default value does not make sense for return types + return false; + } + else { + KFunction function = getKotlinFunction(param.getMethod(), param.getConstructor()); + if (function != null) { + KParameter parameter = getKotlinParameter(function, index); + return parameter.isOptional(); + } + else { + return false; + } + } + } } } diff --git a/spring-core/src/test/kotlin/org/springframework/core/KotlinMethodParameterTests.kt b/spring-core/src/test/kotlin/org/springframework/core/KotlinMethodParameterTests.kt index 2157099a4ce8..c76bac028218 100644 --- a/spring-core/src/test/kotlin/org/springframework/core/KotlinMethodParameterTests.kt +++ b/spring-core/src/test/kotlin/org/springframework/core/KotlinMethodParameterTests.kt @@ -36,12 +36,18 @@ class KotlinMethodParameterTests { lateinit var nonNullableMethod: Method + lateinit var withDefaultParameterMethod: Method + + lateinit var withoutDefaultParameterMethod: Method + @Before @Throws(NoSuchMethodException::class) fun setup() { nullableMethod = javaClass.getMethod("nullable", String::class.java) nonNullableMethod = javaClass.getMethod("nonNullable", String::class.java) + withDefaultParameterMethod = javaClass.getMethod("withDefaultParameter", String::class.java) + withoutDefaultParameterMethod = javaClass.getMethod("withoutDefaultParameter", String::class.java) } @@ -57,6 +63,11 @@ class KotlinMethodParameterTests { assertFalse(MethodParameter(nonNullableMethod, -1).isOptional()) } + @Test + fun `Method parameter default value`() { + assertTrue(MethodParameter(withDefaultParameterMethod, 0).hasDefaultValue()) + assertFalse(MethodParameter(withoutDefaultParameterMethod, 0).hasDefaultValue()) + } @Suppress("unused", "unused_parameter") fun nullable(p1: String?): Int? = 42 @@ -64,4 +75,10 @@ class KotlinMethodParameterTests { @Suppress("unused", "unused_parameter") fun nonNullable(p1: String): Int = 42 + @Suppress("unused", "unused_parameter") + fun withDefaultParameter(p1: String = "42") = 42 + + @Suppress("unused", "unused_parameter") + fun withoutDefaultParameter(p1: String) = 42 + } diff --git a/spring-web/src/main/java/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java index 8c2599728689..349d4be61bb6 100644 --- a/spring-web/src/main/java/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java @@ -111,13 +111,15 @@ public final Object resolveArgument(MethodParameter parameter, @Nullable ModelAn else if (namedValueInfo.required && !nestedParameter.isOptional()) { handleMissingValue(namedValueInfo.name, nestedParameter, webRequest); } - arg = handleNullValue(namedValueInfo.name, arg, nestedParameter.getNestedParameterType()); + arg = handleNullValue(namedValueInfo.name, arg, nestedParameter); } else if ("".equals(arg) && namedValueInfo.defaultValue != null) { arg = resolveStringValue(namedValueInfo.defaultValue); } - if (binderFactory != null) { + // if we have a resolved arg here (not null), we must convert in any case + // if we do not have a resolved arg here (null), only convert if we do not have a default value + if (binderFactory != null && (arg != null || !nestedParameter.hasDefaultValue())) { WebDataBinder binder = binderFactory.createBinder(webRequest, null, namedValueInfo.name); try { arg = binder.convertIfNecessary(arg, parameter.getParameterType(), parameter); @@ -235,12 +237,14 @@ protected void handleMissingValue(String name, MethodParameter parameter) throws * A {@code null} results in a {@code false} value for {@code boolean}s or an exception for other primitives. */ @Nullable - private Object handleNullValue(String name, @Nullable Object value, Class paramType) { + private Object handleNullValue(String name, @Nullable Object value, MethodParameter param) { + Class paramType = param.getNestedParameterType(); if (value == null) { if (Boolean.TYPE.equals(paramType)) { return Boolean.FALSE; } - else if (paramType.isPrimitive()) { + // primitive parameters may only be null if they have a default value + else if (paramType.isPrimitive() && !param.hasDefaultValue()) { throw new IllegalStateException("Optional " + paramType.getSimpleName() + " parameter '" + name + "' is present but cannot be translated into a null value due to being declared as a " + "primitive type. Consider declaring it as object wrapper for the corresponding primitive type."); diff --git a/spring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java b/spring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java index 694c51d47bf9..c851c0fddb11 100644 --- a/spring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java +++ b/spring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java @@ -19,8 +19,15 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; + +import kotlin.reflect.KFunction; +import kotlin.reflect.KParameter; +import kotlin.reflect.jvm.ReflectJvmMapping; import org.springframework.core.DefaultParameterNameDiscoverer; +import org.springframework.core.KotlinDetector; import org.springframework.core.MethodParameter; import org.springframework.core.ParameterNameDiscoverer; import org.springframework.lang.Nullable; @@ -55,6 +62,11 @@ public class InvocableHandlerMethod extends HandlerMethod { private ParameterNameDiscoverer parameterNameDiscoverer = new DefaultParameterNameDiscoverer(); + private static final Object KOTLIN_NOT_CHECKED = new Object(); + + // has type Object to avoid hard dependency on Kotlin, only ever holds KFunction instances + @Nullable + private Object kotlinFunction = KOTLIN_NOT_CHECKED; /** * Create an instance from a {@code HandlerMethod}. @@ -205,7 +217,13 @@ private Object resolveProvidedArgument(MethodParameter parameter, @Nullable Obje */ protected Object doInvoke(Object... args) throws Exception { ReflectionUtils.makeAccessible(getBridgedMethod()); + if (kotlinFunction == KOTLIN_NOT_CHECKED) { + initKotlinFunction(); + } try { + if (kotlinFunction != null) { + return KotlinDelegate.doKotlinCall(this, kotlinFunction, args); + } return getBridgedMethod().invoke(getBean(), args); } catch (IllegalArgumentException ex) { @@ -232,6 +250,15 @@ else if (targetException instanceof Exception) { } } + private void initKotlinFunction() { + if (KotlinDetector.isKotlinPresent() && KotlinDetector.isKotlinType(getBeanType())) { + kotlinFunction = KotlinDelegate.initKotlinCache(this); + } + else { + kotlinFunction = null; + } + } + /** * Assert that the target bean class is an instance of the class where the given * method is declared. In some cases the actual controller instance at request- @@ -279,4 +306,42 @@ protected String getDetailedErrorMessage(String text) { return sb.toString(); } + private static class KotlinDelegate { + + @Nullable + public static Object initKotlinCache(InvocableHandlerMethod method) { + KFunction function = ReflectJvmMapping.getKotlinFunction(method.getBridgedMethod()); + // we only need to do the call via Kotlin reflection if we have at least one optional parameter + if (function != null && function.getParameters().stream().anyMatch(KParameter::isOptional)) { + return function; + } + else { + return null; + } + } + + public static Object doKotlinCall(InvocableHandlerMethod method, Object kotlinFunction, Object[] args) { + KFunction function = (KFunction) kotlinFunction; + Map argMap = new HashMap<>(); + int index = 0; + for (KParameter parameter : function.getParameters()) { + switch (parameter.getKind()) { + case INSTANCE: + argMap.put(parameter, method.getBean()); + break; + case VALUE: + if (!parameter.isOptional() || args[index] != null) { + argMap.put(parameter, args[index]); + } + index++; + break; + case EXTENSION_RECEIVER: + default: + throw new UnsupportedOperationException("Unsupported Kotlin function parameter type: " + parameter.getKind()); + } + } + return function.callBy(argMap); + } + } + } diff --git a/spring-web/src/test/kotlin/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverKotlinTests.kt b/spring-web/src/test/kotlin/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverKotlinTests.kt index bbefd748cb18..8f2d0d12e294 100644 --- a/spring-web/src/test/kotlin/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverKotlinTests.kt +++ b/spring-web/src/test/kotlin/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverKotlinTests.kt @@ -62,6 +62,8 @@ class RequestParamMethodArgumentResolverKotlinTests { lateinit var nonNullableMultipartParamRequired: MethodParameter lateinit var nonNullableMultipartParamNotRequired: MethodParameter + lateinit var defaultParam: MethodParameter + @Before fun setup() { @@ -75,7 +77,8 @@ class RequestParamMethodArgumentResolverKotlinTests { val method = ReflectionUtils.findMethod(javaClass, "handle", String::class.java, String::class.java, String::class.java, String::class.java, MultipartFile::class.java, MultipartFile::class.java, - MultipartFile::class.java, MultipartFile::class.java)!! + MultipartFile::class.java, MultipartFile::class.java, + String::class.java)!! nullableParamRequired = SynthesizingMethodParameter(method, 0) nullableParamNotRequired = SynthesizingMethodParameter(method, 1) @@ -86,6 +89,8 @@ class RequestParamMethodArgumentResolverKotlinTests { nullableMultipartParamNotRequired = SynthesizingMethodParameter(method, 5) nonNullableMultipartParamRequired = SynthesizingMethodParameter(method, 6) nonNullableMultipartParamNotRequired = SynthesizingMethodParameter(method, 7) + + defaultParam = SynthesizingMethodParameter(method, 8) } @Test @@ -138,6 +143,18 @@ class RequestParamMethodArgumentResolverKotlinTests { resolver.resolveArgument(nonNullableParamNotRequired, null, webRequest, binderFactory) as String } + @Test + fun resolveDefaultValueWithoutParameter() { + val result = resolver.resolveArgument(defaultParam, null, webRequest, binderFactory) + assertNull(result) + } + + @Test + fun resolveDefaultValueWithParameter() { + request.addParameter("name", "123") + val result = resolver.resolveArgument(defaultParam, null, webRequest, binderFactory) + assertEquals("123", result) + } @Test fun resolveNullableRequiredWithMultipartParameter() { @@ -215,7 +232,6 @@ class RequestParamMethodArgumentResolverKotlinTests { resolver.resolveArgument(nonNullableMultipartParamNotRequired, null, webRequest, binderFactory) as MultipartFile } - @Suppress("unused_parameter") fun handle( @RequestParam("name") nullableParamRequired: String?, @@ -226,7 +242,9 @@ class RequestParamMethodArgumentResolverKotlinTests { @RequestParam("mfile") nullableMultipartParamRequired: MultipartFile?, @RequestParam("mfile", required = false) nullableMultipartParamNotRequired: MultipartFile?, @RequestParam("mfile") nonNullableMultipartParamRequired: MultipartFile, - @RequestParam("mfile", required = false) nonNullableMultipartParamNotRequired: MultipartFile) { + @RequestParam("mfile", required = false) nonNullableMultipartParamNotRequired: MultipartFile, + + @RequestParam("name") paramDefaultValue: String = "42") { } } From a61f7b425b2d71304641ced2426cc7eb3151ae05 Mon Sep 17 00:00:00 2001 From: Take Weiland Date: Fri, 16 Mar 2018 10:53:35 +0100 Subject: [PATCH 2/2] Use equals instead of identity comparison --- .../web/method/support/InvocableHandlerMethod.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java b/spring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java index c851c0fddb11..08c95a436a9d 100644 --- a/spring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java +++ b/spring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java @@ -217,7 +217,7 @@ private Object resolveProvidedArgument(MethodParameter parameter, @Nullable Obje */ protected Object doInvoke(Object... args) throws Exception { ReflectionUtils.makeAccessible(getBridgedMethod()); - if (kotlinFunction == KOTLIN_NOT_CHECKED) { + if (KOTLIN_NOT_CHECKED.equals(kotlinFunction)) { initKotlinFunction(); } try {