jackson-databind icon indicating copy to clipboard operation
jackson-databind copied to clipboard

Problem deserializing Java 8 `Optional` with Polymorphic Deserialization with Jackson-databind 2.12 vs 2.8

Open coriedai opened this issue 3 years ago • 13 comments

Describe the bug We recently tried to upgrade our application's Jackson dependencies from 2.8.x to 2.12.x. After the upgrade though, we noticed some incompatible behaviors regarding the serialization and de-serialization results.

Code Setup

We have a POJO called RequestParameter, and it contains a metadata field:

private final Optional<Map<String, Object>> metadata;

This field is initialized in the constructor like the following:

this.metadata = Optional.of(ImmutableMap.copyOf(metadata.get()));

(The ImmutableMap here is the Guava ImmutableMap).

We use Mixins with the following configurations to specify the default type resolving behavior for the Map interface: (This might not be the best way to specify the default resolving behaviors for polymorphic fields, but our code was already setup this way..)

    @JsonTypeInfo(use = JsonTypeInfo.Id.NONE, defaultImpl = HashMap.class)
    public static class MapMixin {}

Note that we are using JsonTypeInfo.Id.NONE here to drop the type information for Map.

How we create our ObjectMapper:

        ObjectMapper mapper = new ObjectMapper()
                  .setSerializationInclusion(JsonInclude.Include.NON_NULL)
                  .enableDefaultTyping(DefaultTyping.NON_FINAL, As.PROPERTY)
                  .disable(SerializationFeature.FAIL_ON_EMPTY_BEANS)
                  .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);

        mapper.addMixInAnnotations(ServerMessage.class, FrameworkMessageMixin.class);
        mapper.addMixInAnnotations(ClientMessage.class, FrameworkMessageMixin.class);
        mapper.registerSubtypes(...);

        // some more configurations
        objectMapper.enableDefaultTypingAsProperty(DefaultTyping.NON_CONCRETE_AND_ARRAYS,
                JsonCoderSpecification.TYPE_PROPERTY);
        objectMapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY);
        objectMapper.setVisibility(PropertyAccessor.GETTER, JsonAutoDetect.Visibility.NONE);
        objectMapper.setVisibility(PropertyAccessor.IS_GETTER, JsonAutoDetect.Visibility.NONE);
        objectMapper.registerModule(new Jdk8Module());
        objectMapper.addMixIn(Map.class, MapMixin.class);
        objectMapper.addMixIn(List.class, ListMixin.class);
        objectMapper.addMixIn(Set.class, SetMixin.class);

Observed Behaviors

Let's say we have a RequestParameter POJO, and it's metadata field has one entry in it: "testKey":"testValue". Below are the serialization and deserialization results for this POJO under different Jackson versions:

With 2.8.x

// Serialized result
{"__type__":"RequestParameter", "metadata":["java.util.Optional",{"testKey":"testValue"}]}

// Deserialized result for the metadata field (based on POJO toString)
metadata=Optional[{testKey=testValue}]

With 2.12.x (and even 2.10.x)

// Serialized result
{"__type__":"RequestParameter", "metadata":{"__type__":"com.google.common.collect.SingletonImmutableBiMap","testKey":"testValue"}}

// Deserialized result for the metadata field (based on POJO toString)
metadata=Optional[{__type__=com.google.common.collect.SingletonImmutableBiMap, testKey=testValue}]

As we can see, with 2.12.x, the JsonTypeInfo.Id.NONE specified in the Mixin is not really working, as the class/type info is still somehow added during the serialization process. The Optional wrapper for the metadata field is also somehow missing in the serialized result. As a result, when we try to deserialize the String back to the RequestParameter POJO, we end up having a Map with 2 entries, which is inaccurate.

Please help look into this incompatible behavior. Thank you!

Version information Old: Jackson-databind = 2.8.x; Jackson-datatype-jdk8 = 2.8.x;

New: Jackson-databind = 2.12.x; Jackson-datatype-jdk8 = 2.12.x;

**Issues that are related

  • https://github.com/FasterXML/jackson-modules-java8/issues/59
  • https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.10#changes-compatibility

coriedai avatar Mar 31 '21 00:03 coriedai

I think I'd need a self-contained reproduction (unit test) here, as the combination is quite complex. If possible, it'd be good to try to reproduce this with AtomicReference instead of Optional, and regular Maps since test could be added here (as opposed to JDK8 datatype module).

cowtowncoder avatar Mar 31 '21 03:03 cowtowncoder

Any updates on this issue? Thanks!

coriedai avatar Apr 01 '21 17:04 coriedai

@coriedai ^^^^^^

cowtowncoder avatar Apr 01 '21 18:04 cowtowncoder

If we change the metadata's type to Map<String, Object> and remove the Jdk8Module(), the serialization behavior would be consistent between 2.8 and 2.12.

