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

Serialization and Deserialization of Kotlin data class fails on PolymorphicTypeValidator with Any #819

Open
1 task done
effx13 opened this issue Jul 14, 2024 · 15 comments

Comments

@effx13
Copy link

effx13 commented Jul 14, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

When trying to deserialize class that contains kotlin value class, Jackson throws InvalidTypeIdException with missing type id property '@class' on ObjectMapper.DefaultTyping.NON_FINAL
Despite adding @JsonCreator annotation, I got the same result.

And trying to serialize above class with ObjectMapper.DefaultTyping.NON_FINAL, Jackson throws JsonMappingException with class ServerName cannot be cast to class java.lang.String (ServerName is in unnamed module of loader 'app'; java.lang.String is in module java.base of loader 'bootstrap') (through reference chain: TestDto["serverName"])

I'm using PolymorphicTypeValidator and Any class to do serialization and deserialization in Redis. But no matter what settings and annotations I use, the serialization and deserialization fails.

Version Information

JVM 21
Kotlin 1.8

jackson-core:2.17.2
jackson-databind:2.17.2
jackson-annotations:2.17.2
jackson-datatype-jsr310:2.17.2
jackson-module-kotlin:2.17.2

Reproduction

@JvmInline
value class ServerName(
  val value: String,
) {
  companion object {
    @JsonCreator
    @JvmStatic
    fun fromValue(value: String): ServerName {
      return ServerName(value)
    }
  }
}

data class TestDto(
  val id: Int,
  val serverName: ServerName
)

val objectMapper = ObjectMapper()
  .registerKotlinModule()
  .registerModule(JavaTimeModule())
  .activateDefaultTyping(
    BasicPolymorphicTypeValidator.builder().allowIfBaseType(Any::class.java).build(),
    ObjectMapper.DefaultTyping.NON_FINAL, // or EVERYTHING
    JsonTypeInfo.As.PROPERTY,
  )

// on NON_FINAL
fun main() {
  val serverName = ServerName("TEST")
  val testDto = TestDto(1, serverName)

  val serialized = objectMapper.writeValueAsBytes(testDto)
  println(String(serialized))
  // Expected output: {"id":1,"serverName":"TEST"}
  // Actual output: {"id":1,"serverName":{"value":"TEST"}}

  val deserialized = objectMapper.readValue(serialized, Any::class.java) // Because of RedisSerializer
  println(deserialized)
  // Expected output: TestDto(id=1, serverName=ServerName(value=TEST))
  // Actual output: Exception in thread "main" com.fasterxml.jackson.databind.exc.InvalidTypeIdException: Could not resolve subtype of [simple type, class java.lang.Object]: missing type id property '@class'
}

// on EVERYTHING
fun main() {
  val serverName = ServerName("TEST")
  val testDto = TestDto(1, serverName)

  val serialized = objectMapper.writeValueAsBytes(testDto)
  println(String(serialized))
  // Expected output: {"id":1,"serverName":"TEST"}
  // Actual output: {"id":1,"serverName":{"value":"TEST"}}

  val deserialized = objectMapper.readValue(serialized, Any::class.java) // Because of RedisSerializer
  println(deserialized)
  // Expected output: TestDto(id=1, serverName=ServerName(value=TEST))
  // Actual output: Exception in thread "main" com.fasterxml.jackson.databind.JsonMappingException: class ServerName cannot be cast to class java.lang.String (ServerName is in unnamed module of loader 'app'; java.lang.String is in module java.base of loader 'bootstrap') (through reference chain: TestDto["serverName"])
}

Expected behavior

No response

Additional context

No response

@effx13 effx13 added the to-evaluate Issue that has been received but not yet evaluated label Jul 14, 2024
@cowtowncoder
Copy link
Member

Kotlin issues belong under jackson-module-kotlin in general, will transfer.

@k163377
Copy link
Contributor

k163377 commented Oct 13, 2024

Created a PR because I found a problem with databind.
FasterXML/jackson-databind#4749

Will check again after this is merged.

@k163377 k163377 added enhancement and removed to-evaluate Issue that has been received but not yet evaluated labels Oct 14, 2024
@k163377
Copy link
Contributor

k163377 commented Oct 14, 2024

Checked.

This is a new feature addition and an implementation policy should be discussed.
I am not familiar with this feature and would like to hear any opinions on the implementation policy.


@effx13 has submitted “serverName”: “TEST” as an expectation, but I disagree with this for now.

kotlin-module treats properties defined in value class as if they were typed.
If so, shouldn't “serverName”:[“${type name}”, “TEST”] be the expected value?

As for the implementation, the base class of ValueClassUnboxSerializer will be changed to StdScalarSerializer.
The following is a tentative test for the prototype.

