@JsonPropertyOrder causes deserialization failure (unresolved forward reference)

[cowwoc]

Jackson 2.4.4 is able to successfully deserialize forward references, but it fails to do so if @JsonPropertyOrder requires serializing "parent" references after "child".

Testcase: https://gist.github.com/cowwoc/e31e96e093a2981f90bf

Inserting "parent" before "child" in @JsonPropertyOrder, or removing "child" fixes the problem.

[cowwoc]

More generally speaking, the workaround is to insert all other properties before "child".

To fix this bug Jackson will need to implement the following logic:

1. Given CURRENT contains property X
2. While deserializing CURRENT we run across property X
3. If X references CURRENT then deserialize all other properties of CURRENT before attempting to construct X.

[cowtowncoder]

It really should not be necessary, esp. given that there really is no guarantee that other parts of systems would honor the ordering. So while this may be true in working around the issue, what really needs to happen is to ensure that forward references are correctly handled, even in presence of explicit ordering.

Attempts at reordering were done before forward-reference handling and should not actually be needed any more. I have been thinking of possible removing these altogether, given that required buffering is not free.

[pgelinas]

This is more complicated then just "child before parent" problem. If you do the same test, but with only two "Task" object, it works with that property order. The current solution for forward reference is able to handle any ordering of properties and parent-child relationship without re-ordering; there has to be something very specific regarding this case which does not work properly.

By simplifying the test case to one simple class, everything works fine:

    @JsonPropertyOrder({"@id", "child"})
    @JsonIdentityInfo(generator = ObjectIdGenerators.IntSequenceGenerator.class)
    public static class Task {
        public Task child;
        public Task parent;

        protected Task() throws NullPointerException {}

        public Task getChild() {
            return child;
        }

        public Task getParent() {
            return parent;
        }
    }

I am guessing that generics resolution here is what's causing the problem, giving the complexity of it in the test case. I'll add more and more complexity to the test case to see what exactly is causing the problem.

[cowwoc]

You're right.

This issue only occurs at 3 levels and only with Generics. If I remove all type parameters (even at 3 levels) everything works fine.

[pgelinas]

Alright, so what is causing the problem here is the constructor parameters coupled with the nesting of the parent. If you remove the constructor everything works well, even with the generics. I did saw something recently related to that on another message, but I can't seem to find it.

[cowwoc]

@pgelinas Right, because once you remove the forward declaration there can't be any forward declaration error :) But that's not really a solution.

[pgelinas]

No, that's not removing forward declaration, it can still be resolved through other means, like public field in this case. There is a problem with constructor parameter being used for forward reference resolution and it seems to be related to multiple level, because with only 1 level of nesting it works. I can't investigate this further right now because my dev environment is foobar at the moment.

[cowwoc]

When you say a public field, are you referring to a public final field?

[pgelinas]

Yes, Jackson is able to assign final fields, because, you know, wizardry and stuff. So with this case, provide a default private constructor, remove the @JsonProperty annotation from the constructor and it just works (tm).

[cowwoc]

Okay, but I don't really like the idea of having to maintain separate constructors for Jackson and user-code. Constructor-based injection has the advantage of allowing me to validate inputs in a single place. With your suggestion, I lose that.

For now I'll just camp out until this issue is fixed. (Sorry, but I am super fixated on constructor injection)

[pgelinas]

Haha, I understand, that's perfectly fine. I don't suggest this as the way to fix the issue, it's just a temporary workaround for now. I'd really like to fix the remaining issues about forward references.

[cowtowncoder]

@cowwoc As much as I like immutability via constructor-parameters, it is a huge minefield in combination with other things, having chicken-and-egg problem, for any references that may need to be resolved in arbitrary order. And since reordering can not be guaranteed to work reliably OR efficiently (not just both), I really think you need to consider this a hard limitation.
Put another way, solving this may changing references to not be creator parameters is an easy way to reliably solve the problem: not optimal, but something that can and will work. Unlike trying to fight against constructor-parameter proble.

[cowwoc]

@cowtowncoder (Thinking out loud)

In the case of my testcase, it is technically possible to re-construct the hierarchy using POJO public methods. No magic/reflection is necessary. The difficulty is figuring out the construction order programmatically. Meaning, your code needs to figure out it needs to do this:

        Task1 task1 = new Task1();
        Task2 task2 = new Task2(task1);
        Task3 task3 = new Task3(task2);
        task1.child = Optional.of(task2);
        task2.child = Optional.of(task3);

In such a case, I don't think we have a "chicken and egg" problem (correct me if I'm wrong).

Are you saying that even for such a case it is technically difficult to figure out the reconstruction order programmatically? If so, perhaps we could move from an injection model to a Service Locator pattern (similar to what I do at https://bitbucket.org/cowwoc/pouch/wiki/Home). Meaning, maybe you can give users some registry and let them reconstruct the object themselves (they should know the proper order).

[pgelinas]

Since this work with only 1 level of nesting, this means that constructor parameter with forward reference works as intended. The problem we are seeing here might be caused by multiple nesting which trigger a corner case. I'm confident that this can be fixed rather simply, it's just a matter of proper debugging, which I'd like to do but can't right now. I'll try to fix this Monday.

[cowtowncoder]

@cowwoc I just have no faith in ordering-based solution for deserialization. Serialization works in a way to guarantee first-definition-then-reference property, but other tools do not hold this invariant. Trying to reorder incoming JSON is something that I do not think is practical because, like @pgelinas mentions, problems comes from multi-level dependencies, and may be complicated further by polymorphic typing.

So I have no interest in crawling through that rathole. If you want to investigate this thoroughly you are free to investigate where things lead. But I wasted enough time on that approach as is.

[cowwoc]

@cowtowncoder I am hoping we can find some middle-ground here. I mean, on the one hand I agree with you that the complexity can get out of hand for the general case but on the other hand I am hoping that we can find a mechanism that enables users to aid the deserialization process.

I mentioned the Service Locator pattern above. That might not be the solution but it's the general direction I'm heading in.

Before I go on, I want to point out the following: in this testcase "child" references "parent" but "child" is deserialized before some of parent's properties. Regardless of what solution we come up with, you will absolutely have to deserialize those other properties before constructing "child". Meaning, some amount of ordering and buffering is inevitable (and technically impossible to remove). If you disagree, please explain what I'm missing.

Now, getting back to possible solutions: Are you familiar with QueryDSL's class Tuple? http://www.querydsl.com/static/querydsl/3.2.0/apidocs/com/mysema/query/Tuple.html

It is a user-generated class (a detail we'll get back to later on) but essentially I'm saying Jackson could provide a Tuple to a user. The user would then get() each property one at a time until they reach child. At this point, the user would have enough information to construct Child themselves. They would then invoke Tuple.get(Parent.child) which would return a new Tuple that consists of the child's properties. They would then use that Tuple in Child to deserialize its own properties and so on.

QueryDSL does this in a type-safe manner by generating Tuple subclasses at build-time. If you don't like that idea, you can simply return Object and let users cast to the right type (in fact, Java8's new type system might be smart enough that you could get it to cast automatically).

Anyway, I want to get your feedback on this general concept and we can move on from there. In practice I am advocating shifting the ordering complexity out of Jackson and into the user's hand. In this particular case I believe it would be quite easy for the user to handle themselves.

The only thing Jackson would need to handle is deserializing JSON to Java types and buffering the intermediate properties until the user requests them.

[cowtowncoder]

At this point I am not sure if the original problem still exists (a few bugs have been fixed that could have triggerd this). If it is, a reproduction with 2.8.0 would be needed. For now, I'll close as there's nothing for me work on here.