FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.52k stars 1.38k forks source link

Sporadic failures on deserialization of cyclic object graph #2152

Open nlisker opened 6 years ago

nlisker commented 6 years ago

When deserializing a cyclic object graph, sometimes an exception will be thrown

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot resolve ObjectId forward reference using property 'propertyName' (of type someType): Bean not yet resolved

Iv'e prepared a reproduction example: https://github.com/nlisker/JacksonRepoExample

The project contains:

After setting up the project, execute serializeCycle several times (you might need 20). Sometimes it will work, sometimes it will throw:

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot resolve ObjectId forward reference using property 'landPlot' (of type impls2.StructureImpl2): Bean not yet resolved

with a very long stack trace.

The method testSerializeCycle runs serializeCycle in a loop, but you will notice that either all attempts fail or all succeed. This tells me that the thing that causes the difference is not in the process of creating a POJO, serializing it, and then deserializing it. Instead, that difference happens each time the JVM is started. I suspect that there is a dependence on the order of values returned from a map, which is randomized on each JVM startup since Java 9. The order of the values in the JSON is different, and sometimes it will fail and sometimes not depending on this order.

If in interfaces.Structure you comment out the block:

int getOwner();

Status getStatus();

StructureType getType();

int getDefaultRate();

int getRate();

int getTimeToBuild();

then the error does not occur (at least after many attempts). These properties are unrelated to the cyclic dependency of LandPlot and Structure, which is further evidence that the deserialization process is doing something wrong.

cowtowncoder commented 5 years ago

I can not think of why ordering of entries in a Map would matter, but if this requires Java 9, that could be relevant for detection. I develop Jackson on Java 8 JVM, although have occasionally tested it on Java 9. I assume reproduction is done with a recent version, like 2.9.7.

I hope someone has time to look into this: but at this point I do not have time to look deeper myself.

I would also suggest that look into JSOG ID generator might matter -- problem is that originally Jackson's Object Id was assumed to be, and designed for, scalar values (Strings and numbers). Use of JSON Object for ids seems silly, and I am not big fan of JSOG approach; but support was added to allow that in limited case of a JSON Object with exactly one property. At any rate, that could be relevant.

stickfigure commented 5 years ago

It's hard to imagine this has anything to do with the JSOG code since there's almost no code there; it just makes a slight tweak to the serialization/deserialization of references. Jackson is doing all the work. I would be shocked if this same behavior didn't happen with regular scalar ids.

Cannot resolve ObjectId forward reference sounds like a ref is being processed before an id. Tatu, I presume Jackson runs depth-first and expects ids before refs? Is everything being stored in LinkedHashMap?

It's got to be one of two cases:

  1. The ids/refs are being written out of order. This should be easy to check - when there is a failure, look at the serialized form. If it looks right then the problem is #2.
  2. On deserialization, the values are getting read into a structure that doesn't preserve order (ie HashMap).

I don't really understand what goes on under the covers in Jackson, unfortunately. But given the behavior we're seeing, #2 seems more likely.

stickfigure commented 5 years ago

P.S. Tatu if you have a better suggestion for how to structure JSON so that non-Java environments can deserialize cyclic object references, I'd love to hear it. Using scalar values as references requires the deserializer to have a hardcoded schema in advance (so it knows the difference between a ref and an actually-scalar value), which doesn't work in javascript/python/ruby/etc.

cowtowncoder commented 5 years ago

@stickfigure my dislike of wrapping things in JSON Objects is somewhat general, and not really JSOG-specific. I did not know some languages would have issue with scalar ids, given that other data formats use straight string ids by default (XML, YAML). But if that is the reason it is good, something new I learned today.

Now: Jackson does not assume that ids are always read in order, given that this can not be relied on (some platforms would not store things in arrival order anyway). This means that references and reference ids are stored as necessary, lazily resolved as necessary. But this does lead to potential problems if reference is passed via constructor: combination is potentially impossible to support. So that is one possibility.

At any rate, I do not recall any reason why storage of ids in order would be necessary or required: but I understand how error message might lead to such conclusion. What message does try to say is that with all the information processed, id was not found. Reference to "forward-reference" does mean that by the time reference id was seen, referred-to object had not yet been seen or instantiated. But it's bit redundant as if it was seen, we'd never run into this problem anyway (because resolution would succeed)

I guess another possibility could be that maybe id-to-Object mapping was not being retained for some reason.

But non-reliability is odd.

stickfigure commented 5 years ago

Is it possible to enhance that error message so that it shows the toString() of the id? Maybe that will help clarify.

Regarding XML/YAML - they both have references built into the grammar, so the parser never has to wonder if a value is a reference or some sort of scalar. We don't have that luxury with JSON... is {"thing": 123} a reference or is it literally the value 123? If you're deserializing this in javascript/python/ruby/etc, you can't look up the Java class...

cowtowncoder commented 5 years ago

Yes, improvement to error message would make sense, if possible. One caveat being that it may be omitted for some reason... like not being accessible, perhaps.

Now, as to YAML/XML, I can see that with YAML (one of parts of YAML I like is that it does have explicit type-id/anchor concepts). But with XML this is debatable: it depends on what is thought to be core xml I guess, as well as on whether tools require somethnig additional like XML schema. Anyway. That's neither here nor there I guess.

nlisker commented 5 years ago

Jackson does not assume that ids are always read in order, given that this can not be relied on (some platforms would not store things in arrival order anyway). This means that references and reference ids are stored as necessary, lazily resolved as necessary. But this does lead to potential problems if reference is passed via constructor: combination is potentially impossible to support. So that is one possibility.

So is it possible that the example I provided is just wrong usage and Jackson behaves correctly? If so, I'd like to know what the correct way to do what I'm attempting is.

Generally, if I have an object C whose reference is stored in A and B and I serialize A and B, I don't want C to be serialized twice (let's say that C is large) and I want after deserialization reference equality between the C in (the deserialized) A and the C in B, that means that C is constructed only once. So C will get an identity (like @id: 1) in one of A or B and in the other it will be replaced by a reference (like @ref: 1). To my understanding, the first occurrence will always be assigned an id and the rest will be referenced. When deserializing, isn't the same order guaranteed to be retained?

Did I simplify too much and lost the cause of the problem?

cowtowncoder commented 5 years ago

Correct: when Identity info is used, "full" value is only serialized first time it is encountered during serialization, and after that all references are written as reference id. And on deserialization, only one Object is instantiated and all references (first and others) should point to that instance.

But the point I made about ordering is that while Jackson itself does guarantee document order, it is possible that some other package is used for modifying json data, possibly reordering properties of JSON Objects, which may change content so that a reference is encountered before "first" instance of serialization.

Even without such cases, however, it is also possible that ordering guarantee is not helpful. For example when using @JsonCreator for passing references: Object can not be constructed before all creator parameters have been processed, and somewhere within those parameters may well be reference to object that is not yet created. This case is very difficult if not impossible (for some cases it probably is ... f.ex if all values use @JsonCreator) to solve.

I don't know if these cases occur here (probably not?)

nlisker commented 5 years ago

@JsonCreator is not being used and I don't see any other library that can change parameter order. I think that the best way to proceed will be to look at the reproduction example I posted, it will relieve all speculations.

cowtowncoder commented 5 years ago

Ok. @JsonCreator is bigger problem, although there are some others (esp. @JsonUnwrapped) which could cause issues -- I assume not related.

As to other libraries/gateways/proxies: I did not mean that occurs in your case, but that this is the reason why code can not guarantee, nor relies, on values deserialized being found in "correct" order. That is, deserializer does not and can not assume it always gets full definition before references to it. That's all I was trying to say.

nlisker commented 4 months ago

Has this been removed from the release plans because it's covered by #3964 or because it's old?

cowtowncoder commented 4 months ago

If you mean why label "2.14" was removed it's because that is obsolete -- 2.14 branch has been closed for a while. But I don't think anyone has been working on this; not aware of any specific plans.

So that label removal did not really have any particular significance wrt issue.