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

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

Open
ThanksForAllTheFish opened this issue Jul 23, 2024 · 10 comments
Open

Comments

@ThanksForAllTheFish
Copy link

ThanksForAllTheFish commented Jul 23, 2024

Describe your Issue

Java records are final, as well as Kotlin data classes; in version 2.17, DefaultTyping deprecates the EVERYTHING option (from #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

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

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
Copy link
Author

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

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Aug 11, 2024
@dannywils
Copy link

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

@cowtowncoder
Copy link
Member

@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
Copy link

@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
Copy link
Member

yawkat commented Sep 2, 2024

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

@cowtowncoder
Copy link
Member

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
Copy link

jebeaudet commented Sep 5, 2024

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
Copy link
Member

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);

@nedphae
Copy link

nedphae commented Oct 28, 2024

maybe this helps, use a custom RedisSerializer, wrap value when serialize, and check value type when deserialize to ensure compatibility

open class Wrapper(
  @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, include = JsonTypeInfo.As.PROPERTY, property = "class")
  val value: Any? = null
)

class WrapperGenericJackson2JsonRedisSerializer(mapper: ObjectMapper) : GenericJackson2JsonRedisSerializer(mapper) {

  override fun serialize(value: Any?): ByteArray {
    var tempValue = value
    if (value != null) {
      if (Modifier.isFinal(value.javaClass.modifiers)) {
        tempValue = Wrapper(value)
      }
    }

    return super.serialize(tempValue)
  }

  override fun deserialize(source: ByteArray?): Any {
    val wrapper = super.deserialize(source)
    return if (wrapper is Wrapper) {
      wrapper.value!!
    } else {
      wrapper
    }
  }

}

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

No branches or pull requests

6 participants