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

James M Snell <jasnell@gmail.com> Mon, 20 May 2013 23:54 UTC

Return-Path: <jasnell@gmail.com>
X-Original-To: apps-discuss@ietfa.amsl.com
Delivered-To: apps-discuss@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 2B80121F89A5 for <apps-discuss@ietfa.amsl.com>; Mon, 20 May 2013 16:54:40 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -3.561
X-Spam-Level:
X-Spam-Status: No, score=-3.561 tagged_above=-999 required=5 tests=[AWL=-5.462, BAYES_50=0.001, J_CHICKENPOX_54=0.6, MANGLED_FORM=2.3, RCVD_IN_DNSWL_LOW=-1]
Received: from mail.ietf.org ([12.22.58.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id vYSSdK5WRokB for <apps-discuss@ietfa.amsl.com>; Mon, 20 May 2013 16:54:36 -0700 (PDT)
Received: from mail-oa0-f45.google.com (mail-oa0-f45.google.com [209.85.219.45]) by ietfa.amsl.com (Postfix) with ESMTP id E30B421F88FB for <apps-discuss@ietf.org>; Mon, 20 May 2013 16:54:35 -0700 (PDT)
Received: by mail-oa0-f45.google.com with SMTP id j6so28897oag.4 for <apps-discuss@ietf.org>; Mon, 20 May 2013 16:54:35 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; bh=r4o/qxV3ImO3asx86hxNIh6YR1mgwj5uKlUE6oM9szA=; b=jnoZ3HHp1bRZPlLcPJP2AW57b6vgHA6QTGnr4vYI3Dbe3D0cuOBt0kL2oWKolHOrY2 kIm8UKzYh6XaVaRcA0CnzPdWGFw22ADR7t0y7kisUE9TFZ8jlj9rk7kMnbfSvq1EeZ7P K+OrqBr60E83frFBu9Pq7U9Qt09wXwZH1DOptDtafgbSUosH0vYvRGf8cP7iQOAEhfwl skgLAaFq/fMLB02t+VSXUTlmapwNqhMABymoJTSE+r6Al80Op9VzymNtxHyZr1C5at84 W95qnT0tfxlDa/x206h6iHD8uYX4anWuSPblcxhREPVk59YdMhphtF9EYVmg52iMH0Dz 3rug==
X-Received: by 10.182.246.198 with SMTP id xy6mr28372040obc.1.1369094075510; Mon, 20 May 2013 16:54:35 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.60.3.137 with HTTP; Mon, 20 May 2013 16:54:14 -0700 (PDT)
In-Reply-To: <255B9BB34FB7D647A506DC292726F6E1151A56EEE3@WSMSG3153V.srv.dir.telstra.com>
References: <CABL+ZB6AQGp8WUeH=HZrfm4VCQnmYFQB5vBGP0yghAZUhsjCQg@mail.gmail.com> <255B9BB34FB7D647A506DC292726F6E1151A34C59E@WSMSG3153V.srv.dir.telstra.com> <CABL+ZB6tkggqrdjPFz38j=AXGuueSbANzER24Njm-o3a2S=MZA@mail.gmail.com> <255B9BB34FB7D647A506DC292726F6E1151A56E41B@WSMSG3153V.srv.dir.telstra.com> <CABL+ZB6=X9vTwDZ9ORPWm9u7PNSaaisLvenSRifmjsO6BCOuxA@mail.gmail.com> <CABP7Rbc7VxOG3Y7zapNQ2oW=yO9=PitACzRLntzXXnQ1dj+04A@mail.gmail.com> <255B9BB34FB7D647A506DC292726F6E1151A56EEE3@WSMSG3153V.srv.dir.telstra.com>
From: James M Snell <jasnell@gmail.com>
Date: Mon, 20 May 2013 16:54:14 -0700
Message-ID: <CABP7RbcdVj43pKMj=vwJsrY1+YJpmWqDhD8KCd-zd-qDg3bcmA@mail.gmail.com>
To: "Manger, James H" <James.H.Manger@team.telstra.com>
Content-Type: text/plain; charset=UTF-8
Cc: "apps-discuss@ietf.org" <apps-discuss@ietf.org>
Subject: Re: [apps-discuss] draft-snell-merge-patch-08 in Ruby
X-BeenThere: apps-discuss@ietf.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: General discussion of application-layer protocols <apps-discuss.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/apps-discuss>, <mailto:apps-discuss-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/apps-discuss>
List-Post: <mailto:apps-discuss@ietf.org>
List-Help: <mailto:apps-discuss-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/apps-discuss>, <mailto:apps-discuss-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 20 May 2013 23:54:40 -0000

On Mon, May 20, 2013 at 4:50 PM, Manger, James H
<James.H.Manger@team.telstra.com> wrote:
>> Given James M's example cases, the results should be:
>>
>> > Document + patch = ?
>>
>> > [1,2]  +  {"a":"foo","b":null}
>>
>>   Result: {"a":"foo"}
>
> The JavaScript in draft-snell-merge-patch-08 incorrectly gives the result as [1,2].
>

Fixed in the todays updated draft.

>
>> > {"a":"foo"}  +  {"b":[3,null,{"x":null}]}
>>
>>   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.
>

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.

>
>> > {"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.
>
> 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.

>
>> If there is an error in the javascript impl in the draft, I'll make
>> 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
>

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

- James

> --
> James Manger