From b59d728b6490ffc290c9c4a832aa0f843599403e Mon Sep 17 00:00:00 2001 From: b5 Date: Wed, 4 Mar 2020 21:01:36 -0500 Subject: [PATCH] fix(diff): sorting output must respect integer addresses before we were treating them like strings --- .gitignore | 3 +- README.md | 5 +- deepdiff.go | 10 +- deepdiff_test.go | 127 +++++++++++++++------- delta.go | 30 ++++- format.go | 33 +++--- patch.go | 9 -- testdata/graphs/diff_graph_1.deltas.json | 80 -------------- testdata/graphs/diff_graph_1.dot | 56 ---------- testdata/graphs/nested_scalar.deltas.json | 31 ------ testdata/graphs/nested_scalar.dot | 28 ----- 11 files changed, 136 insertions(+), 276 deletions(-) delete mode 100755 testdata/graphs/diff_graph_1.deltas.json delete mode 100755 testdata/graphs/diff_graph_1.dot delete mode 100755 testdata/graphs/nested_scalar.deltas.json delete mode 100755 testdata/graphs/nested_scalar.dot diff --git a/.gitignore b/.gitignore index b85834b..e071fb1 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ coverage.txt testdata/graph.dot deepdiff.test -deepdiff \ No newline at end of file +deepdiff +testdata/graphs \ No newline at end of file diff --git a/README.md b/README.md index 5661381..a773a9d 100644 --- a/README.md +++ b/README.md @@ -113,9 +113,8 @@ func main() { panic(err) } - // Diff will use default configuration to produce a slice of Deltas - // that describe the structured changes. by default Diff will not updates - // only inserts & deletes + // Diff will use default configuration to produce a slice of Changes + // by default Diff will not generate update change only inserts & deletes diffs, err := deepdiff.Diff(a, b) if err != nil { panic(err) diff --git a/deepdiff.go b/deepdiff.go index 36201b9..b0a6458 100644 --- a/deepdiff.go +++ b/deepdiff.go @@ -11,9 +11,9 @@ import ( // Config are any possible configuration parameters for calculating diffs type Config struct { - // Setting Changes to true will have diff represent in-place value shifts + // Setting CalcChanges to true will have diff represent in-place value shifts // as changes instead of add-delete pairs - Changes bool + CalcChanges bool } // DiffOption is a function that adjust a config, zero or more DiffOptions @@ -33,7 +33,7 @@ func New(opts ...DiffOption) *DeepDiff { } return &DeepDiff{ - changes: cfg.Changes, + changes: cfg.CalcChanges, } } @@ -53,7 +53,7 @@ func (dd *DeepDiff) StatDiff(ctx context.Context, a, b interface{}) (Deltas, *St return deepdiff.diff(ctx), deepdiff.stats, nil } -// Stat calculates the DiffStata between two documents +// Stat calculates the DiffStats between two documents func (dd *DeepDiff) Stat(ctx context.Context, a, b interface{}) (*Stats, error) { deepdiff := &diff{changes: dd.changes, d1: a, d2: b, stats: &Stats{}} deepdiff.diff(ctx) @@ -372,6 +372,7 @@ func (d *diff) calcDeltas(t1, t2 node) (dts Deltas) { // (eg, empty object is a leaf node) if delta := compareScalar(match, n, p[len(p)-1]); delta != nil { n.SetChangeType(DTUpdate) + // TODO (b5) - restore support for change calculation, add tests // if d.changes { // // addDelta(root, delta, p) // dts = append(dts, delta) @@ -454,6 +455,7 @@ func sortDeltasAndMaybeCalcStats(deltas Deltas, st *Stats) { } } +// TODO (b5) - restore this. We need this if we want to show moves. // // calcReorderDeltas creates deltas that describes moves within the same parent // // it starts by calculates the largest (order preserving) common subsequence between // // two matched parent compound nodes. Background on LCSS: diff --git a/deepdiff_test.go b/deepdiff_test.go index 7bfe9ed..e7f9ae0 100644 --- a/deepdiff_test.go +++ b/deepdiff_test.go @@ -225,22 +225,37 @@ func TestBasicDiffing(t *testing.T) { {Type: DTContext, Path: StringAddr("c"), Value: []interface{}{float64(3)}}, }, }, + { + "two inserts", + `[ + { "fruit": "apple", "color": "red" }, + { "fruit": "banana", "color": "yellow" }, + { "fruit": "cherry", "color": "red" } + ]`, + `[ + { "fruit": "apple", "color": "red" }, + { "fruit": "blueberry", "color": "blue" }, + { "fruit": "cherry", "color": "red" }, + { "fruit": "durian", "color": "green" } + ]`, + Deltas{ + {Type: DTContext, Path: IndexAddr(0), Value: map[string]interface{}{"color": string("red"), "fruit": string("apple")}}, + {Type: DTDelete, Path: IndexAddr(1), Value: map[string]interface{}{"color": string("yellow"), "fruit": string("banana")}}, + {Type: DTInsert, Path: IndexAddr(1), Value: map[string]interface{}{"color": string("blue"), "fruit": string("blueberry")}}, + {Type: DTContext, Path: IndexAddr(2), Value: map[string]interface{}{"color": string("red"), "fruit": string("cherry")}}, + {Type: DTInsert, Path: IndexAddr(3), Value: map[string]interface{}{"color": string("green"), "fruit": string("durian")}}, + }, + }, - // TODO (b5) - another problematic diff case + // TODO (b5) - problematic test case // { // "diff object and array children", // `{"a":[0,1,2], "b": true }`, // `{"a":{"foo": [0,1,2] }, "b": false }`, - // Deltas{}, - // }, - - // TODO (b5) - I think this should be an error. These inputs don't share a - // common root data type - // { - // "object-to-array root", - // `[ [1,2,3], [4,5,6], [7,8,9] ]`, - // `{ "foo": [1,2,3], "baz": { "bat": [false]}}`, - // Deltas{}, + // Deltas{ + // {Type: DTContext, Path: RootPath{}, Value: map[string]interface{}{"a": []interface{}{float64(0), float64(1), float64(2)}, "b": bool(true)}}, + // {Type: DTInsert, Path: RootAddr{}, Value: } + // }, // }, } @@ -276,7 +291,39 @@ func TestChangeDiffs(t *testing.T) { }, } - RunTestCases(t, cases, func(c *Config) { c.Changes = true }) + RunTestCases(t, cases, func(c *Config) { c.CalcChanges = true }) +} + +func TestDeltaSorting(t *testing.T) { + cases := []TestCase{ + { + "more than 10 index-addressed deltas", + `[[0],[1],[2],[3],[4],[5],[6],[7],[8],[9],[10],[11],[12],[13],[14]]`, + `[[0],[1],[2],[3],[4],[5],[6],[7],[8],["apple splat"],[10],[11],[12],[13],[14]]`, + Deltas{ + {Type: DTContext, Path: IndexAddr(0), Value: []interface{}{float64(0)} }, + {Type: DTContext, Path: IndexAddr(1), Value: []interface{}{float64(1)} }, + {Type: DTContext, Path: IndexAddr(2), Value: []interface{}{float64(2)} }, + {Type: DTContext, Path: IndexAddr(3), Value: []interface{}{float64(3)} }, + {Type: DTContext, Path: IndexAddr(4), Value: []interface{}{float64(4)} }, + {Type: DTContext, Path: IndexAddr(5), Value: []interface{}{float64(5)} }, + {Type: DTContext, Path: IndexAddr(6), Value: []interface{}{float64(6)} }, + {Type: DTContext, Path: IndexAddr(7), Value: []interface{}{float64(7)} }, + {Type: DTContext, Path: IndexAddr(8), Value: []interface{}{float64(8)} }, + {Type: DTContext, Path: IndexAddr(9), Deltas: Deltas{ + {Type: DTDelete, Path: IndexAddr(0), Value: float64(9) }, + {Type: DTInsert, Path: IndexAddr(0), Value: "apple splat" }, + }}, + {Type: DTContext, Path: IndexAddr(10), Value: []interface{}{float64(10)} }, + {Type: DTContext, Path: IndexAddr(11), Value: []interface{}{float64(11)} }, + {Type: DTContext, Path: IndexAddr(12), Value: []interface{}{float64(12)} }, + {Type: DTContext, Path: IndexAddr(13), Value: []interface{}{float64(13)} }, + {Type: DTContext, Path: IndexAddr(14), Value: []interface{}{float64(14)} }, + }, + }, + } + + RunTestCases(t, cases) } func TestInsertGeneralizing(t *testing.T) { @@ -337,33 +384,27 @@ func TestInsertGeneralizing(t *testing.T) { func TestNestedScalar(t *testing.T) { cases := []TestCase{ - // { - // "single nested scalar change no scalar matches", - // `{ "structure": { "formatConfig": { "headerRow": false }}}`, - // `{ "structure": { "formatConfig": { "headerRow": true }}}`, - // Deltas{ - // {Type: DTDelete, Path: RootAddr{}, Value: map[string]interface{}{ - // "structure" : map[string]interface{}{ - // "formatConfig": map[string]interface{}{ - // "headerRow": false, - // }, - // }, - // }}, - // {Type: DTInsert, Path: RootAddr{}, Value: map[string]interface{}{ - // "structure" : map[string]interface{}{ - // "formatConfig": map[string]interface{}{ - // "headerRow": true, - // }, - // }, - // }}, - // // {Type: DTContext, Path: StringAddr("structure"), Deltas: Deltas{ - // // { Type: DTContext, Path: StringAddr("formatConfig"), Deltas: Deltas{ - // // {Type: DTDelete, Path: StringAddr("headerRow"), Value: false }, - // // {Type: DTInsert, Path: StringAddr("headerRow"), Value: true }, - // // }}, - // // }}, - // }, - // }, + { + "single nested scalar change no scalar matches", + `{ "structure": { "formatConfig": { "headerRow": false }}}`, + `{ "structure": { "formatConfig": { "headerRow": true }}}`, + Deltas{ + {Type: DTDelete, Path: RootAddr{}, Value: map[string]interface{}{ + "structure" : map[string]interface{}{ + "formatConfig": map[string]interface{}{ + "headerRow": false, + }, + }, + }}, + {Type: DTInsert, Path: RootAddr{}, Value: map[string]interface{}{ + "structure" : map[string]interface{}{ + "formatConfig": map[string]interface{}{ + "headerRow": true, + }, + }, + }}, + }, + }, { "single nested scalar change one scalar match", `{ "qri": "ds:0", "structure": { "formatConfig": { "headerRow": false }}}`, @@ -392,11 +433,17 @@ func TestRootChanges(t *testing.T) { cases := []TestCase{ { - "large structure overflow", + "object-to-array", `{ "qri": "ds:0" }`, `[ "ds:0", ["rank","probability_of_automation","soc_code","job_title"] ]`, Deltas{}, }, + { + "array-to-object root", + `[ [1,2,3], [4,5,6], [7,8,9] ]`, + `{ "foo": [1,2,3], "baz": { "bat": [false]}}`, + Deltas{}, + }, } RunTestCases(t, cases) diff --git a/delta.go b/delta.go index c871aa7..13a2af6 100644 --- a/delta.go +++ b/delta.go @@ -119,7 +119,15 @@ func (RootAddr) MarshalJSON() ([]byte, error) { type sortableAddrs []Addr func (a sortableAddrs) Len() int { return len(a) } -func (a sortableAddrs) Less(i,j int) bool { return a[i].String() < a[j].String() } +func (a sortableAddrs) Less(i,j int) bool { + if ii, ok := a[i].Value().(int); ok { + if jj, ok := a[j].Value().(int); ok { + return ii < jj + } + } + + return a[i].String() < a[j].String() +} func (a sortableAddrs) Swap(i,j int) { a[i], a[j] = a[j], a[i] } @@ -165,7 +173,7 @@ type Deltas []*Delta func (ds Deltas) Len() int { return len(ds) } // opOrder determines a total order for operations. It's crucial that deletes -// comve *before* inserts in diff script presentation +// come *before* inserts in diff script presentation var opOrder = map[Operation]uint{ DTDelete: 0, DTContext: 1, @@ -173,12 +181,22 @@ var opOrder = map[Operation]uint{ DTUpdate: 3, } -// Less returns trus if the value at index i is a smaller sort quantity than +// Less returns true if the value at index i is a smaller sort quantity than // the value at index j func (ds Deltas) Less(i, j int) bool { - iAddr := ds[i].Path.String() - jAddr := ds[j].Path.String() - return iAddr < jAddr || (iAddr == jAddr && opOrder[ds[i].Type] < opOrder[ds[j].Type]) + iAddr := ds[i].Path + jAddr := ds[j].Path + if iAddr.Eq(jAddr) { + return opOrder[ds[i].Type] < opOrder[ds[j].Type] + } + + if a, ok := ds[i].Path.Value().(int); ok { + if b, ok := ds[j].Path.Value().(int); ok { + return a < b + } + } + + return iAddr.String() < jAddr.String() } // Swap reverses the values at indicies i & J diff --git a/format.go b/format.go index 1c4b02e..b0bd9b9 100644 --- a/format.go +++ b/format.go @@ -8,8 +8,8 @@ import ( "strings" ) -// FormatPrettyString is a convenice wrapper that outputs to a string instead of -// an io.Writer +// FormatPrettyString is a convenience wrapper that outputs to a string instead +// of an io.Writer func FormatPrettyString(changes Deltas, colorTTY bool) (string, error) { buf := &bytes.Buffer{} if err := FormatPretty(buf, changes, colorTTY); err != nil { @@ -27,19 +27,23 @@ func FormatPretty(w io.Writer, changes Deltas, colorTTY bool) error { var colorMap map[Operation]string if colorTTY { - colorMap = map[Operation]string{ - Operation("close"): "\x1b[0m", // end color tag - - DTContext: "\x1b[37m", // netural - DTInsert: "\x1b[32m", // green - DTDelete: "\x1b[31m", // red - DTUpdate: "\x1b[34m", // blue - } + colorMap = ttyColorMap() } return formatPretty(w, changes, 0, colorMap) } +func ttyColorMap() map[Operation]string { + return map[Operation]string{ + Operation("close"): "\x1b[0m", // end color tag + + DTContext: "\x1b[37m", // netural + DTInsert: "\x1b[32m", // green + DTDelete: "\x1b[31m", // red + DTUpdate: "\x1b[34m", // blue + } +} + func formatPretty(w io.Writer, changes Deltas, indent int, colorMap map[Operation]string) error { for _, d := range changes { dataStr := "" @@ -74,14 +78,7 @@ func FormatPrettyStats(w io.Writer, diffStat *Stats, colorTTY bool) { var colorMap map[Operation]string if colorTTY { - colorMap = map[Operation]string{ - Operation("close"): "\x1b[0m", // end color tag - - DTContext: "\x1b[37m", // netural - DTInsert: "\x1b[32m", // green - DTDelete: "\x1b[31m", // red - DTUpdate: "\x1b[34m", // blue - } + colorMap = ttyColorMap() } formatStats(w, diffStat, colorMap) diff --git a/patch.go b/patch.go index 1d48ff9..883d482 100644 --- a/patch.go +++ b/patch.go @@ -34,33 +34,24 @@ func patch(target reflect.Value, delta *Delta) (reflect.Value, error) { // value to the updated child if len(delta.Deltas) > 0 { for _, dlt := range delta.Deltas { - // fmt.Printf("patching %s on target %#v\n", dlt.Path, target) patchedChild, err := patch(child(target, delta.Path), dlt) if err != nil { return target, err } - // fmt.Printf("patch output: %#v\n", patchedChild) - target, err = set(target, patchedChild, delta.Path) if err != nil { return target, err } - - // fmt.Printf("post-patch-set target: %#v\n\n", target) } } switch delta.Type { case DTInsert: - // fmt.Printf("applying insert to %s on target %#v\n", delta.Path, target) target, err = insert(target, reflect.ValueOf(delta.Value), delta.Path) case DTDelete: - // fmt.Printf("applying delete to %s on target %#v\n", delta.Path, target) target, err = remove(target, delta.Path) - // fmt.Printf("post-delete target %#v\n", target) case DTUpdate: - // fmt.Printf("applying update to %s on target %#v\n", delta.Path, target) target, err = set(target, reflect.ValueOf(delta.Value), delta.Path) } diff --git a/testdata/graphs/diff_graph_1.deltas.json b/testdata/graphs/diff_graph_1.deltas.json deleted file mode 100755 index 639f85d..0000000 --- a/testdata/graphs/diff_graph_1.deltas.json +++ /dev/null @@ -1,80 +0,0 @@ -[ - [ - "-", - "a", - 100 - ], - [ - "+", - "a", - 99 - ], - [ - " ", - "bar", - false - ], - [ - " ", - "baz", - null, - [ - [ - " ", - "a", - null, - [ - [ - "-", - "b", - 4 - ], - [ - "+", - "b", - 5 - ], - [ - " ", - "c", - false - ], - [ - " ", - "d", - "apples-and-oranges" - ] - ] - ], - [ - "-", - "e", - null - ], - [ - "+", - "e", - "thirty-thousand-something-dollars" - ], - [ - "+", - "f", - false - ], - [ - "-", - "g", - "apples-and-oranges" - ] - ] - ], - [ - " ", - "foo", - [ - 1, - 2, - 3 - ] - ] - ] \ No newline at end of file diff --git a/testdata/graphs/diff_graph_1.dot b/testdata/graphs/diff_graph_1.dot deleted file mode 100755 index b105627..0000000 --- a/testdata/graphs/diff_graph_1.dot +++ /dev/null @@ -1,56 +0,0 @@ -digraph { - subgraph cluster_t1 { - label="t1"; - t1 [label="", tooltip="weight: 58"]; - t1 -> t1baz; - t1 -> t1foo; - t1 -> t1a; - t1 -> t1bar; - t1baz [label="/baz", tooltip="weight: 45"]; - t1baz -> t1baza; - t1baz -> t1baze; - t1baz -> t1bazg; - t1baza [label="/baz/a", tooltip="weight: 25"]; - t1baza -> t1bazab; - t1baza -> t1bazac; - t1baza -> t1bazad; - t1foo [label="/foo", tooltip="weight: 4"]; - t1foo -> t1foo0; - t1foo -> t1foo1; - t1foo -> t1foo2; - } - subgraph cluster_t2 { - label="t2"; - t2 [label="", tooltip="weight: 76"]; - t2 -> t2a; - t2 -> t2bar; - t2 -> t2baz; - t2 -> t2foo; - t2foo [label="/foo", tooltip="weight: 4"]; - t2foo -> t2foo0; - t2foo -> t2foo1; - t2foo -> t2foo2; - t2baz [label="/baz", tooltip="weight: 64"]; - t2baz -> t2baza; - t2baz -> t2baze; - t2baz -> t2bazf; - t2baza [label="/baz/a", tooltip="weight: 25"]; - t2baza -> t2bazac; - t2baza -> t2bazad; - t2baza -> t2bazab; - } - - t2 -> t1[color=red,penwidth=1.0]; - t2foo -> t1foo[color=red,penwidth=1.0]; - t2foo0 -> t1foo0[color=red,penwidth=1.0]; - t2foo1 -> t1foo1[color=red,penwidth=1.0]; - t2foo2 -> t1foo2[color=red,penwidth=1.0]; - t2a -> t1a[color=red,penwidth=1.0]; - t2bar -> t1bar[color=red,penwidth=1.0]; - t2baz -> t1baz[color=red,penwidth=1.0]; - t2baze -> t1baze[color=red,penwidth=1.0]; - t2baza -> t1baza[color=red,penwidth=1.0]; - t2bazab -> t1bazab[color=red,penwidth=1.0]; - t2bazac -> t1bazac[color=red,penwidth=1.0]; - t2bazad -> t1bazad[color=red,penwidth=1.0]; -} \ No newline at end of file diff --git a/testdata/graphs/nested_scalar.deltas.json b/testdata/graphs/nested_scalar.deltas.json deleted file mode 100755 index 9dbbeec..0000000 --- a/testdata/graphs/nested_scalar.deltas.json +++ /dev/null @@ -1,31 +0,0 @@ -[ - [ - " ", - "qri", - "ds:0" - ], - [ - " ", - "structure", - null, - [ - [ - " ", - "formatConfig", - null, - [ - [ - "-", - "headerRow", - false - ], - [ - "+", - "headerRow", - true - ] - ] - ] - ] - ] - ] \ No newline at end of file diff --git a/testdata/graphs/nested_scalar.dot b/testdata/graphs/nested_scalar.dot deleted file mode 100755 index bc8b7ab..0000000 --- a/testdata/graphs/nested_scalar.dot +++ /dev/null @@ -1,28 +0,0 @@ -digraph { - subgraph cluster_t1 { - label="t1"; - t1 [label="", tooltip="weight: 12"]; - t1 -> t1qri; - t1 -> t1structure; - t1structure [label="/structure", tooltip="weight: 7"]; - t1structure -> t1structureformatConfig; - t1structureformatConfig [label="/structure/formatConfig", tooltip="weight: 6"]; - t1structureformatConfig -> t1structureformatConfigheaderRow; - } - subgraph cluster_t2 { - label="t2"; - t2 [label="", tooltip="weight: 11"]; - t2 -> t2qri; - t2 -> t2structure; - t2structure [label="/structure", tooltip="weight: 6"]; - t2structure -> t2structureformatConfig; - t2structureformatConfig [label="/structure/formatConfig", tooltip="weight: 5"]; - t2structureformatConfig -> t2structureformatConfigheaderRow; - } - - t2 -> t1[color=red,penwidth=1.0]; - t2qri -> t1qri[color=red,penwidth=1.0]; - t2structure -> t1structure[color=red,penwidth=1.0]; - t2structureformatConfig -> t1structureformatConfig[color=red,penwidth=1.0]; - t2structureformatConfigheaderRow -> t1structureformatConfigheaderRow[color=red,penwidth=1.0]; -} \ No newline at end of file