Skip to content

Serialization issue with polymorphism and not typed field #1594

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

Closed
vsct-jburet opened this issue Apr 7, 2017 · 15 comments
Closed

Serialization issue with polymorphism and not typed field #1594

vsct-jburet opened this issue Apr 7, 2017 · 15 comments

Comments

@vsct-jburet
Copy link

Verified with 2.8.7 & 2.9.0.pr2

With theses classes:

@JsonTypeInfo(use= JsonTypeInfo.Id.NAME)
@JsonTypeIdResolver(ValueTypeIdResolver.class)
interface Value {
}

class ValueA implements Value {
}

class WithNoTypedValue {
    public Object v;
    WithNoTypedValue(Object v) {
        this.v = v;
    }
}

This test fails:

        ObjectMapper mapper = new ObjectMapper();
        ValueA a = new ValueA();
        //ok ->
        Assert.assertEquals("{\"@type\":\"a\"}", mapper.writeValueAsString(a)); 
        WithNoTypedValue no = new WithNoTypedValue(a);
        //fail, @type not present ->
        Assert.assertEquals("{\"v\":{\"@type\":\"a\"}}", mapper.writeValueAsString(no));

Workarounds:

  1. Type the value (ie : public Value v;)

  2. If you can't, force the serializer:

        //need to clone (mapper.copy()) to avoid cache issue
        final JsonSerializer<Object> serializer = mapper.copy().getSerializerProviderInstance().findTypedValueSerializer(ValueA.class, false, null);
        final TypeSerializer typeSerializer = mapper.getSerializerProviderInstance().findTypeSerializer(mapper.constructType(ValueA.class));
        mapper.registerModule(new SimpleModule()
        .addSerializer(new StdSerializer<ValueA>(ValueA.class) {
            public void serialize(ValueA valueA, JsonGenerator jsonGenerator, SerializerProvider serializerProvider) throws IOException {
                  serializer.serializeWithType(valueA, jsonGenerator, serializerProvider, typeSerializer);
            }

            @Override
            public void serializeWithType(ValueA value, JsonGenerator gen, SerializerProvider serializers, TypeSerializer typeSer) throws IOException {
                serializer.serializeWithType(value, gen, serializers, typeSer);
            }
        }));
@Stephan202
Copy link
Contributor

Stephan202 commented Apr 12, 2017

This is a regression in version 2.8.8; it works with 2.8.7. I was about to file a ticket for this. Since I also prepared a reproduction case, here it is:

public final class Issue1594Test {
    @JsonTypeInfo(include = As.PROPERTY, property = "type", use = Id.NAME)
    @JsonSubTypes(@Type(name = "IMPL", value = SomeImpl.class))
    public interface SomeIface {}

    @JsonTypeName("IMPL")
    public static final class SomeImpl implements SomeIface {}

    @Test
    public void testSerialization() throws IOException {
        final ObjectMapper m = new ObjectMapper().registerModule(new Jdk8Module());

        // Passes.
        assertEquals(m.writeValueAsString(new SomeImpl()), "{\"type\":\"IMPL\"}");

        // Fails. Yields "{}".
        assertEquals(m.writeValueAsString(Optional.of(new SomeImpl())), "{\"type\":\"IMPL\"}");
    }
}

Since this affects uses of Optional, which I expect to be quite widespread, this is quite critical.

@Stephan202
Copy link
Contributor

Hmm, @vsct-jburet, if your test fails with 2.8.7, then perhaps this is not exactly the same issue. Interesting.

@cowtowncoder
Copy link
Member

@vsct-jburet Code sample seems to be missing ValueTypeIdResolver, without which it wouldn't really work. Would it be possible to include that?

But I am not even quite sure how it should ever work: no type information exists to infer that contained value should be polymorphic. The only way for a property with Object value could be polymorphic is via default typing (or mix-in, or AnnotationIntrospector overrides).

So I am bit puzzled as to why this would ever have worked the way you want...

Same applies to included unit test: I don't see how it should ever have worked to be honest.

@Stephan202
Copy link
Contributor

Stephan202 commented Apr 12, 2017

