Move Kotlin value class unboxing to InvocableHandlerMethod

Before this commit, in Spring Framework 6.2, Kotlin value class unboxing was done at CoroutinesUtils level, which is a good fit for InvocableHandlerMethod use case, but not for other ones like AopUtils. This commit moves such unboxing to InvocableHandlerMethod in order to keep the HTTP response body support while fixing other regressions. Closes gh-33943
2024-11-27 16:27:37 +01:00
parent ea3bd7ae0c
commit 1aede291bb
6 changed files with 320 additions and 62 deletions
--- 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
@@ -30,6 +30,8 @@ import kotlin.reflect.KType;
 import kotlin.reflect.full.KClasses;
 import kotlin.reflect.jvm.KCallablesJvm;
 import kotlin.reflect.jvm.ReflectJvmMapping;
+import reactor.core.publisher.Mono;
+import reactor.core.publisher.SynchronousSink;

 import org.springframework.context.MessageSource;
 import org.springframework.core.CoroutinesUtils;
@@ -288,7 +290,8 @@ public class InvocableHandlerMethod extends HandlerMethod {
 	 * @since 6.0
 	 */
 	protected Object invokeSuspendingFunction(Method method, Object target, Object[] args) {
-		return CoroutinesUtils.invokeSuspendingFunction(method, target, args);
+		Object result = CoroutinesUtils.invokeSuspendingFunction(method, target, args);
+		return (result instanceof Mono<?> mono ? mono.handle(KotlinDelegate::handleResult) : result);
 	}


@@ -298,7 +301,7 @@ public class InvocableHandlerMethod extends HandlerMethod {
 	private static class KotlinDelegate {

 		@Nullable
-		@SuppressWarnings({"deprecation", "DataFlowIssue"})
+		@SuppressWarnings("DataFlowIssue")
 		public static Object invokeFunction(Method method, Object target, Object[] args) throws InvocationTargetException, IllegalAccessException, NoSuchMethodException {
 			KFunction<?> function = ReflectJvmMapping.getKotlinFunction(method);
 			// For property accessors
@@ -333,10 +336,33 @@ public class InvocableHandlerMethod extends HandlerMethod {
 			}
 			Object result = function.callBy(argMap);
 			if (result != null && KotlinDetector.isInlineClass(result.getClass())) {
-				return result.getClass().getDeclaredMethod("unbox-impl").invoke(result);
+				result = unbox(result);
 			}
 			return (result == Unit.INSTANCE ? null : result);
 		}
+
+		private static void handleResult(Object result, SynchronousSink<Object> sink) {
+			if (KotlinDetector.isInlineClass(result.getClass())) {
+				try {
+					Object unboxed = unbox(result);
+					if (unboxed != Unit.INSTANCE) {
+						sink.next(unboxed);
+					}
+					sink.complete();
+				}
+				catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException ex) {
+					sink.error(ex);
+				}
+			}
+			else {
+				sink.next(result);
+				sink.complete();
+			}
+		}
+
+		private static Object unbox(Object result) throws InvocationTargetException, IllegalAccessException, NoSuchMethodException {
+			return result.getClass().getDeclaredMethod("unbox-impl").invoke(result);
+		}
 	}

 }
--- a/spring-web/src/test/kotlin/org/springframework/web/method/support/InvocableHandlerMethodKotlinTests.kt
+++ b/spring-web/src/test/kotlin/org/springframework/web/method/support/InvocableHandlerMethodKotlinTests.kt
@@ -16,14 +16,18 @@

 package org.springframework.web.method.support

+import kotlinx.coroutines.delay
 import org.assertj.core.api.Assertions
 import org.junit.jupiter.api.Test
+import org.springframework.core.MethodParameter
 import org.springframework.util.ReflectionUtils
+import org.springframework.web.bind.support.WebDataBinderFactory
 import org.springframework.web.context.request.NativeWebRequest
 import org.springframework.web.context.request.ServletWebRequest
