From 31743f2df998727d78656c4b034c0ea588ef6edb Mon Sep 17 00:00:00 2001 From: fnoopv Date: Mon, 23 Mar 2026 11:12:03 +0800 Subject: [PATCH] fix: factorization dropping operations when paths share a string prefix --- differ.go | 15 +- differ_test.go | 170 +++++++++++++++++- .../jsonpatch/options/factorization.json | 21 +++ 3 files changed, 204 insertions(+), 2 deletions(-) diff --git a/differ.go b/differ.go index 0009452..d842c06 100644 --- a/differ.go +++ b/differ.go @@ -482,7 +482,7 @@ func (d *Differ) add(path string, v interface{}, doc string, lcs bool) { // The "from" location MUST NOT be a proper prefix // of the "path" location; i.e., a location cannot // be moved into one of its children. - if !strings.HasPrefix(path, op.Path) { + if !hasProperPathPrefix(op.Path, path) { d.patch = d.patch.remove(idx) if !lcs { d.patch = d.patch.append(OperationMove, op.Path, path, v, v, 0) @@ -529,6 +529,19 @@ func (d *Differ) findRemoved(v interface{}) int { return -1 } +func hasProperPathPrefix(prefix, path string) bool { + if prefix == emptyPointer { + return path != emptyPointer + } + if !strings.HasPrefix(path, prefix) { + return false + } + if len(path) == len(prefix) { + return false + } + return path[len(prefix)] == separator +} + func (d *Differ) applyOpts(opts ...Option) { for _, opt := range opts { if opt != nil { diff --git a/differ_test.go b/differ_test.go index c49580c..129986b 100644 --- a/differ_test.go +++ b/differ_test.go @@ -27,8 +27,10 @@ type testcase struct { type patchGetter func(tc *testcase) Patch func TestRFCCases(t *testing.T) { + // https://datatracker.ietf.org/doc/html/rfc6902#appendix-A runCasesFromFile(t, "testdata/tests/jsonpatch/rfc.json", Factorize(), LCS()) -} // https://datatracker.ietf.org/doc/html/rfc6902#appendix-A +} + func TestArrayCases(t *testing.T) { runCasesFromFile(t, "testdata/tests/jsonpatch/array.json") } func TestObjectCases(t *testing.T) { runCasesFromFile(t, "testdata/tests/jsonpatch/object.json") } func TestRootCases(t *testing.T) { runCasesFromFile(t, "testdata/tests/jsonpatch/root.json") } @@ -325,6 +327,172 @@ func Test_issue29_alt(t *testing.T) { } } +func Test_issue45(t *testing.T) { + src := []byte(`{"key":{"b":{"$numberInt":"1"}},"name":"b1"}`) + tgt := []byte(`{"key":{"bb":{"$numberInt":"1"}},"name":"b1"}`) + + patch, err := CompareJSON(src, tgt) + if err != nil { + t.Fatal(err) + } + if len(patch) != 2 { + t.Fatalf("expected 2 operations without factorize, got %d", len(patch)) + } + if op := patch[0]; op.Type != OperationRemove || op.Path != "/key/b" { + t.Fatalf("expected first operation to remove /key/b, got %s at %s", op.Type, op.Path) + } + if op := patch[1]; op.Type != OperationAdd || op.Path != "/key/bb" { + t.Fatalf("expected second operation to add /key/bb, got %s at %s", op.Type, op.Path) + } + + patch, err = CompareJSON(src, tgt, Factorize()) + if err != nil { + t.Fatal(err) + } + if len(patch) != 1 { + t.Fatalf("expected 1 operation with factorize, got %d", len(patch)) + } + if op := patch[0]; op.Type != OperationMove || op.From != "/key/b" || op.Path != "/key/bb" { + t.Fatalf("expected move from /key/b to /key/bb, got %s from %s to %s", op.Type, op.From, op.Path) + } +} + +func TestHasProperPathPrefix(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + prefix string + path string + want bool + }{ + // root pointer + { + name: "root is a proper prefix of any non-root path", + prefix: "", + path: "/a", + want: true, + }, + { + name: "root is a proper prefix of a deep path", + prefix: "", + path: "/a/b/c", + want: true, + }, + { + name: "root is NOT a proper prefix of itself", + prefix: "", + path: "", + want: false, + }, + // exact match + { + name: "identical paths are not a proper prefix", + prefix: "/a", + path: "/a", + want: false, + }, + { + name: "identical deep paths are not a proper prefix", + prefix: "/a/b", + path: "/a/b", + want: false, + }, + // genuine parent → child relationship + { + name: "direct child is a proper prefix", + prefix: "/a", + path: "/a/b", + want: true, + }, + { + name: "grandchild is a proper prefix", + prefix: "/a", + path: "/a/b/c", + want: true, + }, + { + name: "nested direct child is a proper prefix", + prefix: "/key/b", + path: "/key/b/child", + want: true, + }, + // sibling paths that share a string prefix + { + name: "sibling path /key/bb is NOT a proper prefix of /key/b", + prefix: "/key/bb", + path: "/key/b", + want: false, + }, + { + name: "sibling path /key/b is NOT a proper prefix of /key/bb", + prefix: "/key/b", + path: "/key/bb", + want: false, + }, + { + name: "sibling /a is NOT a proper prefix of /ab", + prefix: "/a", + path: "/ab", + want: false, + }, + { + name: "sibling /foo/bar is NOT a proper prefix of /foo/barz", + prefix: "/foo/bar", + path: "/foo/barz", + want: false, + }, + // unrelated paths + { + name: "completely different paths", + prefix: "/a", + path: "/b", + want: false, + }, + { + name: "path is shorter than prefix", + prefix: "/a/b/c", + path: "/a/b", + want: false, + }, + // array index paths + { + name: "array parent is a proper prefix of element", + prefix: "/0", + path: "/0/name", + want: true, + }, + { + name: "array index /1 is NOT a proper prefix of /10", + prefix: "/1", + path: "/10", + want: false, + }, + { + name: "array index /1 is NOT a proper prefix of /11", + prefix: "/1", + path: "/11", + want: false, + }, + { + name: "array index /10 is a proper prefix of /10/x", + prefix: "/10", + path: "/10/x", + want: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := hasProperPathPrefix(tc.prefix, tc.path) + if got != tc.want { + t.Errorf("hasProperPathPrefix(%q, %q) = %v, want %v", + tc.prefix, tc.path, got, tc.want) + } + }) + } +} + func Benchmark_sortStrings(b *testing.B) { if testing.Short() { b.Skip() diff --git a/testdata/tests/jsonpatch/options/factorization.json b/testdata/tests/jsonpatch/options/factorization.json index d5e3ca0..3b034ff 100644 --- a/testdata/tests/jsonpatch/options/factorization.json +++ b/testdata/tests/jsonpatch/options/factorization.json @@ -100,6 +100,27 @@ { "op": "move", "from": "/a", "path": "/b" }, { "op": "add", "path": "/c", "value": 1 } ] +}, { + "name": "factorization does not drop sibling add with shared prefix", + "before": { + "key": { + "b": { + "$numberInt": "1" + } + }, + "name": "b1" + }, + "after": { + "key": { + "bb": { + "$numberInt": "1" + } + }, + "name": "b1" + }, + "patch": [ + { "op": "move", "from": "/key/b", "path": "/key/bb" } + ] },{ "name": "impossible factorization", "before": [