Subject: Re: [apps-discuss] Working Group Last Call: draft-ietf-appsawg-json-merge-patch

From: "Murray S. Kucherawy" <[email-masked]>
Date: Fri, 15 Nov 2013 16:59:54 -0800

Colleagues,

This WGLC ended more than a month ago with only a single reviewer.  There
has been no activity since.

Could we please get some volunteers to review and comment on it?  We really
need to get it over to the IESG before we can accept new work; too many WG
documents open at once has gotten us into trouble in the past.  The other
option is to park it indefinitely, but we'd really rather not have to
resort to that.

Thanks,
-MSK

On Fri, Oct 4, 2013 at 12:19 PM, Murray S. Kucherawy <[email-masked]>wrote:

> A quick reminder that this Working Group Last Call ends today.  So far
> there's been one review on this thread, which is hardly enough to be able
> to send it to the IESG.  Could we please get some more eyes on this and
> some substantive review comments?
>
> -MSK
>
>
> On Tue, Sep 17, 2013 at 12:16 PM, Murray S. Kucherawy <[email-masked]
> > wrote:
>
>> This note begins Working Group Last Call for
>> draft-ietf-appsawg-json-merge-patch, ending October 4, 2013.
>>
>> Please review the document and post review comments to this list.  Please
>> indicate in your reply whether you support publication in its current form,
>> with your constructive comments accommodated, or if it needs more
>> development.  A simple "+1" is not especially useful.  The more detailed
>> your review comments are, the more useful they are.
>>
>> We also need to see enough of these to be able to say unsarcastically
>> that the document has consensus support of the working group.
>>
>> Thanks,
>>
>> -MSK, APPSAWG co-chair
>>
>>
>

---

From: Bjoern Hoehrmann <[email-masked]>
Date: Sat, 16 Nov 2013 15:55:51 +0100

* Murray S. Kucherawy wrote:
>Please review the document and post review comments to this list.

In http://tools.ietf.org/html/draft-ietf-appsawg-json-merge-patch-01 the
title seems misleading, it just mentions the media type but according to
the introduction the document also defines the format. I think emphasis
should be given to the format, not some identifier string.

In section 1 I found

  The Merge Patch format generally supports two types of changes:
  removing and setting JSON object members.  JSON arrays are
  essentially treated the same as JSON primitives: the existing value
  is replaced, but not partially modified.

confusingly worded. Overall I think the introduction does not do a good
job explaining how a "Merge Patch" format is different from an ordinary
"patch" format.

I think RFC 2119 boilerplate does not belong in an Introduction section,
it should go into a "Terminology", "Document conventions", "Conformance"
or similarily named section.

In section 2, the first example has rather inconsistent white space use;
white space should be normalised to avoid distracting readers thinking
the example might be about white space.

I find the references to HTTP PATCH unhelpful, for instance, "When used
within an HTTP PATCH request, it is the responsibility of the server
receiving and processing the request to inspect the payload entity and
determine the specific set of operations that are to be performed to
modify the target resource." seems entirely redundant with the semantics
of the PATCH method and does not seem to belong in the document. But I
don't terribly mind that.

Also in section 2,

  Upon receiving the request, the server is responsible for inspecting
  the payload and determining, based on it's own understanding of the
  target resource media type and the underlying data model of the
  target resource, what specific operations will be applied to modify
  the resource.

This makes it sound like the document does not actually define how the
result after applying a "Merge Patch" to a document will look like. I'm
not sure that is the intended interpretation. Later in the document I
get the impression that the server can only decide whether to accept the
request at all.

I do not really understand why `null` is handled the way it is, it seems
for instance with wholesale replacements where `null` cannot be "delete"
it could be fine to use the value e.g. to handle cases where you need to
have explicit `null`s. I think having no method to have explicit `null`s
is likely hazardous.

Including a `charset` parameter for the media type strikes me as very
bad idea.

In section 3 the field "Applications that use this media type" should
identify the class of application that would support the type, not give
a list of products ("revision control systems" versus "GNU Patch"), so
"None" is never a proper value.

I have not looked at the appendices.
-- 
Björn Höhrmann · mailto:[email-masked] · http://bjoern.hoehrmann.de
Am Badedeich 7 · Telefon: +49(0)160/4415681 · http://www.bjoernsworld.de
25899 Dagebüll · PGP Pub. KeyID: 0xA4357E78 · http://www.websitedev.de/

---

