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 parameter default values in HTTP handler methods #21139

Closed
spring-projects-issues opened this issue Mar 15, 2018 · 17 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) theme: kotlin An issue related to Kotlin support type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

Take Weiland opened SPR-16598 and commented

Support specifying defaults for e.g. @RequestMapping via Kotlin parameter default values like so:

@GetMapping
fun get(@RequestParam limit: Int = 20)

Such a method should behave equivalent to @RequestMapping with defaultValue, except that the value does not need to be processed by converters, etc.

This can be implemented in org.springframework.web.method.support.InvocableHandlerMethod using Kotlin Reflection (KCallable#callBy), which allows omitting parameters with default values.


Affects: 5.0.4

Referenced from: pull request #1741

2 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

Alex Rader commented

In Kotlin @RequestBody required=false when use default arguments
spring-projects/spring-boot#12691

@spring-projects-issues
Copy link
Collaborator Author

Sébastien Deleuze commented

As discussed in the PR, current Kotlin API force us to integrate that as a rather involved and not very elegant way, I will raise and discuss that point with Kotlin team. As a consequence, I think it is more reasonable to target 5.2 for that one.

@spring-projects-issues spring-projects-issues added type: enhancement A general enhancement in: web Issues in web modules (web, webmvc, webflux, websocket) labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 5.x Backlog milestone Jan 11, 2019
@AlexTrotsenko
Copy link

Is there any updates on this issue?

@cmdjulian
Copy link

I'm also interested in this feature.

@jlous
Copy link

jlous commented Feb 5, 2021

The pull-request was closed in August 2020.

@jlous
Copy link

jlous commented Feb 5, 2021

Resolving the value in Spring (like a defaultValue) is not feasible, because Kotlin's default is not a simple value but a piece of essentially arbitrary object-internal kotlin code, which Spring has no access to.

An alternative approach would be to use @jvmoverloads to have kotlin generate explicit permutations of the method signature. Spring would need to select the right one depending on which params are present, in stead of rejecting them all as ambiguous endpoint mappings like it does now. One would still need to define a sane course of action if that annotation is missing. Bad request?

In any case: the situation today is that a kotlin endpoint with a default value will run without incident right up until a request without the parameter arrives, in which case it will just throw an NPE from way inside Spring's parameter mapping. That has got to be the wrong approach whichever way you look at it.

@sdeleuze sdeleuze added the theme: kotlin An issue related to Kotlin support label Jan 19, 2022
@sdeleuze sdeleuze changed the title Support Kotlin parameter default values in HTTP handler methods [SPR-16598] Support Kotlin parameter default values in HTTP handler methods Feb 21, 2023
@igorwojda
Copy link

@jlous I would like to understand this issue a bit more. What exactly is required on the Kotlin side to make this work as expected with Spring (no duplicated default values)? Perhaps the ability to access default value via reflection?

@igorwojda
Copy link

igorwojda commented Feb 23, 2023

I have opened an issue in Kotlin issue tracker: https://youtrack.jetbrains.com/issue/KT-56893/Provide-a-way-to-read-default-argument-values

@sdeleuze sdeleuze modified the milestones: 6.x Backlog, 6.1.0-M1 Feb 23, 2023
@sdeleuze sdeleuze self-assigned this Feb 23, 2023
@sdeleuze
Copy link
Contributor

Let's try to see if we can support that in 6.1 (no promise but I will try).

@sdeleuze
Copy link
Contributor

As discussed in the Kotlin issue, we are probably going to handle this with a refined variant of the original PR.

@sdeleuze
Copy link
Contributor

I have pushed support for Kotlin parameter default values in handler methods. It allows to write:
@RequestParam value: String = "default"
as an alternative to:
@RequestParam(defaultValue = "default") value: String

Both Spring MVC and WebFlux are supported, including on suspending functions.

The changes are quite involved, so I would appreciate feedback based on Spring Framework 6.1.0-SNAPSHOT.

sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Nov 22, 2023
This commit fixes a regression introduced by spring-projectsgh-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 spring-projectsgh-31648
@Quantum64
Copy link

This change caused a regression in our project related to calling suspend functions with @JvmInline value class arguments in @Lazy constructor injected beans.

java.lang.IllegalArgumentException: object is not an instance of declaring class
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at kotlin.reflect.jvm.internal.calls.ValueClassAwareCaller.call(ValueClassAwareCaller.kt:190)
	at kotlin.reflect.jvm.internal.KCallableImpl.callDefaultMethod$kotlin_reflection(KCallableImpl.kt:207)
	at kotlin.reflect.full.KCallables.callSuspendBy(KCallables.kt:74)
	at org.springframework.core.CoroutinesUtils.lambda$invokeSuspendingFunction$2(CoroutinesUtils.java:124)
	at kotlin.coroutines.intrinsics.IntrinsicsKt__IntrinsicsJvmKt$createCoroutineUnintercepted$$inlined$createCoroutineFromSuspendFunction$IntrinsicsKt__IntrinsicsJvmKt$4.invokeSuspend(IntrinsicsJvm.kt:270)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
	at kotlinx.coroutines.EventLoop.processUnconfinedEvent(EventLoop.common.kt:68)
	at kotlinx.coroutines.internal.DispatchedContinuationKt.resumeCancellableWith(DispatchedContinuation.kt:375)
	at kotlinx.coroutines.intrinsics.CancellableKt.startCoroutineCancellable(Cancellable.kt:30)
	at kotlinx.coroutines.intrinsics.CancellableKt.startCoroutineCancellable$default(Cancellable.kt:25)
	at kotlinx.coroutines.BuildersKt__Builders_commonKt.withContext(Builders.common.kt:172)
	at kotlinx.coroutines.BuildersKt.withContext(Unknown Source)

@sdeleuze
Copy link
Contributor

sdeleuze commented Nov 28, 2023

@Quantum64 Please create a dedicated issue with a reproducer.

@efemoney
Copy link

efemoney commented Nov 29, 2023

@sdeleuze This change caused another regression using extension receivers. Not necessarily a regression, more a bug as the change just completely ignores Kind.EXTENSION_RECEIVER.

I think java's lack of exhaustive checks for non-expression switch statements is to blame here.

@sdeleuze
Copy link
Contributor

sdeleuze commented Dec 8, 2023

@efemoney Could you please create a new issue with a reproducer please (attached archive or link to a repository)?

@efemoney
Copy link

efemoney commented Dec 8, 2023

Sure, let me do that.

@efemoney
Copy link

@sdeleuze I see you just fixed this yesterday. Looking forwrd to the release. Sorry I didnt get to creating an issue.

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) theme: kotlin An issue related to Kotlin support type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

8 participants