josephburnett / jd

JSON diff and patch
MIT License
1.89k stars 52 forks source link

Null becomes empty object #20

Closed kevburnsjr closed 4 years ago

kevburnsjr commented 4 years ago

Why are nulls represented as empty objects in diffs? https://play.golang.org/p/1i9R5Xek5Ea

package main

import (
    "fmt"

    "github.com/josephburnett/jd/lib"
)

func main() {
    a, _ := jd.ReadJsonString(`{"foo":"bar"}`)
    b, _ := jd.ReadJsonString(`{"foo":null}`)
    diff := a.Diff(b)
    fmt.Println(diff.Render())
    p, _ := a.Patch(diff)
    fmt.Println(p.Json())
}
@ ["foo"]
- "bar"
+ {}

{"foo":{}}

This seems technically incorrect since null is a value type in JSON.

I'm trying to unmarshal a json node from a diff element into a struct and it's not working because the diff changes the type from null to empty object.

For now I've worked around it using omitempty, but I've got to think there are cases where preserving null in a diff/patch situation is important. The presence or omission of a null value can sometimes have meaning, so "just use omitempty" is probably not a complete solution.

kevburnsjr commented 4 years ago

Inspection of tests suggests this is in fact a bug.
https://github.com/josephburnett/jd/blob/master/lib/null_test.go#L21

The reason this test isn't failing is because the helper function checkDiff parses the expected value rather than performing a direct comparison.
https://github.com/josephburnett/jd/blob/master/lib/common_test.go#L93

Running both the actual and expected value through the diff parser only ensures that the output is deterministic. Other than that, you're just comparing the result with itself which is prone to error.

kevburnsjr commented 4 years ago

Fixing test helper to perform direct comparison produces 5 failed tests
https://github.com/kevburnsjr/jd/commit/ff809297014f486fd456e79486b6c5cf556bfb76

$ go test
--- FAIL: TestArrayDiff (0.00s)
    common_test.go:120: [1 2 3].Diff([1 {} 3]) =
        @ [1]
        - 2
        + {}
        . Want @ [1]
        - 2
        + null
        .
--- FAIL: TestNullDiff (0.00s)
    common_test.go:120: {}.Diff({}) =
        @ []
        - {}
        . Want @ []
        - null
        .
    common_test.go:120: {}.Diff({}) =
        @ []
        + {}
        . Want @ []
        + null
        .
--- FAIL: TestObjectDiff (0.00s)
    common_test.go:120: {map[a:1] map[]}.Diff({map[a:{}] map[]}) =
        @ ["a"]
        - 1
        + {}
        . Want @ ["a"]
        - 1
        + null
        .
--- FAIL: TestStringDiff (0.00s)
    common_test.go:120: {}.Diff(abc) =
        @ []
        - {}
        + "abc"
        . Want @ []
        - null
        + "abc"
        .
FAIL
exit status 1
FAIL    github.com/josephburnett/jd/lib 0.026s
josephburnett commented 4 years ago

@kevburnsjr nice find! And thanks for the pull request and test fix.

kevburnsjr commented 4 years ago

No problem :+1:

However, I don't expect to make any more contributions. I decided to use a different json diff/patch library for my app because it represents patches as application/json-patch+json in accordance with RFC 6902. I always prefer to use standardized formats where I can for long term portability.

josephburnett commented 4 years ago

Yeah, that's a more standard format. I've been using this one because I find it much easier to read. E.g. in unit test results.