import com.fasterxml.jackson.annotation.JsonCreator
import com.fasterxml.jackson.annotation.JsonTypeInfo
import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.databind.jsontype.BasicPolymorphicTypeValidator
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule
import com.fasterxml.jackson.module.kotlin.registerKotlinModule
import kotlin.test.Test

class GitHub819 {
    @JvmInline
    value class ServerName(val value: String) {
        companion object {
            @JsonCreator
            @JvmStatic
            fun fromValue(value: String): ServerName {
                return ServerName(value)
            }
        }
    }

    data class TestDto(
        val serverName: ServerName
    )

    // on EVERYTHING
    @Test
    fun everything() {
        val objectMapper = ObjectMapper()
            .registerKotlinModule()
            .registerModule(JavaTimeModule())
            .activateDefaultTyping(
                BasicPolymorphicTypeValidator.builder().allowIfBaseType(Any::class.java).build(),
                ObjectMapper.DefaultTyping.EVERYTHING,
                JsonTypeInfo.As.PROPERTY,
            )

        val serverName = ServerName("TEST")
        val testDto = TestDto(serverName)

        val serialized = objectMapper.writeValueAsString(testDto)
        // -> {"@class":"com.fasterxml.jackson.module.kotlin.test.github.GitHub819$TestDto","serverName":["com.fasterxml.jackson.module.kotlin.test.github.GitHub819$ServerName","TEST"]}
        println(serialized)

        val deserialized = objectMapper.readValue(serialized, Any::class.java) // Because of RedisSerializer
        // -> TestDto(serverName=ServerName(value=TEST))
        println(deserialized)
    }
}

As a side note, as far as the prototype is concerned, I feel that it would be difficult to implement in any other way.

@effx13
Copy link
Author

effx13 commented Oct 14, 2024

@k163377
Thank you for check my issue.

I agree, as you said, in BasicPolymorphicTypeValidator it should come out as “serverName”:[“${type name}”, “TEST”].
so is it correct that this feature is not implemented yet? I tried various methods, but I was unable to serialization and deserialization on BasicPolymorphicTypeValidator.

@k163377
Copy link
Contributor

k163377 commented Oct 14, 2024

I agree, as you said, in BasicPolymorphicTypeValidator it should come out as “serverName”:[“${type name}”, “TEST”].

👍

so is it correct that this feature is not implemented yet?

First, a version that incorporates the fix for the bug is required(2.18.1 or later).

After that, you may be able to solve your use case by setting up a custom serializer based on StdScalarSerializer.

As for the kotlin-module, the changes could be small, but require a lot of testing.

@cowtowncoder
Copy link
Member

Quick note: 2.18.1 not yet released; will be released in near future (2-3 weeks), but need to combine with other fixes.

@k163377
Copy link
Contributor

k163377 commented Dec 31, 2024

@cowtowncoder
I would like to fix this problem, but I am not familiar with PolymorphicTypeValidator, so I have a some questions.

First, I think the work is basically just to change the parent class from StdSerializer to StdScalarSerializer.
As for verification, I think it is sufficient to test that the results of the processing of the target match the results of the normal class for all combinations of ObjectMapper.DefaultTyping and JsonTypeInfo.As.
Is this correct?

I was also checking the implementation of AtomicIntegerSerializer to fix the Kotlin specific numeric types (Unsigned Integer types), and I found that acceptJsonFormatVisitor is implemented for each of them.
https://github.com/FasterXML/jackson-databind/blob/9e63036ba7719639e2cd1ab1e33eef0eb669a517/src/main/java/com/fasterxml/jackson/databind/ser/std/StdJdkSerializers.java#L130-L135

I found that kotlin-module does not implement this, is there any problem?

object UIntSerializer : StdSerializer<UInt>(UInt::class.java) {
private fun readResolve(): Any = UIntSerializer
override fun serialize(value: UInt, gen: JsonGenerator, provider: SerializerProvider) =
gen.writeNumber(value.toLong())
}

@cowtowncoder
Copy link
Member

Ok, first, missing acceptJsonFormatVisitor would only affect Schema (JSON Schema, protoc schema, Avro schema) generation, not regular serialization. But ideally that method would be implemented.

@cowtowncoder
Copy link
Member

Second: PolymorphicTypeValidator is only relevant wrt security; it is used to allow or deny polymorphic deserialization. So it only needs to be set for testing purposes (in this case) and should not matter wrt actual problem.

And yes, testing over different combinations makes sense.

FWTW, I am not 100% convinced original reporter really should (have to) use Default Typing -- almost in every case it'd be much better to use a Wrapper class with @JsonTypeInfo like so:

public class Wrapper {
  @JsonTypeInfo(...)
  public BaseType value; // `BaseType` may be `Any` or `Object`, f.ex.
}

to avoid complexities of Default Typing applicability and need for PolymorphicTypeValidator.

But for some reason Redis users keep on relying on Default Typing it seems.

