Skip to content
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

Deserialization of a polymorphic type based on presence of a property #1627

Closed
lpandzic opened this issue May 18, 2017 · 19 comments
Closed

Deserialization of a polymorphic type based on presence of a property #1627

lpandzic opened this issue May 18, 2017 · 19 comments

Comments

@lpandzic
Copy link
Contributor

I'd like a feature on jackson for deserialization of polymorphic type where type selection strategy should be based on presence of a property. I know this was mentioned before but there was no consensus about how it should be solved.

Example:
For jsons like:
{'foo':''}
{'bar':''}

and type definitions like:

@JsonSubTypes({
        @JsonSubTypes.Type(value=Foo.class, name="foo"),
        @JsonSubTypes.Type(value=Bar.class, name="bar"),
})
interface FooBar {
}
@Value
class Foo {
    private final String foo;
}
@Value
class Bar {
    private final String bar;
}

When I deserialise FooBar with {'foo':''} I should get an instance of a Foo and similar for {'bar':''} I should get an instance of a Bar.

I have a working prototype where you have to specify a new custom deserialiser like:

public class FooBarDeserialiser extends PresentPropertyPolymorphicDeserialiser<FooBar> {

    public FooBarDeserialiser() {
        super(FooBar.class);
    }
}

and this is a valid approach, but a better one, in my opinion, would be if this had first class support for example with JsonTypeInfo:

@JsonTypeInfo(use = JsonTypeInfo.Id.PROPERTY_NAME) // or PROPERTY_NAME_PRESENCE
@JsonSubTypes({
        @JsonSubTypes.Type(value=Foo.class, name="foo"),
        @JsonSubTypes.Type(value=Bar.class, name="bar"),
})
interface FooBar {
}

Thoughts?

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 7, 2017

I could have sworn there was already an issue filed years ago, but I guess not.

As far as desirability of such a feature, I think that yes, it would be nice to have. It has been requested a few times, and I can see benefits.

There are two main questions regarding implementing this:

  1. How should it be configured via annotations (how much new support to add)
  2. Should existing type id interfaces and interaction be changed to support implementation
  3. Is it enough to only base this on presence of properties, or also on values (there is no upper bound on kinds of things users might want to use....)
    • One interesting (IMO) approach would be to allow use of JsonPointer for matching, and perhaps even optional value to match -- values to match could be more than one level deep in hierarchy

I guess there are 2 major approaches (minimal / maximal).

Minimal approach would simply allow listing of properties, existence of which would infer matching subtype to use. Implementation just might be possible without major changes to type id resolver, for deserialization. For serialization it might be even easier as it would be similar to basic PROPERTY (and EXISTING_PROPERTY) handling, but omit actual writing of type property.

Maximal (or, "non-minimal"?) approach, on the other hand, would require more changes.
For one, instead of requiring finding and extraction of String type id, it should bind the whole value as JsonNode, and pass that to Type Id Deserializer.
Similarly, annotations to use might need to be more elaborate; although if JsonPointer expression was used, perhaps not that much more elaborate. Also, if simple equality was used, we could require use of basic String equality of matching JsonNode, against String constant in annotation. This should still cover use cases of int and long ids, which are probably the main alternatives to Strings.

Does above make sense? Timeline-wise, this would be 3.x (perhaps even 3.0) feature I think?

@cowtowncoder cowtowncoder added the 3.x Issues to be only tackled for Jackson 3.x, not 2.x label Jun 7, 2017
@berniegp
Copy link

If anyone cares, I've written a simple custom deserializer that handles this case here: https://stackoverflow.com/a/50013090/1030527

@lpandzic
Copy link
Contributor Author

I've written a solution quite some time ago (not long after this issue was created) that proved to work well. It leverages existing annotation JsonSubTypes to select the class to deserialize to for name of present property.
Code and test/example:

public class PresentPropertyPolymorphicDeserializer<T> extends StdDeserializer<T> {

    private final Map<String, Class<?>> propertyNameToType;

    public PresentPropertyPolymorphicDeserializer(Class<T> vc) {
        super(vc);
        this.propertyNameToType = Arrays.stream(vc.getAnnotation(JsonSubTypes.class).value())
                                        .collect(Collectors.toMap(Type::name, Type::value));
    }

    @Override
    public T deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
        ObjectMapper objectMapper = (ObjectMapper) p.getCodec();
        ObjectNode object = objectMapper.readTree(p);
        for (String propertyName : propertyNameToType.keySet()) {
            if (object.has(propertyName)) {
                return deserialize(objectMapper, propertyName, object);
            }
        }

