Subject: [apps-discuss] draft-snell-merge-patch-08 in Ruby

From: Steve Klabnik <[email-masked]>
Date: Sat, 18 May 2013 12:58:09 -0700

Hey there!

James suggested that I post to this list, so here I am.

I'm a committer to Ruby on Rails, and Rails 4 is near a release. One
of the changes in Rails 4 is the usage of PATCH, which means I'm
checking up on various diff media types. Mark Nottingham brought my
attention to draft-snell-merge-patch as a simpler variant of
json-patch.

Anyway, I have a _mostly_ working implementation of the latest draft
as a gem, here: https://github.com/steveklabnik/json-merge_patch .
It's only missing the "Recursively apply rule 2" portion. Hopefully
I'll be finishing it off soon.

I'd say that overall this draft was super simple to implement,
especially given Ruby's (like many scripting languages) ease of
mapping JSON directly to its rich Hash and Array types in the language
core.

I may have one or two thoughts on the language of the spec, but I need
to re-read it and take some notes this time around.

- Steve

---

From: "Manger, James H" <[email-masked]>
Date: Mon, 20 May 2013 10:14:49 +1000

Steve,

It is good to hear about a Ruby json-merge-patch. There are a couple of cases where I think draft-snell-merge-patch gets it wrong, example of which are tabulated at http://id.cto.telstra.com/json/merge.html. I would be interested to know what your implementation does in these cases?

Document + patch = ?

[1,2]  +  {"a":"foo","b":null}

{"a":"foo"}  +  {"b":[3,null,{"x":null}]}

{"a":"foo"}  +  null

--
James Manger

> ----------
> From: [email-masked] [mailto:apps-discuss-
> [email-masked]] On Behalf Of Steve Klabnik
> Sent: Sunday, 19 May 2013 5:58 AM
> 
> Hey there!
>

---

From: Steve Klabnik <[email-masked]>
Date: Sun, 19 May 2013 18:34:25 -0700

As of the 1.1 release I did:

irb(main):001:0> require 'json/merge_patch'
=> true
irb(main):002:0> JSON.merge(%q'[1,2]', %q'{"a":"foo","b":null}')
=> "{\"a\":\"foo\"}"
irb(main):003:0> JSON.merge(%q'{"a":"foo"}', %q'{"b":[3,null,{"x":null}]}')
=> "{\"a\":\"foo\",\"b\":[3,{\"x\":null}]}"
irb(main):004:0> JSON.merge(%q'{"a":"foo"}', %q'null')
JSON::MergeError: JSON::MergeError
    from /Users/steve/.gem/ruby/2.0.0/gems/json-merge_patch-1.1.0/lib/json/merge_patch.rb:20:in
`rescue in merge'
    from /Users/steve/.gem/ruby/2.0.0/gems/json-merge_patch-1.1.0/lib/json/merge_patch.rb:16:in
`merge'
    from (irb):4
    from /opt/rubies/2.0.0-p0/bin/irb:12:in `<main>'

'null' isn't valid JSON, right? That'd account for your second discrepancy.

You can play around with the 1.1 version here, if you don't want to
bother getting Ruby installed: http://json-merge-patch.herokuapp.com/

You can see the translation I did directly to Ruby in this commit:
https://github.com/steveklabnik/json-merge_patch/commit/e70f762375b2928ca86c269354563da58af1e005

I've since done some refactoring.

The initial direct translation failed some of my unit tests. I think
that this was one of the bigger issues:

       else if (patch instanceof Array)
         orig = purge_nulls(patch);
       else if (is_primitive(patch))
         orig = patch;

Reading the spec:

 If either the root of the JSON data provided in the payload or
       the root of the target resource are JSON Arrays, the target
       resource is to be replaced, in whole, by the provided data.

Therefore, I think it should be this:

       else if (patch instanceof Array || orig instanceof Array)
         orig = purge_nulls(patch);
       else if (is_primitive(patch) or is_primitive(orig))
         orig = patch;

I believe this accounts for your first discrepancy.

There's also a few lines that I'm not sure are reachable. Check the
git history for more details.

---

From: "Manger, James H" <[email-masked]>
Date: Mon, 20 May 2013 13:26:35 +1000

> 'null' isn't valid JSON, right? That'd account for your second

'null' is a valid JSON value.
JavaScript will happily accept JSON.parse('null').
RFC4627 "JSON" limits application/json content to be an object or array, but my guess is that that will be relaxed to allow any JSON value shortly.

> bother getting Ruby installed: http://json-merge-patch.herokuapp.com/

Presumably the idea of this site was to return the result of the merge-patch. At the moment it is just echoing the patch.

> https://github.com/steveklabnik/json-merge_patch/blob/master/lib/json/merge_patch.rb

The latest changes to merge_patch.rb don't look right.

Why both removing array elements that are null (obj.compact)? Arrays in merge-patch are effectively primitive (like true, false, number, string). There is no reason for the merge functionality to look inside an array.

A name:null element *in a patch* mean delete the name field in the document. Such a field *in a document*, however, is fine. You shouldn't delete a field from a document that isn't mentioned in the patch -- regardless of whether or not the document field's value is null.

--
James Manger

---

From: Steve Klabnik <[email-masked]>
Date: Sun, 19 May 2013 21:00:14 -0700

> 'null' is a valid JSON value.

Bummer:

(main):001:0> require 'json'
=> true
irb(main):002:0> JSON.parse("null")
JSON::ParserError: 757: unexpected token at 'null'
    from /Users/steve/.gem/ruby/2.0.0/gems/json-1.8.0/lib/json/common.rb:155:in
`parse'
    from /Users/steve/.gem/ruby/2.0.0/gems/json-1.8.0/lib/json/common.rb:155:in
