Add support for Kotlin Value Classes

[spring-projects-issues]

Wyatt Smith opened DATACMNS-1517 and commented

Here is an example with the bug: https://github.com/wyattjsmith1/SpringDataBug

When this runs, there is an IndexOutOfBoundsException. This is caused by kotlin's synthetic constructor. I believe the issues is that synthetic constructors aren't filtered before.

buildPreferredConstructor at org.springframework.data.mapping.model.PreferredConstructorDiscoverer.Discoverers#discover, but there is probably a better solution to this.

Changing AccountId in the data class to a String causes the application to work properly. Admittedly, inline classes are still experimental for Kotlin, but this might be worth investigating

---

Issue Links:

• DATAGRAPH-1330 Properties with type of Kotlin inline class has mangled name

7 votes, 10 watchers

[spring-projects-issues]

Mark Paluch commented

I switched this ticket from a bug report to a new feature because inline classes weren't there when we built support for Kotlin. Let's see whether and how we can support this type of classes

[spring-projects-issues]

Ben Madore commented

Just as another data point, ran into this same issue today, (should have searched here first instead of google!). Would love if this worked

[spring-projects-issues]

Mark Paluch commented

Classes, that use Kotlin inline classes in a constructor receive a synthetic public constructor while the actual constructor is made private. We've seen a similar behavior with data classes but there the synthetic constructor is accepting defaulting masks. Our code currently can find and use synthetic constructors but only if they also use a defaulting mask as in data classes.

The newly introduced Kotlin behavior has two effects:

1. Spring's ParameterNameDiscoverer returns a name array that does not contain a name for the synthetic argument (e.g. constructor takes 4 arguments [real 3 arguments + synthetic constructor marker] but the name array contains only 3 name items). Therefore we see an IndexOutOfBoundException during the class introspection.
2. KotlinClassGeneratingEntityInstantiator expects synthetic constructors that contain a DefaultConstructorMarker also accept one or more defaulting mask arguments. Types that accept a inline class argument don't contain defaulting masks.

Basically, if inline classes would not mess with the public constructor, things would work as expected without further do. After investigating a bit on this topic, support of types accepting inline classes requires a bigger effort to make it work

[spring-projects-issues]

Ben Madore commented

I wonder if it's worth opening up a ticket against kotlin to provide this as feedback on their Inline Class implementation which is still "experimental" and thus should be possible to change? I can open up a ticket there referencing this - though i suppose the Spring team might have more direct channels of communication there?

[spring-projects-issues]

Ben Madore commented

Opened https://youtrack.jetbrains.com/issue/KT-36320 with a link back to this issue

[spring-projects-issues]

gaerfield commented

Possible intermediate workaround: https://stackoverflow.com/a/60652012/5029667

[matthewadams]

NB: I'm not using inline classes, just data classes here.

I'm seeing something similar to this on a brand new, greenfield project using Kotlin 1.5.10, Spring Boot 2.5.1, Spring Data 2021.0.1 (w/Spring Data Commons 2.5.1 & Spring Data MongoDB 3.2.1). I'm using immutable data classes for my @Documents and their value(s). When I attempt to read a Schedule from mongo, I get the exception org.springframework.data.mapping.MappingException: Parameter org.springframework.data.mapping.PreferredConstructor$Parameter@80a6e44c does not have a name!. The parameter in question is, indeed, the DefaultConstructorMarker.

The offending data class is :

sealed interface Availability {
    companion object {
        val CONVENTIONAL_8TO5_WITH_LUNCH_RECURRENCE: CronString = "0/30 8-12,13-17 * * MON-FRI"
        val CONVENTIONAL_8TO5_RECURRENCE: CronString = "0/30 8-17 * * MON-FRI"
        val CONVENTIONAL_9TO5_WITH_LUNCH_RECURRENCE: CronString = "0/30 9-12,13-17 * * MON-FRI"
        val CONVENTIONAL_9TO5_RECURRENCE: CronString = "0/30 9-17 * * MON-FRI"
        val DEFAULT_RECURRENCE = CONVENTIONAL_9TO5_WITH_LUNCH_RECURRENCE
        val DEFAULT_SLOT_MINUTES = 30U.toUByte()
        val DEFAULT_ALLOWED_BOOKING_COUNT = 1U.toUByte()
    }
    val handle: String
    val slotMinutes: UByte
    val allowedBookingCount: UByte
    val name: String?
    val effectivity: Interval?
}
data class RecurringAvailability(
    val recurrence: CronString = DEFAULT_RECURRENCE,
    override val handle: String = UUID.randomUUID().toString(),
    override val slotMinutes: UByte = DEFAULT_SLOT_MINUTES,
    override val allowedBookingCount: UByte = DEFAULT_ALLOWED_BOOKING_COUNT,
    override val effectivity: Interval? = null,
    override val name: String? = null,
) : Availability

