StackOverflowError on recursive references

[jbescos]

By default, when an object contains a reference of itself, there is a StackOverflowError. This is because ObjectSerializer doesn't check this.

I was not able to see if this error started to happen from recent changes. It seems it is there for long time, or maybe the default serializer was AdaptedObjectSerializer.

This pull request should fix the next issues:
#343
#305

[aguibert]

I'm not sure if this will work in all cases. For example, what would happen if an object has two separate references to the same object?
For example:

public static class A {
  public B ref1;
  public B ref2;
}
public static class B {
  public String prop = "foo";
}

A a = new A();
B b = new B();
a.ref1 = b;
b.ref2 = b;

If we serialize a, it should be:

{ 
  "ref1": { 
    "prop": "foo"
  },
  "ref2": {
    "prop": "foo"
  }
}

but with this PR I think we would get:

{ 
  "ref1": { 
    "prop": "foo"
  }
}

[jbescos]

@aguibert thanks for taking a look into this. It is interesting to add this test case, so I have created it in a new test case with your example.

The result is the expected, because before it tries to serialize a.ref2, the a.ref1 was already removed from the processed object hashset.

[aguibert]

thanks for adding the test @jbescos, I'd still like to add a few more test scenarios before merging this PR. Mainly I want to make sure we don't skip serialization of duplicate objects that are not recursive.

• test a chain of duplicate objects:

Foo foo = new Foo("foo");
Chain c1 = new Chain("c1");
Chain c2 = new Chain("c2");
c1.linksTo = c2;
c1.has = foo;
c2.has = foo;
jsonb.toJson(c1);

Should result in:

{ 
  "name": "c1",
  "has" : { "bar": "foo" },
  "linksTo": {
    "name": "c2",
    "has" : { "bar": "foo" }
  }
}

I think this scenario will break because we will add foo as a processed object when we serialize c1, and then when the code traverses down to the child object c2 it hasn't hit the finally block yet which removes foo as a processed object.

• Test the above object setup with an Adapter, because the AdaptedObjectSerializer also calls add/removeProcessedObject() and I wonder if they will interfere with each other
• Test the above object setup with a Serializer, for the same reason as the Adapter

[jbescos]

@aguibert I have tested with default jsonb and using the AdaptedObjectSerializer and it works as expected.

But I had to deep inside in the code and now I have a better understanding.

There are 2 serializers that are taking care about recursive references: AdaptedObjectSerializer and UserSerializerSerializer. SerializerBuilder#build() is the class that will give the required serializer.

AdaptedObjectSerializer is activated when you specify a custom adapter. For example:

Jsonb adapterSerializerJsonb = JsonbBuilder.create(new JsonbConfig()
            .withAdapters(new ChainAdapter(), new FooAdapter()));

UserSerializerSerializer is activated when you specify a custom serializer, for example:

Jsonb userSerializerJsonb = JsonbBuilder.create(new JsonbConfig()
            .withSerializers(new ChainSerializer(), new FooSerializer()));

But note that when any of previous serializers are activated, the ObjectSerializer will not be activated. This is because your custom serializer/adapter is going to replace ObjectSerializer.

In conclusion, AdaptedObjectSerializer and UserSerializerSerializer could interfere with ObjectSerializer because they share the same context (same HashSet). But in practice it will not happen.

[jbescos]

@aguibert after the tests I have added do you think it is safe to be merged?.

[aguibert]

hi @jbescos thanks for raising this PR!

I'm not sure if this approach will work though, I believe we need to do more advanced checking for already processed objects, to ensure we don't skip processing two different fields that happen to reference the same object.

[jbescos]

It was closed by github because I had to remove previous commits (they were not signed).

As there were 0 commits in the PR, the PR was automatically closed.

I'm reopening now.

[aguibert]

I would prefer we don't use loops like this in the unit tests, instead just make the calls on 2 separate lines. The benefit here is that when a test fails, we can easily determine which Jsonb instance the failure occurred in. If we do a loop we can't tell just by looking at the stack trace

[aguibert]

same comment applies in a few other cases too

[jbescos]

Okay, in the other hand if we want to add a new Jsonb for the tests you need to add it in every test method. Humans make mistakes doing this kind of mechanical work, but anyway we have code reviewers :).

[aguibert]

I'm not sure what the point of this test is? If we did properly deserialize a recursive reference these objects would be equal to each other. (I'm not saying we need to make that behavior work, because it's impossible afaik, I'm just saying I'm not sure what this test is asserting)

[jbescos]

I wanted to verify that objects with same values are not the same instance. I know it is tricky that this would happen because there should be some caching. But in case somebody in the future decide to reuse instances because of performance reasons, this test will catch it.

In this case, Chain doesn't implement equals to check values, so it only checks the reference. I have changed the assert to illustrate my intentions:
assertFalse(recursive.getLinksTo() == recursive);

[jbescos]

Thinking it a bit more, it makes no sense to have this test.

Because 'recursive' and 'recursive.getLinksTo()' are never equals from any point of view. First one has a not null linksTo, and the second has it null.

The test would be valid when if we are testing exactly objects with the same values, in a collection for example:

        List<Chain> repeatedObjects = jsonb.fromJson("[{\"name\":\"test\"},{\"name\":\"test\"}]", new TestTypeToken<List<Chain>>(){}.getType());
        assertFalse(repeatedObjects.get(0) == repeatedObjects.get(1));

But I think this is out of the scope of this pull request.

I'm going to remove this test.

[aguibert]

The code in this test is fine but it should be called testSerialize2ReferencesSameObject since we are serializing, not deserializing in it

[jbescos]

Correct, I have updated it.

[aguibert]

looks good now, thanks @jbescos