Skip to content

Improvements to serialization of value class. #527

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

Merged
merged 7 commits into from
Jan 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions release-notes/CREDITS-2.x
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ wrongwrong (k163377@github)
* #456: Refactor KNAI.findImplicitPropertyName()
* #449: Refactor AnnotatedMethod.hasRequiredMarker()
* #521: Fixed lookup of instantiators
* #527: Improvements to serialization of `value class`.

Dmitri Domanine (novtor@github)
* Contributed fix for #490: Missing value of type JsonNode? is deserialized as NullNode instead of null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ import java.lang.reflect.AccessibleObject
import java.lang.reflect.Constructor
import java.lang.reflect.Field
import java.lang.reflect.Method
import kotlin.reflect.KClass
import kotlin.reflect.KFunction
import kotlin.reflect.KMutableProperty1
import kotlin.reflect.KProperty1
import kotlin.reflect.KType
import kotlin.reflect.full.createType
import kotlin.reflect.full.declaredMemberProperties
import kotlin.reflect.full.memberProperties
import kotlin.reflect.jvm.*


Expand Down Expand Up @@ -59,6 +61,43 @@ internal class KotlinAnnotationIntrospector(private val context: Module.SetupCon
return super.findCreatorAnnotation(config, a)
}

// Find a serializer to handle the case where the getter returns an unboxed value from the value class.
override fun findSerializer(am: Annotated): ValueClassBoxSerializer<*>? = when (am) {
is AnnotatedMethod -> {
val getter = am.member.apply {
// If the return value of the getter is a value class,
// it will be serialized properly without doing anything.
if (this.returnType.isUnboxableValueClass()) return null
}

val kotlinProperty = getter
.declaringClass
.kotlin
.let {
// KotlinReflectionInternalError is raised in GitHub167 test,
// but it looks like an edge case, so it is ignored.
try {
it.memberProperties
} catch (e: Error) {
null
}
}?.find { it.javaGetter == getter }

(kotlinProperty?.returnType?.classifier as? KClass<*>)
?.takeIf { it.isValue }
?.java
?.let { outerClazz ->
@Suppress("UNCHECKED_CAST")
ValueClassBoxSerializer(outerClazz, getter.returnType)
}
}
// Ignore the case of AnnotatedField, because JvmField cannot be set in the field of value class.
else -> null
}

// Perform proper serialization even if the value wrapped by the value class is null.
override fun findNullSerializer(am: Annotated) = findSerializer(am)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually work as expected? Call should only be made if the value class itself is null; not for wrapped value. I may be wrong since I am not familiar with types here, just suggest validation of all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the property itself is null, the return value from the java getter will be in an unboxed state, so the ValueClassBoxSerializer will not be used.
This is verified by the following comparison.
https://github.com/FasterXML/jackson-module-kotlin/pull/527/files#diff-7c6b0a8c396b759fd146df36577c7a8fb3cbe94406e6912a729f6e028413411bR49-R51

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. As long as unit tests cover expected behavior it's probably fine.

One other note: "null serializer" is only ever used for writing null value or placeholder, and never anything more advanced. So one typically returns sort of special constant value serializing implementation, instead of delegating.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cowtowncoder @tatu-at-datastax
Sorry if I'm not reading this correctly.

The findNullSerializer returns a serializer for values that are non-null in Kotlin.
I meant to add a comment to explain that.

I think it might have been better to have a private function named something like findSerializerForValueClass and call the same function in findSerializer/findNullSerializer.
If you have any comments or implementations that are easier to understand, please let me know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is simply this: method findNullSerializer is called by Jackson only EVER when there is a literal JVM-level null to serialize. Regular serializers are NEVER given null values to serialize; they are not expected to (have to) do null checks, nor know what to serialize.

In that sense it seems/seemed odd that implementation would delegate to findSerializer(): I am not sure why this is done. It seems rather like it should not be defined at all.

But then again Kotlin module sometimes overrides things I do not expect it to, partially since null handling in Kotlin is quite different from plain Java.

