Fix tail-recursive deserializer (Lagom's #3241)

[ignasi35]

Fixes JsonNodeDesrializer. See lagom/lagom#3241 for context and lagom/lagom#3241 (comment) for a workaround.

When tail recursing, the previous implementation would advance the cursor even when the recursion was about to complete. As a consequence, when invoking Jackson's readValue (or readValueAsTree, or any equivalent) from within a deserialization the cursor would skip some events leading to skipped properties and data loss.

[ignasi35]

@Mergifyio backport 2.8.x

[mergify]

Command backport 2.8.x: pending

Waiting for the pull request to get merged

[ignasi35]

Moving the nextToken() into this case is the fix.

[ignasi35]

Using jp.readValueAsTree() is necessary to reproduce the error.

It's in this invocation where the cursor ends up corrupted. When entering the invocation, jp is pointing too far away. Imagine we're parsing the following String:

  """
          |{
          |  "createdAt": 1234,
          |  "child": {
          |    "updatedAt": 555,
          |    "updatedBy": "another-user"
          |  },  // expected position of the cursor
          |  "updatedBy":   /*actual position of the cursor */  "some-user",
          |  "updatedAt": 5678
          |}
          |""".stripMargin

[mkurz]

Besides a typo which I pushed a commit for, I think I understood the problem and the fix and the tests looks good to me 👍

[jrudolph]

LGTM

[jrudolph]

Shouldn't that have been covered by the test?

[ignasi35]

IDK. I wonder if Jackson is clever enough to try both the name on @JsonProperty and the field name.

Otherwise, it's possible the name in the JsonProperty annotation is not being used. I'll have a look and remove the annotations if they're not necessary.

[ignasi35]

This is nuts! The following code which uses named properties but all names are wrong still makes the test pass:

  @JsonCreator
  public Parent(
      @JsonProperty("a45") @NonNull Long createdAt,
      @JsonProperty("a42") Child child,
      @JsonProperty("foo") @NonNull Long updatedAt,
      @JsonProperty("bar") @NonNull String updatedBy)

But the following code which doesn't use the annotation, fails:

  @JsonCreator
  public Parent(@NonNull Long createdAt, 
    Child child,
    @NonNull Long updatedAt,
    @NonNull String updatedBy) {

I've even removed the setters from the class to make sure those were not being picked up by Jackson.

[mkurz]

Shouldn't that have been covered by the test?

To be honest, I thought as well there is something fishy, however because the test failed as expected in 16d6339 and got fixed by 400f7a8 I thought this isn't really relevant for what the test is originally about.

IDK. I wonder if Jackson is clever enough to try both the name on @JsonProperty and the field name.

Jackson can't look at the parameters names, because we don't compile with the -parameters flag, which is needed to store parameter names into class files so reflection can make use of it.

Based on your crazy example Ignasi, I went even further and tried:

  @JsonCreator
  public Parent(
      @JsonProperty("a45") @NonNull Long a,
      @JsonProperty("a42") Child b,
      @JsonProperty("foo") @NonNull Long c,
      @JsonProperty("bar") @NonNull String d) {
    this.createdAt = a;
    this.child = b;
    this.updatedAt = c;
    this.updatedBy = d;
  }

So not even the parameters names match. As expected even this passes the test.

My understanding of how Jackson works is that you have to mark parameters with @JsonProperty so they are taken into account. If a jackson property name can not be found in the actual json I guess Jackson tries to figure out the best matching json node, properly based on the order of the json properties. I googled a bit and it looks like order is somehow important.

Anyway, I think this Jackson magic is not really relevant for the original bug fix, so LGTM.

[ignasi35]

I've reviewed the implementations of Parent and Child to make sure Jackson uses the constructor arguments (removed setters, made fields private and final, etc...).

I've also removed the JsonProperty annotation from the Child since those are not required because the Child class uses a custom serializer.

[mergify]

Command backport 2.8.x: success

Backports have been created

• #10916 Fix tail-recursive deserializer (Lagom's #3241) (backport #10840) has been created for branch 2.8.x