From: Tim Bray <[email-masked]>
Date: Sat, 16 Nov 2013 07:57:24 -0800

I'd worry a little bit that the lack of reaction is a symptom of a general
lack of interest. I'm not sure the document should be advanced if nobody
cares about it.
On Nov 15, 2013 5:00 PM, "Murray S. Kucherawy" <[email-masked]> wrote:

> Could we please get some volunteers to review and comment on it?  We
> really need to get it over to the IESG before we can accept new work; too
> many WG documents open at once has gotten us into trouble in the past.  The
> other option is to park it indefinitely, but we'd really rather not have to
> resort to that.
>
> Thanks,
> -MSK
>
>
>
>> On Tue, Sep 17, 2013 at 12:16 PM, Murray S. Kucherawy <
>> [email-masked]> wrote:
>>
>>> Please indicate in your reply whether you support publication in its
>>> current form, with your constructive comments accommodated, or if it needs
>>> more development.  A simple "+1" is not especially useful.  The more
>>> detailed your review comments are, the more useful they are.
>>>
> _______________________________________________
> apps-discuss mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/apps-discuss
>
>

---

From: Paul Hoffman <[email-masked]>
Date: Sat, 16 Nov 2013 08:24:36 -0800

Apologies for the late review. I know I put this on my calendar for a while ago, but it was lost.

This document seems mostly fine. I don't knowingly use HTTP PATCH, so I cannot say whether or not it works well within that context. From a strictly JSON point of view, the rules seem sensible other than the following:

   Because null values carry specific meaning within a merge patch
   document, JSON Arrays contained within the merge patch document
   SHOULD NOT contain null members.  Likewise, object members within the
   array SHOULD NOT contain any members with null values.  If null
   members are contained anywhere within the Array, those MUST be
   ignored and handled as if the null member were not specified at all.
   The null values MUST NOT appear within the modified result.  For
   instance, within the merge patch document, the array value ["a",
   null, "b"] is equivalent to ["a", "b"] and the array value ["a",
   {"z":1, "y":null},"b"] is equivalent to ["a",{"z":1},"b"].

Those SHOULD NOTs are horrible and will lead to lack of interop for no good reason. It would be much simpler to say:

   Because null values carry specific meaning within a merge patch
   document, JSON Arrays contained within the merge patch document
   SHOULD NOT contain null members, even within array or object
   members within the array.

This matches the prohibition earlier in the document. Yes, it further limits what you can patch to, but it certainly also makes it more reliable. If this change is made to the document, at least one of the examples needs to be changed as well.

--Paul Hoffman

---

From: Ned Freed <[email-masked]>
Date: Sat, 16 Nov 2013 13:21:28 -0800 (PST)

> should be given to the format, not some identifier string.

I agree. In this case the media type registration is incidental to the format
specification.

				Ned

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Sun, 17 Nov 2013 22:29:05 -0800

> lack of interest. I'm not sure the document should be advanced if nobody

Right, if it stays parked for a protracted period, we would probably just
mark it "Dead" (from a WG perspective).

-MSK

---

From: James M Snell <[email-masked]>
Date: Sun, 17 Nov 2013 22:41:22 -0800

There are existing implementers of the current spec.  If the wg doesn't
move it forward,  I'll resubmit as an independent submission.
On Nov 17, 2013 10:29 PM, "Murray S. Kucherawy" <[email-masked]> wrote:

>> I'd worry a little bit that the lack of reaction is a symptom of a
>> general lack of interest. I'm not sure the document should be advanced if
>> nobody cares about it.
>>
>

---

From: Julian Reschke <[email-masked]>
Date: Mon, 18 Nov 2013 09:45:12 +0100

> move it forward,  I'll resubmit as an independent submission.

This spec is needed. I don't care particularly whether it'll be a WG 
draft or an independent submission.

Best regards, Julian

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Mon, 18 Nov 2013 02:13:20 -0800

> move it forward,  I'll resubmit as an independent submission.

...which is fine, of course, unless this needs Proposed Standard status,
which is what you're currently requesting.  The independent stream cannot
produce standards track documents.  So unless you'd be satisfied with
Informational status, it needs to go through a WG (though not necessarily
this one) or be sponsored by an Area Director; either way, there needs to
be consensus recorded to proceed.  If there are only a couple of people
willing to review it, demonstrating that will be an uphill battle.

I think getting some people to (finally!) post comments in here is probably
the simplest path forward.  I'm not familiar enough with this space to be
able to suggest whom you might ask specifically, however.

-MSK