I'll let you and Kotlin module owners decide here what to do; I just try to give input from Jackson databind side to help develop things :)


/**
* Subclasses can be detected automatically for sealed classes, since all possible subclasses are known
* at compile-time to Kotlin. This makes [com.fasterxml.jackson.annotation.JsonSubTypes] redundant.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import com.fasterxml.jackson.databind.SerializerProvider
import com.fasterxml.jackson.databind.ser.Serializers
import com.fasterxml.jackson.databind.ser.std.StdSerializer
import java.math.BigInteger
import kotlin.reflect.KClass

object SequenceSerializer : StdSerializer<Sequence<*>>(Sequence::class.java) {
override fun serialize(value: Sequence<*>, gen: JsonGenerator, provider: SerializerProvider) {
Expand Down Expand Up @@ -71,3 +72,20 @@ internal class KotlinSerializers : Serializers.Base() {
else -> null
}
}

// This serializer is used to properly serialize the value class.
// The getter generated for the value class is special,
// so this class will not work properly when added to the Serializers
// (it is configured from KotlinAnnotationIntrospector.findSerializer).
internal class ValueClassBoxSerializer<T : Any>(
private val outerClazz: Class<out Any>, innerClazz: Class<T>
) : StdSerializer<T>(innerClazz) {
private val boxMethod = outerClazz.getMethod("box-impl", innerClazz)

override fun serialize(value: T?, gen: JsonGenerator, provider: SerializerProvider) {
// Values retrieved from getter are considered validated and constructor-impl is not executed.
val boxed = boxMethod.invoke(null, value)

provider.findValueSerializer(outerClazz).serialize(boxed, gen, provider)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package com.fasterxml.jackson.module.kotlin.test.github

import com.fasterxml.jackson.core.JsonGenerator
import com.fasterxml.jackson.databind.SerializerProvider
import com.fasterxml.jackson.databind.annotation.JsonSerialize
import com.fasterxml.jackson.databind.module.SimpleModule
import com.fasterxml.jackson.databind.ser.std.StdSerializer
import com.fasterxml.jackson.module.kotlin.jacksonMapperBuilder
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
import org.junit.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotEquals

// Most of the current behavior has been tested on GitHub464, so only serializer-related behavior is tested here.
class GitHub524 {
@JvmInline
value class HasSerializer(val value: Int?)
class Serializer : StdSerializer<HasSerializer>(HasSerializer::class.java) {
override fun serialize(value: HasSerializer, gen: JsonGenerator, provider: SerializerProvider) {
gen.writeString(value.toString())
}
}

@JvmInline
value class NoSerializer(val value: Int?)

data class Poko(
// ULong has a custom serializer defined in Serializers.
val foo: ULong = ULong.MAX_VALUE,
// If a custom serializer is set, the ValueClassUnboxSerializer will be overridden.
val bar: HasSerializer = HasSerializer(1),
val baz: HasSerializer = HasSerializer(null),
val qux: HasSerializer? = null,
// If there is no serializer, it will be unboxed as the existing.
val quux: NoSerializer = NoSerializer(2)
)

@Test
fun test() {
val sm = SimpleModule()
.addSerializer(Serializer())
val writer = jacksonMapperBuilder().addModule(sm).build().writerWithDefaultPrettyPrinter()

// 18446744073709551615 is ULong.MAX_VALUE.
assertEquals(
"""
{
"foo" : 18446744073709551615,
"bar" : "HasSerializer(value=1)",
"baz" : "HasSerializer(value=null)",
"qux" : null,
"quux" : 2
}
""".trimIndent(),
writer.writeValueAsString(Poko())
)
}

class SerializeByAnnotation(@get:JsonSerialize(using = Serializer::class) val foo: HasSerializer = HasSerializer(1))

@Test
fun failing() {
val writer = jacksonObjectMapper().writerWithDefaultPrettyPrinter()

// JsonSerialize is not working now.
assertNotEquals(
"""
{
"foo" : "HasSerializer(value=1)"
}
""".trimIndent(),
writer.writeValueAsString(SerializeByAnnotation())
)
}
}