Subject: Re: [apps-discuss] Appsdir review of draft-ietf-appsawg-json-patch-06

From: Mark Nottingham <[email-masked]> (by way of S Moonesamy <[email-masked]>)
Date: Thu, 15 Nov 2012 15:23:37 -0800

Hi Henry,

Thanks for the review. Responses below.

On 16/11/2012, at 4:06 AM, Henry S. Thompson <[email-masked]> wrote:

 > I have been selected as the Applications Area Directorate reviewer for
 > this draft (for background on appsdir, please see
 > http://trac.tools.ietf.org/area/app/trac/wiki/ApplicationsAreaDirectorate).
 >
 > Please resolve these comments along with any other Last Call comments
 > you may receive. Please wait for direction from your document shepherd
 > or AD before posting a new version of the draft.
 >
 > Document: draft-ietf-appsawg-json-patch-06
 > Title: JSON Patch
 > Reviewer: Henry S. Thompson
 > Review Date: 15 November 2012
 > IETF Last Call Date: 2012-11-23
 >
 > Summary: This draft is almost ready for publication as an Proposed
 >         Standard but has a related set of issues that should be fixed
 >         before publication
 >
 > Note that I have not read previous drafts of or comments on this
 > document -- I presume I was asked to review as a fresh pair of eyes.
 > If the result is to repeat some previously-expressed comments, so be
 > it.
 >
 > Major Issues:
 >
 > 4.1 This operation ('add') seems underspecified in several respects.
 >
 >  1) I would have expected a short statement of the actual effect
 >     under each bullet, e.g. "The specified value becomes the entire
 >     contents of the document";

I'll add these.

 >  2) I would have expected error conditions to be specified under each
 >     bullet.  I assume it is an error for "path" to be empty and the
 >     document to have content.  The discussion of index errors wrt
 >     arrays could/should move, and the "common use" 'note', which
 >     isn't a note at all, should be specific to objects.

I'll look at reorganising these (is this really a *major* issue?).

 >  3) I take it you also need to say that if an _existing_ member of an
 >     object is referenced, that's not an error (note that it _is_ a
 >     JSON Pointer error), and the result is to increase the number of
 >     such members by one, with the new value.

I think that'll be the outcome of #1 above, yes.

 >  I think in fact you might be better off to handle this by cases and
 >  avoid all 'intentional' JSON Pointer errors, along the lines of
 >
 >   a) If the "path" is "" and the document is empty then the "value"
 >      becomes the document;
 >
 >   b) If the path excluding its last step identifies an object, then
 >      "value" is added at the end of that object, with the name given by
 >      the decoded reference token of the last step;
 >
 >   c) If the path excluding its last step identifies an array then
 >
 >      i) If the last step's reference token encodes an integer i with
 >         0 <= i < |array|, then . . .
 >      ii) If the last step's reference token is '-' or encodes
 >          |array|, then . . .
 >
 >   Otherwise, the operation raises an error.

The "excluding its last step" is awkward. The error conditions in 
json pointer were put there for exactly this kind of use; is it the 
word 'error' that's causing you concern?

 > 4.2, 4.3 Nothing is said about the case where a path which identifies a
 >     multiply-valued object member is given, which per JSON Pointer is
 >     a pointer error.

To be clear - are you saying that we should handle the case where an 
error in the underlying JSON bubbles up through JSON Pointer? E.g.,

{  "foo": "bar", "foo": "baz" }

?

 > 4.4 Given the real complexity of both 'remove' and 'add', 'move'
 >    really should be defined by reference to those operations, rather
 >    than repeating (imperfectly at the moment) their definitions.
 >    This would have the additional benefit of removing the necessity
 >    for the side conditions, all of which will now follow from the
 >    definitions of 'remove' and 'add' (except the invalidity of
 >    replacing the root, which should as far as I can see be allowed -
 >    { "op": "move", "path": "/a/b", "to": "" } seems perfectly
 >    reasonable to me).

To do that, I think we'd need to change "to" to "from", as the 
semantics of "path" are so different.

I'm not averse to doing so, as long as we can get to agreement about 
it quickly. Anyone object?

 > 4.5 Again, although the first side condition would have to remain,
 > this should be defined by reference to 'add'.  See below under Minor
 > Issues wrt the second side condition

To do that, I think we'd need to change "to" to "from", as the 
semantics of "path" are so different.

