spring-projects / spring-data-redis

Provides support to increase developer productivity in Java when using Redis, a key-value store. Uses familiar Spring concepts such as a template classes for core API usage and lightweight repository style data access.
https://spring.io/projects/spring-data-redis/
Apache License 2.0
1.77k stars 1.17k forks source link

`GenericJackson2JsonRedisSerializer` can't deserialize previously serialized `Stream.toList()` #2697

Open gcharondkt opened 1 year ago

gcharondkt commented 1 year ago

Hi there,

I'm facing an issue with the GenericJackson2JsonRedisSerializer() when using a @Cacheable annotation on a method returning a List resulting of a Stream.toList() operation.

I can easily reproduce with the following unit test :

    List<Integer> toSerialize = Stream.of(2953).toList();

    @Test
    void test() {
        RedisSerializer<Object> serializer = RedisSerializer.json();

        var serialized = serializer.deserialize(serializer.serialize(new ArrayList<>(toSerialize))); // This is OK as the list is embedded in an ArrayList
        serialized = serializer.deserialize(serializer.serialize(toSerialize)); // EXCEPTION !!
    }

Please tell me if i'm doing something wrong.

SpringBoot version : 3.1.3

Stacktrace :

Caused by: com.fasterxml.jackson.databind.exc.InvalidTypeIdException: Could not resolve type id '2953' as a subtype of `java.lang.Object`: no such class found
 at [Source: (byte[])"[2953]"; line: 1, column: 6]
    at com.fasterxml.jackson.databind.exc.InvalidTypeIdException.from(InvalidTypeIdException.java:43)
    at com.fasterxml.jackson.databind.DeserializationContext.invalidTypeIdException(DeserializationContext.java:2084)
    at com.fasterxml.jackson.databind.DeserializationContext.handleUnknownTypeId(DeserializationContext.java:1575)
    at com.fasterxml.jackson.databind.jsontype.impl.ClassNameIdResolver._typeFromId(ClassNameIdResolver.java:76)
    at com.fasterxml.jackson.databind.jsontype.impl.ClassNameIdResolver.typeFromId(ClassNameIdResolver.java:66)
    at com.fasterxml.jackson.databind.jsontype.impl.TypeDeserializerBase._findDeserializer(TypeDeserializerBase.java:159)
    at com.fasterxml.jackson.databind.jsontype.impl.AsArrayTypeDeserializer._deserialize(AsArrayTypeDeserializer.java:97)
    at com.fasterxml.jackson.databind.jsontype.impl.AsArrayTypeDeserializer.deserializeTypedFromArray(AsArrayTypeDeserializer.java:53)
    at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer.deserializeTypedFromAny(AsPropertyTypeDeserializer.java:238)
    at com.fasterxml.jackson.databind.deser.std.UntypedObjectDeserializerNR.deserializeWithType(UntypedObjectDeserializerNR.java:115)
    at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:74)
    at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323)
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4825)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3874)
    at org.springframework.data.redis.serializer.JacksonObjectReader.lambda$create$0(JacksonObjectReader.java:54)
    at org.springframework.data.redis.serializer.GenericJackson2JsonRedisSerializer.deserialize(GenericJackson2JsonRedisSerializer.java:250)
    ... 70 more
mp911de commented 1 year ago

Would you mind attaching the full stack trace?

gcharondkt commented 1 year ago

Oh yes sorry i've forgotten, here it is.

In debug mode, I can see a difference between the two serialized values :

Caused by: com.fasterxml.jackson.databind.exc.InvalidTypeIdException: Could not resolve type id '2953' as a subtype of `java.lang.Object`: no such class found
 at [Source: (byte[])"[2953]"; line: 1, column: 6]
    at com.fasterxml.jackson.databind.exc.InvalidTypeIdException.from(InvalidTypeIdException.java:43)
    at com.fasterxml.jackson.databind.DeserializationContext.invalidTypeIdException(DeserializationContext.java:2084)
    at com.fasterxml.jackson.databind.DeserializationContext.handleUnknownTypeId(DeserializationContext.java:1575)
    at com.fasterxml.jackson.databind.jsontype.impl.ClassNameIdResolver._typeFromId(ClassNameIdResolver.java:76)
    at com.fasterxml.jackson.databind.jsontype.impl.ClassNameIdResolver.typeFromId(ClassNameIdResolver.java:66)
    at com.fasterxml.jackson.databind.jsontype.impl.TypeDeserializerBase._findDeserializer(TypeDeserializerBase.java:159)
    at com.fasterxml.jackson.databind.jsontype.impl.AsArrayTypeDeserializer._deserialize(AsArrayTypeDeserializer.java:97)
    at com.fasterxml.jackson.databind.jsontype.impl.AsArrayTypeDeserializer.deserializeTypedFromArray(AsArrayTypeDeserializer.java:53)
    at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer.deserializeTypedFromAny(AsPropertyTypeDeserializer.java:238)
    at com.fasterxml.jackson.databind.deser.std.UntypedObjectDeserializerNR.deserializeWithType(UntypedObjectDeserializerNR.java:115)
    at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:74)
    at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323)
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4825)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3874)
    at org.springframework.data.redis.serializer.JacksonObjectReader.lambda$create$0(JacksonObjectReader.java:54)
    at org.springframework.data.redis.serializer.GenericJackson2JsonRedisSerializer.deserialize(GenericJackson2JsonRedisSerializer.java:250)
    ... 70 more
