Support `@JsonUnwrapped` with `@JsonCreator`

[cowtowncoder]

(note: follow-up for #265)

Current implementation of @JsonUnwrapped deserialization requires handling of all other properties first, and only then reconstructing unwrapped objects. This works ok via fields/setters, but can not work with @JsonCreator passed values.

But with a major rewrite it should be possible to make this work. This probably requires more support for actually determining unwrapped-properties a priori, instead of just buffering anything that doesn't match. Or maybe it only requires more logic in handling of creator.

[gaba-xyz]

Any update on this? Would be nice with a way to get @JsonUnwrapped working for deserialisation as well.

[cowtowncoder]

@OverlyExcessive It works just fine with deserialization if you use setters (or fields). Note that you do NOT have to pass ALL properties via creator; mixing and matching creator and setters/fields is legal.

This is not to say it wouldn't be great to allow passing unwrapped properties via Creator methods, just that this is the one aspect with unwrapped properties that does not work.

[gaba-xyz]

@cowtowncoder
You're correct, thanks!

[tpavs]

+1 Any update on this?

[frost13it]

Implementation of this is necessary to use @JsonUnwrapped in Kotlin.

[plcarmel]

Hi ! This is my first attempt a contributing to the Jackson code base. I hope you will like my proposal to fix the current issue. It is a work in progress though. Comments and feedback are welcome.

Analysis of the current code

I am working on a Kotlin project and I really want to be able to have an unwrapped property as a parameter of a creator method so, in order to fix this, I started to have a look at the code. I must say that I am a bit surprised by the amount of duplicated code in the deserialization process. That's in part because execution paths branch off very early and we end-up using a different method for each one of the annotation combination.

I made a graph to illustrate what is going on:
deserialize.pdf

I think that relying on a more generic approach would help to simplify the code, which would have many benefits, like the ability to easily implement the requested feature. The goal is to have only one main execution path for all the different scenarios.

Proposed solution

@cowtowncoder, you identified the problem pretty well: we obviously cannot wait until the end to construct the unwrapped property since we need sometimes need it in the creator method.

A tree of operations

My proposed approach is to build a tree of operations that need to be performed in order to successfully deserialize an object of a given class. That tree can then be used to perform the deserialisation itself.

Building the tree

1. Identify the properties that are required in order to instantiate the class to deserialize. For classes where _propertyBasedCreator is true, we need to have all the properties required to call one of the creator methods. Same thing for other classes, although the list of properties to gather will be zero.
2. For each one of the properties that are annotated with @JsonUnwrapped, perform the same analysis recursively. After this step is complete, we will end up with a hierarchy of required operations. Let's consider those two classes, for example:

public class Location {
  public int x;
  public in y;
  @JsonCreator Location(
    @JsonProperty(value = "x", required=true) int x,
    @JsonProperty(value="y", required=true) int y
  ) { /*...*/ }
}

public class Test {
  public String name;
  public Location location;
  @JsonPropery(required=true) public String someOtherThing;
  @JsonProperty public String someOptionalThing;
  @JsonCreator Test(
    @JsonProperty(value="name", required=true) String name,
    @JsonUnwrapped @JsonProperty(value="location", required=true) Location location
  ) { /* ... */ }
}

We would have this hierarchy of required operations to perform:

Instantiate class Test
|-  Call creator function
    |-  Deserialize property name
    |-  Instantiate class Location
        |-  Call creator function
            |-  Deserialize property x
            |-  Deserialize property y

Executing the deserialization

Once the operation tree is built, then...

1. Build a map to be able to quickly find a deserialize operation based on the field name.
2. Check if the root operation is flagged as having been performed. If so, we are done with the instantiation of the class. Continue to step 6.
3. Read a token from the parser and, based on the field name, feed the the value to the appropriate deserialization operation. If no operation can be found, handle the unknown property as usual, possibly buffering it. If the token is an "end of object", signal it to every operation, starting with the leaves of the tree up to the root. If the root operation still has not been performed, raise an error.
4. Notify parents of the operation that one of their dependency has been updated. Continue until the root has been reached.
5. Go back to step 2.
6. Read the instantiated object by calling the getResult() method on the root object.
7. Set the other properties using the buffered tokens.
8. Set other properties using the remaining tokens the way it is done right now, possibly through the current vanilla execution path, assuming we deal with the other
   @JsonUnwrapped properties first.

Error handling

If a requirement is not met, we would be able to generate a hierarchical exception like

Exception: Failed to instantiate class Test
Caused by Exception: Failed to instantiate class Location
Caused by Exception: Failed to deserialize required property "x"
Caused by Exception: Required property "x" is missing

Going one step further

Put all operations in the graph

I suggest that we go one step further down the generalization road and have a required deserialization operation to perform for each required property of the class. And optional properties should also be represented by the same kind of operations. Those optional operations can also have dependencies on required operations. For one thing, the class needs to be instantiated first, right ? For the example above, we would have this acyclic directed graph to build:
operations.pdf

Rules for building the graph

Here are the rules that would be used to build the graph:

1. Successfully deserializing an object requires instantiating it first.
2. Each required property needs to be set.
3. Setting a property of a class requires instantiating the class first.
4. Instantiating an object requires calling the default constructor or calling a creator function.
5. Calling a creator function requires collecting the required properties first, either through instantiation or deserialization.
6. Collecting a property can be done by deserializing it or by instantiating it (if unwrapped) or by calling anySetter (if provided and if we are at the end of the object).

Execution

Tokens will be fed to the deserialization operations like explained in the first part of this document. One advantage, here, is that we don't need a separate buffer; all the needed deserialization operations are already present in the tree, and this is where the value will be stored while waiting for the class to be instantiated. That is because the set operation will be blocked as long the the instantiate operation it depends on has not been performed.

Performance considerations

For a given class, a template of the graph would have to be built once for all. After that, we could copy it quickly when performing each new deserialization.

[cowtowncoder]

@plcarmel First of all, thank you for investigating this, and writing above analysis.

I agree on parts of amount of duplication being unfortunate; parts of this are for optimization, although earlier choices then do lead to combinatorial explosion. This is further compounded by separate builder-based deserializer and even properties-as-array variant.

Now: on rewrite as suggested... I have 2 main concerns:

1. Performance. Adding more state can be good for being able to handle more cases, as well as to make processing more understandable. But adding another layer of abstraction for actual execution (I am not concerned about construction of such execution plan, fwtw) tends to have non-trivial overhead for optimal path. This both due to additional abstraction on probably book keeping. Latter might have upside too, of course, wrt handling of "required" properties (which are currently only considered if passed via constructor)
2. Fundamental changes tend to add new failure cases on short term, even if longer term they can pay off by reducing complexity and allowing easier fixes.

Of these, (1) is easy enough to measure, although probably only after implementation is complete enough. It could be that my concerns are overblown.
And perhaps it could be possible to avoid construction of various state objects for cases where more complex processing is not needed.

Regarding (2), I'd be tempted to suggest that if such rewrite was to be done, it should go with Jackson 3.x, to reduce likelihood of major issues with 2.x usage and some of extensions.
I am guessing that changes would be needed to internal APIs, potentially breaking some use cases where users use sub-classing for handlers -- "advanced" usage that is using extension points that have not necessarily been design as such (but that in Java are difficult to really indicate as "do not extend", due to various reasons).

With that, I would be interested in seeing how a prototype could be made. I don't think I have time to work on this directly this, but I would do my best to help, read code and so on.

ps. Something that just occurred to me is that one area where I have actually thought about alternate implementation is non-blocking/async parsing -- jackson-core has such mode, but databind can not really use it. Deserialization in non-blocking mode would by necessity require different approach, feeding (push model) tokens, to be feasible (IMO). This is different from streaming level where pull-like approach is still feasible (and which is why it has been implemented).
So one possible idea would be to start prototyping "alt databind" that would be built using push approach. If and when it worked well for that side, proving sufficient efficiency (non-blocking parsing itself is within 10% of blocking one, based on my testing), perhaps retrofit/rewrite of "standard" input would be more practical?

[plcarmel]

@cowtowncoder, what you say makes a lot of sense and I sure don't want to break anything on a module that is used by every Java project on earth, nor slow it down.

I think that building a prototype for the push approach is a good idea, as those two things fit together pretty well.

I still have to figure out the details of how I am going to come up with something that can be tested quickly, and with a reasonable effort.

For future reference, here's the feature request on jackson-core for a non-blocking parsing: FasterXML/jackson-core#57

[cowtowncoder]

@plcarmel Yes. Also: it goes without saying that any incremental improvements for existing handling, simplifications, are welcome if you find some. Sometimes working on a prototype other orthogonal improvements are found as well.

Wrt async handling: parsing itself is fully implemented for streaming core: but the question is how to make databinding work with it -- right now things would not work since databinding can not really deal with JsonToken.NOT_AVAILABLE indicator.
In addition, Smile format (~= binary json) implements it, and if there was enough interest, implementations should be quite straight-forward for CBOR, and at least possible for most other formats (like protobuf and avro)

[plcarmel]

I think an easy option to have the deserialization perform well is to optimize the operation graph after it has been created, just like a compiler would do with the graph created from source code.

Large sections of the graph can be replaced with just one node that performs vanilla processing using the current algorithm, for example (provided deserialization is done in blocking mode).