Hello,

I am forwarding the review of draft-ietf-appsawg-json-patch-06 
performed by Henry S. Thompson.

Regards,
S. Moonesamy

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: (44) 131 650-4587, e-mail: ht@inf.ed.ac.uk
                        URL: http://www.ltg.ed.ac.uk/~ht/
  [mail from me _always_ has a .sig like this -- mail without it is forged spam]