Subject: [apps-discuss] draft-pbryan-json-patch-04 - comments

From: Vadim Zaliva <[email-masked]>
Date: Thu, 8 Mar 2012 13:46:08 -0800

Hi!

I was looking for JSON patch formats available and found this draft. It looks nice - concise and to the point. I would like to share few comments I have.

1. I am not sure if 'test' operation should be part of it. The purpose of the patch is to modify the document. 'test' operation either succeeds of fails, but it is unclear how this information will be communicated or used. It almost looks like a API operation rather than patch format instruction. I would argue that in current state it should not be included in the format. 

I could see a case when it could be included as a part of conditional syntax. In this case 'test' becomes a container for other operations which would be executed only if the test succeeds. For example:

{ "test": "/baz", "value": "qux",
	"onsuccess" : [
	 	{ "replace": "/baz", "value": "boo" },
	 	{ "add": "/baz1", "value": "qux" }
	]
}

2. In most examples "value" property holds simple scalar values (strings or integers) or arrays of such. I think it should be possible to use arbitrary complex JSON data structures as such values. I hope this was the intention. Perhaps adding an example with more complex value (object or array) could illustrate this very powerful capability. For example A.1 could be modified as following;

An example target JSON document:

   {
       "foo": "bar"
   }

   A JSON Patch document:

   [
       { "add": "/baz", "value": {"a":1,"b":2} }
   ]

   The resulting JSON document:

   {
       "baz": {"a":1,"b":2},
       "foo": "bar"
   }

Sincerely,
Vadim

---

From: "Paul C. Bryan" <[email-masked]>
Date: Thu, 08 Mar 2012 21:36:23 -0800

On Thu, 2012-03-08 at 13:46 -0800, Vadim Zaliva wrote:

> Hi!
> 
> I was looking for JSON patch formats available and found this draft.
> It looks nice - concise and to the point. I would like to share few
> comments I have.
> 
> 1. I am not sure if 'test' operation should be part of it. The purpose
> of the patch is to modify the document. 'test' operation either
> succeeds of fails, but it is unclear how this information will be
> communicated or used. It almost looks like a API operation rather than
> patch format instruction. I would argue that in current state it
> should not be included in the format. 
> 
> I could see a case when it could be included as a part of conditional
> syntax. In this case 'test' becomes a container for other operations
> which would be executed only if the test succeeds. For example:
> 
> 

I'd be interested in others' views on this.

> 2. In most examples "value" property holds simple scalar values
> (strings or integers) or arrays of such. I think it should be possible
> to use arbitrary complex JSON data structures as such values. I hope
> this was the intention.

Yes, this was the intention.

>  Perhaps adding an example with more complex value (object or array)
> could illustrate this very powerful capability. For example A.1 could
> be modified as following;
> 

Thanks, I'll aim to add this to -02. (-01 should be out tomorrow.)

Paul

---

From: Mike Acar <[email-masked]>
Date: Mon, 12 Mar 2012 13:53:41 -0700

Hi,

On 03/08/2012 09:36 PM, Paul C. Bryan wrote:
>> patch format instruction.

I'm unsure about test as well. It seems strange to me to express "change 
this document if it looks like this" in a patch; if the application 
creating the patch doesn't know how the document looks, how can it 
create a patch?

>> which would be executed only if the test succeeds.

I'm even more unsure about this. Could you nest them, for example?

> I'd be interested in others' views on this.

Paul, is there an archived discussion of the "test" operation, 
particularly regarding use cases?

-- 
Mike Acar -                                 - macar at cloudmark dot com

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Mon, 12 Mar 2012 21:13:13 +0000

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On Behalf Of Vadim Zaliva
> Sent: Thursday, March 08, 2012 1:46 PM
> To: [email-masked]
> Subject: [apps-discuss] draft-pbryan-json-patch-04 - comments
> 
> Hi!
> 
> I was looking for JSON patch formats available and found this draft. It
> looks nice - concise and to the point. I would like to share few
> comments I have.
> 
> patch format instruction. I would argue that in current state it should
> not be included in the format.

Maybe I'm too firmly rooted in UNIX patch/diff land, but I don't think I agree.  "test" is useful up-front to ensure you're about to modify what you intend to modify, and give you a chance to abort otherwise.  In this case, "test" would be used to confirm you're in a context you want to modify, much like the lines of context around a traditional UNIX patch.

I would say that "test" is not useful except before any add/modify/remove instructions.  If it fails to establish context, the whole patch aborts.

> this was the intention. Perhaps adding an example with more complex
> value (object or array) could illustrate this very powerful capability.
> For example A.1 could be modified as following;
> 

That seems a reasonable thing to consider.

-MSK

---

From: "Paul C. Bryan" <[email-masked]>
Date: Tue, 13 Mar 2012 08:18:09 -0700

> creating the patch doesn't know how the document looks, how can it 

I'm surprised that it seems so strange, as this is very much like how
text-based diff/patches work. It allows assumptions about the state of
the resource being modified to be a precondition to successfully
modifying the resource. With HTTP, it's ideal if precondition headers be
used, but this is not always feasible, nor is HTTP the only avenue for
using JSON Patch.

> particularly regarding use cases?

This was originally discussed on the JSON Patch Google Group discussion.

Paul

---

From: Mike Acar <[email-masked]>
Date: Tue, 20 Mar 2012 16:37:59 -0700