I'm not averse to doing so, as long as we can get to agreement about 
it quickly. Anyone object?

 > Minor Issues:
 >
 > 1. This is perfectly clear, but I still missed it: This spec. (and
 >   JSON Pointer) are about _documents_ and not Javascript objects.
 >   It's very easy to slip into thinking about objects, and indeed the
 >   spec. itself talks about the target as if it consisted of objects
 >   and arrays!  It would of course be obnoxious to require you to say
 >   "JSON encoding of an object" every time, instead of just "object",
 >   but I wonder if it wouldn't be a good idea to say, here in the
 >   introduction, precisely that -- that you will (mostly) talk about
 >   the target as if it had a root, consisted of arrays and objects
 >   with members and values, etc., but what you will always _mean_ is
 >   being pointed to, tested, changed, etc. is the _JSON encoding_ of
 >   those things.

It *can* be about JavaScript objects; that was a use case for some 
people, resulting in a bit of the dancing that we do.

 > 3. In a similar vein, wrt the patch document itself, I think it would
 >   be better to say e.g. "A JSON Patch document is a JSON [RFC4627]
 >   document which [represents/encodes/whatever] an array of objects."

OK.

 > 4.5 I don't understand the reason for barring copying to an existing
 >    member.  The rest of the spec. seems content to allow multiple
 >    identically-named members to be specified. . .  You would, I
 >    guess, need to say that where it goes, but if you defined by
 >    reference to "add" as I've given it above, that would be taken
 >    care of.

We took similar language out of add (see thread 'json-patch: "op": 
"insert" and "op": "put"'). I think it's reasonably to take it out of 
copy as well -- others?

 > 4.6 When you get to string-string equality the object / encoding
 >    duality discussed above at 1. becomes a problem -- you have
 >    treated the patch as an object up until now, in which case any
 >    escaping will already have been dealt with.  If one took the words
 >    in this case literally, the following test would succeed:
 >
 >     Target { "a":"\b" }
 >
 >     Patch  [ { "op": "test", "path": "/a", "value":"\\b" } ]
 >
 >    which is presumably not what you meant.  It's clear from the
 >    wording you've used in this case that you _meant_ to refer here
 >    (and only here) to the _encoding_ of the value of the "value"
 >    property of the patch, not its value. Phew!

I think we can address this by making it clear that all of the 
operations are specified in term of JSON primitives (i.e., after 
escaping), but examples are JSON documents, and giving a few more 
examples to illustrate the corner cases. Make sense?

Cheers,

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

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

---

From: "Manger, James H" <[email-masked]>
Date: Fri, 16 Nov 2012 16:13:54 +1100

> I'm not averse to doing so, as long as we can get to agreement about it
> quickly. Anyone object?

Sounds ok.

> I'm not averse to doing so, as long as we can get to agreement about it
> quickly. Anyone object?

Sounds ok.

> "insert" and "op": "put"'). I think it's reasonably to take it out of

I agree, let copy replace an existing target.

An example showing an add, move, or copy replacing an existing value would help.

--
James Manger

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 16 Nov 2012 18:06:12 +1100

> >     contents of the document";

In SVN.

> I'll look at reorganising these (is this really a *major* issue?).

In SVN

> I think that'll be the outcome of #1 above, yes.

Sorry, replay that? If an existing member of an object is referenced, that member will be replaced...

> The "excluding its last step" is awkward. The error conditions in json pointer were put there for exactly this kind of use; is it the word 'error' that's causing you concern?
> 
> To be clear - are you saying that we should handle the case where an error in the underlying JSON bubbles up through JSON Pointer? E.g.,
> 
> To do that, I think we'd need to change "to" to "from", as the semantics of "path" are so different.
> 
> I'm not averse to doing so, as long as we can get to agreement about it quickly. Anyone object?

In SVN

> To do that, I think we'd need to change "to" to "from", as the semantics of "path" are so different.
> 
> I'm not averse to doing so, as long as we can get to agreement about it quickly. Anyone object?

In SVN

> It *can* be about JavaScript objects; that was a use case for some people, resulting in a bit of the dancing that we do.
> 

In SVN

> We took similar language out of add (see thread 'json-patch: "op": "insert" and "op": "put"'). I think it's reasonably to take it out of copy as well -- others?

In SVN

> I think we can address this by making it clear that all of the operations are specified in term of JSON primitives (i.e., after escaping), but examples are JSON documents, and giving a few more examples to illustrate the corner cases. Make sense?
> 
> 
> Cheers,
> 
> 
> --
> apps-discuss mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/apps-discuss

---

From: [email-masked] (Henry S. Thompson)
Date: Fri, 16 Nov 2012 11:14:47 +0000

> Thanks for the review. Responses below.

[some responses here, some to a subsequent message]

> I'll look at reorganising these (is this really a *major* issue?).

I swithered about this -- I agree not 'major' in the sense of "this is
broken and must be fixed", but by the time I finished I thought "yes,
'major' in the sense that I'm suggesting a substantial structural change"

>>   b) If the path excluding its last step identifies an object,
>>  ...
>
> The "excluding its last step" is awkward. The error conditions in json
> pointer were put there for exactly this kind of use; is it the word
> 'error' that's causing you concern?