Serialization output for both versions:

{"__type__":"RequestParameter", "metadata":{"testKey":"testValue"}}

I guess something in the updated Jackson-datatype-jdk8 is breaking the Mixin/type configurations here.

coriedai avatar Apr 02 '21 03:04 coriedai

I am almost certain this has nothing to do with mix-in handling: modules do not have any effect on mix-in resolution.

But I'd still need a self-enclosed unit test (with all relevant types declared) to show exact expected behavior with assertions (printing out json only shows what happens, not what'd be expected). Ideally minimized to leave out settings that had no effect.

Like I said, too, use of Optional should be replaceable with AtomicReference -- they are both "ReferenceType"s. Without this, we must move this issue to Java8 modules repo for reproduction: databind tests cannot depend on other modules (no circular dependencies).

I do not add suggestions just for fun of it, but to help reproduce problems in a way that I can help solve them.

cowtowncoder avatar Apr 02 '21 16:04 cowtowncoder

POJO:

import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import lombok.Builder;
import lombok.Value;

@Value
@JsonIgnoreProperties(ignoreUnknown = true)
@JsonDeserialize(builder = RequestParameter.RequestParameterBuilder.class)
public class RequestParameter {

  private final AtomicReference<Map<String, Object>> metadata;

  @Builder
  private RequestParameter(final AtomicReference<Map<String, Object>> metadata) {
    this.metadata = metadata;
  }

  @JsonPOJOBuilder(withPrefix = "")
  public static class RequestParameterBuilder{}

  public boolean equals(Object o) {
    // self check
    if (this == o)
      return true;
    // null check
    if (o == null)
      return false;
    // type check and cast
    if (getClass() != o.getClass())
      return false;

    RequestParameter parameter = (RequestParameter) o;

    return this.metadata.get().equals(parameter.metadata.get());
  }
}

Unit Test:

import static org.junit.Assert.assertEquals;

import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;

import com.fasterxml.jackson.annotation.JsonAutoDetect;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.PropertyAccessor;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationFeature;
import com.fasterxml.jackson.databind.jsontype.NamedType;
import org.junit.Test;

public class DebugTest {
  public static final String TYPE_PROPERTY = "__type__";

  @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = TYPE_PROPERTY)
  public static class MessageMixin {}

  @JsonTypeInfo(use = JsonTypeInfo.Id.NONE, defaultImpl = HashMap.class)
  public static class MapMixin {}

  @JsonTypeInfo(use = JsonTypeInfo.Id.NONE, defaultImpl = ArrayList.class)
  public static class ListMixin {}

  @JsonTypeInfo(use = JsonTypeInfo.Id.NONE, defaultImpl = HashSet.class)
  public static class SetMixin {}

  @Test
  public void debugTest() throws IOException {
    // create and configure ObjectMapper
    ObjectMapper mapper = new ObjectMapper();
    mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
    mapper.disable(SerializationFeature.FAIL_ON_EMPTY_BEANS);
    mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
    mapper.registerSubtypes(new NamedType(RequestParameter.class, "RequestParameter"));

    mapper.enableDefaultTypingAsProperty(ObjectMapper.DefaultTyping.NON_CONCRETE_AND_ARRAYS, TYPE_PROPERTY);
    mapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY);
    mapper.setVisibility(PropertyAccessor.GETTER, JsonAutoDetect.Visibility.NONE);
    mapper.setVisibility(PropertyAccessor.IS_GETTER, JsonAutoDetect.Visibility.NONE);

    mapper.addMixIn(RequestParameter.class, MessageMixin.class);
    mapper.addMixIn(Map.class, MapMixin.class);
    mapper.addMixIn(List.class, ListMixin.class);
    mapper.addMixIn(Set.class, SetMixin.class);

    // Prepare POJO
    Map<String, Object> metadata = new HashMap<>();
    metadata.put("testKey", "testValue");
    RequestParameter requestParameter =
        RequestParameter.builder().metadata(new AtomicReference<>(metadata)).build();

    // Test
    String serializedResult = mapper.writeValueAsString(requestParameter);
    System.out.println("serializedResult: " + serializedResult);

    RequestParameter deserializedResut = mapper.readValue(serializedResult, RequestParameter.class);
    System.out.println("deserializedResut: " + deserializedResut);

    assertEquals(requestParameter, deserializedResut);
  }
}

Test results