@cowtowncoder, I updated the unit test; the subtype now has a @JsonTypeName. (I assume that's what you were referring too. But interestingly, version 2.8.7 does indeed work without it.)

So to be clear: the issue also exists with the @JsonTypeName("IMPL") annotation.

@cowtowncoder
Copy link
Member

@Stephan202 No: what I mean is this: type of thing you serialize is seen as Optional<?>, which is about same as Optional<Object>. Object has no @JsonTypeInfo. As such I don't see how or why polymorphic handling would be enabled.

Note that this is different from passing SomeImpl directly: in this case type is set as SomeImpl (similar to how type for Optional is, via getClass(), set as type-erased Optional), and polymorphic handling can be determined.

It is very unfortunate if handling for Optional has somehow used runtime type during serialization, since while it has produced outcome you expect (and I can see why this would be beneficial, if it did work). And even more unfortunate that patch would introduce a change of course.
But... if I am not mistaken, this usage is not one supported, and as a result no unit test cover it (because its working is basically accidental).

Now: as to fixing usage: this is same as the problem with serializing/deserializing any generic types.
Most commonly it affects Lists and Maps, like:

    List<MyPojo> stuff = new ArrayList<>();
    // ... 
    String json = mapper.writeValueAsString(stuff);
    // no type info, even if `MyPojo` is polymorphic, due to type erasure!

and one solution is to explicitly specify type to use:

    mapper.writerFor(new TypeReference<List<MyPojo>>() { })
         .writeValueAsString(stuff);
    // and for optionals then
    mapper.writerFor(new TypeReference<Optional<SomeIface>() { })
         .writeValueAsString(opt);

@cowtowncoder
Copy link
Member

FWTW I think change was wrt fix for

FasterXML/jackson-modules-java8#17

and specifically:

FasterXML/jackson-modules-java8@92cb782

which does look for value serializer -- without wrapping for type serializer for type -- whereas old code was looking for possibly type-wrapped serializer. This would explain observed difference.

@Stephan202 I think this is due to accidental detection/inclusion of type id handling for runtime type -- and as I said, this is not how detection should work, or how it's done for containers like Collections and Maps. But it is obviously very unfortunate to change behavior in a patch.

I am not 100% sure how to proceed here. I would be willing to change behavior back for 2.8.x, wrt "accidentally working" (since consistency within a minor version 2.8.x is more important than fix to expected behavior across modules). But I also wouldn't want to break issue 17 that was fixed.
For 2.9 I think new behavior is correct (unless I am missing something).

@cowtowncoder
Copy link
Member

@vsct-jburet on work-arounds: (1) is proper fix, not just work-around; (2) is incorrect in multiple ways (I can elaborate on that if anyone's interested). But there's 3rd possibility too:

  • Use @JsonTypeInfo on Object value property.

which is alternative to (1).

@Stephan202
Copy link
Contributor

@cowtowncoder: thanks for the detailed analysis!

Interestingly, I hit this issue in our code base in the context of a server-side Spring MVC test (using MockMvc etc.), where the endpoint under test was effectively defined as follows:

@RequestMapping(value = "/someresource/{id}", method = RequestMethod.GET)
public Optional<SomeResource> getResourceById(@PathVariable final String id) {
    return this.myService.getResourceById(id);
}

If I understand correctly, in this case the type information is not erased (right?). So should I file a ticket with the Spring project to ensure that code such as the above will also work in Jackson 2.9? (I assume that it's the responsibility of the Spring framework to provide this type information to Jackson.)

@vsct-jburet
Copy link
Author

@cowtowncoder here is the ValueTypeIdResolver (this a simplified version for the UT) :

public class ValueTypeIdResolver extends TypeIdResolverBase {

    public ValueTypeIdResolver() {    }

    public String idFromValue(Object value) {  return "a";    }

    public String idFromValueAndType(Object value, Class<?> suggestedType) { return "a"; }

    public Id getMechanism() {  return JsonTypeInfo.Id.NAME;  }

    @Override
    public JavaType typeFromId(DatabindContext context, String id) throws IOException {
        return context.getConfig().constructType(ValueA.class);
    }
}

But I don't understand why this should not work. Jackson knows the class at runtime, so why not using it?

My use case : I want to serialize an object, but don't know the type. It could be a subclass of Value, or something else that does not implements Value. Deserialization is out of my scope, I focus only on serialization. So 1) and 3) do not match the use case.

If you think it is out of the scope of Jackson, close the issue. I agree that it is a corner case.

@cowtowncoder
Copy link
Member

@Stephan202 If signature from method is passed in its generic resolvable form it should not be type erased, correct. Many/most frameworks don't do that, but perhaps Spring MVC does?

@cowtowncoder
Copy link
Member

cowtowncoder commented Apr 13, 2017

@vsct-jburet Type of value is used for serialization; but for polymorphic base type it can not and should not used because it will not be available during deserialization -- during deserialization base type must be derived from declared type (or passed by caller in case of root value).
I don't think I am explaining this very well unfortunately...

I'll try again then. So: value serializer (contents) and type serializer (polymorphic type, iff enabled) are accessed separately. Former is based on actual runtime type (although optimized for final classes etc) as necessary; but type serializer is based on declared type. Part of this is due to requirement to be able to deserializer (more) reliably; part is for performance and simplicity -- requiring type serializer to be accessed dynamically all the time would be problematic; especially as the two (value / type serializers) need to coordinate their operation.
Note, however, that although fetching and assignment of TypeSerializer is somewhat static (once property is introspected, handler is created), its operation wrt type id is fully dynamic: type id of value type is handled.

However: it would seem to me that for this case, per-property annotation should work:

class WithNoTypedValue {
    @JsonTypeInfo(use= JsonTypeInfo.Id.NAME)
    @JsonTypeIdResolver(ValueTypeIdResolver.class)
    public Object v;

    WithNoTypedValue(Object v) {
        this.v = v;
    }
}

where you just need to enable polymorphic typing, along with custom type id resolver.

Or: if you simply want any and all Object valued properties to be serialize with polymorphic handling, ObjectMapper may be configured to do that:

 mapper.enableDefaultTyping(DefaultTyping.JAVA_LANG_OBJECT);

and that would also force type inclusion.

@cowtowncoder
Copy link
Member

One more note: yes, I think we are talking about two different problems (but sort of related):

  • @vsct-jburet is observing something that I think is existing behavior (unless I am mistaken)
  • @Stephan202 is observing a regression (or, alas, feature, albeit unintentionally introduced in a patch) -- but something that would be change for 2.9

@daniel-shuy
Copy link

Interestingly, I hit this issue in our code base in the context of a server-side Spring MVC test (using MockMvc etc.), where the endpoint under test was effectively defined as follows:

@RequestMapping(value = "/someresource/{id}", method = RequestMethod.GET)
public Optional<SomeResource> getResourceById(@PathVariable final String id) {
    return this.myService.getResourceById(id);
}

If I understand correctly, in this case the type information is not erased (right?). So should I file a ticket with the Spring project to ensure that code such as the above will also work in Jackson 2.9? (I assume that it's the responsibility of the Spring framework to provide this type information to Jackson.)

After much trial and error, I discovered that apparently Spring MVC handles the built-in Collection types (eg. List, Set, Map), but not Optional or Page.

A workaround for the issue with Optional is to simply create a custom JsonSerializer for it, eg.

@JsonComponent
public class OptionalSerializer extends JsonSerializer<Optional<?>> {
    @Override
    public void serialize(Optional<?> value, JsonGenerator gen, SerializerProvider serializers) throws IOException {
        if (value.isPresent()) {
            gen.writeObject(value.get());
        }
    }
}

@Stephan202 did you ever end up creating a ticket with Spring for this issue?

@Stephan202
Copy link
Contributor

@daniel-shuy I don't believe I did, sorry. Checking our internal git log, I see that back then I worked around it. That code has since been deleted. Our current code base doesn't seem to contain another instance of this issue.

@cowtowncoder
Copy link
Member

Not sure how to proceed with this, closing. May be re-filed with a test if reproducible with 2.10.3.

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

4 participants