Serialization doesn't work correctly for objects parametrized by inline class

[sanyarnd]

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.module.kotlin.KotlinModule

inline class A(val value: Int = 0)
inline class B(val value: Int = 0)
inline class C(val value: Int = 0)
class D(val inlineField: A = A())

class Pojo(
    val works: A,
    val alsoWorks: B,
    val worksToo: Collection<D>,
    val broken: Collection<C>
)

val om = ObjectMapper().registerModule(KotlinModule())
println(om.writeValueAsString(Pojo(A(), B(), listOf(D(), D()), listOf(C(), C()))))

Output:

{
  "works": 0,
  "alsoWorks": 0,
  "worksToo": [ {"inlineField":0}, {"inlineField":0} ],
  "broken": [ {"value":0}, {"value":0} ]
}

Jackson version 2.12

[k163377]

If the policy is to always unbox a value class when serializing it, then adding a Serializer to KotlinSerializers seems to be the solution.
I've pushed a prototype of the fix to the following branch.

https://github.com/k163377/jackson-module-kotlin/tree/github_464
https://github.com/FasterXML/jackson-module-kotlin/compare/2.13...k163377:github_464?expand=1

[sanyarnd]

Looks good, because for now I'm using custom serializer as a quick workaroud :)

private class NodeIdSerializer : JsonSerializer<Collection<NodeId>>() {
    override fun serialize(value: Collection<NodeId>, gen: JsonGenerator, serializers: SerializerProvider) {
        return gen.writeArray(value.map(NodeId::value).toTypedArray(), 0, value.size)
    }
}

[dinomite]

@k163377 I like the looks of those branch changes, does that break any other tests?

[k163377]

@dinomite
First, with some changes, I was able to get to a situation where the existing tests would not fail.
https://github.com/FasterXML/jackson-module-kotlin/compare/2.13...k163377:github_464_pr_temp?expand=1

However, it seems to have the following problems.

• This change alone does not work when value class is set to key in Map
• If JsonSerializer is set in the target class, it is likely to be ignored

As for 1, the same problem occurs when sequence is set to key, so it looks like it can be ignored.
As for 2, although it works in the test I created, I'm not sure if it really doesn't cause problems because I don't know in what order the JsonSerializer is judged.

I also feel that there are hidden problems that I am not aware of...

Additional Context

