Subject: [apps-discuss] JSON-Patch and the 'test' operation

From: Mark Nottingham <[email-masked]>
Date: Wed, 5 Sep 2012 17:18:39 +1000

I've just submitted revised json-pointer and json-patch documents, based upon feedback.

As far as I can tell, the only open issue on JSON-Patch is about 'test':
  http://trac.tools.ietf.org/wg/appsawg/trac/ticket/7

In Vancouver, we said that we'd wait for implementation experience to see if 'test' is worthwhile.

In the meantime, I've found two implementations of json-patch "in the wild":
  https://github.com/stefankoegl/python-json-patch
  https://github.com/bruth/jsonpatch-js

Both are somewhat old, and both support 'test'.

As such, I'd suggest we let 'test' remain in the spec (I think there are good arguments for it anyway), and ship it.

Make sense?

--
Mark Nottingham   http://www.mnot.net/

---

From: Julian Reschke <[email-masked]>
Date: Wed, 05 Sep 2012 14:18:06 +0200

> As such, I'd suggest we let 'test' remain in the spec (I think there are good arguments for it anyway), and ship it.

Absolutely.

---

From: "Paul C. Bryan" <[email-masked]>
Date: Wed, 05 Sep 2012 07:34:37 -0700

+1

On Wed, 2012-09-05 at 17:18 +1000, Mark Nottingham wrote:

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

---

From: James M Snell <[email-masked]>
Date: Wed, 5 Sep 2012 08:47:01 -0700

Three points...

1) Re: test ... I would like to see more detail on why "test" would be used
in a patch document. Perhaps just by expanding on the example just a bit.

2) Section 5's discussion of error handling should be expanded a bit. Yes
the changes are atomic but a clear example of exactly what that means would
be helpful.

Example: This json-patch would result in no changes being made to the
document at all.

[
 {"replace": "/a/b/c", "value": 42},
 {"test": "/a/b/c", "value": "C"}
]

3) Unless we specifically want to allow for such things, it should be made
clear that "test" is not intended to be used as a replacement for other
types of http conditionals. For instance, I could imagine someone doing
something like...

[
 {"test": "/foo/last-updated", "value": "2012-12-12T12:12:12Z"},
 ("add": "/a/b/c", "value": 42}
]

Or...

[
 {"test": "/@etag", "value": "ABC123"},
 ("add": "/a/b/c", "value": 42}
]

- James

On Wed, Sep 5, 2012 at 12:18 AM, Mark Nottingham <[email-masked]> wrote:

> As such, I'd suggest we let 'test' remain in the spec (I think there are
> good arguments for it anyway), and ship it.
>
> Make sense?
>
> --

---

From: Phil Hunt <[email-masked]>
Date: Wed, 5 Sep 2012 11:12:48 -0700

I was looking at this spec in the context of SCIM PATCH operations.

As SCIM is intended for provisioning across domains, it becomes possible that the full state of a resource in an external domain is not known. For example, in a replace operation, the existing value is missing, should the replace convert to an add and so forth.  These issues can be further complicated if the service provider is unable or unwilling to give detailed error responses for security reasons.

Having a "test" (or set of conditions) enables a portion of the state of a remote entity to be confirmed before the patch is executed and hopefully keeps error signalling simpler.

Phil

@independentid
www.independentid.com
[email-masked]

On 2012-09-05, at 8:47 AM, James M Snell wrote:

> Three points... 
> 
> 1) Re: test ... I would like to see more detail on why "test" would be used in a patch document. Perhaps just by expanding on the example just a bit.
> 
> 2) Section 5's discussion of error handling should be expanded a bit. Yes the changes are atomic but a clear example of exactly what that means would be helpful.
> 
> Example: This json-patch would result in no changes being made to the document at all.
> 
> [
> 3) Unless we specifically want to allow for such things, it should be made clear that "test" is not intended to be used as a replacement for other types of http conditionals. For instance, I could imagine someone doing something like...
> 
> [

---

From: James M Snell <[email-masked]>
Date: Wed, 5 Sep 2012 11:42:49 -0700

One additional point, I note that with the current "test" operation, the
"value" property is assumed to be required. What if all I want to do is
test for the presence of a property? {"test":"/a/b/c","value":null} can be
used to test that the property is null, but what if I don't care about the
specific value? can I do, {"test":"/a/b/c"} ?

- James

On Wed, Sep 5, 2012 at 8:47 AM, James M Snell <[email-masked]> wrote:

> Three points...
>
> 1) Re: test ... I would like to see more detail on why "test" would be
> used in a patch document. Perhaps just by expanding on the example just a
> bit.
>

---

From: "Paul C. Bryan" <[email-masked]>
Date: Wed, 05 Sep 2012 16:43:34 -0700

On Wed, 2012-09-05 at 08:47 -0700, James M Snell wrote:

> 3) Unless we specifically want to allow for such things, it should be
> made clear that "test" is not intended to be used as a replacement for
> other types of http conditionals.

The specification currently reads:

> The "test" operation tests that a value at the specified location in
> the target document is equal to a specified value.

I could probably come up with a half dozen ways someone could use this
operation outside of its intent. Why should we call this one out
specifically? 

Paul

---

From: James M Snell <[email-masked]>
Date: Wed, 5 Sep 2012 16:52:47 -0700

Generally because people seem to be particularly bad about handling things
like etags and last modified timestamps... e.g.
https://developers.google.com/blogger/docs/3.0/performance#patch

Perhaps it's not necessary, but I've learned not to err on the side of
common sense.

- James

On Wed, Sep 5, 2012 at 4:43 PM, Paul C. Bryan <[email-masked]> wrote:

> **
>  The "test" operation tests that a value at the specified location in the
> target document is equal to a specified value.
>
>

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 10 Sep 2012 21:59:13 +1000

That seems like a useful thing; I've tried it out here:
  http://trac.tools.ietf.org/wg/appsawg/trac/changeset/112

Cheers,

On 06/09/2012, at 4:42 AM, James M Snell <[email-masked]> wrote:

> One additional point, I note that with the current "test" operation, the "value" property is assumed to be required. What if all I want to do is test for the presence of a property? {"test":"/a/b/c","value":null} can be used to test that the property is null, but what if I don't care about the specific value? can I do, {"test":"/a/b/c"} ?
> 
> - James
>

---

From: "Paul C. Bryan" <[email-masked]>
Date: Mon, 10 Sep 2012 10:07:54 -0700

Well, the natural next question is, how do you test for the lack of a
value? Slippery slope...

Paul

---

From: James M Snell <[email-masked]>
Date: Mon, 10 Sep 2012 10:18:14 -0700

Sadly rfc4627 does not allow for the use of the undefined keyword as a
value option so the best we'd have there is the use of null..

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

Seems simple enough.

- James

On Mon, Sep 10, 2012 at 10:07 AM, Paul C. Bryan <[email-masked]> wrote:

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

---

From: "Paul C. Bryan" <[email-masked]>
Date: Mon, 10 Sep 2012 10:34:05 -0700

Unfortunately, null is a testable value and should not be interpreted to
mean lack of value.

The original intent the test operation is to test for a known partial
state of a resource before applying a partial change. We're now veering
out of that into a new territory of "handy features" without any use
case or other explicit scope. I recommend we do not adopt testing for
presence unless we can clearly bound its scope.

Paul

On Mon, 2012-09-10 at 10:18 -0700, James M Snell wrote:

>         Well, the natural next question is, how do you test for the
>         lack of a value? Slippery slope...
>         
>         Paul
>         
>         
>

---

From: James M Snell <[email-masked]>
Date: Mon, 10 Sep 2012 11:46:02 -0700

> Unfortunately, null is a testable value and should not be interpreted to
Indeed, but I'd argue that the two cases are semantically equivalent enough
for the such differences to be irrelevant. Particularly since many (if not
most) json vocabularies tend to treat null and undefined as being
equivalent. There is a clear use case for this and we can bind the scope
simply by saying that, within json patch, undefined and null are equivalent.

- James

> state of a resource before applying a partial change. We're now veering out
> of that into a new territory of "handy features" without any use case or
> other explicit scope. I recommend we do not adopt testing for presence
> unless we can clearly bound its scope.
>
> Paul
>
>

---

From: "Paul C. Bryan" <[email-masked]>
Date: Mon, 10 Sep 2012 11:59:22 -0700

On Mon, 2012-09-10 at 11:46 -0700, James M Snell wrote:

>         Unfortunately, null is a testable value and should not be
>         interpreted to mean lack of value
> 
> 
> Indeed, but I'd argue that the two cases are semantically equivalent
> enough for the such differences to be irrelevant. Particularly since
> many (if not most) json vocabularies tend to treat null and undefined
> as being equivalent.

While there are some languages that do not make this distinction, I must
disagree with your conclusion that it is irrelevant. In JavaScript,
null != undefined. The JSON specification went out of its way to define
an explicit null value. I do not want to create ambiguities in the test
operation, nor do I want to create incompatibilities with different
implementations where the treatment of null may be concerned.

>  There is a clear use case for this and we can bind the scope simply
> by saying that, within json patch, undefined and null are equivalent.

And what is the clear use case for this?

Paul

---

From: Matthew Morley <[email-masked]>
Date: Tue, 11 Sep 2012 19:59:08 -0400

As long as there is a "test" operation, people are going to want to extend
it into something that matches complex conditionals that exist in the base
languages. I'm not convinced of the value in the operation as it exists.
I'm concerned about what it would become as well, to fit people's real
needs needs.

