Skip to content

GH-3344: Treat kotlin.Unit return as null in MMIH #3346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 20, 2020
Merged

GH-3344: Treat kotlin.Unit return as null in MMIH #3346

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e69f4d3
GH-3344: Treat kotlin.Unit return as null in MMIH
artembilan Jul 17, 2020
14caf0f
* Introduce `ClassUtils.isKotlinUnit(Class)` API;
artembilan Jul 20, 2020
db6786c
* Fix since on new `isKotlinUnit()` API
artembilan Jul 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion ...in/java/org/springframework/integration/handler/support/MessagingMethodInvokerHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,7 @@ private void configureLocalMessageHandlerFactory() {
localHandlerMethodFactory.afterPropertiesSet();
}

@Nullable
private Object invokeHandlerMethod(HandlerMethod handlerMethod, ParametersWrapper parameters) {
try {
return handlerMethod.invoke(parameters);
Expand Down Expand Up @@ -1093,13 +1094,18 @@ void setInvocableHandlerMethod(InvocableHandlerMethod newInvocableHandlerMethod)
this.invocableHandlerMethod = newInvocableHandlerMethod;
}

@Nullable
public Object invoke(ParametersWrapper parameters) {
Message<?> message = parameters.getMessage();
if (this.canProcessMessageList) {
message = new MutableMessage<>(parameters.getMessages(), parameters.getHeaders());
}
try {
return this.invocableHandlerMethod.invoke(message);
Object result = this.invocableHandlerMethod.invoke(message);
if (result != null && result.getClass().getName().equals("kotlin.Unit")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not capture this condition as a final boolean by checking the return type in a CTOR (to avoid running this code on every non-null return)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can avoid something indeed, when a return type is void or Unit. However in this case the return type is Object so end-user can return null ()or just don't have return in the function lambda and we don't produce a reply message.
In this case Kotlin for non-return in function lambda produces for us that Unit which is not a null apparently.
So, we produce some weird reply message.
Therefore even if we have some final checks for return type, we still would have some logic to check the returned value to be sure that we don't produce a reply message with Unit payload.

To summarize: when have void return this result is always going to be null, therefore we don't go to check a class name for Unit. If the value not null, it already doesn't matter what void check we have in advance: we still have to check this result for Unit type.

Does it make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we at least do something like...

if (result != null && this.isKotlinLambda && result.getClass().getName().equals("kotlin.Unit")) {

?

This result.getClass().getName().equals("kotlin.Unit") just seems like a lot of overhead for this one corner case and not good for the 99.9% of calls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! OK. Makes sense.
I'll think what we can do...
Thanks for the pointer!

result = null;
}
return result;
}
catch (RuntimeException ex) { // NOSONAR no way to handle conditional catch according Sonar rules
throw ex;
Expand Down
27 changes: 21 additions & 6 deletions ...ng-integration-core/src/test/kotlin/org/springframework/integration/dsl/KotlinDslTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,9 @@
package org.springframework.integration.dsl

import assertk.assertThat
import assertk.assertions.isEqualTo
import assertk.assertions.isGreaterThanOrEqualTo
import assertk.assertions.isInstanceOf
import assertk.assertions.isNotNull
import assertk.assertions.isTrue
import assertk.assertions.size
import assertk.assertions.*
import org.apache.commons.logging.Log
import org.apache.commons.logging.LogFactory
import org.junit.jupiter.api.Test
import org.springframework.beans.factory.BeanFactory
import org.springframework.beans.factory.annotation.Autowired
Expand Down Expand Up @@ -51,6 +48,7 @@ import org.springframework.test.context.junit.jupiter.SpringJUnitConfig
import reactor.core.publisher.Flux
import reactor.test.StepVerifier
import java.util.*
import java.util.concurrent.atomic.AtomicReference
import java.util.function.Function

/**
Expand Down Expand Up @@ -210,6 +208,23 @@ class KotlinDslTests {
assertThat(payload).isInstanceOf(List::class.java).size().isGreaterThanOrEqualTo(1)
}

@Test
fun `no reply from handle`() {
val payloadReference = AtomicReference<String>()
val integrationFlow =
integrationFlow("handlerInputChanenl") {
handle<String> { payload, _ -> payloadReference.set(payload) }
}

val registration = this.integrationFlowContext.registration(integrationFlow).register()

registration.inputChannel.send(GenericMessage("test"))

assertThat(payloadReference.get()).isEqualTo("test")

registration.destroy()
}

@Configuration
@EnableIntegration
class Config {
Expand Down