Skip to content

Deserialize JSON merge patch as JsonValue#26

Merged
cowtowncoder merged 7 commits intoFasterXML:2.13from
patveck:2.13
Nov 20, 2022
Merged

Deserialize JSON merge patch as JsonValue#26
cowtowncoder merged 7 commits intoFasterXML:2.13from
patveck:2.13

Conversation

@patveck
Copy link
Contributor

@patveck patveck commented Nov 15, 2022

Fix for #27

@cowtowncoder
Copy link
Member

Sounds good, as per discussion. Aside from CLA (thanks for the email explaining the situation), would it be possible to add same change for jakarta-jsonp module? Due to awkward relationship between "old" JAXP and new "Jakarta", there are 2 modules which are almost identical, save for dependency to one API version or the other.

Another question is whether JsonPatchDeserializer might need changes too.

@patveck
Copy link
Contributor Author

patveck commented Nov 16, 2022

Thanks! I've applied the same changes for JSON Merge Patch in the jakarta-jsonp module.

Indeed, also JsonPatchDeserializer throws several different types of exceptions when the input is not an array, including an NPE. However, the situation is somewhat different: the relevant RFC explicitly states that a JSON Patch is an array of objects (Section 3, first sentence). Section 5 of the same says that evaluation of the patch should terminate if the a normative requirement is violated (emphasis mine). Indeed, the Glassfish implementation of JsonPatch#apply checks whether all elements are objects.

Consequently, I think it is sufficient for Jackson to check whether the input is an array and there's no need to iterate over its elements (that's the job of JsonPatch#apply). In case the input is not an array, we should throw an exception; I think InvalidFormatException is the appropriate one.

I've updated the PR for JsonPatch, but only in the jsr-353 module for now (I'll update the jakarta-jsonp once jsr-353 is stable).

@cowtowncoder cowtowncoder merged commit 035f5b8 into FasterXML:2.13 Nov 20, 2022
@cowtowncoder cowtowncoder added this to the 2.13.5 milestone Nov 20, 2022
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