Skip to content
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.

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

Support Kotlin value (JvmInline) classes as suspend handler method arguments #27345

Closed
efemoney opened this issue Sep 1, 2021 · 9 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: duplicate A duplicate of another issue theme: kotlin An issue related to Kotlin support

Comments

@efemoney
Copy link

efemoney commented Sep 1, 2021

Affects: Spring Framework 5.3.9, Spring Boot 2.5.4


Using a @JvmInline value class as a handler method argument fails with the following:

java.lang.IllegalArgumentException: object is not an instance of declaring class

My best guess is this happens because, say with the example below:

@JvmInline value class ProductId(val id: String)

@GetMapping("/product")
suspend fun getProduct(@RequestParam productId: ProductId) { ... }

at runtime the getProduct parameter type when viewed from Java reflection is the wrapped value type (String) but when viewed from Kotlin reflection, the parameter is the wrapper value class (ProductId). The error is thrown when the function is invoked from kotlin reflection with String instance arguments (contributed by the RequestParam resolver) but the kotlin reflection API expects the ProductId instance (which it automatically unboxes, exception is thrown here)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 1, 2021
@efemoney efemoney changed the title Support Kotlin value (JvmInline) classes in handler method arguments Support Kotlin value (JvmInline) classes as suspend handler method arguments Sep 17, 2021
@efemoney
Copy link
Author

I have written an inline value class converter and included it into my conversionService

private class KotlinInlineClassConverter(
  private val conversionService: ConversionService,
) : ConditionalGenericConverter {

  override fun getConvertibleTypes() = setOf(ConvertiblePair(Any::class.java, Any::class.java))

  override fun matches(sourceType: TypeDescriptor, targetType: TypeDescriptor): Boolean {
    val targetClass = targetType.type
    if (!targetClass.isInlineValueClass()) return false
    return conversionService.canConvert(sourceType, targetClass.wrappedValueTypeDescriptor())
  }

  override fun convert(source: Any?, sourceType: TypeDescriptor, targetType: TypeDescriptor): Any? {
    val targetClass = targetType.type
    val convertedSource = conversionService.convert(source, sourceType, targetClass.wrappedValueTypeDescriptor())
      ?: return null
    return targetClass.kotlin.primaryConstructor!!.call(convertedSource)
  }

  private fun Class<*>.isInlineValueClass() =
    isKotlinType(this) && annotations.any { it is JvmInline }

  private fun Class<*>.wrappedValueTypeDescriptor() =
    TypeDescriptor.valueOf(kotlin.primaryConstructor!!.parameters.single().type.jvmErasure.java)
}

but this does not seem to work because the @RequestParam & @RequestHeader resolvers take precedence over the custom HandlerMethodArgumentResolvers.

@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Nov 11, 2021
@sdeleuze sdeleuze added the theme: kotlin An issue related to Kotlin support label Jan 19, 2022
@albertocavalcante
Copy link

Hello, is there any update on this issue? Is it being reviewed? Thanks

@sdeleuze sdeleuze self-assigned this Feb 8, 2023
@sdeleuze sdeleuze added the type: enhancement A general enhancement label Feb 9, 2023
@sdeleuze sdeleuze added this to the 6.1.0-M3 milestone Jul 5, 2023
@sdeleuze sdeleuze removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 5, 2023
@sdeleuze
Copy link
Contributor

sdeleuze commented Jul 5, 2023

Related to #28638.

@jhoeller jhoeller modified the milestones: 6.1.0-M3, 6.1.0-M4 Jul 17, 2023
@sdeleuze
Copy link
Contributor

I am currently blocked on this one by a Kotlin reflection behavior that I can't understand.

The invocation of KCallables.callSuspendBy with nullable value class parameter in my implementation of CoroutinesUtils#invokeSuspendingFunction(CoroutineContext, java.lang.reflect.Method, java.lang.Object, java.lang.Object...) fails and I struggle to understand why, since a very close use case with KCallable#callBy on non suspending function works as expected.

I have created a dedicated Kotlin focused repro with 8 unit tests, and only one is failing.
reflection-value-classes.zip.

I am waiting for feedback from the Kotlin team to move forward.

@sdeleuze sdeleuze added the status: waiting-for-feedback We need additional information before we can continue label Aug 10, 2023
@sdeleuze
Copy link
Contributor

This is a Kotlin bug : KT-58887.

@sdeleuze sdeleuze added status: blocked An issue that's blocked on an external project change and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 10, 2023
@sdeleuze sdeleuze modified the milestones: 6.1.0-M4, 6.1.x Aug 10, 2023
@sdeleuze
Copy link
Contributor

sdeleuze commented Oct 25, 2023

The underlying blocking bug has been solved in Kotlin 1.9.20-RC2 via KT-47973, the next issue is on Spring side in order to support mangled method names (probably via leveraging Kotlin reflection rather than Java one for the function invocation).

@michael-wirth
Copy link

With Spring Boot 3.2 and Spring Framework 6.1 this bug is not restricted to suspend functions only, but affects all handler methods using value classes.

It happens in the web and the webflux stack.

@RestController
class MyController {

    @GetMapping("/item/{itemId}")
    fun getItem(@PathVariable itemId: ItemId) = itemId

}

@JvmInline
value class ItemId(val id: String)

Reproduce:
unpack demo.zip and start the DemoApplication

curl http://localhost:8080/item/1

Solution:
Call the Java implementation method.invoke(getBean(), args) instead of KotlinDelegate.invokeFunction by overriding the InvocableHandlerMethod.

Exception:

2023-12-07T00:18:09.170+01:00 ERROR 361719 --- [         task-1] a.w.r.e.AbstractErrorWebExceptionHandler : [c378e2b5-1]  500 Server Error for HTTP GET "/item/1"

java.lang.IllegalStateException: object is not an instance of declaring class
Controller [com.example.demo.MyController]
Method [public java.lang.String com.example.demo.MyController.getItem-DTGngPY(java.lang.String)] with argument values:
 [0] [type=java.lang.String] [value=1] 
	at org.springframework.web.reactive.result.method.InvocableHandlerMethod.lambda$invoke$0(InvocableHandlerMethod.java:181) ~[spring-webflux-6.1.1.jar:6.1.1]
	Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Error has been observed at the following site(s):
	*__checkpoint ⇢ HTTP GET "/item/1" [ExceptionHandlingWebHandler]
Original Stack Trace:
		at org.springframework.web.reactive.result.method.InvocableHandlerMethod.lambda$invoke$0(InvocableHandlerMethod.java:181) ~[spring-webflux-6.1.1.jar:6.1.1]
		at reactor.core.publisher.MonoFlatMap$FlatMapMain.onNext(MonoFlatMap.java:132) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoZip$ZipCoordinator.signal(MonoZip.java:297) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoZip$ZipInner.onNext(MonoZip.java:478) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoPeekTerminal$MonoTerminalPeekSubscriber.onNext(MonoPeekTerminal.java:180) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.FluxDefaultIfEmpty$DefaultIfEmptySubscriber.onNext(FluxDefaultIfEmpty.java:122) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.FluxSwitchIfEmpty$SwitchIfEmptySubscriber.onNext(FluxSwitchIfEmpty.java:74) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.Operators$ScalarSubscription.request(Operators.java:2571) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.Operators$MultiSubscriptionSubscriber.set(Operators.java:2367) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.Operators$MultiSubscriptionSubscriber.onSubscribe(Operators.java:2241) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.FluxFlatMap.trySubscribeScalarMap(FluxFlatMap.java:193) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoFlatMap.subscribeOrReturn(MonoFlatMap.java:53) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.InternalMonoOperator.subscribe(InternalMonoOperator.java:63) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoZip$ZipCoordinator.request(MonoZip.java:220) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoFlatMap$FlatMapMain.request(MonoFlatMap.java:194) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.onSubscribe(MonoIgnoreThen.java:135) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoFlatMap$FlatMapMain.onSubscribe(MonoFlatMap.java:117) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoZip.subscribe(MonoZip.java:129) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.InternalMonoOperator.subscribe(InternalMonoOperator.java:76) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoDefer.subscribe(MonoDefer.java:53) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.subscribeNext(MonoIgnoreThen.java:241) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.onComplete(MonoIgnoreThen.java:204) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoFlatMap$FlatMapMain.onComplete(MonoFlatMap.java:189) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.Operators.complete(Operators.java:137) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoZip.subscribe(MonoZip.java:121) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.InternalMonoOperator.subscribe(InternalMonoOperator.java:76) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.subscribeNext(MonoIgnoreThen.java:264) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoIgnoreThen.subscribe(MonoIgnoreThen.java:51) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.Mono.subscribe(Mono.java:4512) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.publisher.MonoSubscribeOn$SubscribeOnSubscriber.run(MonoSubscribeOn.java:126) ~[reactor-core-3.6.0.jar:3.6.0]
		at reactor.core.scheduler.ExecutorScheduler$ExecutorTrackedRunnable.run(ExecutorScheduler.java:192) ~[reactor-core-3.6.0.jar:3.6.0]
		at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[na:na]
		at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[na:na]
		at java.base/java.lang.Thread.run(Thread.java:1583) ~[na:na]