        throw new IllegalArgumentException("could not infer to which class to deserialize " + object);
    }

    @SuppressWarnings("unchecked")
    private T deserialize(ObjectMapper objectMapper,
                          String propertyName,
                          ObjectNode object) throws IOException {
        return (T) objectMapper.treeToValue(object, propertyNameToType.get(propertyName));
    }
}
public class PresentPropertyPolymorphicDeserializerTest {

    private final ObjectMapper objectMapper;

    public PresentPropertyPolymorphicDeserializerTest() {
        ObjectMapper objectMapper = new ObjectMapper();
        objectMapper.configure(JsonParser.Feature.ALLOW_SINGLE_QUOTES, true);
        SimpleModule module = new SimpleModule();
        module.addDeserializer(FooBar.class, new PresentPropertyPolymorphicDeserializer<>(FooBar.class));
        objectMapper.registerModule(module);
        this.objectMapper = objectMapper;
    }

    @Test
    public void shouldDeserializeFoo() throws IOException {
        // given
        String json = "{'foo':'foo'}";

        // when
        FooBar actual = objectMapper.readValue(json, FooBar.class);

        // then
        then(actual).isEqualTo(new Foo("foo"));
    }

    @Test
    public void shouldDeserializeBar() throws IOException {
        // given
        String json = "{'bar':'bar'}";

        // when
        FooBar actual = objectMapper.readValue(json, FooBar.class);

        // then
        then(actual).isEqualTo(new Bar("bar"));

    }

    @JsonSubTypes({
            @JsonSubTypes.Type(value = Foo.class, name = "foo"),
            @JsonSubTypes.Type(value = Bar.class, name = "bar"),
    })
    static abstract class FooBar {
    }

    @Value
    static class Foo extends FooBar {
        private String foo;
    }

    @Value
    static class Bar extends FooBar {
        private String bar;
    }
}

@cowtowncoder do you think this is good enough for PR?

@cowtowncoder
Copy link
Member

@lpandzic I appreciate the offer but I don't think this fits with the design since it couples content deserialization with type handling (which usually would be handled by TypeDeserializer). I have no problem if users use it (or frameworks, libraries for their own handling), and would be fine adding it as an example in documentation. But Jackson handling itself should try to keep approach consistent.

@lpandzic
Copy link
Contributor Author

Yes I agree, but the functionality should be added in some form to the jackson itself, something like
@JsonTypeInfo(use=Id.PRESENCE, include=As.PROPERTY) or something like that.

@berniegp
Copy link

berniegp commented Apr 25, 2018

@cowtowncoder Can you provide some rough guidelines on how to create and use such a custom TypeDeserializer? I agree that this makes more semantic sense, but I had a look at that class and it seems to be geared towards using a dedicated JSON property holding type information. I was looking for something to implement with an interface like (or whose parameters can be adapted to) this:

Class<?> resolveType(TreeNode node /* + other params? */);

I'm not familiar with all of Jackson's internals and I can't find any useful examples of a custom TypeDeserializer. I understand that a final version would be nice with annotations and all, but an easy to implement interface for a custom TypeDeserializer would I think provide the most value since any scenario can then be easily implemented.

I understand that many (most?) use-cases do not need the intermediate representation as a TreeNode and can work with the token stream directly which is certainly more efficient. It is therefore desirable to not enforce this when it is not required. However, it seems like the easiest solution for switching the polymorphic type based on the presence (or type or value) of an object's property. Is there another way that avoids this intermediate representation?

Also, to give a bit of context, my use-case is a JSON tree of objects where nodes can be branches or leaves so I need to handle embedded (list of) objects.

@cowtowncoder
Copy link
Member

@lpandzic Yes, I am not at all opposed to the idea itself, or for annotation style (although annotation information will limit what may be passed, to some degree). So it is more a question of how to model things to work so that deserializer part only needs to handle actual data, but leave handling of type- and identity-metadata to other handlers. This matters a lot due to wide number of (de)serializers, multiplied by alternative type/id handlers available.

@berniegp TypeSerializer and TypeDeserializer are not really meant to be commonly (or, maybe, at all) implemented by external developers. But as to specific question: yes, they are modeled mostly to work with existing choices and may well need to be extended. Same for annotations.
If this was an easy to do thing it perhaps would be solved, but it is not. I don't think it is insurmountable, but does need lots of thought to solve the challenge of how to either allow enough configuration to be useful for common use cases (there are questions, f.ex, on whether it really is "just existence" or perhaps values too -- and is null existing or not? etc etc), while being simple enough to use.

One way forward would be to start with annotation + use case, showing how test with annotations should pass. To give an idea of how it would be used. My expectation would be that users should not have to implement any of core types.

@lpandzic
Copy link
Contributor Author

lpandzic commented May 2, 2018

@cowtowncoder, I'd propose a model for implementation but current model (JsonTypeInfo and related annotations) isn't 100% clear to me. Does it make sense to expand those annotations and add support for this use case on them?

