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 WG chair(s)
before posting a new version of the draft.

Document: draft-ietf-appsawg-json-merge-patch-02
Title: JSON Merge Patch
Reviewer: Carsten Bormann
Review Date: 2013-12-09
WG Last Call Date: WGLC ended October 4, 2013

Summary: This draft is NOT ready to progress now as a Proposed
Standard and should be revised before an IETF Last Call is issued.

MAJOR:

1) The language defining the operation of the patch mechanism is
unnecessarily confusing.  Several phrases are simply not accessible to
this reader.  E.g., "Any null member contained in the merge patch MUST
be ignored and treated as if those members are undefined."  (Members
in JSON are name/value pairs.  Only the values could be the null
value.  There is no undefined value in JSON.  A member can't be both
ignored and treated in a specific way.)

2) As the algorithm appears to be defined, there is never a reason for
a server to send arrays with null elements.  (In JSON, arrays have
elements, not members; I'm not sure I'm reading some of the text
right.)  The purge_nulls mechanism is therefore completely unneeded.
Why would a sender send nulls in arrays only to have them removed by
the receiver?

By removing this unnecessary processing, the description could be
simplified considerably and implementations could be more performant.
https://developers.google.com/blogger/docs/3.0/performance?hl=en#patch
which was cited as a real-world early inspiration for merge-patch does
not do any of this superfluous processing either.  Rationalizing the
existing algorithm means that an implementation only needs to descend
recursively through objects, never through other JSON values.  Only
this descent is justified, as it is the only productive function of
the merge algorithm.  (The fix also would allow to have nulls in
replacement arrays.)  The value »null« should only ever be special in
the value position of a name/value pair of an object.  The text then
would only need to distingish object and non-object values,
considerably simplifying it and increasing the chance of
interoperability.  Fixing this will also make the example code much
shorter and thus more accessible.

3) The text needs to be modified to take the recent changes to JSON
into account.  While 4627bis is not yet approved, it is likely that
the change to allow non-containers as JSON texts will stand.
Merge-patch would then immediately become obsolete once 4627bis is
approved.

4) The definition of a charset media type parameter is unacceptable
for a +json media type.  (Even if it were acceptable, its semantics
would need to be defined.)

MINOR:

5) Several of the examples contain weird instances of whitespace that
are inconsistent between the pre-patch and the post-patch state. This
is not just about appearance: It might make the reader wonder whether
the patch algorithm is also about changing/updating whitespace.
(Clearly, the algorithm is operating at the JSON data model level.)

6) The SHOULD NOTs at the end of section 2 are confusing, as they
already seem to be implied by processing required (at a MUST level)
further above.

NITS:

Section 2, item 2: s/or/and/

Grüße, Carsten

PS.: The appsdir review template doesn’t have a space for comments.
Let me just add at this point that I like what the draft is trying to
do and I strongly believe it will be a useful specification to have
available.