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

From: "Manger, James H" <[email-masked]>
Date: Sat, 5 Oct 2013 02:34:08 +1000

Comments on draft-ietf-appsawg-json-merge-patch-01:

1. Change the title.
From: "The application/json-merge-patch Media Type"
To  : "JSON Merge-Patch operation"
How the merge-patch operates is the most important aspect. The media type is a useful, but secondary, aspect.

2. Change the media type.
From: application/json-merge-patch
To  : application/merge-patch+json
The "+json" suffix indicates the content is JSON.

3. Keep only one initial example.
Section 1 "Introduction" has an example so section 2 doesn't need another. Don't mix an example and processing rules in the same section.

4. Define the operation as applying to a JSON target.
Page 4 talks about understanding the "target resource media type and the underlying data model of the target resource" as though this JSON Merge-Patch can apply to targets other than JSON. This is not worth mentioning; it only confuses. 
This spec should only define a JSON patch operating on a JSON target.

5. Terminology.
Describe the operation in terms of 'patch' and 'target' JSON values, not "the data provided in the payload" and "target resource".

6. Separate the logical operation from the HTTP PATCH request.
Section 2 can describe the rules for applying 'patch' (a JSON value) to 'target' (a JSON value) to produce 'result' (a JSON value).
Section 3 can describe how to use this with HTTP. That is, with PATCH as the HTTP method, application/merge-patch+json as the media type, 'patch' as the body, and 'target' is the resource.

7. Defines rules for 'patch' and 'target' being any JSON *value* (not just objects and arrays).

8. Don't hunt out nulls in arrays.
Treat any array in 'patch' as opaque. It is pointless to require a processor to walk through an array in 'patch' looking for nulls.
Remove the whole paragraph with "JSON Arrays contained within the merge patch document SHOULD NOT contain null members". The paragraph says nulls inside arrays "MUST be ignored", but requires they be hunted out and removed, which is hardly ignoring them.

9. Removing a non-existent member is not an error.
Change "SHOULD NOT" to "MUST NOT" in "the server SHOULD NOT consider the request to be in error". Better still, in the rules explicitly cover the case of a 'patch' member with value null and no corresponding member in 'target' (ie it is a no op).

10. Remove charset parameter.
Can't we say application/merge-patch+json identifies content that is a UTF-8 encoded JSON value, instead of allowing a charset parameter.
Actually, I guess we should do whatever is defined for "+json" media types.

11.Security Considerations.
The spec defines the changes. Remove the paragraph about the server determining the specific set of change operations to be applied.

12. More succinct example JavaScript implementation.
function merge(orig, patch) {
	var result;
	if (patch === null) {
		// leave result undefined
	}
	else if (typeof patch === 'string' ||
		typeof patch === 'number' ||
		typeof patch === 'boolean' ||
		Array.isArray(patch))
	{
		result = patch;
	}
	else { // patch is an object
		result = {};
		if (orig instanceof Object && !Array.isArray(orig)) {
			for (m in orig)
				result[m] = orig[m];
		}
		for (m in patch) {
			result[m] = merge(result[m], patch[m]);
		}
	}
	return result;
}

--
James Manger

From: [email-masked] [mailto:[email-masked]] On Behalf Of Murray S. Kucherawy
Sent: Wednesday, 18 September 2013 5:16 AM
To: IETF Apps Discuss
Subject: [apps-discuss] Working Group Last Call: draft-ietf-appsawg-json-merge-patch

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: "Murray S. Kucherawy" <[email-masked]>
Date: Fri, 4 Oct 2013 12:19:10 -0700

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