Subject: [apps-discuss] WGLC review: draft-ietf-appsawg-json-patch-06

From: Cyrus Daboo <[email-masked]>
Date: Thu, 15 Nov 2012 10:57:01 -0500

Hi,
Here are my review comments (mostly nits - nothing major):

§1: Introduction focusses on the use of JSON patch in HTTP PATCH, but of 
course it could be used anywhere else a "patch" operation is needed (other 
protocols". It might be worth adding some text explaining the more 
"generic" use case. e.g.:

    This format is also applicable to any protocol that might want to
    process partial updates to a JSON document.

§4.1 talks about the "-" value for arrays, but that value is also 
applicable to "move" and "copy". Should that statement also appear in §4.4 
and §4.5 or be moved to a more "generic" location - e.g. in the main text 
of §4?

§4.6 uses a very basic octet/code-point comparator. Do we care about 
possible unicode normalizations issues? If so that will need to be dealt 
with, if not there ought to be a clarifying statement - in fact an 
"Internationalization Considerations" section may be warranted.

§4.6 numbers - any concern about numeric precision affecting the 
"subtracting one from another" behavior?

§5 when an error occurs it would be handy to have some minimal form of 
error reporting - simply the index of the operation that failed. Can we 
define a JSON format for an HTTP PATCH error response?

§6 XML patch uses a MIME type "patch-ops-error+xml", why is this not 
"patch+json"?

§A.6 - missing comma between members "foo" and "qux" in the two document 
examples.

-- 
Cyrus Daboo

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 16 Nov 2012 10:35:39 +1100

Hi Cyrus,

Thanks for the review.

On 16/11/2012, at 2:57 AM, Cyrus Daboo <[email-masked]> wrote:

> §1: Introduction focusses on the use of JSON patch in HTTP PATCH, but of course it could be used anywhere else a "patch" operation is needed (other protocols". It might be worth adding some text explaining the more "generic" use case. e.g.:
> 

Will do.

> §4.1 talks about the "-" value for arrays, but that value is also applicable to "move" and "copy". Should that statement also appear in §4.4 and §4.5 or be moved to a more "generic" location - e.g. in the main text of §4?

Will have a look.

> §4.6 uses a very basic octet/code-point comparator. Do we care about possible unicode normalizations issues? If so that will need to be dealt with, if not there ought to be a clarifying statement - in fact an "Internationalization Considerations" section may be warranted.

Urgh. 

> §4.6 numbers - any concern about numeric precision affecting the "subtracting one from another" behavior?

mumble. Any suggestion for a better way to specify?

> §5 when an error occurs it would be handy to have some minimal form of error reporting - simply the index of the operation that failed. Can we define a JSON format for an HTTP PATCH error response?

Maybe one day... but not today :)

> §6 XML patch uses a MIME type "patch-ops-error+xml", why is this not "patch+json"?

I think the going answer is that this is not an effort to define a single patch format with multiple serialisations; it's very very specific to the JSON data model, so +json isn't warranted.

> §A.6 - missing comma between members "foo" and "qux" in the two document examples.

ack

Thanks again,

--
Mark Nottingham   http://www.mnot.net/

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 16 Nov 2012 19:32:30 +1100

>>  process partial updates to a JSON document.

In SVN

> §4.1 talks about the "-" value for arrays, but that value is also applicable to "move" and "copy". Should that statement also appear in §4.4 and §4.5 or be moved to a more "generic" location - e.g.…

Move and Copy are now defined in terms of add, so I think this is dealt with.

>> §A.6 - missing comma between members "foo" and "qux" in the two document examples.

In SVN

---

From: Cyrus Daboo <[email-masked]>
Date: Fri, 16 Nov 2012 10:55:48 -0500

Hi Mark,

>> "Internationalization Considerations" section may be warranted.

Advice from i18n "experts" on language would be good. My guess is it might 
be sufficient to allow the patch-processing entity to normalize the two 
strings being tested in some consistent manner for comparison, rather than 
require "code point" equality.

Note: I guess the JSON base spec also has a similar issue in that it talks 
about "names within an object SHOULD be unique" and names are strings - so 
the question of string comparison/normalization comes up there too.

> mumble. Any suggestion for a better way to specify?

"numerically equal" - anyone writing actual code to implement this test 
will use "a == b" rather than "a - b == 0". So how about:

      numbers: are considered equal if their values are numerically equal.

Another point, you have this for the "string" test:

strings: are considered equal if, after unescaping any sequence(s)
      in both strings starting with a reverse solidus, they contain the
      same number of Unicode characters and their code points are
      position-wise equal.

However, I believe the value of a string is the unescaped result of 
processing a string "representation" in the JSON document, so your text on 
unescaping is not needed, and arguably it could cause someone to 
incorrectly "double unescape" a string representation, if they don't pay 
close attention. So I would prefer the following:

strings: are considered equal if they contain the same number of
      Unicode characters and their code points are position-wise equal.

-- 
Cyrus Daboo

---

From: Andrew Sullivan <[email-masked]>
Date: Fri, 16 Nov 2012 11:27:16 -0500

On Fri, Nov 16, 2012 at 10:55:48AM -0500, Cyrus Daboo wrote:
> 
> Advice from i18n "experts" on language would be good. My guess is it
> might be sufficient to allow the patch-processing entity to
> normalize the two strings being tested in some consistent manner for
> comparison, rather than require "code point" equality.

The scare quotes around experts are entirely appropriate in my case.
But yes, normalization is probably a good idea.  What isn't clear to
me is what kind of comparison you want.  If you use NFKC, you maximize
matches, but the danger is that you'll match something you didn't want
to match.

The thing to read is probably http://www.unicode.org/reports/tr15/.

Best,
A

-- 
Andrew Sullivan
[email-masked]

---

From: James M Snell <[email-masked]>
Date: Fri, 16 Nov 2012 08:55:08 -0800

For a patch operation, I'm definitely not convinced that anything other
than codepoint comparison should be allowed. The motivation of the "test"
operation is to ensure that the state of the target resource is what we
expect it to be before or after making a change. Normalization could short
circuit such checks in various situations.

- James

On Fri, Nov 16, 2012 at 8:27 AM, Andrew Sullivan <[email-masked]>wrote:

> _______________________________________________
> apps-discuss mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/apps-discuss
>

---

From: Cyrus Daboo <[email-masked]>
Date: Fri, 16 Nov 2012 12:23:04 -0500

Hi James,

--On November 16, 2012 8:55:08 AM -0800 James M Snell <[email-masked]> 
wrote:

> expect it to be before or after making a change. Normalization could
> short circuit such checks in various situations.
>

If the input value to the test is going to come directly from what was in 
the original JSON document, then what you say seems fine (though I think a 
statement along those lines should be made somewhere to clarify that 
point). However, isn't it possible that a test might be used to see if a 
document has a specific new value present as opposed to the original value?

-- 
Cyrus Daboo

---

From: James M Snell <[email-masked]>
Date: Fri, 16 Nov 2012 09:48:21 -0800

Yes, that's reasonable of course... but even in that case a codepoint based
comparison is still the most reliable.

---

From: John C Klensin <[email-masked]>
Date: Fri, 16 Nov 2012 13:52:10 -0500

--On Friday, November 16, 2012 11:27 -0500 Andrew Sullivan
<[email-masked]> wrote:

>> Advice from i18n "experts" on language would be good. My
>> guess is it might be sufficient to allow the patch-processing
>> entity to normalize the two strings being tested in some
>> consistent manner for comparison, rather than require "code
>> point" equality.
> 
> The scare quotes around experts are entirely appropriate in my
> case. But yes, normalization is probably a good idea.  What
> isn't clear to me is what kind of comparison you want.  If you
> use NFKC, you maximize matches, but the danger is that you'll
> match something you didn't want to match.
> 

And draft-iab-identifier-comparison

  john

---

From: Paul Hoffman <[email-masked]>
Date: Fri, 16 Nov 2012 11:03:15 -0800

On Nov 16, 2012, at 9:48 AM, James M Snell <[email-masked]> wrote:

> Yes, that's reasonable of course... but even in that case a codepoint based comparison is still the most reliable. 

+1. If a system that is doing a patch wants fuzzy comparison, it can build that in itself. If we specify exactly one fuzzy comparison, it will only satisfy part of the market. If we specify a fuzzy comparison framework with one worked out example of normalization, we descend into madness.

i18n is not *needed* for JSON patching; it would be useful in some limited cases.

--Paul Hoffman

---

From: Julian Reschke <[email-masked]>
Date: Sat, 17 Nov 2012 12:53:38 +0100

On 2012-11-16 17:55, James M Snell wrote:
> than codepoint comparison should be allowed. The motivation of the
 > ...

+1000

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Sat, 17 Nov 2012 09:21:44 -0800

This may be an ignorant question, but is this really something about which
JSON pointer/patch needs to worry?  It seems to me that this is a property
of JSON, so any semantics about key equality that exist are defined in
there, and we shouldn't be adding any here.

-MSK

On Sat, Nov 17, 2012 at 3:53 AM, Julian Reschke <[email-masked]>wrote:

> ______________________________**_________________
> https://www.ietf.org/mailman/**listinfo/apps-discuss<https://www.ietf.org/mailman/listinfo/apps-discuss>
>

---

From: Paul Hoffman <[email-masked]>
Date: Sat, 17 Nov 2012 10:18:57 -0800

On Nov 17, 2012, at 9:21 AM, Murray S. Kucherawy <[email-masked]> wrote:

> This may be an ignorant question, but is this really something about which JSON pointer/patch needs to worry?  It seems to me that this is a property of JSON, so any semantics about key equality that exist are defined in there, and we shouldn't be adding any here.

Blame Cyrus. :-) So far, it seems like most people don't want this in the document.

