Subject: [appsdir] List of drafts for review

From: SM <[email-masked]>
Date: Thu, 15 Nov 2012 00:07:06 -0800

Hello,

There are a few drafts which would benefit from 
an APPSDIR review.  I'll list the draft name and 
the reviewer.  I'll set the deadline to November 21 to be on the safe side.

  draft-ietf-6man-uri-zoneid-05 - Yves Lafon

  draft-ietf-appsawg-json-patch-06 - Henry S. Thompson

  draft-ietf-appsawg-json-pointer-05 - Martin Dürst

  draft-ietf-ppsp-problem-statement-11 - Salvatore Loreto

  draft-ietf-imapmove-command-02 - Murray Kucherawy

  draft-ietf-paws-problem-stmt-usecases-rqmts-08 - Carsten Bormann

  draft-ietf-tcpm-experimental-options-02 - Eliot Lear

  draft-arkko-iesg-crossarea-02 - Peter Saint-Andre

  draft-ietf-v6ops-icp-guidance-04 - Jiankang Yao

  draft-ietf-6renum-static-problem-02 - Dave Cridland

  draft-ietf-sipcore-sip-websocket-06 - Alexey Mekinov

  draft-ietf-v6ops-464xlat-08 - Ted Hardie

  draft-salgueiro-vcarddav-kind-device-03 - Larry Masinter

Best regards,
-sm

---

From: "Martin J. Dürst" <[email-masked]>
Date: Thu, 15 Nov 2012 17:31:11 +0900

On 2012/11/15 17:07, SM wrote:
> Hello,
>
> There are a few drafts which would benefit from an APPSDIR review. I'll
> list the draft name and the reviewer. I'll set the deadline to November
> 21 to be on the safe side.
>

My name is already listed in the Ack section, so I'm not sure I'm the 
right person to do it, but I will give it a try. It's short enough, and 
I should be able to it by the deadline. I promise that I will not invest 
as much energy in this review as I did in the last one :-).

Regards,   Martin.

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

---

From: SM <[email-masked]>
Date: Thu, 15 Nov 2012 00:43:36 -0800

Hi Martin,
At 00:31 15-11-2012, Martin J. Dürst wrote:
>My name is already listed in the Ack section, so 
>I'm not sure I'm the right person to do it, but 
>I will give it a try. It's short enough, and I 
>should be able to it by the deadline. I promise 
>that I will not invest as much energy in this 
>review as I did in the last one :-).

Oops, I should not have assigned this one to 
you.  Let's save that energy for another draft. :-)

I'll reassign the review of 
draft-ietf-appsawg-json-pointer-05 to William Mills.

Best regards,
-sm

---

From: [email-masked] (Henry S. Thompson)
Date: Thu, 15 Nov 2012 17:06:39 +0000

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: [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: Mark Nottingham <[email-masked]>
Date: Fri, 16 Nov 2012 10:16:37 +1100

Hi Henry,

Thanks for the review. Responses below.

>     contents of the document";

I'll add these.

>     isn't a note at all, should be specific to objects.

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

>     such members by one, with the new value.

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

>   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?

>     multiply-valued object member is given, which per JSON Pointer is

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

?

>    { "op": "move", "path": "/a/b", "to": "" } seems perfectly

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?

> 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?

>   being pointed to, tested, changed, etc. is the _JSON encoding_ of

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

>   document which [represents/encodes/whatever] an array of objects."

OK.

>    reference to "add" as I've given it above, that would be taken

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?

>    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/