My initial reaction was that I would use this operation to test a 'version'
field within the data at the top level. This way sequential patching could
indicate if a patch is sufficient to bring an object to a certain state.
But really in describing the patch, we are already making the assumption
that we are applying it to the right json later. Or something 'right'
enough.

Conditional matching/logic is something worthy of a spec, but I'm not
convinced this is that specification.

How are people using the test operation in practice now? I'm assuming only
non-structured matching is recommended... or is deep comparison of
structured types suggested?

Would this not be better handled outside of the work description itself?

-- 
Matt (MPCM)

On Mon, Sep 10, 2012 at 2:59 PM, Paul C. Bryan <[email-masked]> wrote:

> mean lack of value
> enough for the such differences to be irrelevant. Particularly since many
> (if not most) json vocabularies tend to treat null and undefined as being
> equivalent.
>
>
> disagree with your conclusion that it is irrelevant. In JavaScript, null !=
> undefined. The JSON specification went out of its way to define an explicit
> null value. I do not want to create ambiguities in the test operation, nor
> do I want to create incompatibilities with different implementations where
> the treatment of null may be concerned.
>
>
>  There is a clear use case for this and we can bind the scope simply by
> saying that, within json patch, undefined and null are equivalent.
>
>

---

From: James M Snell <[email-masked]>
Date: Tue, 11 Sep 2012 17:12:03 -0700

The more I look through this, the more I'm generally inclined to agree. As
was discussed in the separate thread I brought up about extending
json-patch, it is possible to define a super-set of json-patch that
introduces a range of potentially very useful predicate operations that
follow the same fundamental design as json-patch. Done properly, the two
can be layered in interesting ways. "test" can easily fit into the separate
specification, allowing json-patch to focus specifically on defining the
change operations.

- James

On Tue, Sep 11, 2012 at 4:59 PM, Matthew Morley <[email-masked]> wrote:

> My initial reaction was that I would use this operation to test a
> 'version' field within the data at the top level. This way sequential
> patching could indicate if a patch is sufficient to bring an object to a
> certain state. But really in describing the patch, we are already making
> the assumption that we are applying it to the right json later. Or
> something 'right' enough.
>
>>  Unfortunately, null is a testable value and should not be interpreted
>> to mean lack of value
>>
>>

---

From: "Paul C. Bryan" <[email-masked]>
Date: Tue, 11 Sep 2012 18:36:51 -0700

What are people's real needs you're thinking about fitting? It's been
said that there is a concrete use case for extending the functionality
of test, but I've yet to hear anything beyond the theoretical. Without
knowing these, it's all just conjecture to me.

Paul 

On Tue, 2012-09-11 at 19:59 -0400, Matthew Morley wrote:

> As long as there is a "test" operation, people are going to want to
> extend it into something that matches complex conditionals that exist
> in the base languages. I'm not convinced of the value in the operation
> as it exists. I'm concerned about what it would become as well, to fit
> people's real needs needs.
> 
> 
> patching could indicate if a patch is sufficient to bring an object to
> a certain state. But really in describing the patch, we are already
> making the assumption that we are applying it to the right json later.
> Or something 'right' enough.
> 
> How are people using the test operation in practice now? I'm assuming
> only non-structured matching is recommended... or is deep comparison
> of structured types suggested?
> 
>         >         Unfortunately, null is a testable value and should
>         >         not be interpreted to mean lack of value
>         > 
>         > 
>         > Indeed, but I'd argue that the two cases are semantically
>         > equivalent enough for the such differences to be irrelevant.
>         > Particularly since many (if not most) json vocabularies tend
>         > to treat null and undefined as being equivalent.
>         
>         
>         
>         
>         While there are some languages that do not make this
>         distinction, I must disagree with your conclusion that it is
>         irrelevant. In JavaScript, null != undefined. The JSON
>         specification went out of its way to define an explicit null
>         value. I do not want to create ambiguities in the test
>         operation, nor do I want to create incompatibilities with
>         different implementations where the treatment of null may be
>         concerned.
>         
>         
>         
>         
>         > There is a clear use case for this and we can bind the scope

---

From: Mark Nottingham <[email-masked]>
Date: Wed, 12 Sep 2012 11:57:10 +1000

Well, if someone really wanted that, we could do something like

{ "test": "/a/b/c", "present": false }

Do we have a real use case for that?

To be clear -- I do *not* want to throw the kitchen sink in here. Testing for presence seemed easy to support and implement, and personally I think there's a reasonable chance of it being used. 

From here, I think we could do any of:

a) Revert the patch to test for presence
b) Leave it as is (with test for presence
c) Expand to support test for absence, e.g., as above

Personally, my preference would be (b), although I don't have a problem with (a) if it helps us move forward. (c) seems like gilding the lily to me. 

Cheers,

On 11/09/2012, at 3:07 AM, Paul C. Bryan <[email-masked]> wrote:

> Well, the natural next question is, how do you test for the lack of a value? Slippery slope...
> 
> Paul
> 
>> On 06/09/2012, at 4:42 AM, James M Snell <
>> [email-masked]
>> > wrote:
>> 
>> > On Wed, Sep 5, 2012 at 8:47 AM, James M Snell <
>> [email-masked]
>> > wrote:
>> > Three points... 
>> > 
>> > On Wed, Sep 5, 2012 at 12:18 AM, Mark Nottingham <
>> [email-masked]
>> > wrote:

---

From: "Paul C. Bryan" <[email-masked]>
Date: Wed, 12 Sep 2012 09:10:01 -0700

I currently imagine a "container" JSON-Test document, which would
contain a number of conditional expressions as well as the patch
document to apply if such tests prove true. I would wholeheartedly
support such an initiative, especially if it resulted in keeping
JSON-Patch simple. :-)

Paul

On Tue, 2012-09-11 at 17:12 -0700, James M Snell wrote:

> The more I look through this, the more I'm generally inclined to
> agree. As was discussed in the separate thread I brought up about
> extending json-patch, it is possible to define a super-set of
> json-patch that introduces a range of potentially very useful
> predicate operations that follow the same fundamental design as
> json-patch. Done properly, the two can be layered in interesting ways.
> "test" can easily fit into the separate specification, allowing
> json-patch to focus specifically on defining the change operations. 
> 
> 
> 
> - James
> 
> 
>         As long as there is a "test" operation, people are going to
>         want to extend it into something that matches complex
>         conditionals that exist in the base languages. I'm not
>         convinced of the value in the operation as it exists. I'm
>         concerned about what it would become as well, to fit people's
>         real needs needs.
>         
>         
>         My initial reaction was that I would use this operation to
>         test a 'version' field within the data at the top level. This
>         way sequential patching could indicate if a patch
>         is sufficient to bring an object to a certain state. But
>         really in describing the patch, we are already making the
>         assumption that we are applying it to the right json later. Or
>         Conditional matching/logic is something worthy of a spec, but
>         I'm not convinced this is that specification.
>         
>         
>         How are people using the test operation in practice now? I'm
>         assuming only non-structured matching is recommended... or is
>         deep comparison of structured types suggested?
>         
>                 >         Unfortunately, null is a testable value and
>                 >         should not be interpreted to mean lack of
>                 >         value
>                 > 
>                 > 
>                 > Indeed, but I'd argue that the two cases are
>                 > semantically equivalent enough for the such
>                 > differences to be irrelevant. Particularly since
>                 > many (if not most) json vocabularies tend to treat
>                 > null and undefined as being equivalent.
>                 
>                 
>                 
>                 
>                 distinction, I must disagree with your conclusion that
>                 it is irrelevant. In JavaScript, null != undefined.
>                 The JSON specification went out of its way to define
>                 an explicit null value. I do not want to create
>                 ambiguities in the test operation, nor do I want to
>                 create incompatibilities with different
>                 > There is a clear use case for this and we can bind
>                 > the scope simply by saying that, within json patch,
>                 > undefined and null are equivalent.
>                 
>                 
>                 
>

---

From: James M Snell <[email-masked]>
Date: Wed, 12 Sep 2012 09:12:12 -0700

Precisely. I've actually already began sketching up a rough and will..
hopefully.. have the chance to prototype some code for it this weekend to
prove out the idea.

On Wed, Sep 12, 2012 at 9:10 AM, Paul C. Bryan <[email-masked]> wrote:

> **
> I currently imagine a "container" JSON-Test document, which would contain
> a number of conditional expressions as well as the patch document to apply
> if such tests prove true. I would wholeheartedly support such an
> initiative, especially if it resulted in keeping JSON-Patch simple. :-)
>
> Paul
>
>
> only non-structured matching is recommended... or is deep comparison of
>    While there are some languages that do not make this distinction, I
> must disagree with your conclusion that it is irrelevant. In JavaScript,
> null != undefined. The JSON specification went out of its way to define an
> explicit null value. I do not want to create ambiguities in the test

---

From: "Paul C. Bryan" <[email-masked]>
Date: Wed, 12 Sep 2012 09:15:41 -0700

+1!

On Wed, 2012-09-12 at 09:12 -0700, James M Snell wrote:

> hopefully.. have the chance to prototype some code for it this weekend
> to prove out the idea.
> 
> 
>         I currently imagine a "container" JSON-Test document, which
>         would contain a number of conditional expressions as well as
>         the patch document to apply if such tests prove true. I would
>         wholeheartedly support such an initiative, especially if it
>         resulted in keeping JSON-Patch simple. :-)
>         
>         Paul
>         
>         
>         
>         > The more I look through this, the more I'm generally
>         > inclined to agree. As was discussed in the separate thread I
>         > brought up about extending json-patch, it is possible to
>         > define a super-set of json-patch that introduces a range of
>         > potentially very useful predicate operations that follow the
>         > same fundamental design as json-patch. Done properly, the
>         > two can be layered in interesting ways. "test" can easily
>         > fit into the separate specification, allowing json-patch to
>         > focus specifically on defining the change operations. 
>         > 
>         > 
>         > - James
>         > 
>         >         As long as there is a "test" operation, people are
>         >         going to want to extend it into something that
>         >         matches complex conditionals that exist in the base
>         >         languages. I'm not convinced of the value in the
>         >         operation as it exists. I'm concerned about what it
>         >         would become as well, to fit people's real needs
>         >         needs. 
>         >         
>         >         My initial reaction was that I would use this
>         >         operation to test a 'version' field within the data
>         >         at the top level. This way sequential patching could
>         >         indicate if a patch is sufficient to bring an object
>         >         to a certain state. But really in describing the
>         >         patch, we are already making the assumption that we
>         >         are applying it to the right json later. Or
>         >         Conditional matching/logic is something worthy of a
>         >         spec, but I'm not convinced this is that
>         >         specification. 
>         >         
>         >         
>         >         How are people using the test operation in practice
>         >         now? I'm assuming only non-structured matching is
>         >         recommended... or is deep comparison of structured
>         >         types suggested?
>         >         
>         >                 > Indeed, but I'd argue that the two cases
>         >                 > are semantically equivalent enough for the
>         >                 > such differences to be irrelevant.
>         >                 > Particularly since many (if not most) json
>         >                 > vocabularies tend to treat null and
>         >                 > undefined as being equivalent.
>         >                 
>         >                 
>         >                 
>         >                 While there are some languages that do not
>         >                 make this distinction, I must disagree with
>         >                 your conclusion that it is irrelevant. In
>         >                 JavaScript, null != undefined. The JSON
>         >                 specification went out of its way to define
>         >                 an explicit null value. I do not want to
>         >                 create ambiguities in the test operation,
>         >                 nor do I want to create incompatibilities
>         >                 with different implementations where the
>         >                 treatment of null may be concerned. 
>         >                 
>         >                 
>         >                 
>         >                 > There is a clear use case for this and we
>         >                 > can bind the scope simply by saying that,
>         >                 > within json patch, undefined and null are
>         >                 > equivalent.
>         >                 
>         >                 
>         >

---

From: Mark Nottingham <[email-masked]>
Date: Thu, 13 Sep 2012 14:06:23 +1000

So, what does this mean for the document? Should we remove the 'test' operation (which may create further controversy), or leave it in (assuming it will be extended / supplanted if someone designs a 'wrapper')?

Regards,

On 13/09/2012, at 2:15 AM, Paul C. Bryan <[email-masked]> wrote:

>> Precisely. I've actually already began sketching up a rough and will.. hopefully.. have the chance to prototype some code for it this weekend to prove out the idea.
>> 
>> I currently imagine a "container" JSON-Test document, which would contain a number of conditional expressions as well as the patch document to apply if such tests prove true. I would wholeheartedly support such an initiative, especially if it resulted in keeping JSON-Patch simple. :-)
>> 
>> Paul
>> 
>> 
>>> The more I look through this, the more I'm generally inclined to agree. As was discussed in the separate thread I brought up about extending json-patch, it is possible to define a super-set of json-patch that introduces a range of potentially very useful predicate operations that follow the same fundamental design as json-patch. Done properly, the two can be layered in interesting ways. "test" can easily fit into the separate specification, allowing json-patch to focus specifically on defining the change operations. 
>>> 
>>> 
>>> - James
>>> 
>>> As long as there is a "test" operation, people are going to want to extend it into something that matches complex conditionals that exist in the base languages. I'm not convinced of the value in the operation as it exists. I'm concerned about what it would become as well, to fit people's real needs needs. 
>>> 
>>> My initial reaction was that I would use this operation to test a 'version' field within the data at the top level. This way sequential patching could indicate if a patch is sufficient to bring an object to a certain state. But really in describing the patch, we are already making the assumption that we are applying it to the right json later. Or something 'right' enough. 
>>> 
>>> Conditional matching/logic is something worthy of a spec, but I'm not convinced this is that specification. 
>>> 
>>> 
>>> How are people using the test operation in practice now? I'm assuming only non-structured matching is recommended... or is deep comparison of structured types suggested?
>>> 
>>>> Unfortunately, null is a testable value and should not be interpreted to mean lack of value
>>>> 
>>>> Indeed, but I'd argue that the two cases are semantically equivalent enough for the such differences to be irrelevant. Particularly since many (if not most) json vocabularies tend to treat null and undefined as being equivalent.
>>> 
>>> 
>>> While there are some languages that do not make this distinction, I must disagree with your conclusion that it is irrelevant. In JavaScript, null != undefined. The JSON specification went out of its way to define an explicit null value. I do not want to create ambiguities in the test operation, nor do I want to create incompatibilities with different implementations where the treatment of null may be concerned. 
>>> 
>>> 
>>>> There is a clear use case for this and we can bind the scope simply by saying that, within json patch, undefined and null are equivalent.
>>> 
>>>

---

From: Matthew Morley <[email-masked]>
Date: Thu, 13 Sep 2012 08:10:58 -0400

I am not using json patch with any real data/situations so far, so it is
still conjecture from my part, but based with situations I encountered in
real systems in the past. I struggled with a reasonable approach to
conditionals in my rule engine, and adopted a functional but incomplete
mechanism.

With these discussions, I'll forward something over off-list to the people
of this thread, which might explain some of my views. We can bring it on
list, if you feel it won't distract the conversation/topic. I'm happy to
leave test as is, I just see it as a weak point in the specification.

But currently the test operation is limited in that it can only:

   - test that a path exists
   - AND that it resolves to a single known value
   - AND that the value is known at the time of the patch creation

You can combine multiple test operations within a patch, to create
something like a logical (A=1 AND B=2 AND c=3). You can create multiple
patches with unique tests to simulate something like (A=1 OR B=2 OR C=3).

If you were to test for existence, something like { "test":/foo/bar"} seems
to be the clearest to me. Also, currently you cannot perform tests where
the data in the json document itself is the point of consideration ( {
"test":"/foo/bar", match:"/foo/baz"}. This gets closer to the approach I
ended up using, more about paths as relationships within the data.

JSON schema does a good job of describing the shape of objects, and there
are times when that might be a consideration. Simple shapes can be
represented in complex (AND|OR) tests.

Most of the situations I am envisioning, which require more complex test
operations, are were a patch is generated, but it needs to be selectively
applied across a large set of json documents. If you know specifically
which documents you need to apply patches to, you can use the test
operation pretty well. If you can generate multiple specific patches that
also works well. But it is the compound, negative, and relationship tests
that are the issues.

This may not be how you envision json patch being utilized. But the test
operation does not seem to fit into the concept of "a JSON document
structure for expressing a sequence of operations to apply to a JSON
document". test is much more of a " and when to apply them" concept.

'When to apply them' will require a more complex offering, in my opinion. I
don't know currently of a specification that handles this within JSON realm.

On Tue, Sep 11, 2012 at 9:36 PM, Paul C. Bryan <[email-masked]> wrote:

> **
> What are people's real needs you're thinking about fitting? It's been said
> that there is a concrete use case for extending the functionality of test,
> but I've yet to hear anything beyond the theoretical. Without knowing
> these, it's all just conjecture to me.
>
> Paul
>

-- 
Matthew P. C. Morley

---

From: Matthew Morley <[email-masked]>
Date: Thu, 13 Sep 2012 08:15:44 -0400

I'll get some notes together from my prior setup and send them over before
the weekend. Might offer some thoughts, or confirm your approaches, or save
you from some of the stuff I never found clean solutions for.

Parts of my brain are still stuck down some of those rabbit holes... I'm
excited to see this actively being discussed.

On Wed, Sep 12, 2012 at 12:12 PM, James M Snell <[email-masked]> wrote:

>> The more I look through this, the more I'm generally inclined to agree.
>> As was discussed in the separate thread I brought up about extending
>> extend it into something that matches complex conditionals that exist in
>> the base languages. I'm not convinced of the value in the operation as it
>> exists. I'm concerned about what it would become as well, to fit people's
>> real needs needs.
>>
>>

-- 
Matthew P. C. Morley

---

From: Matthew Morley <[email-masked]>
Date: Thu, 13 Sep 2012 08:21:25 -0400

Okay... I was thinking of this specification as being more free standing.
But given "[...] suitable for use with the HTTP PATCH method." this will
probably play against more of my thoughts than I was considering.

On Thu, Sep 13, 2012 at 8:10 AM, Matthew Morley <[email-masked]> wrote:

> If you were to test for existence, something like { "test":/foo/bar"}
> seems to be the clearest to me. Also, currently you cannot perform tests
> where the data in the json document itself is the point of consideration (
> { "test":"/foo/bar", match:"/foo/baz"}. This gets closer to the approach I
> 'When to apply them' will require a more complex offering, in my opinion.
> I don't know currently of a specification that handles this within JSON
> realm.
>
>
>
>> said that there is a concrete use case for extending the functionality of
>> test, but I've yet to hear anything beyond the theoretical. Without knowing

-- 
Matthew P. C. Morley

---

From: "Paul C. Bryan" <[email-masked]>
Date: Thu, 13 Sep 2012 09:30:16 -0700

I could be persuaded to support removing test.

Paul

---

From: Mark Nottingham <[email-masked]>
Date: Sat, 15 Sep 2012 11:11:14 +1000

Does anyone object to removing 'test' from json-patch?

Regards,

On 14/09/2012, at 2:30 AM, Paul C. Bryan <[email-masked]> wrote:

>> On 13/09/2012, at 2:15 AM, Paul C. Bryan <
>> [email-masked]
>> > wrote:
>> 
>> > +1!
>> > 
>> >> On Wed, Sep 12, 2012 at 9:10 AM, Paul C. Bryan <
>> [email-masked]
>> > wrote:
>> >>> On Tue, Sep 11, 2012 at 4:59 PM, Matthew Morley <
>> [email-masked]
>> > wrote:
>> >>> On Mon, Sep 10, 2012 at 2:59 PM, Paul C. Bryan <
>> [email-masked]
>> > wrote:
>> >>> 
>> >>>> On Mon, Sep 10, 2012 at 10:34 AM, Paul C. Bryan <
>> [email-masked]
>> > wrote:

---

From: Mike McCabe <[email-masked]>
Date: Sat, 15 Sep 2012 04:37:55 -0700

> Does anyone object to removing 'test' from json-patch?

I'm working on a production system for the Internet Archive that uses 
json-patch to express metadata modifications.

So far, it's been a great fit - and a very timely discovery.  (I was 
about to roll my own.)  Thank you!

For some uses, we need protection against concurrent writes.  I was 
planning on using the 'test' operation for this (with a 'version' field) 
as 'optimistic' concurrency control.  (Locking isn't a good fit, as we 
want to support writes from outside organizations.)

I'd prefer that we not throw out 'test' without a good idea of what 
might replace it, as there's no flexible, 'in-band' way to get 
optimistic concurrency without it.

I'll try to answer a few of the objections to 'test' that I've seen:

* It's unnecessary, as the other patch operations assume knowledge about 
the document.  (That is, errors in patch application will catch 
unexpected concurrent changes.)

-> While other operations can also fail if the document has changed such 
that they can't apply, this won't catch changes that might have occurred 
elsewhere in the document (that could be semantically important)

(Also, most array operations will succeed even if the array has changed.)

* It's unneeded given conditional HTTP, eTag or other mechanisms.

-> json-patch is not HTTP PATCH, so these approaches may not be available.

They're 'out-of-band' - part of the information needed to apply the 
patch is outside of the patch (and not a JSON document)

They're less flexible; it's easy to imagine a larger JSON document that 
had sub-documents, where only the sub-documents required atomic changes; 
each could get it's own version field if needed.

Sometimes concurrency control isn't important, or the only requirement 
is that we don't overwrite if someone else got there first.

* It's different from the other operations.

-> Other operations can abort the patch, just as test can; it's easily 
implemented as an operation that happens to not change anything.

* It's an incomplete part of an otherwise-useful generic comparison 
facility.

-> For help with concurrency, 'test' *doesn't* need to be a generic 
comparison facility.  If I needed to do that, it would be more flexible 
to pull document snippets with json-pointer, then do comparisons myself. 
  Testing value equality is enough; testing existence might be useful 
for some cases.

On Thu, Sep 13, 2012 at 8:10 AM, Matthew Morley <[email-masked]> wrote:
> Most of the situations I am envisioning, which require more complex
 > test operations, are were a patch is generated, but it needs to be
 > selectively applied across a large set of json documents. If you know
 > specifically which documents you need to apply patches to, you can use
 > the test operation pretty well. If you can generate multiple specific
> patches that also works well. But it is the compound, negative, and
 > relationship tests that are the issues.

I do see how a more general facility would be useful in this case.  (Is 
such a facility useful outside of json-patch?)

Can we get a proposal before removing 'test'?

My implementation of json-patch is here:

https://github.com/mikemccabe/json-patch-php

A JSON test suite for json-patch is here:

https://github.com/mikemccabe/json-patch-tests

Regards,
Mike

---

From: "Paul C. Bryan" <[email-masked]>
Date: Sun, 16 Sep 2012 08:42:07 -0700

Alright, I propose we keep the test operation as documented in
draft-ietf-appsawg-json-patch-03.

Paul

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 17 Sep 2012 18:10:08 +1200

Personally -- I'm +1 on that; it has a certain amount of utility, and while it doesn't do everything, I think it's good enough.

On 17/09/2012, at 3:42 AM, Paul C. Bryan <[email-masked]> wrote:

> Alright, I propose we keep the test operation as documented in draft-ietf-appsawg-json-patch-03.
> 
> Paul
> 
>> On Thu, Sep 13, 2012 at 8:10 AM, Matthew Morley <
>> [email-masked]
>> > wrote:
>> > On 14/09/2012, at 2:30 AM, Paul C. Bryan <
>> [email-masked]
>> > wrote:
>> >
>> >>> Mark Nottingham
>> http://www.mnot.net/

--
Mark Nottingham
http://www.mnot.net/

---

From: "Manger, James H" <[email-masked]>
Date: Mon, 17 Sep 2012 17:32:51 +1000

The JSON patch spec looks good: draft-ietf-appsawg-json-patch-04.

There is no explicit mention of extensibility of the JSON patch syntax. Presumably new operations (beyond add/remove/replace/move/copy/test) could be added, with the understanding that "old" implementations will reject such documents [section 4 says an unknown operation is an error]. Adding extra fields to an existing operation is less clear (eg {"add":"/foo/1","value":"baz","repeat":10}). There is no explicit mention that that would be an error. Perhaps implementations will ignore unrecognized fields.

Could more sophisticated future test rules define, say, a "test2" operation? Or is it the expectation that such a syntax would need a new media type?

Each operation lists a full JSON pointer from the root of the resource. If you want to replace a dozen fields in a deeply nested object you need to repeat the path to that object a dozen times. This may be a reasonable compromise to avoid a more complex patch structure (particularly if compression removes much of the overhead), but I wasn't sure if any alternative had been considered. For example, {"patch":"/fee/fie/fo/fum/foo","value":[{"add":"x","value":"baz"},{"add":"y","value":3}]}.

Would I be opening a can of worms (or doing some bike-shedding) if I asked why application/json-patch instead of application/patch+json?

--
James Manger

---

From: "Paul C. Bryan" <[email-masked]>
Date: Mon, 17 Sep 2012 12:33:16 -0700

On Mon, 2012-09-17 at 17:32 +1000, Manger, James H wrote:

> There is no explicit mention of extensibility of the JSON patch
> syntax. Presumably new operations (beyond
> add/remove/replace/move/copy/test) could be added, with the
> understanding that "old" implementations will reject such documents
> [section 4 says an unknown operation is an error]. Adding extra fields
> to an existing operation is less clear (eg
> {"add":"/foo/1","value":"baz","repeat":10}). There is no explicit
> mention that that would be an error. Perhaps implementations will
> ignore unrecognized fields.

I had envisioned a container document type, which would enumerate a set
of conditions that must be met before processing the operations an
associated (contained) JSON Patch document. Taking this approach means
that there is no confusion over what's a JSON Patch document vs. the
extension document type. Since JSON Patch is an array of operation
objects, such a structure would remain relatively straightforward.

More concretely, perhaps something like:

{
    "tests": [
        { "less-than": "/some/path", "value": 42 },
        { "exists": "/some/other/path" },
        { "not-exists": "/yet/another/path" },
        { "contains": "/foo/bar", "value": "Major-General" }
    ], "application/json-patch": [
        { "replace": "/some/path", "value": 42 },
        { "delete": "/some/other/path" },
        { "add": "/yet/another/path", "value": "bar!" }
    ]
}

Thoughts?

> Could more sophisticated future test rules define, say, a "test2"
> operation? Or is it the expectation that such a syntax would need a
> new media type?

I would say any extension should require its own media type.

> Each operation lists a full JSON pointer from the root of the
> resource. If you want to replace a dozen fields in a deeply nested
> object you need to repeat the path to that object a dozen times. This
> may be a reasonable compromise to avoid a more complex patch structure
> (particularly if compression removes much of the overhead), but I
> wasn't sure if any alternative had been considered. For example,
> {"patch":"/fee/fie/fo/fum/foo","value":[{"add":"x","value":"baz"},{"add":"y","value":3}]}.

We've considered alternatives in the past, but concluded that given the
verbosity of the JSON Patch document, the likely strategy to reduce size
would be through compression. As such, multiple similar paths would
compress fairly well.

> Would I be opening a can of worms (or doing some bike-shedding) if I
> asked why application/json-patch instead of application/patch+json?

Probably. I originally proposed application/patch+json. The main problem
with application/patch+json is that it implies that there are other
representations of application/patch, not just JSON (e.g.
application/patch+xml). In this case, JSON Patch is meant to address
only JSON documents in a JSON format. Furthermore, I'm not yet aware of
a specification for JSON like there is is for XML that promotes the
addition of +json. Therefore, I'd say that application/json-patch is
probably the better choice.

Paul

---

From: Matthew Morley <[email-masked]>
Date: Tue, 18 Sep 2012 07:42:21 -0400

+1
Better to leave it in place if is is being used.

On Mon, Sep 17, 2012 at 2:10 AM, Mark Nottingham <[email-masked]> wrote:

> Personally -- I'm +1 on that; it has a certain amount of utility, and
> while it doesn't do everything, I think it's good enough.
>
>
> >> >>> So, what does this mean for the document? Should we remove the
> 'test' operation (which may create further controversy), or leave it in
> (assuming it will be extended / supplanted if someone designs a 'wrapper')?
> >> >>>
> >> >>> Regards,
> >> >>>
> >> >>>
> >> >>>>> Precisely. I've actually already began sketching up a rough and
> will.. hopefully.. have the chance to prototype some code for it this
> weekend to prove out the idea.
> >> >>>>>
> contain a number of conditional expressions as well as the patch document
> to apply if such tests prove true. I would wholeheartedly support such an
> agree. As was discussed in the separate thread I brought up about extending
> >> >>>>>> As long as there is a "test" operation, people are going to want
> to extend it into something that matches complex conditionals that exist in
> >> >>>>>> My initial reaction was that I would use this operation to test
> a 'version' field within the data at the top level. This way sequential
> assuming only non-structured matching is recommended... or is deep
> comparison of structured types suggested?
> >> >>>>>>
> equivalent enough for the such differences to be irrelevant. Particularly
> since many (if not most) json vocabularies tend to treat null and undefined
> as being equivalent.
> >> >>>>>>
> >> >>>>>>
> distinction, I must disagree with your conclusion that it is irrelevant. In
> JavaScript, null != undefined. The JSON specification went out of its way
> to define an explicit null value. I do not want to create ambiguities in
> the test operation, nor do I want to create incompatibilities with
> different implementations where the treatment of null may be concerned.
> >> >>>>>>
> >> >>>>>>

-- 
Matthew P. C. Morley

---

From: Mark Nottingham <[email-masked]>
Date: Tue, 18 Sep 2012 10:27:02 -0700

I agree with the approach that Paul advocates here. 

James is right that the document is currently silent about extra properties on the operation. Do people have strong feelings about whether this should result in an error condition, or be ignored?

On 17/09/2012, at 12:33 PM, Paul C. Bryan <[email-masked]> wrote:

> I had envisioned a container document type, which would enumerate a set of conditions that must be met before processing the operations an associated (contained) JSON Patch document. Taking this approach means that there is no confusion over what's a JSON Patch document vs. the extension document type. Since JSON Patch is an array of operation objects, such a structure would remain relatively straightforward.
> 
> We've considered alternatives in the past, but concluded that given the verbosity of the JSON Patch document, the likely strategy to reduce size would be through compression. As such, multiple similar paths would compress fairly well.
> 
> Probably. I originally proposed application/patch+json. The main problem with application/patch+json is that it implies that there are other representations of application/patch, not just JSON (e.g. application/patch+xml). In this case, JSON Patch is meant to address only JSON documents in a JSON format. Furthermore, I'm not yet aware of a specification for JSON like there is is for XML that promotes the addition of +json. Therefore, I'd say that application/json-patch is probably the better choice.
> 
> Paul

--
Mark Nottingham
http://www.mnot.net/

---

From: "Paul C. Bryan" <[email-masked]>
Date: Tue, 18 Sep 2012 10:42:11 -0700

On Tue, 2012-09-18 at 10:27 -0700, Mark Nottingham wrote:

> James is right that the document is currently silent about extra
> properties on the operation. Do people have strong feelings about
> whether this should result in an error condition, or be ignored?

It was definitely intended to be an error condition. On this point, the
specification reads:

    "It is an error condition if an operation object contains no
recognized operation member or more than one operation member."

I suppose it would be stronger to say something like:

    "It is an error condition if an operation object does not contain an
operation member documented herein, or more than one operation member."

Paul

---

From: James M Snell <[email-masked]>
Date: Tue, 18 Sep 2012 10:49:09 -0700

+1 on having it be an error condition.

Because of the potentially significant negative impact that an improperly
applied patch operation could have on the target resource, any
not-understood operation, object or member MUST result in an error
condition being reported and the patch rejected.

- James

On Tue, Sep 18, 2012 at 10:42 AM, Paul C. Bryan <[email-masked]> wrote:

> properties on the operation. Do people have strong feelings about whether
> this should result in an error condition, or be ignored?
>
>

---

From: Mark Nottingham <[email-masked]>
Date: Tue, 18 Sep 2012 16:55:55 -0700

Going with:

>    It is an error condition if an operation object does not contain an
>    operation member, contains an operation member other than one of
>    those defined by this document, or more than one operation member.
> 
>    Likewise, it is an error condition if an operation object has a
>    member that is not explicitly allowed by this document.

On 18/09/2012, at 10:49 AM, James M Snell <[email-masked]> wrote:

> Because of the potentially significant negative impact that an improperly applied patch operation could have on the target resource, any not-understood operation, object or member MUST result in an error condition being reported and the patch rejected. 
> 
> - James
> 
> It was definitely intended to be an error condition. On this point, the specification reads:
> 
>     "It is an error condition if an operation object contains no recognized operation member or more than one operation member."
> 
>     "It is an error condition if an operation object does not contain an operation member documented herein, or more than one operation member."
> 
> Paul
> 

--
Mark Nottingham
http://www.mnot.net/

---

From: "Manger, James H" <[email-masked]>
Date: Thu, 20 Sep 2012 00:52:38 +1000

The consensus sounds like "MUST understand everything"; no ignoring unrecognized fields. That is ok, but we haven't picked a good syntax to match that rule.

The "obvious" way to parse a patch operation object (eg {"add":"/foo","value":"bar"}) is to:

1. Loop through the keys in the object until you find one that is in the set of known operations: "add", "remove", "test"…

2. Extract the elements associated with that specific operation, eg the "value" for an "add" operation.

That "obvious" process will not detect two operations in an object, or unexpected extra fields. Implementations will have to do non-trivial extra work to make those checks. Many will not do that, or will do it incorrectly, simply because valid cases will work regardless of that extra code. The result is likely to be security problems. For instance, one implementation parsing {"value":"bar", "add":"/foo", "remove":"/foo"} will see an "add" operation, another will see a "remove" operation (and a third will report an error), simply based on how implementation-specific string hash codes are calculated.

The nice thing about a JSON object is that you can add all sorts of extra elements without affecting existing elements. Given we explicitly don't want this feature, perhaps a better syntax would be a JSON array: starting with the operation name, then the operation-specific parameters.

For example:
instead of  {"add":"/foo", "value":"bar"}
have        ["add", "/foo", "bar"]

Duplicate operations cannot occur (there can only be one first element in an array). Preventing unexpected parameters is as simple as ensuring the array is the right length for the specific operation.

The array approach should also be simpler to map to a per-operation function call with arguments.

--
James Manger

> -----Original Message-----
> From: [email-masked] [mailto:apps-discuss-
> [email-masked]] On Behalf Of Mark Nottingham
> Sent: Wednesday, 19 September 2012 9:56 AM
> To: James M Snell
> Cc: [email-masked]
> Subject: Re: [apps-discuss] JSON Patch: extensibility
> 
> Going with:
> 
> > Because of the potentially significant negative impact that an
> improperly applied patch operation could have on the target resource,
> any not-understood operation, object or member MUST result in an error
> >     "It is an error condition if an operation object does not contain
> an operation member documented herein, or more than one operation
> member."
> >
> > Paul

---

From: James M Snell <[email-masked]>
Date: Wed, 19 Sep 2012 08:09:20 -0700

An array based approach carries along many of it's own issues, especially
if we do decide to extend things later on. What likely would have been
ideal (albeit more verbose) are distinct "op" and "ref" codes to identify
the patch operation and json-pointer -- e.g. {"op":"add", "ref":"/a/b/c",
"value": "123"}. This helps us to enforce the single operation rule,
simplifies processing and maintains better long term extensibility at the
cost of a few extra bits on the wire.

- James

On Wed, Sep 19, 2012 at 7:52 AM, Manger, James H <
[email-masked]> wrote:

> The consensus sounds like "MUST understand everything"; no ignoring
> unrecognized fields. That is ok, but we haven't picked a good syntax to
> match that rule.
>
> 1. Loop through the keys in the object until you find one that is in the
> set of known operations: "add", "remove", "test"…
>
> That "obvious" process will not detect two operations in an object, or
> unexpected extra fields. Implementations will have to do non-trivial extra
> work to make those checks. Many will not do that, or will do it
> incorrectly, simply because valid cases will work regardless of that extra
> code. The result is likely to be security problems. For instance, one
> implementation parsing {"value":"bar", "add":"/foo", "remove":"/foo"} will
> see an "add" operation, another will see a "remove" operation (and a third
> will report an error), simply based on how implementation-specific string
> hash codes are calculated.
>
> The nice thing about a JSON object is that you can add all sorts of extra
> elements without affecting existing elements. Given we explicitly don't
> want this feature, perhaps a better syntax would be a JSON array: starting
> with the operation name, then the operation-specific parameters.
>
> For example:
> Duplicate operations cannot occur (there can only be one first element in
> an array). Preventing unexpected parameters is as simple as ensuring the
> array is the right length for the specific operation.
>

---

From: "Paul C. Bryan" <[email-masked]>
Date: Wed, 19 Sep 2012 11:03:36 -0700

Check out the op/path/value notation in
http://tools.ietf.org/html/draft-pbryan-json-patch-00 and note its
similarity to what you're proposing. Consensus was that separate op/path
was too verbose, and in response was collapsed into operator becoming
member name.

I suggest that the MUST definitely applies to the client submitting the
patch document. I don't think that the server MUST ensure that the
client conformed to the spec. If the client constructs something like:
{ "add": "/some/path", "remove": "/some/other/path", "value": "some
value" }, then the client is clearly violating the spec, the the results
will be unpredictable on the server. If you want your server to be
paranoid, then it should account for all members in the operation object
and reject non-conformance, but I don't expect every server to do so.

Paul

On Wed, 2012-09-19 at 08:09 -0700, James M Snell wrote:

> An array based approach carries along many of it's own issues,
> especially if we do decide to extend things later on. What likely
> would have been ideal (albeit more verbose) are distinct "op" and
> "ref" codes to identify the patch operation and json-pointer -- e.g.
> {"op":"add", "ref":"/a/b/c", "value": "123"}. This helps us to enforce
> the single operation rule, simplifies processing and maintains better
> long term extensibility at the cost of a few extra bits on the wire. 
> 
> 
> 
> - James
> 
> 
> On Wed, Sep 19, 2012 at 7:52 AM, Manger, James H
> <[email-masked]> wrote:
> 
>         The consensus sounds like "MUST understand everything"; no
>         ignoring unrecognized fields. That is ok, but we haven't
>         picked a good syntax to match that rule.
>         
>         1. Loop through the keys in the object until you find one that
>         is in the set of known operations: "add", "remove", "test"…
>         
>         2. Extract the elements associated with that specific
>         operation, eg the "value" for an "add" operation.
>         
>         That "obvious" process will not detect two operations in an
>         object, or unexpected extra fields. Implementations will have
>         to do non-trivial extra work to make those checks. Many will
>         not do that, or will do it incorrectly, simply because valid
>         cases will work regardless of that extra code. The result is
>         likely to be security problems. For instance, one
>         implementation parsing {"value":"bar", "add":"/foo",
>         "remove":"/foo"} will see an "add" operation, another will see
>         a "remove" operation (and a third will report an error),
>         simply based on how implementation-specific string hash codes
>         are calculated.
>         
>         The nice thing about a JSON object is that you can add all
>         sorts of extra elements without affecting existing elements.
>         Given we explicitly don't want this feature, perhaps a better
>         syntax would be a JSON array: starting with the operation
>         name, then the operation-specific parameters.
>         
>         For example:
>         Duplicate operations cannot occur (there can only be one first
>         element in an array). Preventing unexpected parameters is as
>         simple as ensuring the array is the right length for the
>         specific operation.
>         
>         The array approach should also be simpler to map to a
>         per-operation function call with arguments.
>         
>         --
>         James Manger
>         
>         
>         
>         > > On Tue, Sep 18, 2012 at 10:42 AM, Paul C. Bryan
>         <[email-masked]>
>         > wrote:
>         > > It was definitely intended to be an error condition. On
>         this point,
>         > the specification reads:
>         > >

---

From: James M Snell <[email-masked]>
Date: Wed, 19 Sep 2012 11:21:24 -0700

Yes, it is a clear violation of the spec but the server needs to be able to
defend the resource against potentially ambiguous change operations. Also,
even if the MUST applies to the client creating and submitting the patch,
the only place we can be sure the MUST level requirement is being properly
implemented is on the server where the patch document is being consumed and
processed.

Also consider that there is an outside chance of a security risk posed by
malicious clients. For example, consider the case where a malicious client
sends a patch operation:

  {"add":"a/b/harmless","replace":"/a/b/harmful","value":"some_value"}.

Now consider a server implementation composed of multiple tiered processes.
The first tier is responsible for basic validation of the request. Through
experimentation, the malicious client is able to determine that this first
tier processes the payload in such a way that it sees only the initial
"add" operation and path. Based on the business rules for validation, the
server determines that adding the "some_value" to location "a/b/harmless"
is ok and the validation passes. However, the client discovers that the
lower tiers in the system calculates hashes differently, causing it to see
the "replace" operation before the "add". Assuming the "obvious" processing
model discussed previously, this lower processing tier, assuming the
operation has already passed inspection by the validation tier, applies the
potentially harmful "replace" operation.

Yes, this is rather contrived example but, as we have seen in many real
world examples, malicious parties love to take advantage of various spec
ambiguities and differences in the way various application components come
together to process data. Use of a single op member mitigates this
particular issue by ensuring that that validation and processing tiers are
always looking at the same piece of data. It's never left up to chance
based on the arbitrary ordering of members in a parsed JSON object.

- James

On Wed, Sep 19, 2012 at 11:03 AM, Paul C. Bryan <[email-masked]> wrote:

> patch document. I *don't* think that the server MUST ensure that the
> client conformed to the spec. If the client constructs something like: {
> "add": "/some/path", "remove": "/some/other/path", "value": "some value" },
> then the client is *clearly* violating the spec, the the results will be
> unpredictable on the server. If you want your server to be paranoid, then
> it should account for all members in the operation object and reject
> non-conformance, but I don't expect every server to do so.
>
> Paul
>
>

---

From: Francis Galiegue <[email-masked]>
Date: Wed, 19 Sep 2012 20:26:28 +0200

On Wed, Sep 19, 2012 at 8:21 PM, James M Snell <[email-masked]> wrote:
> experimentation, the malicious client is able to determine [...]

Sorry to jump into the discussion like this, but...

Isn't JSON Patch supposed to be a way of _processing_ JSON values?
What gains, if any, would there be for JSON Patch proper to
acknowledge security concerns?

/me runs away,
-- 
Francis Galiegue, [email-masked]
JSON Schema: https://github.com/json-schema
"It seems obvious [...] that at least some 'business intelligence'
tools invest so much intelligence on the business side that they have
nothing left for generating SQL queries" (Stéphane Faroult, in "The
Art of SQL", ISBN 0-596-00894-5)

---

From: James M Snell <[email-masked]>
Date: Wed, 19 Sep 2012 11:37:12 -0700

By it's own description, JSON Patch is "a ... document structure for
expressing a sequence of operations to apply to a JSON document." As such,
if that document structure allows for potentially ambiguous conditions to
arise in the identification and processing of those operations, and such
ambiguities can potentially have security ramifications, then those
security concerns must absolutely be discussed. Simply stating that there
MUST only be one operation member in the object is not quite enough. It is
also critical to describe what could happen if the requirement is not
enforced.

It is also important to deal with the fact that the ambiguity could be
eliminated entirely by choosing an alternative, slightly more verbose
syntax for the expression of the operation and pointer. I get that a
decision was made previously to go with the less verbose option; but what's
not yet clear is whether that was the best option to take. Personally, when
it comes to describing change operations, I'd prefer precise and verbose
over ambiguous.

- James

---

From: "Paul C. Bryan" <[email-masked]>
Date: Wed, 19 Sep 2012 11:52:36 -0700

On Wed, 2012-09-19 at 11:21 -0700, James M Snell wrote:

> Yes, it is a clear violation of the spec but the server needs to be
> able to defend the resource against potentially ambiguous change
> operations. Also, even if the MUST applies to the client creating and
> submitting the patch, the only place we can be sure the MUST level
> requirement is being properly implemented is on the server where the
> patch document is being consumed and processed. 

Some server implementations may elect to reject ambiguous change
operations. It is an error condition; evaluation of the JSON patch
document SHOULD terminate. 

> Also consider that there is an outside chance of a security risk posed
> by malicious clients. For example, consider the case where a malicious
> client sends a patch operation: 
> 
> Now consider a server implementation composed of multiple tiered
> processes. The first tier is responsible for basic validation of the
> request. Through experimentation, the malicious client is able to
> determine that this first tier processes the payload in such a way
> that it sees only the initial "add" operation and path. Based on the
> business rules for validation, the server determines that adding the
> "some_value" to location "a/b/harmless" is ok and the validation
> passes. However, the client discovers that the lower tiers in the
> system calculates hashes differently, causing it to see the "replace"
> operation before the "add". Assuming the "obvious" processing model
> discussed previously, this lower processing tier, assuming the
> operation has already passed inspection by the validation tier,
> applies the potentially harmful "replace" operation.

It is an error condition, and processing SHOULD terminate. In this
particular case, it seems particularly important that it does terminate.
And this all implies that the client must not create such constructs if
it expects it to be processed successfully. 

> Yes, this is rather contrived example but, as we have seen in many
> real world examples, malicious parties love to take advantage of
> various spec ambiguities and differences in the way various
> application components come together to process data. Use of a single
> op member mitigates this particular issue by ensuring that that
> validation and processing tiers are always looking at the same piece
> of data. It's never left up to chance based on the arbitrary ordering
> of members in a parsed JSON object.

I agree that an "op" member solves this, at the expense of verbosity
that was dismissed at the time. And when I considered it, I didn't see
as much complexity in validation: if I find and "add" member, I check
for a "value" member and confirm that there are only 2 members in the
object. If I find a "remove" member, I confirm there is only one member
in the object.

Paul

---

From: James M Snell <[email-masked]>
Date: Wed, 19 Sep 2012 12:13:33 -0700

On Wed, Sep 19, 2012 at 11:52 AM, Paul C. Bryan <[email-masked]> wrote:

> Yes, it is a clear violation of the spec but the server needs to be able
> to defend the resource against potentially ambiguous change operations.
> Also, even if the MUST applies to the client creating and submitting the
> patch, the only place we can be sure the MUST level requirement is being
> properly implemented is on the server where the patch document is being
> consumed and processed.
>
>
> *Some* server implementations may elect to reject ambiguous change
> operations. It *is* an error condition; evaluation of the JSON patch
> document *SHOULD* terminate.
>

The point here is that server implementations ought to be *required* to
reject ambiguous change operations... not only "Some". It MUST NOT be left
up to implementations to choose on this particular point. I can see no
justifiable reason for this to be a SHOULD rather than a MUST.

> request. Through experimentation, the malicious client is able to determine
> that this first tier processes the payload in such a way that it sees only
> the initial "add" operation and path. Based on the business rules for
> validation, the server determines that adding the "some_value" to location
> "a/b/harmless" is ok and the validation passes. However, the client
> discovers that the lower tiers in the system calculates hashes differently,
> causing it to see the "replace" operation before the "add". Assuming the
> "obvious" processing model discussed previously, this lower processing
> tier, assuming the operation has already passed inspection by the
> validation tier, applies the potentially harmful "replace" operation.
>
>
> It *is* an error condition, and processing *SHOULD* terminate. In this
> And this all implies that the client must not create such constructs if it
> expects it to be processed successfully.
>
>
Agreed. The more important point, however, is that servers are required to
validate the change operation to ensure that the client is doing it's job
correctly, and current syntax makes such validation more difficult than it
really ought to be, especially in the long term if we decide to extend the
possible range of operations.

When using something like "op":"replace" as an alternative, however, this
additional validation check becomes entirely unnecessary and the
possibility of ambiguity dissolves completely. For me, this is much more
advantageous than saving a few bytes on the wire.

- James

> I agree that an "op" member solves this, at the expense of verbosity that
> was dismissed at the time. And when I considered it, I didn't see as much
> complexity in validation: if I find and "add" member, I check for a "value"
> member and confirm that there are only 2 members in the object. If I find a
> "remove" member, I confirm there is only one member in the object.
>
Paul

---

From: "Paul C. Bryan" <[email-masked]>
Date: Wed, 19 Sep 2012 16:42:03 -0700

On Wed, 2012-09-19 at 12:13 -0700, James M Snell wrote:

[snip]

> The point here is that server implementations ought to be *required*
> to reject ambiguous change operations... not only "Some". It MUST NOT
> be left up to implementations to choose on this particular point. I
> can see no justifiable reason for this to be a SHOULD rather than a
> MUST.

So, I interpret your recommendation to be to change the text in §5 to:

    If an error condition occurs, evaluation of the JSON Patch document
MUST terminate and application of the entire patch document SHALL NOT be
deemed successful.

Frankly, I don't have a problem with this. Any objections from others in
the working group? If not, I'll support it.

[snip]

> When using something like "op":"replace" as an alternative, however,
> this additional validation check becomes entirely unnecessary and the
> possibility of ambiguity dissolves completely. For me, this is much
> more advantageous than saving a few bytes on the wire.

Well, there was significant feedback against separate operation and path
members at the time. Personally, I'm ambivalent on this; I like both
approaches for different reasons, though the current method wins out by
a small margin. That said, if strong consensus to change to bifurcating
to separate op/path were to be established, I wouldn't resist.

Paul

---

From: "Manger, James H" <[email-masked]>
Date: Thu, 20 Sep 2012 10:35:34 +1000

> An array based approach carries along many of it's own issues, especially if we do decide to extend things later on.

James Snell, could you expand on those issues a bit?
Extending does not seem that hard with the array approach.
If a future operation needs a bunch of optional parameters for instance (so naming them is more convenient than remembering their position in an argument list), that is easy to support: define one item in the array for that operation to be a JSON object that holds those parameters.

The 10 patch examples from draft-ietf-appsawg-json-patch-04#appendix-A in array syntax:

A.1.  [["add", "/baz", "qux"]]
A.2.  [["add", "/foo/1", "qux"]] 
A.3.  [["remove", "/baz"]]
A.4.  [["remove", "/foo/1"]]
A.5.  [["replace", "/baz", "boo"]]
A.6.  [["move", "/foo/waldo", "/qux/thud"]]
A.7.  [["move", "/foo/1", "/foo/3"]]
A.8.  [["test", "/baz", "qux"], ["test", "/foo/1", 2]]
A.9.  [["test", "/baz", "bar"]]
A.10. [["add", "/child", {"grandchild":{}}]]

These all look simple to understand and parse/process. I am much more confident that any vaguely reasonable implementation cannot confuse these operations, or fail to reject an unrecognized operation, even with a maliciously-constructed invalid patch.

One downside is that you have to remember it is ["copy", from, to], not ["copy", to, from] -- you don't have the "to" label to remind you. That feels like a better compromise than the current syntax (little confidence that many implementations will properly check validity) or the verbosity of James Snell's ideal syntax {"op":"add", "ref":"/baz", "value":"qux"}.

--
James Manger

---

From: James M Snell <[email-masked]>
Date: Wed, 19 Sep 2012 22:53:28 -0700

On Wed, Sep 19, 2012 at 5:35 PM, Manger, James H <
[email-masked]> wrote:

> especially if we do decide to extend things later on.
>
> If a future operation needs a bunch of optional parameters for instance
> (so naming them is more convenient than remembering their position in an
> argument list), that is easy to support: define one item in the array for
> that operation to be a JSON object that holds those parameters.
>
>
The main issue, aside from the basic possible extensibility ramifications
is that it just *feels* like an entirely pointless optimization... a change
that buys us absolutely nothing in terms of real functionality and simply
trades one set of potential issues for another. The spec version of code
smell.

To be honest, I'm still wondering what's wrong with my slightly more
verbose option.

- James

> These all look simple to understand and parse/process. I am much more
> confident that any vaguely reasonable implementation cannot confuse these
> operations, or fail to reject an unrecognized operation, even with a
> maliciously-constructed invalid patch.
>
> One downside is that you have to remember it is ["copy", from, to], not
> ["copy", to, from] -- you don't have the "to" label to remind you. That
> feels like a better compromise than the current syntax (little confidence
> that many implementations will properly check validity) or the verbosity of
> James Snell's ideal syntax {"op":"add", "ref":"/baz", "value":"qux"}.
>
> --
> James Manger
>
>

---

From: Daniel Renfer <[email-masked]>
Date: Thu, 20 Sep 2012 01:58:44 -0400

I'm going to have to give a +1 to the separate op/ref approach from an
implementation standpoint. I've just been playing around with what it
would take to implement this in clojure and discovered that parsing
would be a lot easier if I had just 1 key to check to get my
operation. This would allow me to have an open-ended implementation
where I could easily add new ops as they come up.

I also think it makes the story for extension properties much easier.
For instance, how about an "optional" key for operations that are
allowed to fail without failing the whole operation? Just an example.

On Thu, Sep 20, 2012 at 1:53 AM, James M Snell <[email-masked]> wrote:
>
>
> On Wed, Sep 19, 2012 at 5:35 PM, Manger, James H
> The main issue, aside from the basic possible extensibility ramifications is
> that it just *feels* like an entirely pointless optimization... a change
> To be honest, I'm still wondering what's wrong with my slightly more verbose
> option.
>
> - James
>
>>

---

From: "Manger, James H" <[email-masked]>
Date: Thu, 20 Sep 2012 17:40:26 +1000

> >> > An array based approach 

> > The main issue, aside from the basic possible extensibility
> > ramifications is that it just *feels* like an entirely pointless
> > optimization... a change that buys us absolutely nothing in terms of
> > real functionality and simply trades one set of potential issues for
> > another. The spec version of code smell.

Given that each operation is likely to be mapped to a function call such as add(ptr, val), an array approach ["add", ptr, val] seems quite natural. The object-based approach *feels* like an entirely pointless optimization... an optimization to support some potential extensibility that we explicitly don't want.

Extensions by adding new operations is potentially useful -- it is equally easy with an array approach or an object-with-op-member approach.

Extensions by adding new parameters to previously defined operations sounds bad -- so the fact that that may be easier with an object-with-op-member approach is not actually an advantage.

> > To be honest, I'm still wondering what's wrong with my slightly more

Its verbosity ;)
A key with the totally-generic name "value" is not a good sign that the syntax matches the model well.

I suggest that the patch spec should say:
1. "it is an error condition if an operation name is unrecognized"
2. "unrecognized parameters MUST be ignored"
It is so likely that some/many implementations will not bother to check that there are no unrecognized parameters that the spec should not imply that anyone can rely on those checks occurring.

The syntax needs to be changed from the current form (object-with-op-name-as-key) to either array or object-with-op-member for this pair of statements to work.

--
James Manger

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 21 Sep 2012 11:37:07 -0700

On 19/09/2012, at 4:42 PM, Paul C. Bryan <[email-masked]> wrote:

>> When using something like "op":"replace" as an alternative, however, this additional validation check becomes entirely unnecessary and the possibility of ambiguity dissolves completely. For me, this is much more advantageous than saving a few bytes on the wire.
> 
> Well, there was significant feedback against separate operation and path members at the time. Personally, I'm ambivalent on this; I like both approaches for different reasons, though the current method wins out by a small margin. That said, if strong consensus to change to bifurcating to separate op/path were to be established, I wouldn't resist.

My .02 -  I'd support op/path, as long as we can get there quickly and not rathole on it. 

Does anyone object strongly to changing the syntax of an operation to something approximately like:

{ "op": "add", "path": "/a/b/c", "value": "foo" }

Cheers,

--
Mark Nottingham
http://www.mnot.net/

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 21 Sep 2012 11:46:00 -0700

Hi James,

> Extensions by adding new parameters to previously defined operations sounds bad -- so the fact that that may be easier with an object-with-op-member approach is not actually an advantage.

I understand your arguments, and don't disagree on any point. People naturally reach for an object because of its properties when extended, and we *don't* want extensibility here.

However, my sense is that it's going to be hard to convince people to change to an array-based syntax. Could be wrong, but that's what I see from here.

> 1. "it is an error condition if an operation name is unrecognised"

We're already there...

> It is so likely that some/many implementations will not bother to check that there are no unrecognized parameters that the spec should not imply that anyone can rely on those checks occurring.

I'd be OK with this, as long as we clarify that this means that the format explicitly doesn't allow extensions, and that we can get consensus quickly. Others?

--
Mark Nottingham
http://www.mnot.net/

---

From: Phillip Hallam-Baker <[email-masked]>
Date: Fri, 21 Sep 2012 15:05:51 -0400

On Fri, Sep 21, 2012 at 2:37 PM, Mark Nottingham <[email-masked]> wrote:

> approaches for different reasons, though the current method wins out by a
> small margin. That said, if strong consensus to change to bifurcating to
> separate op/path were to be established, I wouldn't resist.
>

I prefer this to the -04 version:
{ "add": "/a/b/c", "value": [ "foo", "bar" ] }

"/a/b/c" does look to me to be a path attribute to me.

But my preferred version would be

{"add" : {"path": "/a/b/c", "value": "foo" }}

That would seem to me to be the right way to do it. One advantage being
that it ensures that the operation always comes first. The following is
equivalent but much harder to follow:

{  "path": "/a/b/c", "op": "add", "value": "foo" }

-- 
Website: http://hallambaker.com/

---

From: James M Snell <[email-masked]>
Date: Fri, 21 Sep 2012 12:08:52 -0700

On Fri, Sep 21, 2012 at 11:46 AM, Mark Nottingham <[email-masked]> wrote:

> > It is so likely that some/many implementations will not bother to check
> that there are no unrecognized parameters that the spec should not imply
> that anyone can rely on those checks occurring.
>
> I'd be OK with this, as long as we clarify that this means that the format
> explicitly doesn't allow extensions, and that we can get consensus quickly.
> Others?
>
>
I'm ok with this. I just want to make sure that it's crystal clear to
everyone that the semantics of an existing change operation cannot be
modified by adding new attributes down the road. That is, if some client
application happens to send {"op":"test", "path": "/a/b/c", "value": "foo",
"ignore_case":"true"}, the "ignore_case" will be ignored completely. There
can be absolutely zero ambiguity on that point.

- James

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 21 Sep 2012 12:11:46 -0700

On 21/09/2012, at 12:08 PM, James M Snell <[email-masked]> wrote:

> I'm ok with this. I just want to make sure that it's crystal clear to everyone that the semantics of an existing change operation cannot be modified by adding new attributes down the road. That is, if some client application happens to send {"op":"test", "path": "/a/b/c", "value": "foo", "ignore_case":"true"}, the "ignore_case" will be ignored completely. There can be absolutely zero ambiguity on that point.

+1

--
Mark Nottingham
http://www.mnot.net/

---

From: Martin Thomson <[email-masked]>
Date: Fri, 21 Sep 2012 15:38:20 -0700

> {"add" : {"path": "/a/b/c", "value": "foo" }}

Interesting, what does this do then?

{"add" : {"path": "/a/b/c", "value": "foo" }, "delete": {"path": "/a/b/c"}}

---

From: Martin Thomson <[email-masked]>
Date: Fri, 21 Sep 2012 15:51:25 -0700

+1

I observe that there is no way to define a new type of test in -04.  {
"test_ignore_case":"/a/b/c", "value": "foo" } is a new unsupported
operation, but I didn't see rules on what to do with those.  Do
unsupported/unknown operations cause processing to fail?  If so, then
this would be the right way to implement a case-ignoring test.

---

From: James M Snell <[email-masked]>
Date: Sat, 22 Sep 2012 09:54:15 -0700

+1 on using the separate op/path/value syntax

---

From: Phillip Hallam-Baker <[email-masked]>
Date: Sat, 22 Sep 2012 13:39:53 -0400

> {"add" : {"path": "/a/b/c", "value": "foo" }, "delete": {"path": "/a/b/c"}}

Well I would probably want to prohibit that as ambiguous and insist on an
array:

[{"add" : {"path": "/a/b/c", "value": "foo" }}, {"delete": {"path":
"/a/b/c"}}]

It would probably be a good idea to limit commands to one command per slot.
That is what I do in my protocol compiler. If you need more than one
command then use an array.

In my schema I have:

Type add
    String path
    String value
    Sealed                  // raise exception to unknown tags.

Type delete
    String path
    String value
    Sealed

That looks pretty clean to me. Adding a move command also looks pretty
clean:

Type move
    String from
    String to
    Sealed

If you have an application where you have an option of doing things in
parallel then you might have something like:

{"Seq" : [
     {"Par" : [
           {"add" : {"path": "/a/b/c", "value": "foo" }}, {"add" : {"path":
"/a/b/d", "value": "foo" }}]},
     {"delete": {"path": "/a/b/c"}}] }

That would be nonsense for a patch application (unless the patches were to
separate files). But it could make a lot of sense in a different context.

JSON syntax seems to be pretty well adapted to doing this sort of thing. It
looks pretty convenient.

---

From: "Manger, James H" <[email-masked]>
Date: Mon, 24 Sep 2012 10:48:08 +1000

> >>>>> An array based approach
...
> > Given that each operation is likely to be mapped to a function call
> such as add(ptr, val), an array approach ["add", ptr, val] seems quite
> natural. The object-based approach *feels* like an entirely pointless
> optimization... an optimization to support some potential extensibility
> that we explicitly don't want.
> >
> > Extensions by adding new operations is potentially useful -- it is
> equally easy with an array approach or an object-with-op-member
> approach.
> >
> > Extensions by adding new parameters to previously defined operations
> sounds bad -- so the fact that that may be easier with an object-with-
> op-member approach is not actually an advantage.

> I understand your arguments, and don't disagree on any point. People
> naturally reach for an object because of its properties when extended,
> and we *don't* want extensibility here.
> 
> However, my sense is that it's going to be hard to convince people to
> change to an array-based syntax. Could be wrong, but that's what I see
> from here.

Perhaps we should randomise the order of name:value pairs in the spec examples as a subtle reminder that you do have to iterate through them all to find a name that is an operation (with draft 04 syntax).
  {"value":"qux", "add":"/baz"}

Including examples of operations that MUST be rejected would be a good hint as to any extra effort required to implement those checks. For instance, the following MUST be rejected:
  {"value":"qux", "add":"/baz", "\u0072emove":"/baz"}

Or with op:name syntax, the following MUST be rejected (not silently treated as an "add" or "remove"):
  {"value":"qux", "path":"/baz", "op":"add", "op":"remove"}

Whereas the following is a valid "add" operation (ignore the parameter with an unknown name "OP")
  {"OP":"remove", "value":"qux", "path":"/baz", "\u006Fp":"\u0061dd"}

Seeing examples like these makes an array-based approach look simpler and safer.

> > It is so likely that some/many implementations will not bother to
> check that there are no unrecognized parameters that the spec should
> not imply that anyone can rely on those checks occurring.

> I'd be OK with this, as long as we clarify that this means that the
> format explicitly doesn't allow extensions, and that we can get
> consensus quickly. Others?

Saying "unrecognized parameters MUST be ignored" means new parameters can be inserted in future, with the clear understanding that old implementations will ignore them so they can only be optional hints.

--
James Manger

---

From: "Paul C. Bryan" <[email-masked]>
Date: Sun, 23 Sep 2012 17:53:45 -0700

+1 on reinstating "op".

Paul

---

From: "Manger, James H" <[email-masked]>
Date: Mon, 24 Sep 2012 11:01:50 +1000

> Well I would probably want to prohibit that as ambiguous and insist on
> an array:
> 

Phillip,
Tweak your preferred syntax (for a single operation) from:
  {"add" : {"path": "/a/b/c", "value": "foo" }}
to:
  ["add" , {"path": "/a/b/c", "value": "foo" }]
and prohibiting Martin’s example that introduces ambiguity will occur naturally.

(The whole patch doc is an array of these operations, regardless of whether an array or object syntax is chosen for each individual operation)

--
James Manger

---

From: Phillip Hallam-Baker <[email-masked]>
Date: Sun, 23 Sep 2012 21:36:16 -0400

On Thu, Sep 20, 2012 at 3:40 AM, Manger, James H <
[email-masked]> wrote:

> Given that each operation is likely to be mapped to a function call such
> as add(ptr, val), an array approach ["add", ptr, val] seems quite natural.
> The object-based approach *feels* like an entirely pointless

["add", ptr, val]

remove the comas, change the brackets and we get

("add" ptr val)

I seem to remember that from somewhere.

---

From: Mark Nottingham <[email-masked]>
Date: Sun, 23 Sep 2012 21:50:37 -0400

Ok, it sounds like we want to go there. I'll work on it. 

Sent from my iPhone