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

Steve Klabnik <steve@steveklabnik.com> Mon, 20 May 2013 01:34 UTC

Return-Path: <steve@steveklabnik.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 ACB3921F8EA6 for <apps-discuss@ietfa.amsl.com>; Sun, 19 May 2013 18:34:27 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.978
X-Spam-Level:
X-Spam-Status: No, score=-1.978 tagged_above=-999 required=5 tests=[BAYES_00=-2.599, FM_FORGED_GMAIL=0.622, NO_RELAYS=-0.001]
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 WP823Tc0EOlk for <apps-discuss@ietfa.amsl.com>; Sun, 19 May 2013 18:34:26 -0700 (PDT)
Received: from mail-ia0-x229.google.com (mail-ia0-x229.google.com [IPv6:2607:f8b0:4001:c02::229]) by ietfa.amsl.com (Postfix) with ESMTP id F2E5C21F8EA4 for <apps-discuss@ietf.org>; Sun, 19 May 2013 18:34:25 -0700 (PDT)
Received: by mail-ia0-f169.google.com with SMTP id k38so7033019iah.14 for <apps-discuss@ietf.org>; Sun, 19 May 2013 18:34:25 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type:x-gm-message-state; bh=vB4lLm3HdgBOc9Fa+0wss9wWmUvnbiu7j2+I6VKt38E=; b=Lxrd1XwCvO6neYPUlSTUMMXjxrDRJQHpXLN0+PkX5gz/8gfiq0XupCK96EwFEzehtc tGNKH0qq5YdJUKnFKWYLOHV/doFk60inTQAEbbwOjWN5KVzPPSlFc6g85XcC7IZJmJVT k1W1jNlmlPCLxgU5h5+tElj2mg28NeOPE8HnwV+ITiWreeUpPl0FecixE5LzJEwbaxMN AP5rfVXpM0TGrp8pgRCNWwjFjjDbF+75RJHimWd428863VzlDZpbUz39FkadJSprfAgD DymzH9HqNOrIV4L6OSP9tQEYazvM7W+u0cPrUmvkwEAsLbbZL1HmxoWtB0Ch3pjEorlQ V2Ag==
MIME-Version: 1.0
X-Received: by 10.50.108.17 with SMTP id hg17mr3950949igb.57.1369013665318; Sun, 19 May 2013 18:34:25 -0700 (PDT)
Received: by 10.64.163.3 with HTTP; Sun, 19 May 2013 18:34:25 -0700 (PDT)
In-Reply-To: <255B9BB34FB7D647A506DC292726F6E1151A34C59E@WSMSG3153V.srv.dir.telstra.com>
References: <CABL+ZB6AQGp8WUeH=HZrfm4VCQnmYFQB5vBGP0yghAZUhsjCQg@mail.gmail.com> <255B9BB34FB7D647A506DC292726F6E1151A34C59E@WSMSG3153V.srv.dir.telstra.com>
Date: Sun, 19 May 2013 18:34:25 -0700
Message-ID: <CABL+ZB6tkggqrdjPFz38j=AXGuueSbANzER24Njm-o3a2S=MZA@mail.gmail.com>
From: Steve Klabnik <steve@steveklabnik.com>
To: "Manger, James H" <James.H.Manger@team.telstra.com>
Content-Type: text/plain; charset=ISO-8859-1
X-Gm-Message-State: ALoCoQnVuRRy3fivhiYgjkFJZTlEvjc+dKDmvSEg7ZLM5SZ0IJquDjas+Za6l9WkEEPTwMXpz9jG
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 01:34:27 -0000

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.