FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.51k stars 1.37k forks source link

Question: DefaultTyping and java records/kotlin data classes #4636

Open ThanksForAllTheFish opened 1 month ago

ThanksForAllTheFish commented 1 month ago

Describe your Issue

Java records are final, as well as Kotlin data classes; in version 2.17, DefaultTyping deprecates the EVERYTHING option (from https://github.com/FasterXML/jackson-databind/issues/4160), however there are legit use cases to keep it, ie: when generic deserialization is needed for framework integration.

Is it possible to reverse the decision an "undeprecate" DefaultTyping.EVERYTHING?

For instance, in the following example (Kotlin because I got used to Kotlin, but I think the same applies to Java records too), using Redis in combination spring-data-redis as a cache, I could achieve proper generic deserialization only by using DefaultTyping.EVERYTHING.

@Caching(
    Cacheable(
      DISTRIBUTED_REDIS_CACHE,
      cacheManager = DISTRIBUTED_CACHE_MANAGER
    ),
  ]
)
open fun getPolicy(objectId: ObjectId): SomeDataClass { ... }

data class SomeDataClass(val value: String)

@Bean(DISTRIBUTED_CACHE_MANAGER)
  fun distributedCacheManager(
    redisConnectionFactory: RedisConnectionFactory,
    objectMapper: ObjectMapper
  ): RedisCacheManager = RedisCacheManager.builder(redisConnectionFactory)
    .withCacheConfiguration(
      DISTRIBUTED_REDIS_CACHE,
      RedisCacheConfiguration.defaultCacheConfig()
        .serializeValuesWith(
          fromSerializer(
            GenericJackson2JsonRedisSerializer(
              objectMapper.copy().activateDefaultTyping(objectMapper.polymorphicTypeValidator, ObjectMapper.DefaultTyping.EVERYTHING) //activating EVERYTHING on default typing, otherwise generic deser results in `LinkedHashMap
            )
          )
        )
    )
    .build()

Disclaimer: there might another way to get generic deserialization working, I experimented for a while and could only come up with this, but in no way I claim that this is the only solution.

Please also let me know if you think this is better addressed in Spring/Redis, to be honest I think something might be improved there to achieve the same result

cowtowncoder commented 1 month ago

No; addition of EVERYTHING was a mistake as far as I can see.

But note that deprecation will not mean dropping of the option during 2.x -- it will remain. Will be removed from 3.0 tho.

As to better mechanism: to me, use of simple wrapper makes sense to completely avoid Default Typing usage:

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

this will add type information for every value similar to EVERYTHING

ThanksForAllTheFish commented 1 month ago

thanks for the feedback, and thanks for taking the time to provide an alternative

dannywils commented 3 weeks ago

@cowtowncoder can you provide a full example of how to generically use the wrapper to avoid Default Typing usage?

cowtowncoder commented 3 weeks ago

@dannywils I included example above:

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

and you'd create wrapped value, serialize/deserialize it as expected with ObjectMapper.writeValue() / ObjectMapper.readValue(), extract value.

If you have specific question, please elaborate. I don't know your usage or expectations.

dannywils commented 1 week ago

@cowtowncoder I’ll elaborate on my question.

In my case, similar to the original poster, I’m using the spring-data-redis integration in which the serialization method is configured as follows:

RedisCacheConfiguration.defaultCacheConfig()
        .serializeValuesWith(
          fromSerializer(
            GenericJackson2JsonRedisSerializer(
              objectMapper
            )
          )
        )

The @Cacheable annotation can be used on any method, and so the return type of the method is not known by the caching implementation and it needs to work in a generic way, even for Kotlin data classes which are final.

@Cacheable
fun getAll(): Set<MyCustomDataClass>
@Cacheable
fun getSingle(): MyCustomDataClass

Using EVERYTHING, the objectMapper would be configured as follows:

