Subject: [apps-discuss] Feedback on draft-ietf-appsawg-json-patch-00

From: Mike Acar <[email-masked]>
Date: Thu, 1 Mar 2012 12:11:23 -0800

(Resending, because my original submission seems to have been lost...)

Hi,

I've been asked to review the JSON Pointer and Patch draft RFCs. I've 
had some discussion about them off-list, and have been advised to bring 
my issues to the list. And so, I am. :)

I've scanned the archived list discussion of the patch and pointer RFCs, 
so I don't think these issues have come up here already. Let me know if 
they have.

My first concern with Patch is the test operation; 
http://tools.ietf.org/html/draft-ietf-appsawg-json-patch-00 sec 4.5 
defines test:

    4.5.  test

    The "test" operation tests that a value at the specified location in
    the target document is equal to a specified value.  The operation
    object contains a "value" member, which specifies the value to test
    for.

What constitutes equality when the values in question can be trees of 
objects and arrays? Can this even be usefully defined in general?

My next concern is with locations. Currently, the locations point to the 
value to operate upon; in the case of "add", this pointer is to a value 
which does not yet exist. This conflicts with the definition of a JSON 
Pointer, which says:

    If a reference token is being evaluated against a concrete JSON
    document, the implementation MAY evaluate each token against a
    concrete value, and terminate evaluation with an error condition if a
    evaluation fails to resolve a concrete value.

