Builder Deserialization with JsonCreator Value vs Array

[vjkoskela]

There seems to be a difference in behavior when deserializing JSON into an object via a Builder when that Builder has a @JsonCreator method on it when that method takes a primitive vs an array.

Consider a pojo class like this:

    private static final class TestBean {

        public int getI() {
            return _i;
        }

        private TestBean(final Builder builder) {
            _i = builder._i;
        }

        private final int _i;
    }

Then consider a Builder for the pojo that supports deserialization value from a value as well as an object (or via code calling setX):

    public static final class Builder  {

        public Builder() { }

        @JsonCreator
        public Builder(final Integer value) {
            _i = value;
        }

        public void setI(final Integer value) {
            _i = value;
        }

        private Integer _i;
    }

This combination can be deserialized from {"i":3} and from 3.

However, if we instead declare a @JsonCreator method that accepts a List<Object> then the deserialization is only supported from [3] and not from {"i":3}. For example, replacing the above @JsonCreator annotated constructor with:

        @JsonCreator
        public Builder(final List<Object> jsonArray) {
            setI((int) jsonArray.get(0));
        }

This appears to be because in BeanDeserializerBase.java#L222 the value of _vanillaProcessing is set based in part on the value of _nonStandardCreation which considers _valueInstantiator.canCreateUsingArrayDelegate() but does not consider instantiation from a primitive value to be "non standard creation".

I am not sure which is the desired behavior. That is, it is an over-sight that with a @JsonCreator accepting a value that the same pojo can be deserialized from an object OR whether the array case should also support deserialization from an object.

In the case that we don't want the flexibility - and it seems reasonable that if you declare a @JsonCreator that this how the object should be built from JSON - then fix would be to include canCreateFromXXX for the various primitive types in the computation of _nonStandardCreation; although I am unclear on what the broader implications of this migh be.

In the case that we want to allow creation of objects from both an alternate representation (e.g. array or primitive) as well as from an object representation then we need a more fine-grained check when delegating to the vanilla path. In particular, we should go down that path if there is non-standard only because of an array delegate (but it's an object representation in json). This will likely lead to evaluating the "vanilla-ness" of a particular deserialization based at deserialization time instead of upfront.

Jackson Version: 2.10.0

[cowtowncoder]

I think I will need bit more detail just to make sure, since I can make the test pass:

1. Is this using @JsonDeserialize(builder=Builder.class) formally (missing from sample, build() method too)
2. @JsonCreator should probably use mode = JsonCreator.MODE.Delegating just to make sure introspection does not detect i as Mode.PROPERTIES (if parameter name module is added)

Primitive (well, "small set of scalars" really, int/Integer, long/Long, boolean/Boolean, String) creators should be fine even in "vanilla" processing.

[cowtowncoder]

Can not reproduce currently; may be reopened with a test.

[vjkoskela]

I created what I think are tests demonstrating this issue:

#2703

Let me know if I've missed something, and apologies for going MIA for so long. This is a great project and ends up as a dependency in just about every project I ever undertake.

[cowtowncoder]

Thanks! No problem wrt time delay; I have no problem re-opening active cases, just want to actively manage active case list (since 300+ issues are hard to keep track off)

[cowtowncoder]

Ok, I'll have to see but my first impression here is that this might be unsupported and unsupportable case: that is, you can not have both array-delegating and "regular" delegating creators. At least currently this is the case: only one can be used.

Error message is misleading, of course, so at very least would be good to see if that could be improved.

But... I will double-check if my thinking on exclusivity (delegating vs array-delegating) is or is not valid. I may be forgetting something here.

[vjkoskela]

you can not have both array-delegating and "regular" delegating creators

I'm assuming "deleting" refers to @JsonCreator. If so, the examples in the tests only have a single delegating creator each, one example from a primitive and the other from an array. Can you clarify?

===

What concerns me is the inconsistent impact of the creators based on type on the normal (non-@JsonCreator based) object deserialization. Specifically, that it's possible to have a primitive creator and still retain deserialization from objects, but it's not possible to have an array creator and retain deserialization from objects.

While I like the idea of being able to specify an alternate creation path while still retaining the object deserialization semantics, the inconsistency was surprising. Should @JsonCreator based deserialization prevent deserialization via any other means?

At least documentation and a clearer error message would be advisable.

It's been a while since I dove into it, but as I mentioned above and in the review, on the surface it would seem possible to address this if the vanilla-ness of the deserialization were deferred until the shape of the input were determined. Specifically, the deserialization is vanilla unless the input shape matches the creator argument shape (object => map, array => array/list, primitive => primitive). Not sure what the consequences of deferring this decision are.

[cowtowncoder]

side note: created #2707 and implemented it; but since it is visible API change (sort of), has to go in 2.12.0.

[cowtowncoder]

@vjkoskela Looking at this bit more, I think I now understand the mechanism. And it is not quite what I expected.

Basically, there are couple of different classes of creators for POJOs, added in roughly this order over time

1. "Default" (0-argument) -- used for traditional POJOs, where creation is separate from setting values (create + deserialize)
2. Scalar creators: close-coupling with a small number of primitive/simple types, discoverable without annotation -- String, int, long, boolean (and matching wrappers). Essentially delegating but special case later generalized
3. Properties-based Creator (constructor, static factory): construction and deserialization coupled (deserializer has to read and buffer values)
4. Full delegation: originally just one method, but with contribution separate "array delegation" was added.

Of these, 1 and 4 are mutually exclusive: you can not have both delegating and traditional default constructor with setters approach, only one.
This is the problem here as existence of "array delegating" prevents use of expected traditional POJO creation.
I think I was wrong earlier, and that it would be legal to have "delegating" AND "array-delegating".

Having said all that, I will see if it would actually be possible to support intended usage here. It seems possible that maybe combination might be supportable.

In addition, this gives me some ideas of how to maybe improve the handling for 3.x (if not earlier). Division in many cases is by "shape" of incoming type, and maybe one could annotate @JsonCreators to support more interesting delegation -- so that one could match JsonToken.VALUE_NUMBER_INT to BigInteger, or any other type that can be deserialized from integral number.
Similarly for other shapes as well. This would reduce need for hard-coded mappings to small set of types.

[cowtowncoder]

Interestingly enough, just commenting out array-delegation as indicate, like so:

        _nonStandardCreation = (_unwrappedPropertyHandler != null)
            || _valueInstantiator.canCreateUsingDelegate()
//            || _valueInstantiator.canCreateUsingArrayDelegate()
            || _valueInstantiator.canCreateFromObjectWith()
            || !_valueInstantiator.canCreateUsingDefault()
            ;

makes test pass, does not cause new failures with existing tests. I will have to go through code with more thought to see what other implications there might be but this might be easy to solve, for particular case here.

[vjkoskela]

In addition, this gives me some ideas of how to maybe improve the handling for 3.x (if not earlier).

That would be amazing!

When I originally read the code, I was surprised at the complexity, but years of incremental additions have a way of doing that! -- totally understandable. If you come up with a path forward for 2.x please post here, I may have some cycles over the next few weeks to take a look. I'm back in Java land for a while and certainly if I run into this again it would give me a reason to invest some cycles.

For context, the specific use cases that brought this up were extending two existing serialization models from primitive (case 1) and array (case 2) to object. So we created an object to represent the new model in each case, and added @JsonCreator methods to support the old models (from primitive in one case and from array in another). That's when we noticed that only the primitive @JsonCreator path allowed us to also deserialize from the new object shape as well.

I also updated the tests, looks like you did too - thank you - and they now include variants that do not deserialize through the Builder (just into it) and behave the same way. So I don't think that the deserialization through the builder is related.

#2708