mp911de commented 1 year ago

The difference between the two values is that the ArrayList variant is non-final while Stream.toList() produces a final and private type ImmutableCollections$ListN. The typing mechanism skips adding type hints if the type is final and originates from the java. namespace as these types are typically private and cannot be accessed via reflection.

On the other side, we attempt to preserve custom collection types when reading arrays. Unfortunately, all we have to read the serialized value is java.lang.Object as type hint and we need to determine the type to read, so we default to parse the type hint.

The suggested workaround is to wrap the resulting value in a value object instead of caching the top-level value. Paging @christophstrobl for further insights.

gcharondkt commented 1 year ago

I have implemented a workaround like this :

@Cacheable
public List<Integer> getValue() {
    return new ArrayList<>(result.stream().toList());
}

Instead of this :

@Cacheable
public List<Integer> getValue() {
    return result.stream().toList();
}

But still, i thought it was a good idea to open an issue because the error is difficult to detect : the execution was not stopped by any exception, the only impact is that the @Cacheable was not working, resulting in many unexpected API calls.

Also, even if I understand why this is raising an error, in my opinion a serializer should not serialize something that he won't be able to deserialize, don't you think ?

mp911de commented 1 year ago

Also, even if I understand why this is raising an error, in my opinion a serializer should not serialize something that he won't be able to deserialize, don't you think ?

I fully agree, I just do not have an idea how to tackle top-level arrays as we don't seem to have sufficient hooks to either always or never enable typing.

jxblum commented 1 year ago

I reproduced this problem in the following test case method that I am currently adding to the GenericJackson2JsonRedisSerializerUnitTests class in Spring Data Redis to debug and explore possible solutions.

@Test // GH-2697
void serializingDeserializingIntegerListIsHandledCorrectly() {

    GenericJackson2JsonRedisSerializer redisSerializer = 
        new GenericJackson2JsonRedisSerializer();

    List<Integer> integers = Stream.of(2953).toList();
    //List<Integer> integers = List.of(2953);
    //List<Integer> integers = new ArrayList<>(Stream.of(2953).toList());
    //List<Integer> integers = new LinkedList<>(List.of(2953));

    Object deserializedIntegers = 
        redisSerializer.deserialize(redisSerializer.serialize(integers));

    assertThat(deserializedIntegers)
        .isInstanceOf(List.class)
        .asInstanceOf(InstanceOfAssertFactories.type(List.class))
        .extracting(list -> list.get(0))
        .isEqualTo(2953);

    deserializedIntegers = 
        redisSerializer.deserialize(redisSerializer.serialize(deserializedIntegers));

    assertThat(deserializedIntegers)
        .isInstanceOf(List.class)
        .asInstanceOf(InstanceOfAssertFactories.type(List.class))
        .extracting(list -> list.get(0))
        .isEqualTo(2953);
}

Clearly, you can see that I am playing around with different combinations of List types.

NOTE: I was also playing around with multiple serializations/deserializations to make sure what we return in the workable case still works after the first serialization/deserialization.

What many developers fail to realize is that the Collection types returned by operations like Stream.toList(), or List.of(..), and even Arrays.asList(..), which seemingly is benign when looking at the source code, because Arrays.asList(..) returns an "ArrayList" even, though it is NOT a java.util.ArrayList, but rather a java.util.Arrays.ArrayList instead! So, these are NOT typical, public (and standard) Collection types in the java.util package that developers are familiar with and use most often (they are "internal").

This leads to many surprises when users try to use the List, such as in a mutating manner, which they are not. These Collections effectively are read-only once created.

Additionally, as @mp911de pointed out, these Collection types are private, or often package private, such as the (internal) Collection types in the java.util.Collections class.

I agree with Mark. I think we have room for improvement, but I am not exactly certain what that is yet. I will continue to think on this next week and talk with both @mp911de and @christophstrobl about possible ideas.

NathanD001 commented 9 months ago

The issue is that ImmutableListN is a final class

it does work if you use ObjectMapper.DefaultTyping.EVERYTHING however I'd rather not use that solution, and Jackson is removing it in 3.0 https://github.com/FasterXML/jackson-databind/blob/2.17/src/main/java/com/fasterxml/jackson/databind/ObjectMapper.java#L227

I also found an alternative using Guava ImmutableList instead of Java ImmutableListN and jackson-datatype-guava

.collect(ImmutableList.toImmutableList()) intead of .toList()

then add cacheObjectMapper.registerModule(new GuavaModule());

I do wish it would work with out either of these workarounds. Does anyone else have suggestions?