@heruan
Copy link

heruan commented Apr 1, 2019

Any update on this?

@RobertDiebels
Copy link

RobertDiebels commented Jun 13, 2019

Upvoted. This would be extremely useful when you:

  1. Don't want to include type information in your JSON.
  2. Don't want to write a deserializer to handle all this.

@cowtowncoder
Copy link
Member

At this point there is not much use for upvoting: I do not have time to work on this, so it will have to be someone with both time and itch to get it done.

@lpandzic I wish I could suggest specific approach. I think that extending @JsonTypeInfo could work for simple ("is there value for property/-ies A(,B,C)"), but... probably actually it'd need to just plug-in some class as implementation I think. There are actually annotation for custom type id resolvers, but they do not get access to enough information (no full model of contents as JsonNode).
But if an annotation was added, and maybe new style of inclusion (instead of CUSTOM, something like.... FULL_NODE? I'm sure we can figure out better name), then extension points in handling would need to be added just for that type.
But at least AnnotationIntrospector part could be relatively simple (although, JsonTypeInfo can not really refer to types in jackson-databind, so handler interface would either need to be in jackson-annotations, or, maybe, just refer to Class with no bounds).

@FasterXML FasterXML deleted a comment from p117 Aug 9, 2019
@berniegp
Copy link

berniegp commented Oct 14, 2019

Am I right in understanding that JsonTypeInfo and its associated mechanisms expect the very first JSON field of an object to contain the type information? This fits well with the streaming API and avoids reading whole JSON objects in intermediary structures (JsonNode) just to find a "type id" field. If that is the case, deserialization based on the presence of a property as proposed here would need a different mechanism since it must be able to access all properties of a JSON object to inspect it. I think this is what you hint to @cowtowncoder with FULL_NODE. In the case of a nested polymorphic structure (e.g. tree of nodes), it seems like this would be a costly operation with all the intermediary JsonNode instances created.

In short, it doesn't seem possible to build a performant custom deserializer based on the presence of a property. Is this correct?

@lpandzic
Copy link
Contributor Author

lpandzic commented Oct 14, 2019

FYI if anyone is interested, here's the source of the working prototype I mentioned on stackoverflow.

@cowtowncoder
Copy link
Member

@lpandzic Thank you for sharing ths!

@berniegp Yes, I think your summary is correct. The only addition I would make is that although performance is one aspect to it, bigger problem is that existing mechanisms, extension points assume streaming reads, no buffering. So I am not against supporting something that does require buffering (this is already required by polymorphic deserialization if Type Id happens to be "somewhere in the middle"), but it does require bigger changes outside immediate handlers.

@cowtowncoder cowtowncoder added the most-wanted Tag to indicate that there is heavy user +1'ing action label Apr 25, 2020
@koalalam
Copy link

FYI if anyone is interested, here's the source of the working prototype I mentioned on stackoverflow.

Thanks @lpandzic. Does this implementation support @JsonUnwrapped? For example:

class UnwrappedPojo {
String name;
@JsonUnwrapped FooBar foobar;
}

{"name": "This is a foo", "foo": "foo"}
{"name": "This is a bar", "bar": "bar"}

@lpandzic
Copy link
Contributor Author

I've rarely, if ever, used @JsonUnwrapped and haven't tested it. With that said, I don't see why it shouldn't work.

@lpandzic
Copy link
Contributor Author

FYI I've published Infobip Jackson Extension module which provides a way to tackle this issue:

interface FooBar extends PresentPropertyJsonHierarchy<FooBarType> {
}

@AllArgsConstructor(onConstructor_ = @JsonCreator)
@Value
static class Foo implements FooBar {
    private final String foo;
}

@AllArgsConstructor(onConstructor_ = @JsonCreator)
@Value
static class Bar implements FooBar {
    private final String bar;
}

@Getter
@AllArgsConstructor
enum FooBarType implements TypeProvider {
    FOO(Foo.class),
    BAR(Bar.class);

    private final Class<? extends FooBar> type;
}

@cowtowncoder
Copy link
Member

Now that #43 is implemented for 2.12 -- introducing JsonTypeInfo(use = DEDUCTION), inferring from existence but with no configuration -- does that cover this issue? If so, I will close this one (although obv. for missing features, new options, improvements, new issue can and should be filed). If not, I can leave this open, although would be good to know what is the main difference or missing aspect.

@RobertDiebels
Copy link

@cowtowncoder Very cool! Thanks for the update!

@cowtowncoder cowtowncoder removed 3.x Issues to be only tackled for Jackson 3.x, not 2.x most-wanted Tag to indicate that there is heavy user +1'ing action labels Sep 30, 2020
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

6 participants