On 03/13/2012 08:18 AM, Paul C. Bryan wrote:
> text-based diff/patches work.

But in diff, the "test" is not optional. The diff (AFAIK anyway) always 
expresses "replace this with that". Patch doesn't.

So I guess I should say it's the optionality that makes it a bit weird 
to me - mixing tested and not-tested operations. When would that make sense?

I'm not yet convinced it's a problem, but I wonder about the desired 
semantics and if this is sufficient to get them. For example, I can see 
that test combined with Patch's atomicity could be useful, but it seems 
like it can't protect you from race conditions. Maybe it's sufficient 
for some use cases, though.

> It allows assumptions about the state of the resource being modified
> to be a precondition to successfully modifying the resource. With
> HTTP, it's ideal if precondition headers be used, but this is not
> always feasible, nor is HTTP the only avenue for using JSON Patch.

Mhm.

> This was originally discussed on the JSON Patch Google Group discussion.

I've read some of the archives, but not all, so I'll have another look 
through them for this discussion.

---

From: Julian Reschke <[email-masked]>
Date: Wed, 21 Mar 2012 07:42:47 +0100

On 2012-03-21 00:37, Mike Acar wrote:
> ...

What makes you think it's optional?

Best regards, Julian

---

From: Mike Acar <[email-masked]>
Date: Wed, 21 Mar 2012 10:01:57 -0700

> What makes you think it's optional?

I don't see anything in the spec that requires a test operation in a 
patch. So this conforms:

[
    { "add": "/a/b/c", "value": [ "foo", "bar" ] },
    { "remove": "/foo/bar/baz" }
]

Similarly, this conforms:

[
    { "add": "/a/b/c", "value": [ "foo", "bar" ] },
    { "test": "/a/b/c", "value": [ "foo", "bar" ] }
]

Whether those patch docs make any sense is a question of a particular 
use case.

---

From: Julian Reschke <[email-masked]>
Date: Wed, 21 Mar 2012 18:06:48 +0100

> Whether those patch docs make any sense is a question of a particular

4.6. test

    The "test" operation tests that a value at the specified location in
    the target document is equal to a specified value.  The operation
    object contains a "value" member, which specifies the value to test
    for.  If values are or contain objects or arrays, they must be
    identical (i.e. same order of elements, with the same values).

    Example:

    { "test": "/a/b/c", "value": "foo" }

    It is an error condition if the value at the specified location is
    not equal to the specified value.

Note the last paragraph.

Best regards, Julian

---

From: Mike Acar <[email-masked]>
Date: Wed, 21 Mar 2012 10:43:40 -0700

On 03/21/2012 10:06 AM, Julian Reschke wrote:
>> patch.

> not equal to the specified value.

No, I don't mean that whether a test passes is optional, I mean that a 
document isn't required to have any "test" operations. This is not like 
how text-based diff/patches work; in Patch syntax, those would look like

[
     { "test":"/a/b/c", "value":"oldval" },
     { "replace":"/a/b/c", "value":"newval" },
     { "test":"/foo/bar/baz", "value":"quux" },
     { "replace":"/foo/bar/baz", "value":"xyzzy" },
...
]

Anyway, let me read the rest of the archived Google Groups discussion 
before I comment further.

---

From: Michael Dürig <[email-masked]>
Date: Wed, 21 Mar 2012 18:24:54 +0000

AFAIR the original intention for test wasn't to introduce conditional 
operations but rather to prevent some specific race conditions. Much 
like a "test and set" operation.

Michael

---

From: "Paul C. Bryan" <[email-masked]>
Date: Wed, 21 Mar 2012 11:30:55 -0700

The intent of "test" was to ensure the partial state of a JSON resource
before applying modification(s) to it. Essentially, it's a recognition
that ETag (or equivalent entire-resource version) will be too
coarse-grained in some use cases. So in that sense, it makes the entire
patch document conditional on the tests being successfully applied; it's
definitely not intended to have some operations in a patch document be
applied and others not.

Paul

On Wed, 2012-03-21 at 18:24 +0000, Michael Dürig wrote:

> _______________________________________________
> apps-discuss mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/apps-discuss

---

From: Mike Acar <[email-masked]>
Date: Wed, 21 Mar 2012 12:04:11 -0700

> definitely not intended to have some operations in a patch document be

Right, I understand that the patch is atomic.

My concern has been that "test" alone doesn't save you from race 
conditions; you still need to lock whatever you're going to update, and 
in that case, you might as well lock, read, patch. I wondered how useful 
"test" would be without protocol support that might make it redundant.

But I wasn't thinking about optimistic concurrency control, which "test" 
does indeed let you implement (like ETags).

And if you have an application which requires a pessimistic locking 
strategy, then "test" operations in the patches would be redundant - the 
draft shouldn't require them.

So I've come to agree that "test" makes sense as it is.

---

From: Julian Reschke <[email-masked]>
Date: Wed, 21 Mar 2012 21:37:44 +0100

> in that case, you might as well lock, read, patch. I wondered how useful

You can't do that interoperably in HTTP, unless you include WebDAV 
locking into the picture.

> draft shouldn't require them.

On the other hand, they seem to be inexpensive to implement, but very 
expensive to emulate.

> So I've come to agree that "test" makes sense as it is.

+1 :-)

Best regards, Julian