Skip to content

Commit

Permalink
Treat kotlin.Unit as void in web controllers
Browse files Browse the repository at this point in the history
This commit fixes a regression introduced by gh-21139
via the usage of Kotlin reflection to invoke HTTP
handler methods. It ensures that kotlin.Unit is treated
as void by returning null.

It also polishes CoroutinesUtils to have a consistent
handling compared to the regular case, and adds related
tests to prevent future regressions.

Closes gh-31648
  • Loading branch information
sdeleuze committed Nov 22, 2023
1 parent c329ed8 commit 441e210
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public static Publisher<?> invokeSuspendingFunction(CoroutineContext context, Me
}
return KCallables.callSuspendBy(function, argMap, continuation);
})
.filter(result -> !Objects.equals(result, Unit.INSTANCE))
.filter(result -> result != Unit.INSTANCE)
.onErrorMap(InvocationTargetException.class, InvocationTargetException::getTargetException);

KClassifier returnType = function.getReturnType().getClassifier();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import kotlinx.coroutines.*
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.reactor.awaitSingle
import kotlinx.coroutines.reactor.awaitSingleOrNull
import org.assertj.core.api.Assertions
import org.junit.jupiter.api.Test
import reactor.core.publisher.Flux
Expand Down Expand Up @@ -126,6 +127,24 @@ class CoroutinesUtilsTests {
Assertions.assertThatIllegalArgumentException().isThrownBy { CoroutinesUtils.invokeSuspendingFunction(context, method, this, "foo") }
}

@Test
fun invokeSuspendingFunctionReturningUnit() {
val method = CoroutinesUtilsTests::class.java.getDeclaredMethod("suspendingUnit", Continuation::class.java)
val mono = CoroutinesUtils.invokeSuspendingFunction(method, this) as Mono
runBlocking {
Assertions.assertThat(mono.awaitSingleOrNull()).isNull()
}
}

@Test
fun invokeSuspendingFunctionReturningNull() {
val method = CoroutinesUtilsTests::class.java.getDeclaredMethod("suspendingNullable", Continuation::class.java)
val mono = CoroutinesUtils.invokeSuspendingFunction(method, this) as Mono
runBlocking {
Assertions.assertThat(mono.awaitSingleOrNull()).isNull()
}
}

suspend fun suspendingFunction(value: String): String {
delay(1)
return value
Expand All @@ -146,4 +165,11 @@ class CoroutinesUtilsTests {
return value
}

suspend fun suspendingUnit() {
}

suspend fun suspendingNullable(): String? {
return null
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Map;
import java.util.Objects;

import kotlin.Unit;
import kotlin.reflect.KFunction;
import kotlin.reflect.KParameter;
import kotlin.reflect.jvm.KCallablesJvm;
Expand Down Expand Up @@ -315,7 +316,8 @@ public static Object invokeFunction(Method method, Object target, Object[] args)
}
}
}
return function.callBy(argMap);
Object result = function.callBy(argMap);
return (result == Unit.INSTANCE ? null : result);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,19 @@ class InvocableHandlerMethodKotlinTests {
Assertions.assertThat(value).isEqualTo("true")
}

@Test
fun unitReturnValue() {
val value = getInvocable().invokeForRequest(request, null)
Assertions.assertThat(value).isNull()
}

@Test
fun nullReturnValue() {
composite.addResolver(StubArgumentResolver(String::class.java, null))
val value = getInvocable(String::class.java).invokeForRequest(request, null)
Assertions.assertThat(value).isNull()
}

private fun getInvocable(vararg argTypes: Class<*>): InvocableHandlerMethod {
val method = ResolvableMethod.on(Handler::class.java).argTypes(*argTypes).resolveMethod()
val handlerMethod = InvocableHandlerMethod(Handler(), method)
Expand All @@ -95,6 +108,14 @@ class InvocableHandlerMethodKotlinTests {

fun nullableBooleanDefaultValue(status: Boolean? = true) =
status.toString()

fun unit(): Unit {
}

@Suppress("UNUSED_PARAMETER")
fun nullable(arg: String?): String? {
return null
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.Objects;
import java.util.stream.Stream;

import kotlin.Unit;
import kotlin.coroutines.CoroutineContext;
import kotlin.reflect.KFunction;
import kotlin.reflect.KParameter;
Expand Down Expand Up @@ -326,7 +327,8 @@ public static Object invokeFunction(Method method, Object target, Object[] args,
}
}
}
return function.callBy(argMap);
Object result = function.callBy(argMap);
return (result == Unit.INSTANCE ? null : result);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,20 @@ class InvocableHandlerMethodKotlinTests {
assertHandlerResultValue(result, "override")
}

@Test
fun unitReturnValue() {
val method = NullResultController::unit.javaMethod!!
val result = invoke(NullResultController(), method)
assertHandlerResultValue(result, null)
}

@Test
fun nullReturnValue() {
val method = NullResultController::nullable.javaMethod!!
val result = invoke(NullResultController(), method)
assertHandlerResultValue(result, null)
}


private fun invokeForResult(handler: Any, method: Method, vararg providedArgs: Any): HandlerResult? {
return invoke(handler, method, *providedArgs).block(Duration.ofSeconds(5))
Expand All @@ -186,7 +200,7 @@ class InvocableHandlerMethodKotlinTests {
return resolver
}

private fun assertHandlerResultValue(mono: Mono<HandlerResult>, expected: String) {
private fun assertHandlerResultValue(mono: Mono<HandlerResult>, expected: String?) {
StepVerifier.create(mono)
.consumeNextWith {
if (it.returnValue is Mono<*>) {
Expand Down Expand Up @@ -242,4 +256,14 @@ class InvocableHandlerMethodKotlinTests {
@Suppress("RedundantSuspendModifier")
suspend fun handleSuspending(@RequestParam value: String = "default") = value
}

class NullResultController {

fun unit() {
}

fun nullable(): String? {
return null
}
}
}

0 comments on commit 441e210

Please sign in to comment.