Caused by: java.lang.IllegalArgumentException: object is not an instance of declaring class
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.checkReceiver(DirectMethodHandleAccessor.java:197) ~[na:na]
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:99) ~[na:na]
	at java.base/java.lang.reflect.Method.invoke(Method.java:580) ~[na:na]
	at kotlin.reflect.jvm.internal.calls.ValueClassAwareCaller.call(ValueClassAwareCaller.kt:190) ~[kotlin-reflect-1.9.20.jar:1.9.255-SNAPSHOT]
	at kotlin.reflect.jvm.internal.KCallableImpl.callDefaultMethod$kotlin_reflection(KCallableImpl.kt:207) ~[kotlin-reflect-1.9.20.jar:1.9.255-SNAPSHOT]
	at kotlin.reflect.jvm.internal.KCallableImpl.callBy(KCallableImpl.kt:112) ~[kotlin-reflect-1.9.20.jar:1.9.255-SNAPSHOT]
	at org.springframework.web.reactive.result.method.InvocableHandlerMethod$KotlinDelegate.invokeFunction(InvocableHandlerMethod.java:330) ~[spring-webflux-6.1.1.jar:6.1.1]
	at org.springframework.web.reactive.result.method.InvocableHandlerMethod.lambda$invoke$0(InvocableHandlerMethod.java:172) ~[spring-webflux-6.1.1.jar:6.1.1]
	at reactor.core.publisher.MonoFlatMap$FlatMapMain.onNext(MonoFlatMap.java:132) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoZip$ZipCoordinator.signal(MonoZip.java:297) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoZip$ZipInner.onNext(MonoZip.java:478) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoPeekTerminal$MonoTerminalPeekSubscriber.onNext(MonoPeekTerminal.java:180) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.FluxDefaultIfEmpty$DefaultIfEmptySubscriber.onNext(FluxDefaultIfEmpty.java:122) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.FluxSwitchIfEmpty$SwitchIfEmptySubscriber.onNext(FluxSwitchIfEmpty.java:74) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.Operators$ScalarSubscription.request(Operators.java:2571) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.Operators$MultiSubscriptionSubscriber.set(Operators.java:2367) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.Operators$MultiSubscriptionSubscriber.onSubscribe(Operators.java:2241) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.FluxFlatMap.trySubscribeScalarMap(FluxFlatMap.java:193) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoFlatMap.subscribeOrReturn(MonoFlatMap.java:53) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.InternalMonoOperator.subscribe(InternalMonoOperator.java:63) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoZip$ZipCoordinator.request(MonoZip.java:220) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoFlatMap$FlatMapMain.request(MonoFlatMap.java:194) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.onSubscribe(MonoIgnoreThen.java:135) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoFlatMap$FlatMapMain.onSubscribe(MonoFlatMap.java:117) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoZip.subscribe(MonoZip.java:129) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.InternalMonoOperator.subscribe(InternalMonoOperator.java:76) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoDefer.subscribe(MonoDefer.java:53) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.subscribeNext(MonoIgnoreThen.java:241) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.onComplete(MonoIgnoreThen.java:204) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoFlatMap$FlatMapMain.onComplete(MonoFlatMap.java:189) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.Operators.complete(Operators.java:137) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoZip.subscribe(MonoZip.java:121) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.InternalMonoOperator.subscribe(InternalMonoOperator.java:76) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.subscribeNext(MonoIgnoreThen.java:264) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoIgnoreThen.subscribe(MonoIgnoreThen.java:51) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.Mono.subscribe(Mono.java:4512) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.publisher.MonoSubscribeOn$SubscribeOnSubscriber.run(MonoSubscribeOn.java:126) ~[reactor-core-3.6.0.jar:3.6.0]
	at reactor.core.scheduler.ExecutorScheduler$ExecutorTrackedRunnable.run(ExecutorScheduler.java:192) ~[reactor-core-3.6.0.jar:3.6.0]
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[na:na]
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[na:na]
	at java.base/java.lang.Thread.run(Thread.java:1583) ~[na:na]

@sdeleuze
Copy link
Contributor

sdeleuze commented Dec 7, 2023

Thanks for the update, as of Spring Framework 6.1, we are now using Kotlin reflection instead of Java reflection for all web handler method invocations (in order to support various Kotlin use cases like default parameter values etc.) but we hit here another Kotlin bug, namely KT-64097, which hopefully should fixed pretty fast as Kotlin team works on it actively. Please track the Kotlin issue for the resolution of this issue, and will as well and check everything is fine on Spring side once fixed.

This issue is also likely a duplicate of #31698.

As a consequence, I close this issue on Spring side since this should be fixed on Kotlin side.

@sdeleuze sdeleuze closed this as not planned Won't fix, can't repro, duplicate, stale Dec 7, 2023
@sdeleuze sdeleuze removed this from the 6.1.x milestone Dec 7, 2023
@sdeleuze sdeleuze added status: duplicate A duplicate of another issue for: external-project Needs a fix in external project and removed type: enhancement A general enhancement labels Dec 7, 2023
@sdeleuze sdeleuze removed the for: external-project Needs a fix in external project label Dec 15, 2023
@sdeleuze
Copy link
Contributor

This issue is in fact a duplicate of #31846 closely related to #31698.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: duplicate A duplicate of another issue theme: kotlin An issue related to Kotlin support
Projects
None yet
Development

No branches or pull requests

7 participants