Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stdlib: implement json merge #95

Merged
merged 1 commit into from
Jan 20, 2016
Merged

stdlib: implement json merge #95

merged 1 commit into from
Jan 20, 2016

Conversation

mikedanese
Copy link
Contributor

Adds a merge function to the stdlib as described in:

https://tools.ietf.org/html/rfc7386
https://tools.ietf.org/html/draft-ietf-appsawg-json-merge-patch-07

There's some wierdness around patching nulls. I couldn't figure out how to remove a node from the json object so some of the test cases have an extra null in the expected output which seems okay. I've left a comment where are expected differs from the spec's expected differs from our expected.

@sparkprime
Copy link
Collaborator

Try this on for size:

    merge(target, patch)::
        if std.type(patch) == "object" then 

            local target_fields =
                if std.type(target) == "object" then std.objectFields(target) else [];

            local null_fields = [k for k in std.objectFields(patch) if patch[k] == null];
            local both_fields = std.setUnion(target_fields, std.objectFields(patch));
            {
                [k]:
                    if !std.objectHas(patch, k) then
                        target[k]
                    else if !std.objectHas(target, k) then
                        patch[k]
                    else
                        util.merge(target[k], patch[k])
                for k in std.setDiff(both_fields, null_fields)
            }
        else
            patch

@sparkprime
Copy link
Collaborator

Also, the reason you have some failing test cases is because you've changed std.jsonnet and the line numbers in the "golden" stacktraces for unrelated tests no-longer align. To fix that, use refresh_golden.sh to rewrite the "golden" files with the new content (and sanity check with git diff).

@sparkprime
Copy link
Collaborator

Maybe this should be called mergePatch, and it really calls out for the diff equivalent as well, although not necessarily in this PR.

@sparkprime
Copy link
Collaborator

It looks like 7396 is the better RFC as it obsoletes 7386

@mikedanese
Copy link
Contributor Author

It looks like the pseudocode algorithm for applying a patch did not change between the three RFC revisions so the difference must be in the http parts.

Nice! this seems to work:

    merge(target, patch)::
        if std.type(patch) == "object" then 
            local target_object =
                if std.type(target) == "object" then target else {};

            local target_fields =
                if std.type(target_object) == "object" then std.objectFields(target_object) else [];

            local null_fields = [k for k in std.objectFields(patch) if patch[k] == null];
            local both_fields = std.setUnion(target_fields, std.objectFields(patch));

            {
                [k]:
                    if !std.objectHas(patch, k) then
                        target_object[k]
                    else if !std.objectHas(target_object, k) then
                        patch[k]
                    else
                        std.merge(target_object[k], patch[k]) tailstrict
                for k in std.setDiff(both_fields, null_fields)
            }
        else
            patch,

It fixes all extra nulls except for the one in the last test case.

@sparkprime
Copy link
Collaborator

I think it will work with this as the inner block:

                    if !std.objectHas(patch, k) then
                        target[k]
                    else if !std.objectHas(target, k) then
                        std.merge(null, patch[k])
                    else
                        std.merge(target[k], patch[k])

@mikedanese
Copy link
Contributor Author

I think that is equivalent since std.merge(null, patch[k]) evaluates to patch[k](always hits the else). The problem is with the else return patch branch. To conform to the spec we would have to walk the patch object and strip out all explictly null fields.

@sparkprime
Copy link
Collaborator

Nah std.merge(null, patch[k]) is recursive into patch[k]

std.merge(x, null) evaluates to null though

@mikedanese
Copy link
Contributor Author

Ok I gotcha. I've updated the patch.

@sparkprime
Copy link
Collaborator

Looks like error.equality_function.jsonnet.golden needs updating

@mikedanese mikedanese force-pushed the merge branch 2 times, most recently from 47c580b to 81642a8 Compare January 13, 2016 02:04
@@ -0,0 +1 @@
true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absence of a .golden file means it should expect it to output just true, so you can delete this file.

@mikedanese
Copy link
Contributor Author

@sparkprime addressed comments and build is all green

sparkprime added a commit that referenced this pull request Jan 20, 2016
stdlib: implement json merge
@sparkprime sparkprime merged commit f0f52f3 into google:master Jan 20, 2016
@mikedanese mikedanese deleted the merge branch January 20, 2016 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants