Subject: [apps-discuss] JSON Patch implementation feedback

From: Mark Nottingham <[email-masked]>
Date: Mon, 3 Dec 2012 11:12:42 +1100

I've started collecting implementations of JSON Patch here:
  http://trac.tools.ietf.org/wg/appsawg/trac/wiki/JsonPatch

... and am also discussing both the draft and a test suite with the implementers.

Below is some feedback from Stefan Kögl, who's implementing in Python, forwarded with permission (along with some commentary from me).

Begin forwarded message:

> * Section 4 "Operations" states
> 
> > Other members of operation objects MUST be ignored, unless they are
> > explicitly allowed by the definition of the operation.
> 
> Why has the decision been made to ignore unknown members? I think this could be problematic if the spec is updated later so that existing operations include new keywords (say a "type" member for test). You wouldn't notice that your (outdated) implementation does not support this addition.
> 
> I don't know how/why the decision has been made to ignore unknown members, but please consider this an argument against it.

I explained why we made this decision (to recap: it was felt that some implementations would inevitably ignore unknown extensions, causing a situation similar to HTML). 

Personally, I think we *could* require unknown extensions to be an error; given our approach to extensibility, and with willing implementers, it's a reasonable thing to do.

Failing that, we at least need to document the approach to extensibility that we're talking (i.e., that this format is not extensible, and any modifications / additions need to be expressed using a new media type).

> * The "replace" and "move" operations contain the note
> 
> > The target location MUST exist for the operation to be successful.
> 
> which is clear in the context. The "move" and "copy" operations do not contain such a statement even though they proably should. Also if they did, the term "target location" would be ambiguous, because it could also refer to the "to" value.
> 
> I'd suggest to use a different term for whatever "path" refers to, and add the statement to all operators to which it applies.

(I think he means *re*move, not move in the first line above)

I tend to agree on both counts.

Is there any objecting to adding this clause to "move" and "copy"? 

Regarding the term, how about "path location"?

Cheers (and thanks to Stefan for the feedback!),

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

---

From: "Manger, James H" <[email-masked]>
Date: Mon, 3 Dec 2012 11:56:29 +1100

> > Why has the decision been made to ignore unknown members? I think
> this could be problematic if the spec is updated later so that existing
> operations include new keywords (say a "type" member for test). You
> wouldn't notice that your (outdated) implementation does not support
> this addition.
> >
> > I don't know how/why the decision has been made to ignore unknown
> members, but please consider this an argument against it.
> 
> I explained why we made this decision (to recap: it was felt that some
> implementations would inevitably ignore unknown extensions, causing a
> situation similar to HTML).
> 
> Personally, I think we *could* require unknown extensions to be an
> error; given our approach to extensibility, and with willing
> implementers, it's a reasonable thing to do.
> 
> Failing that, we at least need to document the approach to
> extensibility that we're talking (i.e., that this format is not
> extensible, and any modifications / additions need to be expressed
> using a new media type).

It is extensible.
To define new functionality that must not be ignored you can define a new operation.
To define new functionality that can be safely ignored by old implementations you can define new members for existing operations.

For instance, to define a test of the type of a JSON value you could define a new operations:
  { "op":"test2", "path":"/a/b/c", "type":[] }  -- true iff the value at the given path is an array

Some extra text to document this would be good. Perhaps change the 1st paragraph of section 4 "Operations":

<--from--
   Operation objects MUST have exactly one "op" member, whose value
   indicates the operation to perform.  Its value MUST be one of "add",
   "remove", "replace", "move", "copy" or "test".  The semantics of each
   is defined below.
--to-->
   Operation objects MUST have exactly one "op" member, whose value
   indicates the operation to perform.  The semantics are defined below
   for the values "add", "remove", "replace", "move", "copy" and "test".
   It is an error if the value is not recognized. New operations can
   be defined by RFCs that update this document.
--

> > which is clear in the context. The "move" and "copy" operations do
> not contain such a statement even though they proably should. Also if
> they did, the term "target location" would be ambiguous, because it
> could also refer to the "to" value.
> >
> > I'd suggest to use a different term for whatever "path" refers to,
> and add the statement to all operators to which it applies.
> 
> 

