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

CoroutineUtils assumes Kotlin value class has an accessible constructor #32573

Closed
cloudshiftchris opened this issue Apr 3, 2024 · 2 comments
Closed
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: duplicate A duplicate of another issue theme: kotlin An issue related to Kotlin support

Comments

@cloudshiftchris
Copy link

Affects: Spring Framework 6.1.x / Spring Boot 3.2.x

With Spring Boot 3.1.x (Spring Framework 6.0.x) proxies around Kotlin suspending functions that took a value class were able to handle value class arguments that did not have an accessible constructor, such as:

@JvmInline
public value class TenantCode private constructor(private val codeStr: String) {
    init {
        require(codeRegex.matches(codeStr)) { "Tenant code '$codeStr' must match regex $codeRegex" }
        require(codeStr.length >= 3) { "Tenant code must be >= 3 characters" }
    }

    public companion object {
        private val codeRegex = Regex("[0-9a-z-]+")

        public fun of(code: String): TenantCode {
            return TenantCode(code)
        }
    }

    override fun toString(): String = codeStr
}

Spring 6.1.x, in #32324, refactored CoroutineUtils to require an accessible primary constructor, failing with an IllegalAccessException if the constructor is not accessible.

As a workaround have disabled (currently unneeded) proxying of these interfaces (@repository), in preference to weakening the value class encapsulation.

This broke during a Spring Boot 3.1.5 -> 3.2.4 upgrade.

Its also not clear why instantiation is required, as the argument value already exists and is of the same type - is there perhaps a performance impact here as well?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 3, 2024
@jhoeller jhoeller added type: regression A bug that is also a regression in: core Issues in core modules (aop, beans, core, context, expression) theme: kotlin An issue related to Kotlin support and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 4, 2024
@jhoeller jhoeller added this to the 6.1.6 milestone Apr 4, 2024
@snicoll snicoll changed the title CoroutineUtils assumes Kotlin value class has an accessible constructor (regression) CoroutineUtils assumes Kotlin value class has an accessible constructor Apr 4, 2024
@sdeleuze
Copy link
Contributor

sdeleuze commented Apr 5, 2024

I suspect it is a duplicate of #32536, could you please test with Spring Framework 6.1.6-SNAPSHOT and let me know if you still see this issue?

Instantiation of value class parameters is sadly required due to the fact that Kotlin reflection require the boxed value. That's unfortunate, but that's not something where we are not in control.

@sdeleuze sdeleuze added the status: waiting-for-feedback We need additional information before we can continue label Apr 5, 2024
@cloudshiftchris
Copy link
Author

cloudshiftchris commented Apr 5, 2024

Indeed, 6.1.6-SNAPSHOT addresses this.

6.1.5 fails with

kotlin.reflect.full.IllegalCallableAccessException: java.lang.IllegalAccessException: class kotlin.reflect.jvm.internal.calls.CallerImpl$Method cannot access a member of class clario.tenant.domain.TenantCode with modifiers "private static"
	at kotlin.reflect.jvm.internal.KCallableImpl.call(KCallableImpl.kt:280)
	at org.springframework.core.CoroutinesUtils.lambda$invokeSuspendingFunction$2(CoroutinesUtils.java:132)
	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:104)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:585)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:802)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:706)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:693)
Caused by: java.lang.IllegalAccessException: class kotlin.reflect.jvm.internal.calls.CallerImpl$Method cannot access a member of class clario.tenant.domain.TenantCode with modifiers "private static"
	at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:392)
	at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:674)
	at java.base/java.lang.reflect.Method.invoke(Method.java:560)
	at kotlin.reflect.jvm.internal.calls.CallerImpl$Method.callMethod(CallerImpl.kt:97)
	at kotlin.reflect.jvm.internal.calls.CallerImpl$Method$Static.call(CallerImpl.kt:106)
	at kotlin.reflect.jvm.internal.calls.ValueClassAwareCaller.call(ValueClassAwareCaller.kt:199)
	at kotlin.reflect.jvm.internal.KCallableImpl.call(KCallableImpl.kt:108)
	... 8 common frames omitted

whereas there is no longer a failure on 6.1.6-SNAPSHOT.

Not understanding why instantiation of value-class parameters is required:

  • constructor-impl of Kotlin value class is not called #32324 talks to the constructor not being invoked, in the context of instantiation via Java reflection (not related to proxying);
  • when proxying you have the source and target parameter type and aren't doing type conversions, so boxing should suffice (as the object was previously created - that would mirror what Kotlin does in passing around value class instances).
  • this previously worked with just boxing of value class parameters

Even with allowing access to private constructors that can still subvert the developer's creation strategy for any given object - in my code example above, there's a factory function on the Companion object doing the creation. In that case it doesn't do much, but if it did that would be problematic to have an instance "created" without going through the specified creation strategy.

Perhaps there are several use cases here - instantiation of a value class instance from java via reflection (as noted in #32324), proxying (where value class instance already exists), and perhaps others. Perhaps invokeSuspendingFunction is trying to do too much / has mixed responsibilities?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 5, 2024
@sdeleuze sdeleuze added status: duplicate A duplicate of another issue and removed type: regression A bug that is also a regression status: feedback-provided Feedback has been provided labels Apr 5, 2024
@sdeleuze sdeleuze removed this from the 6.1.6 milestone Apr 5, 2024
@sdeleuze sdeleuze closed this as not planned Won't fix, can't repro, duplicate, stale Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: duplicate A duplicate of another issue theme: kotlin An issue related to Kotlin support
Projects
None yet
Development

No branches or pull requests

4 participants