`parse'
    from (irb):2
    from /opt/rubies/2.0.0-p0/bin/irb:12:in `<main>'

> RFC4627 "JSON" limits application/json content to be an object or array, but my guess is that that will be relaxed to allow any JSON value shortly.

Ahhh, there we go. So it's not a valid JSON value. ;)

> Presumably the idea of this site was to return the result of the merge-patch. At the moment it is just echoing the patch.

Uh oh, that's a bummer. I'll look into that.

> Why both removing array elements that are null (obj.compact)?

I took that to be what this meant:

       Any
       object member contained in the provided data whose value is
       explicitly null is to be treated as if the member was undefined.

Since 'null' means 'remove' as the value of a key, I just assumed all
nulls should be removed. I did find this sentence (which is
everywhere) a tad bit confusing.

This could be me mis-reading things.

---

From: James M Snell <[email-masked]>
Date: Mon, 20 May 2013 09:38:05 -0700

I may need to take another pass at explaining this in the draft.
Essentially the idea is this: In the PATCH document, a null always
means that the key or element must not appear in the final result.

> [1,2]  +  {"a":"foo","b":null}

  Result: {"a":"foo"}

> {"a":"foo"}  +  {"b":[3,null,{"x":null}]}

  Result: {"a":"foo", "b":[3,{}]}

> {"a":"foo"}  +  null

  Result: Invalid. The patch document MUST be a valid JSON document,
which means the root must be either a JSON object or array. A null
patch document is invalid.

If there is an error in the javascript impl in the draft, I'll make
sure that gets corrected in the next iteration.

- James

On Sun, May 19, 2013 at 9:00 PM, Steve Klabnik <[email-masked]> wrote:
[snip]
> _______________________________________________
> apps-discuss mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/apps-discuss

---

From: Steve Klabnik <[email-masked]>
Date: Mon, 20 May 2013 09:40:45 -0700

James, it'd be nice if we could come up with something like
https://github.com/json-patch/json-patch-tests with lots of examples.
What do you think?

---

From: James M Snell <[email-masked]>
Date: Mon, 20 May 2013 09:42:01 -0700

Works for me.

---

From: "Manger, James H" <[email-masked]>
Date: Tue, 21 May 2013 09:50:02 +1000

> > [1,2]  +  {"a":"foo","b":null}

The JavaScript in draft-snell-merge-patch-08 incorrectly gives the result as [1,2].

>   Result: {"a":"foo", "b":[3,{}]}

NO. The result should be: {"a":"foo", "b":[3,null,{"x":null}]}.
Merge-patch does NOT merge arrays; it treats them like other primitives (strings, numbers, booleans) so a merge-patch implementation should NOT be looking inside an array for nulls.

The JavaScript in draft-snell-merge-patch-08 gives neither of these results. It incorrectly gives {"a":"foo", "b":[3,{"x":null}]}

> patch document is invalid.

I thought the one non-controversial change planned for RFC4627 "JSON" was to allow any JSON value (not just arrays and objects) as a JSON-text.

A patch of null certainly should not return the original unchanged (as the JavaScript in draft-snell-merge-patch-08 does). Either throw an error, or return an undefined value.

> sure that gets corrected in the next iteration.

My suggested 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;
}

See it in action at http://id.cto.telstra.com/json/merge.html

--
James Manger

---

From: James M Snell <[email-masked]>
Date: Mon, 20 May 2013 16:54:14 -0700

> The JavaScript in draft-snell-merge-patch-08 incorrectly gives the result as [1,2].

Fixed in the todays updated draft.

> Merge-patch does NOT merge arrays; it treats them like other primitives (strings, numbers, booleans) so a merge-patch implementation should NOT be looking inside an array for nulls.

No, the correct result is that nulls are treated as if they are
undefined and not present, They are to be removed from the result.

> The JavaScript in draft-snell-merge-patch-08 gives neither of these results. It incorrectly gives {"a":"foo", "b":[3,{"x":null}]}

Fixed in todays updated draft.

> I thought the one non-controversial change planned for RFC4627 "JSON" was to allow any JSON value (not just arrays and objects) as a JSON-text.

The draft is based on the current definition of RFC4627 and not on any
"planned" changes that might happen in the future.

> A patch of null certainly should not return the original unchanged (as the JavaScript in draft-snell-merge-patch-08 does). Either throw an error, or return an undefined value.

Agree. An error would be the most appropriate outcome.

> See it in action at http://id.cto.telstra.com/json/merge.html

I will take a more in depth look at this a bit later tonight or tomorrow.

- James