Found a problem with @JsonValue not working when it is given to an instance method of value class (related to #199).

// works
@JvmInline
value class ValueByGetter(@get:JsonValue val value: Int)

// does not work
@JvmInline
value class ValueByMethod(val value: Int) {
    @JsonValue
    fun jsonValue() = value.toString()
}

This is due to the fact that this method is compiled into a static method and no instance method is created.

@JsonValue
@NotNull
public static final String jsonValue_impl/* $FF was: jsonValue-impl*/(int $this) {
   return String.valueOf($this);
}

Due to this problem, setting @JsonValue is currently synonymous with unboxing, but it looks like a new problem similar to #2 will arise when this problem is solved in the future.

[Quantum64]

Value classes will ultimately support multiple fields and compile to primitive classes on the JVM, when they are introduced. Value classes with multiple fields will not support @JvmInline, so ideally we should check for that annotation before auto-unboxing to avoid backward compatibility issues.

[k163377]

@Quantum64
I rely on machine translation to read and write English, so let me check if I'm misreading anything.

The @JvmInline annotation makes it explicit that something special with this class is going on in JVM, and it enables us to support non-annotated value class in the future Valhalla JVM by compiling them using the capabilities of the Project Valhalla.

I read this as "After Java primitive class is introduced by Project Valhalla, there will be a Kotlin value class without @JvmInline (regardless of the number of fields it contains)".
If that's the case, I think you're right that we should include @JvmInline in the decision, since unconditional unboxing could cause problems in the future.
If I were to create a PR, I would rewrite the decision as follows.

type.rawClass.let { clazz -> clazz.annotations.any { it is JvmInline } && clazz.isKotlinClass() } ->
    ValueClassUnboxSerializer

However, I could not read from this document that @JvmInline is never given to Kotlin value class with multiple fields.
Rather, I read from the chapter on Multifield value classes before Valhalla that there is a slight possibility that a Kotlin value class with multiple fields given @JvmInline will also appear.

Am I understanding you correctly so far? Or am I misreading your comments?

[Quantum64]

Yes, I agree with everything you said. After reading a second time, I agree that the language in the specification is not clear enough to completely rule out @JvmInline classes with multiple fields, so I asked the question in the Kotlin Slack channel. We will see if anyone from JetBrains gets back to me.

[k163377]

@Quantum64
Thanks a lot!

If this pattern were to be strictly avoided, it would seem that the number of Java fields would be included in the decision, but that doesn't seem to be a good idea at this point, either in terms of execution cost or code complexity.
If it is unlikely at this point that this will be added, and if this cannot be accomplished with low cost and simple code, I felt that just leaving the context in the comments would be sufficient.

[Quantum64]

I haven't heard back from JB yet but someone else brought up a good point. It would be impossible to use a @JvmInline class with multiple fields as a return type because the fields could not be inlined into the single return type required by the JVM. I think based on this, it is safe to assume that @JvmInline classes will only ever have one field.

[dinomite]

With the merging of #468, value classes should work except as Map keys, in version 2.13. Unfortunately this project's snapshot builds aren't working right now, but if you add the Sonatype snapshot repository you'll be able to use 2.13-SNAPSHOT when they're back up (I'll comment here when that happens).

[cowtowncoder]

@dinomite Can you get snapshots to deploy with mvn deploy, or do you hit an access problem? I'll push a snapshot now, either way.

[dinomite]

@cowtowncoder I cannot, I don't have access to the Jackson Sonatype repo

[cowtowncoder]

@dinomite could you file an issue at https://issues.sonatype.org/ for type OSSRH ? Or actually I could do it, if you could create an account at that Jira tracker (just so I can at-reference you). It should be possible to grant you access just to Kotlin module, I think; same was done wrt Scala module.

[cowtowncoder]

Ah. Here's the request for Scala module:

https://issues.sonatype.org/browse/OSSRH-51205

which would be the way to do it. I think that the account for Sonatype Jira also works for their Nexus, which is why this should work quite fine. I just need to ok your request.

[dinomite]

@cowtowncoder Issued https://issues.sonatype.org/browse/OSSRH-70497

[cowtowncoder]

@dinomite Thanks!

[k163377]

@dinomite
The test I added in #468 seemed to need a few more patterns.

Before the #468 merge, the following three situations will output values that are not unboxed.

• If return a value class that inherits from interface as `interface.
• If the generic abstract getterlike function returns a value class.
• When a nullable value class field is set to a non-null value.

Verification code

import com.fasterxml.jackson.annotation.JsonPropertyOrder
import com.fasterxml.jackson.databind.ObjectWriter
import org.junit.Test

class Github464AdditionalTests {
    interface IValue
    @JvmInline
    value class ValueClass(val value: Int) : IValue

    abstract class AbstractGetter<T> {
        abstract val byAbstractGenericVal: T

        fun <T> getByAbstractGenericFun() = byAbstractGenericVal
    }
    interface IGetter<T> {
        val byInterfaceGenericVal: T

        fun <T> getByInterfaceGenericDefaultFun() = byInterfaceGenericVal
    }

    @JsonPropertyOrder(alphabetic = true)
    data class BoxedFields(
        val asInterface: IValue,
        override val byAbstractGenericVal: ValueClass,
        override val byInterfaceGenericVal: ValueClass,
        val nullableValue: ValueClass?
    ) : AbstractGetter<ValueClass>(), IGetter<ValueClass> {
        constructor(value: ValueClass) : this(value, value, value, value)
    }

    @Test
    fun test() {
        val target = BoxedFields(ValueClass(1))
        val writer: ObjectWriter = jacksonObjectMapper()
            .writerWithDefaultPrettyPrinter()

        println(writer.writeValueAsString(target))
    }
}

output

{
  "asInterface" : {
    "value" : 1
  },
  "byAbstractGenericFun" : {
    "value" : 1
  },
  "byAbstractGenericVal" : 1,
  "byInterfaceGenericDefaultFun" : 1,
  "byInterfaceGenericVal" : 1,
  "nullableValue" : {
    "value" : 1
  }
}

The following materials were used as a reference.
https://kotlinlang.org/docs/inline-classes.html#representation

The change in #468 will also output unboxed values in these situations.
Since this is a test-only change, I will clean up the tests and create another PR after the merge of #470.

Also, with the future update of Kotlin, it seems that giving @JvmField to a field of value class will cause an error, so this point will be fixed as well.

[dinomite]

@k163377 Sorry for the delays in getting back to this and the related PRs—I intend to pick this up in the next few weeks.

[dinomite]

Merged #470.

[dinomite]

A new snapshot build is up

[dinomite]

@k163377 #470 has been merged, will you create that test PR please?

[k163377]

I have created it and would appreciate your review.
#505

[cowtowncoder]

I think merge to master produced some compilation errors (JsonSerializer 2.x -> ValueSerializer 3.0); I tried to resolve some but a bit remains.

[dinomite]

@cowtowncoder Sorry I hadn't fixed those in the master merge, all better now!

[cowtowncoder]

np!