Provide better feedback in recursive custom deserializer error

[fandreuz]

Search before asking

• [X] I searched in the issues and found nothing similar.

Describe the bug

I'm writing a custom deserializer for recursive types, all implementing from a common interface. Some implementations are basically delegates with additional functionalities, see the snippet below for more details. I'm using @JsonUnwrapped in combination with Lombok's @Jacksonized to make this work.

The code below returns the following error:

com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "content" (class Test$A$ABuilder), not marked as ignorable (one known property: "value"])
 at [Source: (String)"{"content" : "hello"}"; line: 1, column: 22] (through reference chain: Test$A$ABuilder["content"])

which makes me think that @JsonUnwrapped is ignored within A.

Reproduction

    interface X {

    }

    @Builder
    @Jacksonized
    @Value
    static class A implements X {
        @JsonUnwrapped
        X value;
    }

    @Builder
    @Jacksonized
    @Value
    static class B implements X {
        String content;
    }

    static class ADeserializer extends StdDeserializer<X> {
        protected ADeserializer() {
            super(X.class);
        }

        @Override
        public X deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
            JsonNode node = p.getCodec().readTree(p);
            if (ctxt.getAttribute("inner") != null) {
                return ctxt.readTreeAsValue(node, B.class);
            }
            ctxt.setAttribute("inner", true);
            return ctxt.readTreeAsValue(node, A.class);
        }
    }

    public static void main(String[] args) throws JsonProcessingException {
        String aJson = "{\"content\" : \"hello\"}";
        SimpleModule module = new SimpleModule();
        module.addDeserializer(X.class, new ADeserializer());
        System.out.println(new ObjectMapper().registerModule(module).readValue(aJson, X.class));
    }

Expected behavior

I found out that the problem can be fixed by adding the following to ADeserializer:

        @Override
        public JsonDeserializer<X> unwrappingDeserializer(NameTransformer unwrapper) {
            return new ADeserializer();
        }

since there's a check that verifies whether the instance returned unwrappingDeserializer is the same as this or not, which I do not completely understand in this context. Are there any docs about this design choice that I could consult?

I think the error above could be improved with a better error message pointing at the solution. I can open a PR to propose my idea, if you think it makes sense.

[cowtowncoder]

I am not quite sure what could be done here: yes, if a deserializer is to support @JsonUnwrapped, it must implement that method, to handle the case. I do not know what @Jacksonized does, that'd be interesting to know. But the exception you get is very general one; a deserializer encountered property it does know anything about. It cannot really assume much about behavior of deserializers it delegates to (deserializers know relatively little about each other).

It is currently legal for deserializers to omit definition of unwrappingDeserializer(). I guess default implementation could, instead of returning this, throw InvalidDefinitionException. But this would be backwards-incompatible change so probably not safe; most likely this could only be done for Jackson 3.0 (master branch) -- but that is doable.

[fandreuz]

Yeah I had the impression that there was something similar going on..

Why can't we reuse the same instance for the field annotated with @JsonUnwrapped? I.e. why do we need the unwrapping != orig check here ?

[JooHyukKim]

Maybe because what the comment right below the check says...? 🤔

// might be cleaner to create new instance; but difficult to do reliably, so:

[JooHyukKim]

Also, as per IntelliJ's Github show history for selection feature, seems like the check was written by @cowtowncoder in 2017, 6 years ago. So naturally, there is only a little chance of him remembering it, unless documented/commented.

[fandreuz]

Maybe because what the comment right below the check says...? 🤔

// might be cleaner to create new instance; but difficult to do reliably, so:

Still this does not explain the rationale behind the check... Why can't we use the same instance?

[cowtowncoder]

First: the comment referenced no longer exists in 2.18 codebase so I can't say for sure, but it seems misleading. Usually new instances are created and used to allow JsonDeserializers (and most handlers) to be stateless and (for most practical purposes) immutable (basically, JsonDeserializer.resolve() must be allowed to make modifications to handle cyclic definitions but that's about it). So the pattern of calling "xxx.withYyy()" means to either return instance as-is if no modifications were needed (property already had value specified), or a newly constructed instance.

Comment seems... odd. Can't explain what it meant.