val objectMapper = jacksonObjectMapper()
            .activateDefaultTyping(
                jacksonObjectMapper().polymorphicTypeValidator,
                // activating EVERYTHING on default typing, otherwise deserialization results in `LinkedHashMap`,
                // or fails due to Kotlin data classes being final.
                ObjectMapper.DefaultTyping.EVERYTHING,
                JsonTypeInfo.As.PROPERTY
            )

But, EVERYTHING is deprecated, so I’m trying to follow your example of wrapping each class in a generic way. Are you suggesting that all return values of all methods annotated with @Cacheable are modified to return the Wrapper class?

val objectMapper = jacksonObjectMapper() // no default typing
data class Wrapper (
    @JsonTypeInfo(use= JsonTypeInfo.Id.CLASS, include=JsonTypeInfo.As.PROPERTY, property=“class”)
    val value: Any
)

@Cacheable
fun getAll(): Wrapper // wraps Set<MyCustomDataClass>

@Cacheable
fun getSingle(): Wrapper // wraps MyCustomDataClass

Or are you suggesting something different?

yawkat commented 1 week ago

You don't need to modify all your Cachable sites. You can just wrap the object you're serializing in the Serializer implementation.

cowtowncoder commented 6 days ago

I am not familiar with the package, or use of @Cachable. But yes, either non-generic Wrapper (to be used with literally any value type), or, if it is more convenient, Wrapper<MyCustomDataClass> for specific type. You just need to attach @JsonTypeInfo to polymorphic value without DefaultTyping.

jebeaudet commented 3 days ago

We also just ran into this problem using RedisTemplate from spring data for a generic cache implementation. Spring uses an unchecked operation here using the RedisSerializer that is a raw instance. Note that this is a trusted cache so nothing cached comes from untrusted input.

So I'm using a simple RedisSerializer<Object> implementation and leverage the type annotations provided by the activateDefaultTyping method. This works well, but it does not for records :

public class JsonRedisSerializer implements RedisSerializer<Object>
{
    private final ObjectMapper objectMapper;

    public JsonRedisSerializer()
    {
        this.objectMapper = getObjectMapper();
    }

    static ObjectMapper getObjectMapper()
    {
        return JsonMapper.builder()
                         // other options here
                         .activateDefaultTyping(LaissezFaireSubTypeValidator.instance,
                                                ObjectMapper.DefaultTyping.NON_FINAL_AND_ENUMS,
                                                JsonTypeInfo.As.WRAPPER_ARRAY)
                         .build();
    }

    @Override
    public byte[] serialize(Object t) throws SerializationException
    {
        try {
            return objectMapper.writeValueAsBytes(t);
        } catch (JsonProcessingException e) {
            throw new SerializationException(e.getMessage(), e);
        }
    }

    @Override
    public Object deserialize(byte[] bytes) throws SerializationException
    {
        if (bytes == null) {
            return null;
        }

        try {
            return objectMapper.readValue(bytes, Object.class);
        } catch (Exception e) {
            throw new SerializationException(e.getMessage(), e);
        }
    }
}

Would having a DefaultTyping that includes records could make sense?

I understand the wrapper workaround, however, it is not backward compatible so it would involve a lengthy migration or a cache key prefix change which is not always feasable.

TIA!

cowtowncoder commented 3 days ago

I would like to avoid changes to DefaultTyping set, in general. And since Records are not extensible, they would not seem like great candidates for inclusion of polymorphic type information. Fundamentally, tho, the problem comes back to type erasure wrt Root values; when writing base type is taken as value.getClass() instead of what you'd normally want (Object.class here).

If anything, it'd be nice to make it easier to have custom definitions, since needs vary a lot. But I do not have concrete ideas of how that could work. (it is already possible to have custom criteria but if I remember correctly it is not super easy to do, compared to use of pre-created set of enum values).

Aside from that, I wonder what'd happen with something like:

// something like this (not sure if it's `writerFor()` or `writer().forType(...)`)
return objectMapper.writerFor(Object.class)
     .writeValueAsBytes(t);