[apps-discuss] Appsdir review of draft-ietf-appsawg-json-patch-06
ht@inf.ed.ac.uk (Henry S. Thompson) (by way of S Moonesamy <sm+ietf@elandsys.com>) Thu, 15 November 2012 17:28 UTC
Return-Path: <sm@elandsys.com>
X-Original-To: apps-discuss@ietfa.amsl.com
Delivered-To: apps-discuss@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 0F90521F89C3 for <apps-discuss@ietfa.amsl.com>; Thu, 15 Nov 2012 09:28:06 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.599
X-Spam-Level:
X-Spam-Status: No, score=-2.599 tagged_above=-999 required=5 tests=[BAYES_00=-2.599]
Received: from mail.ietf.org ([64.170.98.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id oX1yryatpxc3 for <apps-discuss@ietfa.amsl.com>; Thu, 15 Nov 2012 09:28:05 -0800 (PST)
Received: from mx.ipv6.elandsys.com (mx.ipv6.elandsys.com [IPv6:2001:470:f329:1::1]) by ietfa.amsl.com (Postfix) with ESMTP id 7F08221F89C8 for <apps-discuss@ietf.org>; Thu, 15 Nov 2012 09:28:04 -0800 (PST)
Received: from SUBMAN.elandsys.com ([197.224.152.44]) (authenticated bits=0) by mx.elandsys.com (8.14.5/8.14.5) with ESMTP id qAFHRonT006702 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 15 Nov 2012 09:28:00 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=opendkim.org; s=mail2010; t=1353000483; bh=JDKU7Y/RgbM7pilqGJRT3ywI8dWc4TFuju3hp3JmBcQ=; h=Date:To:From:Subject:Cc; b=qYuB+kF2AvQw9GE+5dCq9o08f+L9AB8OAU0zeppKg9FBHmxbrA0JcDOQg7rit3OL3 iSWdkpaFrdLwtmr3/C+jaX5qRkuAmM4naTIMjsZyqLvwTyQkT+xqAkpsf6hPHm5qfl jWpdrcWy2a0N5eWSsQI02qLB4H2/7chmSBDpmbyc=
Message-Id: <6.2.5.6.2.20121115091657.0cf84898@elandnews.com>
X-Mailer: QUALCOMM Windows Eudora Version 6.2.5.6
Date: Thu, 15 Nov 2012 09:19:44 -0800
To: apps-discuss@ietf.org
From: ht@inf.ed.ac.uk
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; format="flowed"
Cc: draft-ietf-appsawg-json-patch.all@tools.ietf.org
Subject: [apps-discuss] Appsdir review of draft-ietf-appsawg-json-patch-06
X-BeenThere: apps-discuss@ietf.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: General discussion of application-layer protocols <apps-discuss.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/apps-discuss>, <mailto:apps-discuss-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/apps-discuss>
List-Post: <mailto:apps-discuss@ietf.org>
List-Help: <mailto:apps-discuss-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/apps-discuss>, <mailto:apps-discuss-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 15 Nov 2012 17:28:06 -0000
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]
- [apps-discuss] Appsdir review of draft-ietf-appsa… Henry S. Thompson
- [apps-discuss] Appsdir review of draft-ietf-appsa… Henry S. Thompson by way of S Moonesamy <sm+ietf@elandsys.com>
- Re: [apps-discuss] [appsdir] Appsdir review of dr… Mark Nottingham
- Re: [apps-discuss] Appsdir review of draft-ietf-a… Mark Nottingham
- Re: [apps-discuss] Appsdir review of draft-ietf-a… Manger, James H
- Re: [apps-discuss] Appsdir review of draft-ietf-a… Mark Nottingham
- Re: [apps-discuss] Appsdir review of draft-ietf-a… Henry S. Thompson
- Re: [apps-discuss] Appsdir review of draft-ietf-a… Henry S. Thompson
- Re: [apps-discuss] Appsdir review of draft-ietf-a… Mark Nottingham
- Re: [apps-discuss] Appsdir review of draft-ietf-a… Henry S. Thompson
- Re: [apps-discuss] Appsdir review of draft-ietf-a… Mark Nottingham