Support for JSON Patch

[glassfishrobot]

Consider adding support for JSON Patch. The RFC is at http://tools.ietf.org/html/rfc6902

Perhaps, it can be used for transforming one JsonObject/JsonArray to another (works well since JsonObject/JsonArray are immutable)

[glassfishrobot]

Reported by jitu

[glassfishrobot]

kchung said: Note there is another alternate spec for patching a JSON document: Json Merge Patch, rfc 7396

This uses a diff patching, and is less verbose. There are some short-comings:

1. It uses null for delete, so you cannot replace a value with null.
2. It does not patch array entries, so you'll need to replace the whole array.

Maybe we should consider supporting both.

[glassfishrobot]

mitemitreski said: @kchung as you said https://tools.ietf.org/html/rfc7396 is the one that is a lot less verbose and it is more practical. In my book, this one should be given a higher priority.

On the other hand, if we already have a JSON-Pointer as part of the spec they both make sense.

[glassfishrobot]

asotobu said: As @mitemitreski said, having json pointer implemented, the hard work is done and we can offer support for both of them, maybe we can start with merge patch and then if we are on time we can implement patch as well, or we can define both at first.

[glassfishrobot]

kchung said: Agreed that we should do both, at least we should not exclude either at first.

In terms of implementation order, I also agree that JSON Pointer should be done first. In fact I believe we should tackle the editing or transformation operations first, because they are lower level operations that both JSON Pointer and JSON can build on. See https://java.net/jira/browse/JSON_PROCESSING_SPEC-67 for more details.

Agreed also that JSON patch should include a diff function.

[glassfishrobot]

asotobu said: I send you my first purpose for Json Patch API:

/**
* <p>{@code Patch} interface represents a common interface for applying patches on a {@link JsonStructure}.<p>
* <p>A patch is a {@code JsonStructure} for expressing operations or modifications to a given JSON document.</p>
**/
public interface Patch {

  /**
  * <p>Apply this patch to a {@code JsonStructure}.</p>
  * @param target to apply the patch to.
  * @return the patched {@code JsonStructure}. Since a {@code JsonObject} or {@code JsonArray} is immutable,
  *         these operation returns a new JSON Object or array.
  */
  JsonStructure apply(JsonStructure target);

}

/**
* <p>Implementation of JSON Patch described at <a href="https://tools.ietf.org/html/rfc6902">rfc 6902</a></p> * <p>JSON Patch is a format (identified by the media type "application/json-patch+json") for expressing a sequence of operations to apply to a target JSON document;
* it is suitable for use with the HTTP PATCH method.</p>
* <p>A JSON Patch document is a JSON document that represents an array of objects.
* Each object represents a single operation to be applied to the target JSON document.</p>
* <p>The following is an example JSON Patch document:</p>
* <pre>
*
* [
*  { "op": "test", "path": "/a/b/c", "value": "foo" },
*  { "op": "remove", "path": "/a/b/c" },
*  { "op": "add", "path": "/a/b/c", "value": [ "foo", "bar" ] },
*  { "op": "replace", "path": "/a/b/c", "value": 42 },
*  { "op": "move", "from": "/a/b/c", "path": "/a/b/d" },
*  { "op": "copy", "from": "/a/b/d", "path": "/a/b/e" }
* ]
* </pre>
* <p> Operations are applied sequentially in the order they
* appear in the array.  Each operation in the sequence is applied to
* the target document; the resulting document becomes the target of the
* next operation.  Evaluation continues until all operations are
* successfully applied or until an error condition is encountered.
* </p>
**/
public JsonPatch implements Patch {

  /**
  * <p>Creates a {@code JsonPatch} object.</p>
  * @param jsonPatch array structure expressing a sequence of operations to apply to a JSON document.
  *        This document follows <a href="https://tools.ietf.org/html/rfc6902">rfc6902</a> notation defined at <a href="https://tools.ietf.org/html/rfc6902#section-3">Section 3 Document Structure</a>.</p>
  **/
  public JsonPatch(JsonArray jsonPatch) {

  }

  /**
  * <p>Apply this json patch document to a {@code JsonStructure}.</p>
  * @param target to apply the patch to.
  * @return the patched {@code JsonStructure}. Since a {@code JsonObject} or {@code JsonArray} is immutable, these operation returns a new JSON Object or array.
  */
  public JsonStructure apply(JsonStructure target) {

  }

}