(fyi, CronString is typealias CronString = String)

Any known workarounds here?

[matthewadams]

Answered my own question, thanks to #2215 (comment). I was using unsigned types. Changing them to Int made the issue go away.

[mp911de]

A few things come together with inline classes: Synthetic constructors that do not follow any known scheme and property accessors with rewritten names. I think one could provide bean introspectors/bean descriptor factories to address specifics of how Kotlin classes using inline/unsigned are compiled by Kotlin. For the constructor issue, it's difficult to find a reasonable approach.

[udalov]

@mp911de Sorry for the delay. Inline class constructors are not supposed to be invoked directly, and that's by design. There's a public static method constructor-impl however, which is used for example by the Kotlin compiler when inline class is created. Maybe you can invoke it too?

[mp911de]

The issue is that the entire instantiation mechanism in Spring Data relies solely on constructors. We discover a persistence constructor, invoke it via reflection and have code to generate bytecode to call constructors via bytecode. Switching to factory methods requires a significant rewrite and, what's more important, we need to adopt a different concept.

[mp911de]

Related: spring-projects/spring-framework#21998

[rowi1de]

This also affects CoroutineCrudRepository with Spring Data R2DBC

Caused by: java.lang.IllegalStateException: Required property null not found for class org.example.DbEntity
	at org.springframework.data.mapping.PersistentEntity.getRequiredPersistentProperty(PersistentEntity.java:190)
	at org.springframework.data.r2dbc.convert.MappingR2dbcConverter$RowParameterValueProvider.getParameterValue(MappingR2dbcConverter.java:731)
	at org.springframework.data.mapping.model.SpELExpressionParameterValueProvider.getParameterValue(SpELExpressionParameterValueProvider.java:53)
	at org.springframework.data.relational.core.conversion.BasicRelationalConverter$ConvertingParameterValueProvider.getParameterValue(BasicRelationalConverter.java:293)
	at org.springframework.data.mapping.model.ClassGeneratingEntityInstantiator.extractInvocationArguments(ClassGeneratingEntityInstantiator.java:313)
	at org.springframework.data.mapping.model.ClassGeneratingEntityInstantiator$EntityInstantiatorAdapter.createInstance(ClassGeneratingEntityInstantiator.java:285)
	at org.springframework.data.mapping.model.ClassGeneratingEntityInstantiator.createInstance(ClassGeneratingEntityInstantiator.java:102)
	at org.springframework.data.relational.core.conversion.BasicRelationalConverter.createInstance(BasicRelationalConverter.java:149)
	at org.springframework.data.r2dbc.convert.MappingR2dbcConverter.createInstance(MappingR2dbcConverter.java:330)
	at org.springframework.data.r2dbc.convert.MappingR2dbcConverter.read(MappingR2dbcConverter.java:127)
	at org.springframework.data.r2dbc.convert.MappingR2dbcConverter.read(MappingR2dbcConverter.java:122)
	at org.springframework.data.r2dbc.convert.EntityRowMapper.apply(EntityRowMapper.java:46)
	at org.springframework.data.r2dbc.convert.EntityRowMapper.apply(EntityRowMapper.java:29)
	at io.r2dbc.postgresql.PostgresqlResult.lambda$map$2(PostgresqlResult.java:123)
	

is it required to create a bug there as well?

[sangyongchoi]

Hi, i am user using spring data with kotlin.
Any progress on this?

[christophstrobl]

As mentioned the original constructor is still present but marked private.

