Subject: [apps-discuss] Reviews, please (was Re: Fwd: New Version Notification for draft-ietf-appsawg-json-merge-patch-02.txt)

From: "Murray S. Kucherawy" <[email-masked]>
Date: Mon, 2 Dec 2013 20:14:39 -0800

On Mon, Nov 18, 2013 at 6:14 PM, Manger, James H <
[email-masked]> wrote:

> >> Title:           JSON Merge Patch
> >> Htmlized:
> >> http://tools.ietf.org/html/draft-ietf-appsawg-json-merge-patch-02
> >> Abstract:
> >>    This specification defines the JSON merge patch format and
> >> processing
> >>    rules.
>
> > 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).
>
> 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):

[...]

Could we get a few more reviews or comments in support of advancing this
document?  We'd really like to get this one moving, but we can't without
some indication that there's consensus support for doing so.

-MSK

---

From: "Matt Miller (mamille2)" <[email-masked]>
Date: Tue, 3 Dec 2013 18:31:52 +0000

On Dec 2, 2013, at 9:14 PM, Murray S. Kucherawy <[email-masked]> wrote:

> On Mon, Nov 18, 2013 at 6:14 PM, Manger, James H <[email-masked]> wrote:
> >> Title:           JSON Merge Patch
> >> Htmlized:
> I feel there are still issues (in addition to the outstanding issuing in my last call comments):  
> [...]
> 
> Could we get a few more reviews or comments in support of advancing this document?  We'd really like to get this one moving, but we can't without some indication that there's consensus support for doing so.
> 

My apologies for the late comments.

I think this document discusses a very useful technology and should progress.  It's understandable and implementable, though I have what I think is one major issue and a few minor issues.

MAJOR ISSUES:

* This document does not explicitly state that a merge patch document can only be a JSON array or JSON object.  This is fine with regards to RFC 4627, but might be problematic given the current and near-future state of JSON specifications and implementations which now -- or shortly will -- allow for any JSON value at the top level.  If a merge patch document only makes sense as a JSON array or JSON object, that stipulation needs to be made explicit; otherwise some accommodation of any top-level value ought to be made.

MINOR ISSUES:

* The abstract seems too light.  While I understand the reluctance to repeat oneself, the abstract does get distributed rather widely, and I've seen people read just the abstract to determine if it probably fits their needs.  Having a fuller abstract can help people determine at a glance if the document is applicable for their needs.

* In section 2. Processing Merge Patch Documents, it states that null values for Object members and Array elements "MUST be ignored" but then states that it is to be treated "as if the member was undefined".  I think "MUST be ignored" is confusing; better to state "MUST be removed".

* Still in section 2. Processing Merge Patch Documents, the last two paragraphs seem to be in conflict given the current order.  The former states a processor MUST fail if a modification cannot be made, but the latter states that a process SHOULD treat the removal of a non-existant member as a no-op.  It would seem just switching the order of the two paragraph would resolve the conflict.

* Yet again in section 2. Processing Merge Patch Documents, I think the SHOULDs about accepting the merge patch are fine, but I think it would help to add some rationales for why a processor might reject what appears to be a perfectly valid merge patch.  One example might be if the merge patch would remove a required member.

- m&m

Matt Miller < [email-masked] >
Cisco Systems, Inc.

---

From: James M Snell <[email-masked]>
Date: Tue, 3 Dec 2013 20:43:06 -0800

I will take another pass at addressing James M's comments later on
this week. I want to wrap this one up also.