No, just that the whole architecture as it stands feels awkward, and
the addition of "-" to JSON Pointer just in order to service Patch
seems especially awkward.  I would think the right change to Pointer
would be to define, say, 'container' as the object or array identified
by the pointer-less-last-step, then you can use that term in Patch.

> {  "foo": "bar", "foo": "baz" }

That's not a JSON error, and it's only a Javascript error in 'strict'
mode, as I understand it.  And 4.1 already deals with this case.

> semantics of "path" are so different.

Sure.

> people, resulting in a bit of the dancing that we do.

I think that use needs to be called out, and the ways in which it has
incompatible semantics warned about -- particularly wrt multiple
members of objects with the same name: this is possible in JSON and
Javascript syntax, but is incoherent in Javascript objects as such.

ht
-- 
       Henry S. Thompson, School of Informatics, University of Edinburgh
      10 Crichton Street, Edinburgh EH8 9AB, SCOTLAND -- (44) 131 650-4440
                Fax: (44) 131 650-4587, e-mail: [email-masked]
                       URL: http://www.ltg.ed.ac.uk/~ht/
 [mail from me _always_ has a .sig like this -- mail without it is forged spam]

---

From: [email-masked] (Henry S. Thompson)
Date: Fri, 16 Nov 2012 11:22:48 +0000

> Sorry, replay that? If an existing member of an object is referenced, that member will be replaced...

What?  If 'add' adds an additional member, and 'move' is 'remove' plus
'add', then 'move' adds, not replaces.

Concrete examples:

Target document:
[ { "key": "apples", "status": "ripe", "count": 3 },
  { "key": "apples", "status": "green", "count": 10 } ]

Ex 1
 Patch: [ { "op": "add", "path": "/1/count", "value": 4 } ]
 Result:
        [ { "key": "apples", "status": "ripe", "count": 3, "count": 4 },
          { "key": "apples", "status": "green", "count": 10 } ]

Ex 2

 Patch: [ { "op": "move", "from": "/2/count", "path": "/1/count" } ]
 Result:
        [ { "key": "apples", "status": "ripe", "count": 3, "count": 10 },
          { "key": "apples", "status": "green" } ]

Does this clarify?

ht

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 19 Nov 2012 18:07:12 +1100

It does, but that isn't the intent, and I think many people would find that result extremely surprising.

The result of ex 1 should be:

       [ { "key": "apples", "status": "ripe", "count": 3 },
         { "key": "apples", "status": "green", "count": 4 } ]

I.e., array references are zero-based, and members are replaced, not repeated, when added to.

---

From: [email-masked] (Henry S. Thompson)
Date: Mon, 19 Nov 2012 10:10:17 +0000

> I.e., array references are zero-based, and members are replaced, not repeated, when added to.

Sorry for the array-base error, which just confuses things, but the
'add' text nowhere says that 'add' == 'replace' for existing members,
and surely that's not obvious, given that 'add' very definitely does
_not_ == 'replace' for existing array members. . .

OTOH, I was just arguing for consistency, _not_ because I believe that
constructing documents with multiple same-name object member settings
was a good thing, so I'm happy if you clarify in 'add' that this does
_not_ happen, and then we're back to a clean state if 'move' and
'copy' are defined in terms of 'add'.

ht

---

From: Mark Nottingham <[email-masked]>
Date: Tue, 27 Nov 2012 12:51:09 +1100

Hi Henry,

I think this is now clearer, thanks to the latest round of editing. It currently says:

>                When the operation is applied, the target location MUST
>                 reference one of:
> 
>                     [...]
> 
>                     A member to add to an existing object - whereupon the
>                     supplied value is added to that object at the indicated
>                     location. If the member already exists, it is replaced
>                     by the specified value.

Cheers,