@k163377
Copy link
Contributor

k163377 commented Jan 1, 2025

@cowtowncoder
Thanks for the reply.
I will consider about the modification policy.

@k163377
Copy link
Contributor

k163377 commented Jan 2, 2025

I did a little research and found it quite difficult to support this.
If a property that returns a value class is defined as non-null, the PolymorphicTypeValidator will not work.

The cause is that the value class compiles to a different representation than the normal class, i.e. the representation obtained during Jackson processing does not match the representation on Kotlin.
The kotlin-module absorbs this difference in representation through various implementations, but it is not perfect.


The following is what I have confirmed.

First, the direct cause is that serializeWithType is not called from BeanPropertyWriter.serializeAsField because BeanPropertyWriter._typeSerializer is null.
https://github.com/FasterXML/jackson-databind/blob/6729641f6a5e9c40c428328e73f5281b8ce529bb/src/main/java/com/fasterxml/jackson/databind/ser/BeanPropertyWriter.java#L731-L735

The BeanPropertyWriter._typeSerializer is null because the BeanSerializerFactory.findPropertyTypeSerializer does not process properties as value class.
https://github.com/FasterXML/jackson-databind/blob/6729641f6a5e9c40c428328e73f5281b8ce529bb/src/main/java/com/fasterxml/jackson/databind/ser/BeanSerializerFactory.java#L308-L319

Theoretically, this problem can be solved if one of the following can be accomplished

  1. Modify the whole thing to handle the case where _typeSerializer is null
  2. Add a process that converts baseType to value class
  3. Implement AnnotationIntrospecutor.findPropertyTypeResolver and TypeResolverBuilder

Of these, I consider 1 to be impractical because its sphere of influence is too broad.
The same goes for 2.

As for 3, I feel it will take time to implement and validate due to lack of knowledge about TypeSerializer.
I am sorry, but I am not going to fix this issue as a priority at this time, as there are many other tasks that need to be done.

@cowtowncoder
Copy link
Member

TypeSerializer is set to non-null if (and only if) type being serialized is expected to use polymorphic type ids. So I don't think you should have to do anything wrt that handling.
But it'd be necessary to know why value class is not considered to be polymorphic.
I am guess that value type is final and there-by

Once again, I think DefaultTyping.NON_FINAL will not include it on serialization.
But when reading, nominal type is Any which WOULD require type id.

Once again, submitted should consider wrapper type I outlined earlier.

Or, alternatively specify "base type" on writing, instead of relying on runtime exact (implementation) type: something like

TestDto value = ...;
// Assuming actual runtime type of `value` is a generated implementation of TestDto?
ObjectWriter w = mapper.writerFor(TestDto.class);

because otherwise type used for determining polymorphic nature (or not) is value.getClass() which I gather is different from declared Kotlin value class.
(this based on your comment wrt Kotlin handling of value class)

I hope above makes sense.

@cowtowncoder
Copy link
Member

Btw, I am not arguing you should work on this @k163377 . But as usual, if there was any way to reduce this to Java-only reproduction, I could have a look. I realize this may not be possible, but if it is.

@k163377
Copy link
Contributor

k163377 commented Jan 5, 2025

@cowtowncoder

But it'd be necessary to know why value class is not considered to be polymorphic.

In kotlin-module, serialization is achieved by a hack using findSerializationConverter and others.

is AnnotatedMethod -> a.findValueClassReturnType()?.let {
if (useJavaDurationConversion && it == Duration::class) {
if (a.rawReturnType == Duration::class.java)
KotlinToJavaDurationConverter
else
KotlinDurationValueToJavaDurationConverter
} else {
cache.getValueClassBoxConverter(a.rawReturnType, it)
}
}

Processes related to JsonSerialize and so on were processed using the results of conversion by the converter, but it seems that they are not done here.
Is it possibly a bug that the result of the findSerializationConverter is not applied to the baseType passed to the findPropertyTypeSerializer?

@cowtowncoder
Copy link
Member

Hmmh. Converters are really problematic since question then is whether decision should be based on original or converted type.

But my initial thinking is that no converter should be used for base type as that is what polymorphic handling is based on. That is, for example:

public class Wrapper {
   @JsonTypeInfo(...)
   public Object anyValue;
}

in this case, "baseType" is (and needs to be) java.lang.Object, but actual runtime type may be something else.

"Value" serializer is based on actual type (unless static typing is forced), and converter is to be applied as well (although, TBH, combination of polymorphic handling and converter is always challenging).

But TypeSerializer is (and needs to be) fetched for "baseType". This so that all actual values use same polymorphic handling settings which is required on deserialization -- there is no runtime type (which is the whole point of polymorphic handling).

So no converter should be applied to "baseType": it is only used to locate TypeSerializer (on serialization, and TypeDeserializer for deserialization).

I hope this makes more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants