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

[[e-mail] (Henry S. Thompson)]

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";

  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.

  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.

  4) I don't see any basis in the JSON Pointer spec. for "[using the]
     '-' character ... to index the end of the array" -- as I read the
     JSON Pointer spec. "/a/b/-" is an error if "/a/b" addresses an
     array.  Similarly, "/a/b/5" is an error if "/a/b" addresses an
     array and the array is of length 5, which is not consistent with
     the implication of "The specified index MUST NOT be greater than
     the number of elements in the array", i.e. that "/a/b/5" is not
     ruled out in this case. If the intent is that a value of n, where
     n is the size of the array, or a -, is allowed to end a path
     which has gotten to an array, and results in an append operation,
     this should be stated explicitly.

     [Ah, subsequent information reveal that wrt '-', the bug is in
     the references, not the text -- the addition of '-' to JSON
     Pointer is in a subsequent draft to the one referenced here!  I
     have to say that if '-' was added purely for use by JSON patch,
     I'd much prefer that you take my suggestion immediately below and
     revert the '-' addition to JSON Pointer.]

  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.

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.

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).

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.

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.

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."

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.

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!

Nits:

[none]

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