-import org.springframework.web.testfixture.method.ResolvableMethod
 import org.springframework.web.testfixture.servlet.MockHttpServletRequest
 import org.springframework.web.testfixture.servlet.MockHttpServletResponse
+import reactor.core.publisher.Mono
+import reactor.test.StepVerifier
 import java.lang.reflect.Method
 import kotlin.reflect.jvm.javaGetter
 import kotlin.reflect.jvm.javaMethod
@@ -33,6 +37,7 @@ import kotlin.reflect.jvm.javaMethod
 *
 * @author Sebastien Deleuze
 */
+@Suppress("UNCHECKED_CAST")
 class InvocableHandlerMethodKotlinTests {

 	private val request: NativeWebRequest = ServletWebRequest(MockHttpServletRequest(), MockHttpServletResponse())
@@ -110,6 +115,12 @@ class InvocableHandlerMethodKotlinTests {
 		Assertions.assertThat(value).isEqualTo("foo")
 	}

+	@Test
+	fun resultOfUnitReturnValue() {
+		val value = getInvocable(ValueClassHandler::resultOfUnitReturnValue.javaMethod!!).invokeForRequest(request, null)
+		Assertions.assertThat(value).isNull()
+	}
+
 	@Test
 	fun valueClassDefaultValue() {
 		composite.addResolver(StubArgumentResolver(Double::class.java))
@@ -138,6 +149,60 @@ class InvocableHandlerMethodKotlinTests {
 		Assertions.assertThat(value).isEqualTo('a')
 	}

+	@Test
+	fun suspendingValueClass() {
+		composite.addResolver(ContinuationHandlerMethodArgumentResolver())
+		composite.addResolver(StubArgumentResolver(Long::class.java, 1L))
+		val value = getInvocable(SuspendingValueClassHandler::longValueClass.javaMethod!!).invokeForRequest(request, null)
+		StepVerifier.create(value as Mono<Long>).expectNext(1L).verifyComplete()
+	}
+
+	@Test
+	fun suspendingValueClassReturnValue() {
+		composite.addResolver(ContinuationHandlerMethodArgumentResolver())
+		val value = getInvocable(SuspendingValueClassHandler::valueClassReturnValue.javaMethod!!).invokeForRequest(request, null)
+		StepVerifier.create(value as Mono<String>).expectNext("foo").verifyComplete()
+	}
+
+	@Test
+	fun suspendingResultOfUnitReturnValue() {
+		composite.addResolver(ContinuationHandlerMethodArgumentResolver())
+		val value = getInvocable(SuspendingValueClassHandler::resultOfUnitReturnValue.javaMethod!!).invokeForRequest(request, null)
+		StepVerifier.create(value as Mono<Unit>).verifyComplete()
+	}
+
+	@Test
+	fun suspendingValueClassDefaultValue() {
+		composite.addResolver(ContinuationHandlerMethodArgumentResolver())
+		composite.addResolver(StubArgumentResolver(Double::class.java))
+		val value = getInvocable(SuspendingValueClassHandler::doubleValueClass.javaMethod!!).invokeForRequest(request, null)
+		StepVerifier.create(value as Mono<Double>).expectNext(3.1).verifyComplete()
+	}
+
+	@Test
+	fun suspendingValueClassWithInit() {
+		composite.addResolver(ContinuationHandlerMethodArgumentResolver())
+		composite.addResolver(StubArgumentResolver(String::class.java, ""))
+		val value = getInvocable(SuspendingValueClassHandler::valueClassWithInit.javaMethod!!).invokeForRequest(request, null)
+		StepVerifier.create(value as Mono<String>).verifyError(IllegalArgumentException::class.java)
+	}
+
+	@Test
+	fun suspendingValueClassWithNullable() {
+		composite.addResolver(ContinuationHandlerMethodArgumentResolver())
+		composite.addResolver(StubArgumentResolver(LongValueClass::class.java, null))
+		val value = getInvocable(SuspendingValueClassHandler::valueClassWithNullable.javaMethod!!).invokeForRequest(request, null)
+		StepVerifier.create(value as Mono<Long>).verifyComplete()
+	}
+
+	@Test
+	fun suspendingValueClassWithPrivateConstructor() {
+		composite.addResolver(ContinuationHandlerMethodArgumentResolver())
+		composite.addResolver(StubArgumentResolver(Char::class.java, 'a'))
+		val value = getInvocable(SuspendingValueClassHandler::valueClassWithPrivateConstructor.javaMethod!!).invokeForRequest(request, null)
+		StepVerifier.create(value as Mono<Char>).expectNext('a').verifyComplete()
+	}
+
 	@Test
 	fun propertyAccessor() {
 		val value = getInvocable(PropertyAccessorHandler::prop.javaGetter!!).invokeForRequest(request, null)
@@ -206,23 +271,58 @@ class InvocableHandlerMethodKotlinTests {

 	private class ValueClassHandler {

-		fun valueClassReturnValue() =
-			StringValueClass("foo")
+		fun valueClassReturnValue() = StringValueClass("foo")

-		fun longValueClass(limit: LongValueClass) =
-			limit.value
+		fun resultOfUnitReturnValue() = Result.success(Unit)

-		fun doubleValueClass(limit: DoubleValueClass = DoubleValueClass(3.1)) =
-			limit.value
+		fun longValueClass(limit: LongValueClass) = limit.value

-		fun valueClassWithInit(valueClass: ValueClassWithInit) =
-			valueClass
+		fun doubleValueClass(limit: DoubleValueClass = DoubleValueClass(3.1)) = limit.value

-		fun valueClassWithNullable(limit: LongValueClass?) =
-			limit?.value
+		fun valueClassWithInit(valueClass: ValueClassWithInit) = valueClass

-		fun valueClassWithPrivateConstructor(limit: ValueClassWithPrivateConstructor) =
-			limit.value
+		fun valueClassWithNullable(limit: LongValueClass?) = limit?.value
+
+		fun valueClassWithPrivateConstructor(limit: ValueClassWithPrivateConstructor) = limit.value
+	}
+
+	private class SuspendingValueClassHandler {
+
+		suspend fun valueClassReturnValue(): StringValueClass {
+			delay(1)
+			return StringValueClass("foo")
+		}
+
+		suspend fun resultOfUnitReturnValue(): Result<Unit> {
+			delay(1)
+			return Result.success(Unit)
+		}
+
+		suspend fun longValueClass(limit: LongValueClass): Long {
+			delay(1)
+			return limit.value
+		}
+
+
+		suspend fun doubleValueClass(limit: DoubleValueClass = DoubleValueClass(3.1)): Double {
+			delay(1)
+			return limit.value
+		}
+
+		suspend fun valueClassWithInit(valueClass: ValueClassWithInit): ValueClassWithInit {
+			delay(1)
+			return valueClass
+		}
+
+		suspend fun valueClassWithNullable(limit: LongValueClass?): Long? {
+			delay(1)
+			return limit?.value
+		}
+
+		suspend fun valueClassWithPrivateConstructor(limit: ValueClassWithPrivateConstructor): Char {
+			delay(1)
+			return limit.value
+		}
 	}

 	private class PropertyAccessorHandler {
@@ -282,4 +382,19 @@ class InvocableHandlerMethodKotlinTests {

 	class CustomException(message: String) : Throwable(message)

+	// Avoid adding a spring-webmvc dependency
+	class ContinuationHandlerMethodArgumentResolver : HandlerMethodArgumentResolver {
+
+		override fun supportsParameter(parameter: MethodParameter) =
+			"kotlin.coroutines.Continuation" == parameter.getParameterType().getName()
+
+		override fun resolveArgument(
+			parameter: MethodParameter,
+			mavContainer: ModelAndViewContainer?,
+			webRequest: NativeWebRequest,
+			binderFactory: WebDataBinderFactory?
+		) = null
+
+	}
+
 }