(section 4, http://tools.ietf.org/html/draft-ietf-appsawg-json-pointer-00)

There's no mention of what to do if the implementation does not return 
an error, leaving the behavior essentially undefined. So to avoid 
depending upon undefined behavior, an implementation of Patch would have 
to parse the Pointer, remove the "target" value from the end, and then 
traverse the JSON objects to find the parent of the target.

I think the Patch location should always point to the parent object or 
array. The operation object can then define the object name or array 
index to operate upon, along with any new value to be added or replaced, 
or location to be moved to, etc. So for example,

	{ "hork":"gak" }

The patch

	[ { "add":"/", "name":"foo", "value":"bar" } ]

gives you { "hork":"gak", "foo":"bar" }

I'm not wedded to this syntax, which is a bit more verbose. But I think 
the issue is significant: as they stand, Patch is specified to depend 
upon undefined behavior in Pointer.

As a stylistic matter, I also think separating operation and location 
into different members would be easier for people to read and programs 
to parse; e.g.

{ "op": "add", "loc":"/", "name":"foo", "value":"bar" }

Just looking at the "op" member is easier than testing each of 
add/remove/test/etc in turn. But that's a matter of taste, and it is 
slightly more verbose.

There's a minor issue with move; section 4.4 says

    This operation is functionally identical to expressing a "remove"
    operation, followed immediately by an "add" operation at the new
    location with the value that was just removed.

    If the location in the "to" member references a member of an existing
    object in the target document, it is an error condition if a value at
    the specified location already exists.

So what about moving an location to itself? If move is "functionally 
identical" to remove/add, then moving /a/b/c to /a/b/c is legal. On the 
other hand, an implementation might check the existence of "to" before 
performing the "move", and then throw an error.

I'm ok with just defining moving a location to itself to be a no-op.

I think those were my practical concerns. I have some feedback on 
Pointer as well, which I'll send in another thread.

-- 
Mike Acar -                                 - macar at cloudmark dot com

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Thu, 1 Mar 2012 20:18:59 +0000

One other thing that came up in conversation is that the json-patch document needs to make a reference (probably normative) to the json-pointer document.

-MSK

---

From: Mike Acar <[email-masked]>
Date: Thu, 1 Mar 2012 12:22:30 -0800

On 03/01/2012 12:18 PM, Murray S. Kucherawy wrote:
> One other thing that came up in conversation is that the json-patch
> document needs to make a reference (probably normative) to the
> json-pointer document.

It does; 
http://tools.ietf.org/html/draft-ietf-appsawg-json-patch-00#section-9.1 :

    9.1.  Normative References

    [JSON Pointer]
               Bryan, P. and K. Zyp, "JSON Pointer", January 2012, <http:
               //tools.ietf.org/html/draft-ietf-appsawg-json-pointer-00>.

---

From: Julian Reschke <[email-masked]>
Date: Thu, 01 Mar 2012 21:32:52 +0100

> //tools.ietf.org/html/draft-ietf-appsawg-json-pointer-00>.

Nit: that reference should actually use the standard ID reference format...

Best regards, Julian

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Thu, 1 Mar 2012 20:43:34 +0000

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On Behalf Of Mike Acar
> Sent: Thursday, March 01, 2012 12:23 PM
> To: [email-masked]
> Subject: Re: [apps-discuss] Feedback on draft-ietf-appsawg-json-patch-00
> 
> http://tools.ietf.org/html/draft-ietf-appsawg-json-patch-00#section-9.1:
> 

Dunno how I missed that.  Nevermind me (though Julian's right, the format needs fixing).

-MSK

---

From: "Paul C. Bryan" <[email-masked]>
Date: Thu, 01 Mar 2012 13:41:27 -0800

> objects and arrays? Can this even be usefully defined in general?

The value within the JSON document should be equal to the value in the
JSON Patch document; if these values contain objects or arrays, they
must be equal (i.e. same order of elements, with the same values).

> (section 4, http://tools.ietf.org/html/draft-ietf-appsawg-json-pointer-00)

An earlier draft of JSON Patch actually had separate parent node and
child to add, as you demonstrate below, as well as calling out a
specific "op" member. It received a number of objections, on the grounds
of being too verbose. I changed the add operation to add the node
specified in the pointer, and changed the language in JSON Pointer
accordingly to allow for this (i.e. the MAY language instead of
MUST/SHOULD). This language is admittedly still a bit awkward.

> traverse the JSON objects to find the parent of the target.

These specifications do very little to define how to handle errors
beyond terminating processing and indicating an error condition.

> I'm ok with just defining moving a location to itself to be a no-op.

Since it is functionally identical to removing then re-adding, then
checking for the presence of the value before removing would not be
functionally identical. If adding the no-op language makes this clearer,
I'd be fine adding it.

Paul

---

From: "Paul C. Bryan" <[email-masked]>
Date: Thu, 01 Mar 2012 13:47:12 -0800

> Nit: that reference should actually use the standard ID reference format...

Is this just removing the space from the reference, or are there other
considerations?

Paul

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Thu, 1 Mar 2012 22:00:11 +0000

Here’s an example:

   [I-D.IETF-MARF-AUTHFAILURE-REPORT]

              Fontana, H., "Authentication Failure Reporting using the

              Abuse Report Format",

              I-D draft-ietf-marf-authfailure-report<http://tools.ietf.org/html/draft-ietf-marf-authfailure-report>, January 2012.

From: [email-masked] [mailto:[email-masked]] On Behalf Of Paul C. Bryan
Sent: Thursday, March 01, 2012 1:47 PM
To: [email-masked]
Subject: Re: [apps-discuss] Feedback on draft-ietf-appsawg-json-patch-00

Is this just removing the space from the reference, or are there other considerations?

Paul

---

From: Julian Reschke <[email-masked]>
Date: Thu, 01 Mar 2012 23:00:33 +0100

> Is this just removing the space from the reference, or are there other

The name of the reference is a separate issue (names in refs do not 
contain whitespace).

The issue I was referring to is that the draft should be cited as 
Internet Draft, such as

	Bryan, P. and K. Zyp, "JSON Pointer", Internet-Draft
	draft-ietf-appsawg-json-pointer-00 (work in progress),
	January 2012.

Best regards, Julian

---

From: Julian Reschke <[email-masked]>
Date: Thu, 01 Mar 2012 23:07:46 +0100

On 2012-03-01 23:00, Murray S. Kucherawy wrote:
>                             Fontana, H.,"Authentication Failure Reporting using the
>
>                             Abuse Report Format",
>
>                             I-Ddraft-ietf-marf-authfailure-report  <http://tools.ietf.org/html/draft-ietf-marf-authfailure-report>, January 2012.

Actually that's bad example.

It doesn't use "Internet-Draft" as seriesInfo, and thus doesn't produce 
the "work in progress" label.

(yes, I'm nitpicking here)

Best regards, Julian

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Thu, 1 Mar 2012 22:09:00 +0000

> From: Julian Reschke [mailto:[email-masked]]
> Sent: Thursday, March 01, 2012 2:08 PM
> To: Murray S. Kucherawy
> Cc: [email-masked]

Is that seriesInfo the thing that makes "(work in progress)" appear?

> (yes, I'm nitpicking here)

Indeed.  :-)

---

From: Julian Reschke <[email-masked]>
Date: Thu, 01 Mar 2012 23:16:29 +0100

> Is that seriesInfo the thing that makes "(work in progress)" appear?

Yes.

>> (yes, I'm nitpicking here)

Sorry for that :-)

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Fri, 2 Mar 2012 21:12:00 +0000

> The value within the JSON document should be equal to the value in the JSON Patch document; if these values
> contain objects or arrays, they must be equal (i.e. same order of elements, with the same values).

I think that needs to be explicit.

> These specifications do very little to define how to handle errors beyond terminating processing and
> indicating an error condition. 

Shouldn't they?

An unrelated point about the same draft:

In your IANA Considerations, I don't think APPSAWG should be your change controller, because there's no guarantee this working group will always exist.  It should be the IESG, the IETF, a Designated Expert, or suchlike.  Also, your Published Specification should just be "[this memo]"; the RFC Editor will replace it.

-MSK, your friendly neighbourhood document shepherd

---

From: Mike Acar <[email-masked]>
Date: Fri, 2 Mar 2012 14:15:42 -0800

> must be equal (i.e. same order of elements, with the same values).

"Same order" applies only to arrays, right?

>> My next concern is with locations.

> of being too verbose.

Heh. Sorry, I didn't see that discussion in the list archive.

> I changed the add operation to add the node specified in the pointer,
> and changed the language in JSON Pointer accordingly to allow for
> this (i.e. the MAY language instead of MUST/SHOULD). This language is
> admittedly still a bit awkward.

The two definitions still yield the problem that a Patch implementation 
has to parse the Pointer, or implement its own Pointer traversal with a 
parent reference. I think that's worse than verbosity. Maybe something like

   { "at" : "/a/b", "add" : "c", "value" : "foo" }
   { "at" : "/a/b", "remove" : "c" }

would be terse enough?

>> an error, leaving the behavior essentially undefined.

Implicitly undefined, that is.

>> So to avoid depending upon undefined behavior, an implementation of
>> Patch would have to parse the Pointer, remove the "target" value
>> from the end, and then traverse the JSON objects to find the parent
>> of the target.
>

I don't think Pointer can even say this MAY be an error: that's one of 
the semantics an application assigns to Pointers (cf 
http://www.ietf.org/mail-archive/web/apps-discuss/current/msg04411.html).

For Patch, in my opinion it MUST be an error if a Pointer reference 
token can't be found in an object or array - but the consequence of that 
is that pointers have to point to parents.

>> So what about moving an location to itself?

> functionally identical. If adding the no-op language makes this
> clearer, I'd be fine adding it.

I think explicitly saying "checking for the presence of the value before 
removing would not be functionally identical" is a bit better, since it 
shows the special case isn't actually special. But either's fine.

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Fri, 2 Mar 2012 22:50:43 +0000

> Sent: Friday, March 02, 2012 2:16 PM
> >> Patch would have to parse the Pointer, remove the "target" value from
> >> the end, and then traverse the JSON objects to find the parent of the
> >> target.
> >
> token can't be found in an object or array - but the consequence of
> that is that pointers have to point to parents.

+1.  This points (hah) back to the pointer document thread: That document can leave handling of failed pointer resolution explicitly undefined, but the application has to say what semantics are to be applied in such cases.  For the patch document, it MUST be an error.

-MSK