--Paul Hoffman

---

From: Cyrus Daboo <[email-masked]>
Date: Sat, 17 Nov 2012 21:41:00 -0500

Hi Murray,

--On November 17, 2012 9:21:44 AM -0800 "Murray S. Kucherawy" 
<[email-masked]> wrote:

> This may be an ignorant question, but is this really something about
> which JSON pointer/patch needs to worry?  It seems to me that this is a
> property of JSON, so any semantics about key equality that exist are
> defined in there, and we shouldn't be adding any here.

No we are not talking about the keys (object member names) but rather 
actual string values which can come from any arbitrary source with its own 
choice of unicode normalization.

Actually the base spec 4627 says nothing about string comparison or 
normalization as it applies to either string values or "key" names. Just a 
statement of fact - no need to act...

-- 
Cyrus Daboo

---

From: Cyrus Daboo <[email-masked]>
Date: Sat, 17 Nov 2012 22:27:57 -0500

Hi Paul,

> Blame Cyrus. :-) So far, it seems like most people don't want this in the

Occasionally it is fun to be a trouble maker :-)

Anyway, my position is this: I am OK with the way the current comparison is 
defined in the spec. However, I am concerned that that an implementor may 
fail to appreciate that normalization could be a problem (and really it 
could).

As I suggested before, all I am really after is some text that clarifies 
the current state - namely that implementations should make sure no 
normalization is done on string values from the original document when 
re-used in a test. But if no one thinks that adds anything - then we can 
leave it be.