The Optional is replaced with AtomicReference as you recommended, but after this change the serialization & deserialization is now not working as expected. Below output is observed for both Jackson-databind 2.8.x and 2.12.x. Not sure if it's related to the original problem of Jackson inconsistency between 2.8.x and 2.12.x.

    [junit] serializedResult: {"__type__":"RequestParameter","metadata":{"__type__":"java.util.HashMap","testKey":"testValue"}}
    [junit] deserializedResut: RequestParameter(metadata={__type__=java.util.HashMap, testKey=testValue})

    [junit] expected:<RequestParameter(metadata={testKey=testValue})> but was:<RequestParameter(metadata={__type__=java.util.HashMap, testKey=testValue})>
    [junit] junit.framework.AssertionFailedError: expected:<RequestParameter(metadata={testKey=testValue})> but was:<RequestParameter(metadata={__type__=java.util.HashMap, testKey=testValue})>

coriedai avatar Apr 06 '21 06:04 coriedai

This could help, although tests also cannot use Lombok. I don't think Lombok usage itself is causing issues, just that no unit test with Lombok dependency can be added and so I can't run this test.

It is possible that mix-in aspect may be related, after all, since this may be related to annotation handling optimizations.

Usage is quite complicated with combination of polymorphic typing, Optional values, Builders.

Last note: I assume reproduction would work specifically with 2.12.2.

cowtowncoder avatar Apr 06 '21 22:04 cowtowncoder

Updated POJO to remove Lombok dependencies:

import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;

@JsonIgnoreProperties(ignoreUnknown = true)
@JsonDeserialize(builder = RequestParameter.RequestParameterBuilder.class)
public class RequestParameter {

  private final AtomicReference<Map<String, Object>> metadata;

  private RequestParameter(final AtomicReference<Map<String, Object>> metadata) {
      this.metadata = metadata;
  }

  public boolean equals(Object o) {
    // self check
    if (this == o)
      return true;
    // null check
    if (o == null)
      return false;
    // type check and cast
    if (getClass() != o.getClass())
      return false;

    RequestParameter parameter = (RequestParameter) o;

    return this.metadata.get().equals(parameter.metadata.get());
  }

  public String toString() {
    return "RequestParameter=(metadata=" + metadata + ")";
  }

  public static RequestParameterBuilder builder() {
    return new RequestParameterBuilder();
  }

  @JsonPOJOBuilder(withPrefix = "")
  public static class RequestParameterBuilder {
    private AtomicReference<Map<String, Object>> metadata;

    RequestParameterBuilder() {}

    public RequestParameterBuilder metadata(AtomicReference<Map<String, Object>> metadata) {
      this.metadata = metadata;
      return this;
    }

    public RequestParameter build() {
      return new RequestParameter(this.metadata);
    }
  }
}

The test results I posted earlier can be reproduced with both jackson 2.8 and 2.12.

As a side note, based on the current test POJO and test suite, if I change the AtomicReference to Optional, and add back the Jdk8Module dependency, then the original inconsistent results can be reproduced.

With 2.8.x, the test would pass:

    [junit] serializedResult: {"__type__":"RequestParameter","metadata":["java.util.Optional",{"testKey":"testValue"}]}
    [junit] deserializedResut: RequestParameter=(metadata=Optional[{testKey=testValue}])

With 2.12.x, the test would fail:

    [junit] serializedResult: {"__type__":"RequestParameter","metadata":{"__type__":"java.util.HashMap","testKey":"testValue"}}
    [junit] deserializedResut: RequestParameter=(metadata=Optional[{__type__=java.util.HashMap, testKey=testValue}])

    [junit] expected:<RequestParameter=(metadata=Optional[{testKey=testValue}])> but was:<RequestParameter=(metadata=Optional[{__type__=java.util.HashMap, testKey=testValue}])>
    [junit] junit.framework.AssertionFailedError: expected:<RequestParameter=(metadata=Optional[{testKey=testValue}])> but was:<RequestParameter=(metadata=Optional[{__type__=java.util.HashMap, testKey=testValue}])>

coriedai avatar Apr 06 '21 23:04 coriedai

Thank you!

One thing I wonder here is the reason for these mix-ins: Map, List and Set default to about suggested types (well, Map defaults to LinkedHashMap not HashMap) anyway -- and if different defaults are desired, there is a simple way by adding "abstract type mapping". Would removing these mix-ins change the test or behavior in measurable way? I am just trying to figure out minimal setting to reproduce the difference.

I assume use of Default Typing is for other reasons, however?

cowtowncoder avatar Apr 07 '21 03:04 cowtowncoder

In our case, the mixins are used for legacy reasons. As you mentioned, there are probably better ways to handle polymorphic type mapping, but our services have been setup this way. If removing the mixins would not change our serialization output, I agree it would be better to get rid of those mixins. Given our services' current setup though (with Optional and 2.8.x), the serialization results will get changed if we remove the mixins (as shown below), and that's why we can't easily migrate off these mixins.

Test results after removing the mixins:

with 2.8, remove mixins, with AtomicReference

    [junit] serializedResult: {"__type__":"RequestParameter","metadata":{"__type__":"java.util.HashMap","testKey":"testValue"}}
    [junit] deserializedResut: RequestParameter=(metadata={testKey=testValue})

with 2.12, remove mixins, with AtomicReference

    [junit] serializedResult: {"__type__":"RequestParameter","metadata":{"__type__":"java.util.HashMap","testKey":"testValue"}}
    [junit] deserializedResut: RequestParameter=(metadata={testKey=testValue})

with 2.8, remove mixins, with Optional

    [junit] serializedResult: {"__type__":"RequestParameter","metadata":["java.util.Optional",{"__type__":"java.util.HashMap","testKey":"testValue"}]}
    [junit] deserializedResut: RequestParameter=(metadata=Optional[{testKey=testValue}])

with 2.12, remove mixins, with Optional

    [junit] serializedResult: {"__type__":"RequestParameter","metadata":{"__type__":"java.util.HashMap","testKey":"testValue"}}
    [junit] deserializedResut: RequestParameter=(metadata=Optional[{testKey=testValue}])

coriedai avatar Apr 08 '21 05:04 coriedai

I don't really have time to dig into this much deeper, but I suspect that the semantic change was to fix an inconsistency in handling, such that in case of "reference type"s (Optional, AtomicReference) polymorphic type should be wrt contents (what is "inside" Optional) and not the wrapper. This is how things work with Collections and Maps; otherwise use of polymorphic values with Optionals does not really work.

If this is the change you are observing (and I think it is), 2.12 behavior is the expected one and 2.8 was considered broken. You can probably register custom serializers, deserializers to change things differently for your own use, if that makes sense.

cowtowncoder avatar Apr 08 '21 17:04 cowtowncoder

I don't follow with why the old behavior would cause issues for polymorphic values inside Optionals, as I see it the only difference is whether an Optional.of(foo) gets serialized as ["java.util.Optional",<serialization of foo>] or just as <serialization of foo> when default typing is enabled. If this was a deliberate change I'm happy to take it on faith, though I think there may be a disconnect because the behavior we're observing for Optionals is inconsistent with what we observe for other collections.

I did some digging and I believe I was able to track down the root cause. In 2.8, Optionals were implemented as a special case where serializeWithType() would wrap the inner value with the Optional type info. In this commit OptionalSerializer was updated to extend from the base ReferenceTypeSerializer which skips the wrapper level, even for the serializeWithType() method.

I'm not super well versed in this codebase, but between the fact that there's separate serialize() and serializeWithType() methods, my suspicion is that the extra type info should be present, as it is for other collections.

E.g. mapper.writerFor(List.class).writeValueAsString(Arrays.asList("foo")) yields ["foo"] without default typing and ["java.util.Arrays$ArrayList",["foo"]] with.

By contrast mapper.writerFor(Optional.class).writeValueAsString("foo") yields "foo" regardless of whether default typing is enabled or not.

A test case outlining this:

public class OptionalTest {

    private final PolymorphicTypeValidator typeValidator = BasicPolymorphicTypeValidator.builder().allowIfBaseType(Object.class).build();

    private ObjectMapper mapper;

    @BeforeEach
    public void setUp() {
        mapper = new ObjectMapper();
        mapper.activateDefaultTyping(typeValidator, ObjectMapper.DefaultTyping.NON_FINAL);
        mapper.registerModule(new Jdk8Module());
    }

    @Test
    public void itSerializesOptionals() throws JsonProcessingException {
        // Given
        final Optional<String> input = Optional.of("foo");
        final String expected = "[\"java.util.Optional\",\"foo\"]";

        // When
        final String actual = mapper.writerFor(Optional.class).writeValueAsString(input);

        // Then
        assertEquals(expected, actual);
    }
}

One quick thing to note is that to the best of my knowledge, regardless of whether default typing is enabled serialization/deserialization is stable within a particular version, the only failures are when attempting to use newer Jackson versions (in my case 2.12) to deserialize objects that were serialized in Jackson 2.8, so I just focused my research/test case on the serialization aspect for brevity.

mprussak avatar Jan 14 '22 20:01 mprussak

I'll make this quick: Polymorphic Type Information for referential types like Optional should always be for content value, and never for "Optional" itself. This is the behavior expected with 2.12 and later, regardless of what 2.8 did. Unlike with other Container types (Collections, Maps), there is no marker (Array, Object) on which Type Id could be attached, so the choice is between contents (almost always more important) or nominal Optional wrapper (rarely more important one).

So, the expectation is that Type Id for content value, if any, should be output (assuming Polymorphic Typing is enabled for (nominal) value type -- if it's not, no Type Id should be written or expected).

Put another way, Type Id for Optional should quite literally never be written by serialization.

cowtowncoder avatar Jan 17 '22 02:01 cowtowncoder