Subject: [apps-discuss] Fwd: New Version Notification for draft-ietf-appsawg-json-merge-patch-02.txt

From: James M Snell <[email-masked]>
Date: Mon, 18 Nov 2013 10:38:39 -0800

Based on feedback. Fairly significant editorial update and one key
technical change. This updates the media type registration to follow
the +json naming policy on the media type (that is,
application/json-merge-patch becomes application/merge-patch+json).

- James

---------- Forwarded message ----------
From:  <[email-masked]>
Date: Mon, Nov 18, 2013 at 10:37 AM
Subject: New Version Notification for draft-ietf-appsawg-json-merge-patch-02.txt
To: "James M. Snell" <[email-masked]>

A new version of I-D, draft-ietf-appsawg-json-merge-patch-02.txt
has been successfully submitted by James M Snell and posted to the
IETF repository.

Filename:        draft-ietf-appsawg-json-merge-patch
Revision:        02
Title:           JSON Merge Patch
Creation date:   2013-11-18
Group:           appsawg
Number of pages: 10
URL:
http://www.ietf.org/internet-drafts/draft-ietf-appsawg-json-merge-patch-02.txt
Status:
http://datatracker.ietf.org/doc/draft-ietf-appsawg-json-merge-patch
Htmlized:
http://tools.ietf.org/html/draft-ietf-appsawg-json-merge-patch-02
Diff:
http://www.ietf.org/rfcdiff?url2=draft-ietf-appsawg-json-merge-patch-02

Abstract:
   This specification defines the JSON merge patch format and processing
   rules.

Please note that it may take a couple of minutes from the time of submission
until the htmlized version and diff are available at tools.ietf.org.

The IETF Secretariat

---

From: "Manger, James H" <[email-masked]>
Date: Tue, 19 Nov 2013 13:14:58 +1100

> application/json-merge-patch becomes application/merge-patch+json).

Most of these changes are great: title, media type, organization.

I feel there are still issues (in addition to the outstanding issuing in my last call comments):

1.
Section 2, step 1 says:

       Any null member contained in
       the merge patch MUST be ignored and treated as if those members
       are undefined.

"null member" is unclear. RFC 4627 "JSON" uses "member" to mean a name/value pair in an object (and we should stick to this meaning). Is a "null member" a name/value pair with a null value, or does it also include a null value in an array? I hope it is the former, but don't think this is what the author intends.

I think it is crazy to remove null values from arrays (merge-patch should treat arrays as opaque primitive values), but if this is the regrettable consensus the text needs to be clearer. Saying a null value "MUST be ignored" and treated as if it is "undefined" does not suggest a patch [1,null,3] is supposed to be converted to [1,3]. This conversion does not ignore the null (it explicitly removes it) and does not treat it as "undefined" (at least in JavaScript an array position can be undefined and it does not shift subsequent values in the array to earlier positions).

"null member .. ignore .. undefined" text appears 3 times (step 1, step 2 3rd point, step 2 4th point) each worded slightly differently.

2.
Section 2 offers two rules:
#1 if either patch or target is an array;
#2 if patch or target is an object.

If merging is really only defined when patch and target are arrays or objects, then this should be explicitly stated. Rule #2 always applies when rule #1 does not so drop the "if" condition.

    1. If the roots of either the merge patch or target resource
       documents are JSON Arrays ...
 
    2. Otherwise the roots of the merge patch and target resource
       documents are both JSON Objects. Iterate through ...

It would be much better to define processing rules for patch and target being any JSON value (including primitive values), not just arrays and objects. Particularly as the example JavaScript implementation (appendix B) explicitly supports patch and target being primitive values so some behaviour is implied by the code, just not stated in the text.

Even if we some want to restrict application/merge-patch+json content (eg a PATCH body) to being an array or object (or even just an object), it would still be worth describing the merge processing for arbitrary JSON values to help implementers writing code.

3.
Appendix B "Example JavaScript Implementation" defines an apply(orig, patch) function that says it "will mutate the passed in object in place as well". This is probably a bad design and is certainly a bad implementation as 'orig' is mutated in some circumstances but not in others; sometimes 'orig' will end up matching the returned result, in other circumstances it is unchanged, and I'm not sure if it can end up somewhere in between.

For instance, 'orig' remains unchanged if it is an array or if 'patch' is an array.

I suggested some alternative code in http://www.ietf.org/mail-archive/web/apps-discuss/current/msg10548.html

--
James Manger