-- 
Cyrus Daboo

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 19 Nov 2012 18:01:30 +1100

On 17/11/2012, at 2:55 AM, Cyrus Daboo <[email-masked]> wrote:

> Hi Mark,
> 
> --On November 16, 2012 10:35:39 AM +1100 Mark Nottingham <[email-masked]> wrote:
> 
> Advice from i18n "experts" on language would be good. My guess is it might be sufficient to allow the patch-processing entity to normalize the two strings being tested in some consistent manner for comparison, rather than require "code point" equality.
> 
> Note: I guess the JSON base spec also has a similar issue in that it talks about "names within an object SHOULD be unique" and names are strings - so the question of string comparison/normalization comes up there too.

It sounds like we want to leave this one alone -- make sense?

I could see adding a note of some sort warning people that canonicalisation isn't done...

> "numerically equal" - anyone writing actual code to implement this test will use "a == b" rather than "a - b == 0". So how about:
> 

in SVN

> However, I believe the value of a string is the unescaped result of processing a string "representation" in the JSON document, so your text on unescaping is not needed, and arguably it could cause someone to incorrectly "double unescape" a string representation, if they don't pay close attention. So I would prefer the following:
> 

in SVN. I think we have a bit more cleaning up to do WRT Henry's comments, to make it clear what we're operating upon.

Cheers and thanks again,