/**
* <p>Implementation of JSON Merge Patch described at <a href="http://tools.ietf.org/html/rfc7386">rfc 7386</a></p> * <p>A JSON merge patch document describes changes to be made to a target
* JSON document using a syntax that closely mimics the document being
* modified.  Recipients of a merge patch document determine the exact
* set of changes being requested by comparing the content of the
* provided patch against the current content of the target document.
* If the provided merge patch contains members that do not appear
* within the target, those members are added.  If the target does
* contain the member, the value is replaced.  Null values in the merge
* patch are given special meaning to indicate the removal of existing
* values in the target.
* </p>
**/
public JsonMergePatch implements Patch {

  /**
  * <p>Creates a {@code JsonMergePatch} object</p>
  * @param jsonMergePatch JSON document that describes changes to be made to a target using syntax defined in <a href="http://tools.ietf.org/html/rfc7386">rfc7386.
  **/
  public JsonMergePatch(JsonObject jsonMergePatch) {

  }

  //... }

[glassfishrobot]

kchung said: @Alex Thanks. Several quick comments.

1. Why do we need the interface Patch? Since 6902 and 7963 are totally different, there is nothing common except in names. It does not really make much sense to combine JsonPatch and JsonMergePatch into one interface.
2. How about just have two concrete classes: JsonPatch and JsonMergePatch.
3. We should include overloaded "apply" method for JsonArray and JsonObject.
4. Maybe a static generatePatch (or just diff) method?

[glassfishrobot]

asotobu said: 1. Probably you were right, I was thinking more in creating a factory of merge methods (path or merge patch) but as I said we can remove it. 2. Agreed 3. Don't understand this. Why apply should be in JsonArray or JsonObject? I was having in mind using JsonPointer operations. For example in pseudocode:

JsonArray patchOperations = ...
target = ...
foreach(JsonObject patch:patchOperations) {
  JsonPointer pointer = new JsonPointer(patch.get("path"));
  if(patch.get("op") is "add") {
    target = pointer.add(target, patch.get("value");
  }
}

4. Probably it is true but maybe we could wait for next iteration.

[glassfishrobot]

kchung said: Regarding 3, what I meant was:

public class JsonPatch {
    JsonStructure apply(JsonStructure target);
    JsonArray apply(JsonArray target);
    JsonObject apply(JsonObject target);
}

So that you can write

JsonArray result = jsonPatch.apply(target);

If target is known to be a JsonArray, without having to use cast on the result.

[glassfishrobot]

asotobu said: Understood yes it has a lot of sense. I am going to merge the PR about JsonPointer and start working on that.

[glassfishrobot]

asotobu said: I have written several tests for checking and I have found some issues that I think it may be changed, I am going to mention here and if you agree I can fix:

• In case you refer an array position with a char instead a number a NumberFormatException is thrown. I think it should throw a JsonException.
• In case you refer an array position with an integer but out of bounds of the array a IndexArrayOutOfBounds is thrown. I think it should throw a JsonException.

In next test cases json patch is not applied because it says that from patch contains the path path expression:

{
            "op": { "op": "move", "from": "/x/a", "path": "/x/a" },
            "target": { "x": { "a": "helo" } },
            "expected": { "x": { "a": "helo" } }
        },
        {
            "op": { "op": "move", "from": "/0", "path": "/0/x" },
            "target": [ "victim", {}, {} ],
            "expected": [ { "x": "victim" }, {} ]
        }

I think that this check is not necessary, I have read the spec and I have not seen anything about this limitation.

And then the other thing I have noticed is this case:

{
            "op": { "op": "replace", "path": "", "value": false },
            "target": { "x": { "a": "b", "y": {} } },
            "expected": false
        }

Which basically replaces a document to a value. If new spec supports adding simple values in root part, it seems that this exception should not be thrown, isn't it?

Alex.

[glassfishrobot]

kchung said: Agreed that JsonException should be thrown instead of NumberFormatException and ArrayIndexOutofBoundsException.

Regarding the check for overlaps in move, the spec says

The "from" location MUST NOT be a proper prefix of the "path" location; i.e., a location cannot be moved into one of its children.

So there should be a check for overlaps. However, the check is not entirely correct and your example should be allowed. In other words, move from "/x/a" to /x/a" is allowed, but not from "/x" to "/x/a".

I think we should disallow "replace" with "" JSON Pointer, because the spec seems to only allow "add" with "". However, you may be right in that any Jsonvalue should be allowed. So, should "add" return a JsonValue instead of a JsonStructure? I don't like this, as it would screw up our nice API. Let me think about this more.

[glassfishrobot]

asotobu said: Yes in move operation it says what you mention but I have found the same in copy operation. For example:

{
            "op": { "op": "copy", "from": "/0", "path": "/0" },
            "node": [ true ],
            "expected": [ true, true ]
        }

it fails with same error, and in case of copy operation there is no reference in rfc about prefixing from and path elements. So for what I understand copy operation should accept prefixing from field from path.

[glassfishrobot]

kchung said: Yes, the spec does not say this is disallowed, so I'll probably remove the check for copy. I still think it does not make sense to copy from /a to /a/b, for instance, but we'll allow it. BTW, move from /a to /a is a nop.

Thinking about whether "add" should return a JsonValue instead of a JsonStructure, I noticed that RFC 6902 refers to 4627 (instead of 7159). I think this is because it was published before 7159. Until 6902 is updated for 7159, we will stick with the original intend and enforce the rule that the root of a JSON tree can only be replaced with a JsonStructure.

I'll fix the code to fix the issues you raised. Thanks.

[glassfishrobot]

kchung said: I've committed the fix. However, I cannot cause ArrayIndexOutOfBoundsException to happen. I thought NodeReference take care of it already.

[glassfishrobot]

asotobu said: Cool, I have a use case where this ArrayIndexOutOfBounds occurs. I will update the code and run the test suite again and if the problem still happens I will fix it and push the changes. I remember that the exception occurs in JsonPointer class and not in NodeReference but I will take a look.

Also I am going to implement the diff operation during next week.

Alex.

[glassfishrobot]

asotobu said: Hi I have found that this operation:

{
            "op": { "op": "replace", "path": "", "value": false },
            "target": { "x": { "a": "b", "y": {} } },
            "expected": false
}

Throws that a value cannot add to root element, only object and array. In previous comment you mention (or I understood) that this was fixed. Do you think that replace should be have the same behavior as add? If you think so I can fix it or if you want you can fix it as well.

Alex.

[glassfishrobot]

kchung said: @Alex Please read my comments (4 before this). I think value can only be JsonStructure in this case.

The spec says

This operation is functionally identical to a "remove" operation for a value, followed immediately by an "add" operation at the same location with the replacement value.

Since we disallow JSON root for remove, we should also disallow it for replace.

[glassfishrobot]

asotobu said: Sorry yes this is true I had in mind the #6 comment above that you mention that you will think about it , and don't ask me why but my brain was still thinking on this and not that you already taken a decision. Sorry

[glassfishrobot]

Issue-Links: blocks JSON_PROCESSING_SPEC-66

[glassfishrobot]

This issue was imported from java.net JIRA JSON_PROCESSING_SPEC-61

[glassfishrobot]

Marked as fixed on Tuesday, March 7th 2017, 9:05:31 am