Skip to content

Fix issue with deserializing nested Results from JSON#31

Merged
jhartz merged 3 commits intomasterfrom
jh/fix-nested-deser-bug
Jun 14, 2021
Merged

Fix issue with deserializing nested Results from JSON#31
jhartz merged 3 commits intomasterfrom
jh/fix-nested-deser-bug

Conversation

@jhartz
Copy link
Contributor

@jhartz jhartz commented Jun 9, 2021

If a JSON body had a type structure with nested Results, we would sometimes fail to parse it. This was because the deserializer looked for the first @result field it found, but it did this using Jackson's findValue(), which essentially does a depth-first search through the JSON and would sometimes find the @result of an inner nested result rather than the outer result.

For example:

{
  "outerField": {
    "innerObject": {
      "innerField": "value",
      "@result": "ERR"
    }
  },
  "@result": "OK"
}

When parsing the outer Result, the deserializer would find the inner "@result": "ERR" rather than the outer "@result": "OK".

This PR fixes that by using Jackson's get() instead of findValue(), which should only find immediate children of the JSON node (without recursively searching down the tree).

I haven't been able to think of a case where something would be depending on the previous findValue() behavior, but I guess it's possible. If someone thinks there could be cases that this change would break, I could update it to do a firstNonNull(get(...), findValue(...)) to try to maintain better backwards compatibility.

/cc @stevegutz @jhaber @szabowexler

@jhartz jhartz requested a review from stevie400 June 9, 2021 14:18
Copy link
Contributor

@stevie400 stevie400 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix seems reasonable, but I'm curious to hear more about the case where you're constructing nested results. My hunch is that a different pattern might serve you better.

}

@Test
public void itDeserializesNestedOkOk() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be serialization tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jhartz
Copy link
Contributor Author

jhartz commented Jun 9, 2021

I'm curious to hear more about the case where you're constructing nested results. My hunch is that a different pattern might serve you better

In this situation, I have an operation that does a batch of work (rendering a single email template for a batch of contacts). The batch can fail due to an issue with the template or with fetching the list of contacts (which are failures for the entire batch), or individual emails in the batch can fail (due to issues with one of the individual contacts) while others succeed.

The type I'm using here is something like:

Result<Map<Contact, Result<ContactSuccess, ContactFailure>>, TemplateFailure>

private static final Result<String, NullValue> NULL_ERR = Result.nullErr();
private static final String NULL_ERR_JSON = "{\"@error\":null,\"@result\":\"ERR\"}";

private static final Result<Map<String, Result<TestBean, TestError>>, Map<String, Result<TestBean, TestError>>> NESTED_OK_OK =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please simplify these types? Combining the maps and results this way really makes it hard to grok this stuff.

@jhartz jhartz merged commit 5469dc9 into master Jun 14, 2021
@jhartz jhartz deleted the jh/fix-nested-deser-bug branch June 14, 2021 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants