Skip to content

Commit

Permalink
Merge pull request #196 from evanphx/b-null-mistype-panic
Browse files Browse the repository at this point in the history
Handle null correctly when introduced by replace. Fixes #171
  • Loading branch information
evanphx authored Jan 12, 2024
2 parents 850009d + a82b43d commit 3da7b27
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 34 deletions.
8 changes: 8 additions & 0 deletions v5/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,10 @@ func doMergePatch(docData, patchData []byte, mergeMerge bool) ([]byte, error) {
}

if isSyntaxError(patchErr) {
if json.Valid(patchData) {
return patchData, nil
}

return nil, errBadJSONPatch
}

Expand All @@ -154,6 +158,10 @@ func doMergePatch(docData, patchData []byte, mergeMerge bool) ([]byte, error) {
patchErr = unmarshal(patchData, patchAry)

if patchErr != nil {
// Not an array either, a literal is the result directly.
if json.Valid(patchData) {
return patchData, nil
}
return nil, errBadJSONPatch
}

Expand Down
34 changes: 3 additions & 31 deletions v5/merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,14 @@ package jsonpatch

import (
"fmt"
"strings"
"testing"
)

func mergePatch(doc, patch string) string {
out, err := MergePatch([]byte(doc), []byte(patch))

if err != nil {
panic(err)
panic(fmt.Sprintf("%s: %s", err, patch))
}

return string(out)
Expand Down Expand Up @@ -166,8 +165,8 @@ var rfcTests = []struct {
{target: `{"a":[{"b":"c"}]}`, patch: `{"a":[1]}`, expected: `{"a":[1]}`},
{target: `["a","b"]`, patch: `["c","d"]`, expected: `["c","d"]`},
{target: `{"a":"b"}`, patch: `["c"]`, expected: `["c"]`},
// {target: `{"a":"foo"}`, patch: `null`, expected: `null`},
// {target: `{"a":"foo"}`, patch: `"bar"`, expected: `"bar"`},
{target: `{"a":"foo"}`, patch: `null`, expected: `null`},
{target: `{"a":"foo"}`, patch: `"bar"`, expected: `"bar"`},
{target: `{"e":null}`, patch: `{"a":1}`, expected: `{"a":1,"e":null}`},
{target: `[1,2]`, patch: `{"a":"b","c":null}`, expected: `{"a":"b"}`},
{target: `{}`, patch: `{"a":{"bb":{"ccc":null}}}`, expected: `{"a":{"bb":{}}}`},
Expand All @@ -183,33 +182,6 @@ func TestMergePatchRFCCases(t *testing.T) {
}
}

var rfcFailTests = `
{"a":"foo"} | null
{"a":"foo"} | "bar"
`

func TestMergePatchFailRFCCases(t *testing.T) {
tests := strings.Split(rfcFailTests, "\n")

for _, c := range tests {
if strings.TrimSpace(c) == "" {
continue
}

parts := strings.SplitN(c, "|", 2)

doc := strings.TrimSpace(parts[0])
pat := strings.TrimSpace(parts[1])

out, err := MergePatch([]byte(doc), []byte(pat))

if err != errBadJSONPatch {
t.Errorf("error not returned properly: %s, %s", err, string(out))
}
}

}

func TestResembleJSONArray(t *testing.T) {
testCases := []struct {
input []byte
Expand Down
35 changes: 32 additions & 3 deletions v5/patch.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,10 @@ func (n *lazyNode) intoDoc() (*partialDoc, error) {

err := unmarshal(*n.raw, &n.doc)

if n.doc == nil {
return nil, ErrInvalid
}

if err != nil {
return nil, err
}
Expand Down Expand Up @@ -308,6 +312,10 @@ func (n *lazyNode) tryDoc() bool {
return false
}

if n.doc == nil {
return false
}

n.which = eDoc
return true
}
Expand All @@ -327,6 +335,18 @@ func (n *lazyNode) tryAry() bool {
return true
}

func (n *lazyNode) isNull() bool {
if n == nil {
return true
}

if n.raw == nil {
return true
}

return bytes.Equal(n.compact(), rawJSONNull)
}

func (n *lazyNode) equal(o *lazyNode) bool {
if n.which == eRaw {
if !n.tryDoc() && !n.tryAry() {
Expand Down Expand Up @@ -466,6 +486,10 @@ func (o Operation) From() (string, error) {

func (o Operation) value() *lazyNode {
if obj, ok := o["value"]; ok {
// A `null` gets decoded as a nil RawMessage, so let's fix it up here.
if obj == nil {
return newLazyNode(newRawMessage(rawJSONNull))
}
return newLazyNode(obj)
}

Expand Down Expand Up @@ -818,7 +842,10 @@ func ensurePathExists(pd *container, path string, options *ApplyOptions) error {
newNode := newLazyNode(newRawMessage(rawJSONObject))

doc.add(part, newNode, options)
doc, _ = newNode.intoDoc()
doc, err = newNode.intoDoc()
if err != nil {
return err
}
}
} else {
if isArray(*target.raw) {
Expand Down Expand Up @@ -1025,12 +1052,14 @@ func (p Patch) test(doc *container, op Operation, options *ApplyOptions) error {
return errors.Wrapf(err, "error in test for path: '%s'", path)
}

ov := op.value()

if val == nil {
if op.value() == nil || op.value().raw == nil {
if ov.isNull() {
return nil
}
return errors.Wrapf(ErrTestFailed, "testing value %s failed", path)
} else if op.value() == nil {
} else if ov.isNull() {
return errors.Wrapf(ErrTestFailed, "testing value %s failed", path)
}

Expand Down
17 changes: 17 additions & 0 deletions v5/patch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,13 @@ var Cases = []Case{
false,
false,
},
{
`{}`,
`[{"op": "replace", "path": "", "value": null}]`,
`null`,
false,
false,
},
}

type BadCase struct {
Expand Down Expand Up @@ -727,6 +734,16 @@ var BadCases = []BadCase{
`[{"op": "copy", "path": "/qux", "from": "/baz"}]`,
false,
},
{
`{ "foo": {"bar": []}}`,
`[{"op": "replace", "path": "/foo/bar", "value": null}, {"op": "add", "path": "/foo/bar/0", "value": "blah"}]`,
false,
},
{
`{}`,
`[{"op": "replace", "path": ""}]`,
true,
},
}

// This is not thread safe, so we cannot run patch tests in parallel.
Expand Down

0 comments on commit 3da7b27

Please sign in to comment.