I thought "path" was being replaced with "from" and "to" as appropriate to the operation. "from" refers to a location in the existing value; "to" refers to a location in the patched value (which will often not exist in the existing value).

  [
     { "op": "test",    "from": "/a/b/c", "value": "foo" },
     { "op": "remove",  "from": "/a/b/c" },
     { "op": "add",       "to": "/a/b/c", "value": [ "foo", "bar" ] },
     { "op": "replace",   "to": "/a/b/c", "value": 42 },
     { "op": "move",    "from": "/a/b/c", "to": "/a/b/d" },
     { "op": "copy",    "from": "/a/b/d", "to": "/a/b/e" }
  ]

[I still think we should ditch the statements that "The target location MUST exist for the operation to be successful". Better to define a sensible "successful" result (eg "remove" is a no-op if "from" does not exist; "move" is equivalent to "remove" on the "from" and "to" locations then, if there was a value at "from", an "add" operation; "replace" is equivalent to "remove" then "add" operations).]

--
James Manger

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 3 Dec 2012 13:39:25 +1100

>  { "op":"test2", "path":"/a/b/c", "type":[] }  -- true iff the value at the given path is an array

That's not a backwards-compatible extension; it fundamentally changes the semantics of the document, and an implementation that ignores it may PATCH when it shouldn't.

>   be defined by RFCs that update this document.

We explicitly agreed to the text before -- i.e., that the set of operations defined by this format is closed.

> I thought "path" was being replaced with "from" and "to" as appropriate to the operation. "from" refers to a location in the existing value; "to" refers to a location in the patched value (which wi…

No. In the current draft, *all* operations have "path", and it always refers to the place that the operation affects (where appropriate, "to"). In operations where there are two relevant locations, the second location is now always "from"; it used to be "to", but that didn't align with the semantics of the other, one-location operations well.

> [I still think we should ditch the statements that "The target location MUST exist for the operation to be successful". Better to define a sensible "successful" result (eg "remove" is a no-op if "f…

The point of this statement in the "remove" and "replace" operations is that if the target isn't there, the patch is probably not written with the state of the document in mind, and should fail. Contrast with "add" (where the intent was to allow authors to add-or-update).

For "move" and "copy" it also seems like a good idea, because if you run a patch and the source of one of these operations isn't existent, you definitely don't know what state the document is in, and the patch should fail.

---

From: "Manger, James H" <[email-masked]>
Date: Mon, 3 Dec 2012 16:01:46 +1100

> <[email-masked]> wrote:
> >
> > It is extensible.
> > To define new functionality that can be safely ignored by old
> implementations you can define new members for existing operations.
> >

> That's not a backwards-compatible extension; it fundamentally changes
> the semantics of the document, and an implementation that ignores it
> may PATCH when it shouldn't.

"op":"test2" is not meant to be backward-compatible. It is not meant to be ignored and it will not be ignored. All code will switch on the "op" value. [In sharp contrast to unexpected extra name/value pairs which most code will ignore so the spec sensibly codifies that must-ignore behaviour.]

> We explicitly agreed to the text before -- i.e., that the set of
> operations defined by this format is closed.

So if another spec like draft-snell-json-test (section 2.5 "Using JSON Prediciate within JSON Patch Documents) defines some operations it can re-use the json-patch syntax with some new "op" values -- but it also needs to define a new media type? Oh well.

> > [I still think we should ditch the statements that "The target
> > location MUST exist for the operation to be successful". Better to
> > define a sensible "successful" result (eg "remove" is a no-op if
> > "from" does not exist; "move" is equivalent to "remove" on the "from"
> > and "to" locations then, if there was a value at "from", an "add"
> > operation; "replace" is equivalent to "remove" then "add"
> > operations).]
> 
> 
> The point of this statement in the "remove" and "replace" operations is
> that if the target isn't there, the patch is probably not written with
> the state of the document in mind, and should fail. Contrast with "add"
> (where the intent was to allow authors to add-or-update).

So to use "add" the author doesn't need to have "the state of the document in mind", but to use "remove" or "replace" the author does. That is painfully inconsistent. Plus the protection for the author is so weak: the value that a "remove" op deletes could have been updated to something completely different from the value the patch author thinks it is, but it will be removed anyway.
Any minor benefit of catching some "typos" in patch docs in an ad hoc manner is outweighed by losing functionality (eg a patch to remove a field that works regardless of whether the field is present), and the temptation it gives to skimp on proper checks (eg with a "test" op, e-tags, or locks).

> For "move" and "copy" it also seems like a good idea, because if you
> run a patch and the source of one of these operations isn't existent,
> you definitely don't know what state the document is in, and the patch
> should fail.

And if the patch succeeds you might have known the doc state or you might not. That's better?

--
James Manger

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 3 Dec 2012 16:17:25 +1100

> "op":"test2" is not meant to be backward-compatible. It is not meant to be ignored and it will not be ignored. All code will switch on the "op" value. [In sharp contrast to unexpected extra name/va…

Introducing a new operation that is safe-to-ignore for processors that don't understand it is theoretically possible, but practically what will they do?

In the cast of your theoretical test2 -- if the patcher doesn't perform the test, and it would have failed, now we have two different behaviours, based upon whether they implement test2; not a good thing in a patch format.

> So if another spec like draft-snell-json-test (section 2.5 "Using JSON Prediciate within JSON Patch Documents) defines some operations it can re-use the json-patch syntax with some new "op" values -- but it also needs to define a new media type? Oh well

Ask discussed, yes -- and James is AFAIK OK with and behind that approach.

> Any minor benefit of catching some "typos" in patch docs in an ad hoc manner is outweighed by losing functionality (eg a patch to remove a field that works regardless of whether the field is presen…

*shrug* I'm not too concerned about consistency for its own sake. People had compelling use cases for 'add', whereas they felt that if you 'remove' something, and it isn't there, you'd want the patch to fail.

I'm not married to any particular approach here, as long as a) it makes sense for implementers and end users, and b) we can get to a decision quickly (as opposed to the Dueling Architects Show that the IETF seems to lean towards these days...*).

To be clear -- I think the proposal you're making is a) to make 'remove' not fail if the target isn't there, and b) to take 'replace' out of the specification (since the only difference in its semantics from 'add' is the ability to fail if the thing you want to set isn't there).

Additionally, if we go down this road, we'll have to decide what to do about similar failures in 'copy' and 'move' -- do you REALLY want failure to find the target to be silently digested? (!?)

What do other folks think?

> And if the patch succeeds you might have known the doc state or you might not. That's better?

By the nature of HTTP, you can't know the state of the resource until you GET it (and even then, it's ephemeral).

However, if you write a patch that says "move x to y" and there isn't an x in the doc, it seems like a pretty sure think that you're patching something sensibly. YMMV.

* N.B. My job title includes the word "Architect."

---

From: James M Snell <[email-masked]>
Date: Sun, 2 Dec 2012 21:17:54 -0800

On Sun, Dec 2, 2012 at 9:01 PM, Manger, James H <
[email-masked]> wrote:

> [snip]
> "op":"test2" is not meant to be backward-compatible. It is not meant to be
> ignored and it will not be ignored. All code will switch on the "op" value.
> [In sharp contrast to unexpected extra name/value pairs which most code
> will ignore so the spec sensibly codifies that must-ignore behaviour.]
>
>
Per the json patch spec, the introduction of an unknown operation should
cause the operation to fail.

> So if another spec like draft-snell-json-test (section 2.5 "Using JSON
> Prediciate within JSON Patch Documents) defines some operations it can
> re-use the json-patch syntax with some new "op" values -- but it also needs
> to define a new media type? Oh well.
>
>
Yes, given the current definition of json-patch, a new media type would be
necessary. While silly, it's not a deal breaker in reality. If you serve up
a json-patch file with predicates to an implementation that does not
understand those predicates, the processing will predictably and correctly
fail. Serve it up to an implementation that understands predicates, and
it'll work... regardless of whatever media type is used.

- James

> So to use "add" the author doesn't need to have "the state of the document
> in mind", but to use "remove" or "replace" the author does. That is
> painfully inconsistent. Plus the protection for the author is so weak: the
> value that a "remove" op deletes could have been updated to something
> completely different from the value the patch author thinks it is, but it
> will be removed anyway.
> Any minor benefit of catching some "typos" in patch docs in an ad hoc
> manner is outweighed by losing functionality (eg a patch to remove a field
> that works regardless of whether the field is present), and the temptation
> it gives to skimp on proper checks (eg with a "test" op, e-tags, or locks).
>
>
>
> _______________________________________________
> apps-discuss mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/apps-discuss
>

---

From: James M Snell <[email-masked]>
Date: Sun, 2 Dec 2012 21:21:02 -0800

On Sun, Dec 2, 2012 at 9:17 PM, Mark Nottingham <[email-masked]> wrote:

> to define a new media type? Oh well
>

To be clear, yes I am. A new media type is just fine -- largely because,
when put into practice, the specific media type is not going to matter too
much. An implementation will either understand predicates inside a
json-patch or it will not. Either way processing the result should lead to
a correct result.

- James

---

From: "Manger, James H" <[email-masked]>
Date: Mon, 3 Dec 2012 17:07:43 +1100

> > So to use "add" the author doesn't need to have "the state of the
> document in mind", but to use "remove" or "replace" the author does.
> That is painfully inconsistent. Plus the protection for the author is
> so weak: the value that a "remove" op deletes could have been updated
> to something completely different from the value the patch author
> thinks it is, but it will be removed anyway.
> >
> manner is outweighed by losing functionality (eg a patch to remove a
> field that works regardless of whether the field is present), and the
> temptation it gives to skimp on proper checks (eg with a "test" op, e-
> tags, or locks).
> 
> *shrug* I'm not too concerned about consistency for its own sake.
> People had compelling use cases for 'add', whereas they felt that if
> you 'remove' something, and it isn't there, you'd want the patch to
> fail.

I need a compelling use case then: will cute do?
A JSON object represents some system state. If it contains "debug":<level> you get lots of logs. Two 1-line curl commands can toggle the state. The first sends the patch {"op":"add", "path":"/debug", "value":2} to turns on debugging; it always works; it is idempotent. The second sends the patch {"op":"remove", "path":"/debug"}. Ideally it would also always work; it would be idempotent. However, currently it can fail: perhaps because debugging was already off, or perhaps because there was some other problem. Annoying.

> I'm not married to any particular approach here, as long as a) it makes
> sense for implementers and end users, and b) we can get to a decision
> quickly (as opposed to the Dueling Architects Show that the IETF seems
> to lean towards these days...*).
> 
> To be clear -- I think the proposal you're making is
> a) to make 'remove' not fail if the target isn't there,

Yes

> and b) to take 'replace' out of the specification

No. You still need to distinguish replacing the value at a given array index, and inserting a new value before a given array index. "replace" can be defined as "remove" then "add".

I would also go a step further so "add" always works -- creating any parent objects that are needed (like "mkdir --parents").

> Additionally, if we go down this road, we'll have to decide what to do
> about similar failures in 'copy' and 'move' -- do you REALLY want
> failure to find the target to be silently digested? (!?)

May as well. Triggering errors isn't useful. Move/copy acting like "remove" when there is no value at "from" can't be less useful than an error.

--
James Manger

---

From: "Paul C. Bryan" <[email-masked]>
Date: Sun, 02 Dec 2012 22:13:29 -0800

On Mon, 2012-12-03 at 16:17 +1100, Mark Nottingham wrote:

> >>> To define new functionality that must not be ignored you can
> define a
> >> new operation.
> >>> For instance, to define a test of the type of a JSON value you
> could
> >> define a new operations:
> >>> { "op":"test2", "path":"/a/b/c", "type":[] }  -- true iff the
> value
> >>> at the given path is an array
> > 
> > "op":"test2" is not meant to be backward-compatible. It is not meant
> to be ignored and it will not be ignored. All code will switch on the
> "op" value. [In sharp contrast to unexpected extra name/value pairs
> which most code will ignore so the spec sensibly codifies that
> must-ignore behaviour.]
> 
> Introducing a new operation that is safe-to-ignore for processors that
> don't understand it is theoretically possible, but practically what
> will they do?
> 
> In the cast of your theoretical test2 -- if the patcher doesn't
> perform the test, and it would have failed, now we have two different
> behaviours, based upon whether they implement test2; not a good thing
> in a patch format.

I agree. I can't presently imagine an operation that could be safe to
ignore. If a new RFC obseletes ours, it can establish a new media type
or some other convention to distinguish itself from the the format we're
currently specifying.    

> > So if another spec like draft-snell-json-test (section 2.5 "Using
> JSON Prediciate within JSON Patch Documents) defines some operations
> it can re-use the json-patch syntax with some new "op" values -- but
> it also needs to define a new media type? Oh well
> 

Agree. 

> > Any minor benefit of catching some "typos" in patch docs in an ad
> hoc manner is outweighed by losing functionality (eg a patch to remove
> a field that works regardless of whether the field is present), and
> the temptation it gives to skimp on proper checks (eg with a "test"
> op, e-tags, or locks).
> 
> I'm not married to any particular approach here, as long as a) it
> makes sense for implementers and end users, and b) we can get to a
> decision quickly (as opposed to the Dueling Architects Show that the
> IETF seems to lean towards these days...*).
> 
> To be clear -- I think the proposal you're making is a) to make
> 'remove' not fail if the target isn't there, and b) to take 'replace'
> out of the specification (since the only difference in its semantics
> from 'add' is the ability to fail if the thing you want to set isn't
> there).
> 

Unless we want to run the gauntlet and further enhance the test
operation as well, I'm not in favour of making failure of these
operations silent.

> By the nature of HTTP, you can't know the state of the resource until
> you GET it (and even then, it's ephemeral).
> 
> However, if you write a patch that says "move x to y" and there isn't
> an x in the doc, it seems like a pretty sure think that you're
> patching something sensibly. YMMV.
> 
> 
> 

Paul

---

From: James M Snell <[email-masked]>
Date: Wed, 5 Dec 2012 15:58:00 -0800

Experimental ruby impl here: https://rubygems.org/gems/json-tools. Has not
yet been thoroughly tested.

Includes basic Pointer, Patch and Predicates support. Updated today to
match the patch-07 update.

- James

On Sun, Dec 2, 2012 at 4:12 PM, Mark Nottingham <[email-masked]> wrote:

> Below is some feedback from Stefan Kögl, who's implementing in Python,
> forwarded with permission (along with some commentary from me).
>
> Begin forwarded message:
>
> > Why has the decision been made to ignore unknown members? I think this
> could be problematic if the spec is updated later so that existing
> wouldn't notice that your (outdated) implementation does not support this
> addition.
> >
> Personally, I think we *could* require unknown extensions to be an error;
> given our approach to extensibility, and with willing implementers, it's a
> reasonable thing to do.
>
> Failing that, we at least need to document the approach to extensibility
> that we're talking (i.e., that this format is not extensible, and any
> modifications / additions need to be expressed using a new media type).
>
>
> > which is clear in the context. The "move" and "copy" operations do not
> contain such a statement even though they proably should. Also if they did,
> the term "target location" would be ambiguous, because it could also refer
> to the "to" value.
> >
> > I'd suggest to use a different term for whatever "path" refers to, and
> add the statement to all operators to which it applies.
>
>

---

From: Mark Nottingham <[email-masked]>
Date: Thu, 6 Dec 2012 15:44:49 +1100

Updated, thanks!

On 06/12/2012, at 10:58 AM, James M Snell <[email-masked]> wrote:

> Experimental ruby impl here: https://rubygems.org/gems/json-tools. Has not yet been thoroughly tested.
> 
> Includes basic Pointer, Patch and Predicates support. Updated today to match the patch-07 update.
> 
> - James
> 
>