FasterXML / jackson-databind

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

JsonNode.toString() throws with msgpack value #2824

Open fan-tom opened 4 years ago

fan-tom commented 4 years ago

Describe the bug After upgrade from 2.9 to 2.10 next code starts throwing exception

JsonNode node = new ArrayNode(new JsonNodeFactory(false), List.of(new POJONode(new MessagePackExtensionType((byte)0, new byte[]{2}))));

System.out.println(node.toString());
// 2.9: [org.msgpack.jackson.dataformat.MessagePackExtensionType@21]
// 2.10: java.lang.RuntimeException: com.fasterxml.jackson.databind.JsonMappingException: 'gen' is expected to be
// MessagePackGenerator but it's class com.fasterxml.jackson.core.json.WriterBasedJsonGenerator
//  at com.fasterxml.jackson.databind.node.InternalNodeMapper.nodeToString(InternalNodeMapper.java:32)
//  at com.fasterxml.jackson.databind.node.BaseJsonNode.toString(BaseJsonNode.java:136)

Exception is thrown due to these lines from org.msgpack:jackson-dataformat-msgpack package

//org.msgpack.jackson.dataformat.MessagePackExtensionType.Serializer#serialize
  public static class Serializer extends JsonSerializer<MessagePackExtensionType>
    {
        @Override
        public void serialize(MessagePackExtensionType value, JsonGenerator gen, SerializerProvider serializers)
                throws IOException, JsonProcessingException
        {
            if (gen instanceof MessagePackGenerator) {
                MessagePackGenerator msgpackGenerator = (MessagePackGenerator) gen;
                msgpackGenerator.writeExtensionType(value);
            }
            else {
                throw new IllegalStateException("'gen' is expected to be MessagePackGenerator but it's " + gen.getClass());
            }
        }
    }

because InternalNodeMapper, used in 2.10 in toString implementation doesn't take extensions into account, just uses default JsonMapper and so on.

I'm not sure is it bug or intended behaviour, but I tried to update package version and faced this issue. There are a lot of places where toString conversion is used (in debug/trace logs, where nodes are simply logged, so no direct calls to toString are performed, it is in logger internals) and I'd like to have some workaround to be sure app will not crash due to this issue. Version information Which Jackson version(s) was this for? 2.10.2 Expected behavior No exception

cowtowncoder commented 4 years ago

Hmmh. I agree, this is bit tricky to analyze. I will have to look deeper into whether there is anything that could be done; embedded POJOs are problematic wrt such usage and handling here is split across multiple packages.

cowtowncoder commented 4 years ago

Note: for Hacktoberfest I would accept a simple unit tests to add on:

https://github.com/FasterXML/jackson-integration-tests

(master is for 2.12 compatibility testing)

and no fix is required otherwise. Added "need-test-case" label to indicate this.

cowtowncoder commented 4 years ago

@fan-tom I suspect the usage as shown might be unsupportable: JsonNode.toString() should only produce JSON, ever, regardless of backend, but type enclosed appears to be MessagePack-specific value. However, since MessagePackExtensionType is defined in jackson-dataformat-msgpack, it technically could change behavior, to produce some other representation for diagnostics purposes. And in fact it probably should at least consider possible case of TokenBuffer being underlying generator: this is required for buffering.

But this is not something that can be solved by jackson-databind, unfortunately.