private AccountNameCheck(java.lang.String arg0, java.lang.String arg1, java.lang.String arg2) { // <init> //(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V
         <localVar:index=0 , name=this , desc=Lcom/example/demo/AccountNameCheck;, sig=null, start=L0, end=L5>
         <localVar:index=1 , name=accountId , desc=Ljava/lang/String;, sig=null, start=L0, end=L5>
         <localVar:index=2 , name=accountName , desc=Ljava/lang/String;, sig=null, start=L0, end=L5>
         <localVar:index=3 , name=id , desc=Ljava/lang/String;, sig=null, start=L0, end=L5>

public synthetic AccountNameCheck(java.lang.String arg0, java.lang.String arg1, java.lang.String arg2, kotlin.jvm.internal.DefaultConstructorMarker arg3) { // <init> //(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
         <localVar:index=0 , name=this , desc=Lcom/example/demo/AccountNameCheck;, sig=null, start=L0, end=L1>
         <localVar:index=1 , name=accountId , desc=Ljava/lang/String;, sig=null, start=L0, end=L1>
         <localVar:index=2 , name=accountName , desc=Ljava/lang/String;, sig=null, start=L0, end=L1>
         <localVar:index=3 , name=id , desc=Ljava/lang/String;, sig=null, start=L0, end=L1>
         <localVar:index=4 , name=$constructor_marker , desc=Lkotlin/jvm/internal/DefaultConstructorMarker;, sig=null, start=L0, end=L1>

So, since the newly created ctor is synthetic and should not be considered a persistence constructor candidate, assuming the DefaultConstructorMarker is added always last, one option would be to go on and try to look up a matching private ctor based on the given constructor arguments having the DefaultConstructorMarker removed.

KFunction<T> primaryConstructor = KClasses.getPrimaryConstructor(JvmClassMappingKt.getKotlinClass(type.getType()));
Constructor<T> javaConstructor = ReflectJvmMapping.getJavaConstructor(primaryConstructor);
if(javaConstructor.isSynthetic()) {
	Class<?>[] parameterTypes = javaConstructor.getParameterTypes();
	if(parameterTypes.length > 0 && parameterTypes[parameterTypes.length-1] == DefaultConstructorMarker.class) {
		try {
			Class[] classes = Arrays.stream(parameterTypes).limit(javaConstructor.getParameters().length - 1).toArray(Class[]::new);
			Constructor<T> declaredConstructor = type.getType().getDeclaredConstructor(classes);
			return Discoverers.buildPreferredConstructor(declaredConstructor, type, entity);
		} catch (NoSuchMethodException e) {
			throw new RuntimeException(e);
		}
	}
}

[sdeleuze]

I will ask a feedback from Kotlin team on that.

[sdeleuze]

@christophstrobl Probably not based on @udalov comment above.

@elizarov provided more insights

By design of the Kotlin inline classes, their "constructor" is not actually constructing an instance. The actual constructor is inside a static constructor-impl method that should be called instead. We cannot move inline class's constructor logic into an actual JVM constructor, since it'll defeat the whole purpose of them being inline classes. The idea of the inline class it to avoid allocation. But in JVM, if you have any logic inside a constructor, then then only way to invoke it is by allocating a new instance on the heap.

[christophstrobl]

@sdeleuze, @elizarov this is true for the inline class itself, the problem is with any type that is using an inline class.

Using the types from the original sample mentioned here.

inline class AccountId(val id: String)

data class AccountNameCheck(val accountId: AccountId, val accountName: String, val id: String)

For the AccountId once can clearly see the mentioned constructor-impl.

public final class com/example/demo/AccountId {
     <ClassVersion=61>
     <SourceFile=DemoApplication.kt>

     private final java.lang.String id;

...

 private synthetic AccountId(java.lang.String arg0) { // <init> //(Ljava/lang/String;)V
         <localVar:index=0 , name=this , desc=Lcom/example/demo/AccountId;, sig=null, start=L0, end=L1>
         <localVar:index=1 , name=id , desc=Ljava/lang/String;, sig=null, start=L0, end=L1>

         L0 {
             aload 0 // reference to self
             invokespecial java/lang/Object.<init>()V
             aload 0 // reference to self
             aload 1
             putfield com/example/demo/AccountId.id:java.lang.String
             return
         }
         L1 {
         }
     }

     public static java.lang.String constructor-impl(java.lang.String arg0) { //(Ljava/lang/String;)Ljava/lang/String;
         <invisAnno:desc = Lorg/jetbrains/annotations/NotNull; , values = []>

         <invisLocalVarAnno:desc = Lorg/jetbrains/annotations/NotNull; , values = []>

         <localVar:index=0 , name=id , desc=Ljava/lang/String;, sig=null, start=L0, end=L1>

         L0 {
             aload 0
             ldc "id" (java.lang.String)
             invokestatic kotlin/jvm/internal/Intrinsics.checkNotNullParameter(Ljava/lang/Object;Ljava/lang/String;)V
             aload 0
             areturn
         }
         L1 {
         }
     }

That's fine so far. The problem is with the AccountNameCheck. Here the constructor is made private and a synthetic one is added.

public final class com/example/demo/AccountNameCheck {
     <ClassVersion=61>
     <SourceFile=DemoApplication.kt>

     private final java.lang.String accountId;
     private final java.lang.String accountName;
     private final java.lang.String id;

     private AccountNameCheck(java.lang.String arg0, java.lang.String arg1, java.lang.String arg2) { // <init> //(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V
         <localVar:index=0 , name=this , desc=Lcom/example/demo/AccountNameCheck;, sig=null, start=L0, end=L2>
         <localVar:index=1 , name=accountId , desc=Ljava/lang/String;, sig=null, start=L0, end=L2>
         <localVar:index=2 , name=accountName , desc=Ljava/lang/String;, sig=null, start=L0, end=L2>
         <localVar:index=3 , name=id , desc=Ljava/lang/String;, sig=null, start=L0, end=L2>

         L0 {
             aload 0 // reference to self
             invokespecial java/lang/Object.<init>()V
         }
         L1 {
             aload 0 // reference to self
             aload 1 // reference to arg0
             putfield com/example/demo/AccountNameCheck.accountId:java.lang.String
             aload 0 // reference to self
             aload 2 // reference to arg1
             putfield com/example/demo/AccountNameCheck.accountName:java.lang.String
             aload 0 // reference to self
             aload 3
             putfield com/example/demo/AccountNameCheck.id:java.lang.String
             return
         }
         L2 {
         }
     }

...

     public synthetic AccountNameCheck(java.lang.String arg0, java.lang.String arg1, java.lang.String arg2, kotlin.jvm.internal.DefaultConstructorMarker arg3) { // <init> //(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
         <localVar:index=0 , name=this , desc=Lcom/example/demo/AccountNameCheck;, sig=null, start=L0, end=L1>
         <localVar:index=1 , name=accountId , desc=Ljava/lang/String;, sig=null, start=L0, end=L1>
         <localVar:index=2 , name=accountName , desc=Ljava/lang/String;, sig=null, start=L0, end=L1>
         <localVar:index=3 , name=id , desc=Ljava/lang/String;, sig=null, start=L0, end=L1>
         <localVar:index=4 , name=$constructor_marker , desc=Lkotlin/jvm/internal/DefaultConstructorMarker;, sig=null, start=L0, end=L1>

         L0 {
             aload 0 // reference to self
             aload 1 // reference to arg0
             aload 2 // reference to arg1
             aload 3 // reference to arg2
             invokespecial com/example/demo/AccountNameCheck.<init>(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V
             return
         }
         L1 {
         }
     }

Also the above has no constructor-impl or anything like it.

Comparing the above to an AccountNameCheck where the AccountId isn't an inline class shows the difference where the original constructor of AccountNameCheck is not turned into a private one and no synthetic ctor is added.

class AccountId(val id: String)

data class AccountNameCheck(val accountId: AccountId, val accountName: String, val id: String)

public final class com/example/demo/AccountNameCheck {
     <ClassVersion=61>
     <SourceFile=DemoApplication.kt>

     private final com.example.demo.AccountId accountId;
     private final java.lang.String accountName;
     private final java.lang.String id;

     public AccountNameCheck(com.example.demo.AccountId arg0, java.lang.String arg1, java.lang.String arg2) { // <init> //(Lcom/example/demo/AccountId;Ljava/lang/String;Ljava/lang/String;)V
         <localVar:index=0 , name=this , desc=Lcom/example/demo/AccountNameCheck;, sig=null, start=L0, end=L3>
         <localVar:index=1 , name=accountId , desc=Lcom/example/demo/AccountId;, sig=null, start=L0, end=L3>
         <localVar:index=2 , name=accountName , desc=Ljava/lang/String;, sig=null, start=L0, end=L3>
         <localVar:index=3 , name=id , desc=Ljava/lang/String;, sig=null, start=L0, end=L3>

         L0 {
             aload 1 // reference to arg0
             ldc "accountId" (java.lang.String)
             invokestatic kotlin/jvm/internal/Intrinsics.checkNotNullParameter(Ljava/lang/Object;Ljava/lang/String;)V
             aload 2 // reference to arg1
             ldc "accountName" (java.lang.String)
             invokestatic kotlin/jvm/internal/Intrinsics.checkNotNullParameter(Ljava/lang/Object;Ljava/lang/String;)V
             aload 3
             ldc "id" (java.lang.String)
             invokestatic kotlin/jvm/internal/Intrinsics.checkNotNullParameter(Ljava/lang/Object;Ljava/lang/String;)V
         }
         L1 {
             aload 0 // reference to self
             invokespecial java/lang/Object.<init>()V
         }
         L2 {
             aload 0 // reference to self
             aload 1 // reference to arg0
             putfield com/example/demo/AccountNameCheck.accountId:com.example.demo.AccountId
             aload 0 // reference to self
             aload 2 // reference to arg1
             putfield com/example/demo/AccountNameCheck.accountName:java.lang.String
             aload 0 // reference to self
             aload 3
             putfield com/example/demo/AccountNameCheck.id:java.lang.String
             return
         }
         L3 {
         }
     }

[elizarov]

I see. I've misidentified your current problem. I believe we have a corresponding issue. Let me find it and see if we can raise its priority. I'll get back to you.

[elizarov]

Let me explain the logic behind the behavior for constructors accepting inline classes. An inline class may encapsulate arbitrary code and checks in its constructor. For example, the aforementined class AccountId(val id: String) might require that id is not empty or matches a specific pattern like this:

@JvmInline
value class AccountId(val id: String) {
    init { require(id.isNotEmpty()) }
}

The corresponding constructor logic gets compiled into constructor-impl. However, when we pass the instance of this inline class around, including the case where it is passed into constructors of other classes, we pass its underlying value id: String, like in this example you've given:

data class AccountNameCheck(val accountId: AccountId, val accountName: String, val id: String)

If the constructor of AccountNameCheck was simply public on JVM, then someone could accidentally pass an arbitrary String from Java source file, thus bypassing the checks in the AccountId constructor. That's why we take measures to hide the actual constructor from the Java code. The "private" constructor you see is just an implementation detail of this hiding and might be changed in any Kotlin version. However, the synthetic constructor with an additional kotlin.jvm.internal.DefaultConstructorMarker parameter is, in fact, a stable ABI contract of how such constructors are compiled and this ABI cannot be changed in the future. It means that you can rely on this ABI.

This constructor is marked as synthetic so that it is not accessible from Java sources. It has an additional parameter, so that it does not conflict with any signature of any constructor that a Kotlin class can potentially define. It is perfectly available via Java reflection. All you have to do is to call this synthetic constructor, passing null as the value of the last parameter. That is exactly what Kotlin compiler generates for Kotlin code that invokes AccountNameCheck constructor.

[christophstrobl]

Thank you @elizarov for the detailed explanation!

Is there a way to map the constructor parameter names from the koltin type to the java?
Because in this case we'd have 4 java parameters but only 3 koltin ones.

Let me explain what we're doing right now.
First, we're looking up the java constructor

KFunction<?> primaryConstructor = KClasses.getPrimaryConstructor(JvmClassMappingKt.getKotlinClass(AccountNameCheck.class));
Constructor<?> javaConstructor = ReflectJvmMapping.getJavaConstructor(primaryConstructor);

which returns public AccountNameCheck(String,String,String,DefaultConstructorMarker) as you mentioned above.
Here the parameter names resolve to arg0, arg, arg2, arg3.

So in order to access the original parameter names we go ahead and obtain the koltin function via ReflectJvmMapping.getKotlinFunction(javaConstructor) to gain more insight (I know we could use the primaryConstructor right away and get the same result) and receive accountId, accountName, id. This leaves us with one unresolved parameter name.

So maybe the way we try to resolve parameter names needs an update to cover the mismatch, and either assign the java parameter name arg3 to the missing slot or something like $defaultConstructorMarker if the parameter type matches the markers one.

[elizarov]

christophstrobl You can safely ignore the DefaultConstructorMarker parameter and assign any name you want to it. It is just a synthetic parameter anyway. You'll never pass any actual value to this parameter, always null.