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

internalGet: truncate capacity on return value #199

Closed
thempatel opened this issue Apr 28, 2020 · 5 comments
Closed

internalGet: truncate capacity on return value #199

thempatel opened this issue Apr 28, 2020 · 5 comments

Comments

@thempatel
Copy link

Using ArrayEach in concert with Set leads to some weird behavior. Specifically that ArrayEach provides slices over the original json object, such that the capacity of the slice is jsonEnd-(startOffsetOfSlice). This interacts poorly with Set when a particular key does not exist in the object being mutated. Given this testcase, you can see the output shows the second JSON object becoming malformed upon setting a non-existent key in the first object.

func TestWhatHappens(t *testing.T) {
	jsonObject := `[{"key1":"Foo","key2":"Bar","key3":"baz"},{"key1":"boo","key2":"dog","key3":"cat","key4":"mouse"}]`
	var allSlices [][]byte
	_, err := jsonparser.ArrayEach([]byte(jsonObject), func(value []byte, _ jsonparser.ValueType, _ int, err error) {
		if err != nil {
			t.Fatalf("failed: %s", err)
		}
		allSlices = append(allSlices, value)
	})

	if err != nil {
		t.Fatalf("failed: %s", err)
	}

	newVal, err := jsonparser.Set(allSlices[0], []byte(strconv.Quote(`mouse`)), "key4")
	if err != nil {
		t.Fatalf("failed %s: ", err)
	}
	fmt.Println(string(newVal))
	fmt.Println(string(allSlices[1]))
}

The suggested fix is to change this line:

value = append(data[:startOffset], append(createInsertComponent(keys[depth:], setValue, comma, object), data[depthOffset:]...)...)

to

value = append(data[:startOffset:startOffset], append(createInsertComponent(keys[depth:], setValue, comma, object), data[depthOffset:]...)...)

which truncates the capacity causing append to allocate a new slice. Alternatively, the pattern in the path currently exists flow can be used, this just happened to be a nifty one-liner

@thempatel
Copy link
Author

Follow-up: It was pointed out to me that the write side is probably fine to use the extra capacity, in which case, the internalGet function should likely restrict capacity on what it returns, up to you!

@thempatel
Copy link
Author

FYI we ended up going with:

return value, dataType, offset, endOffset, nil

return value[:len(value):len(value)], dataType, offset, endOffset, nil

@thempatel thempatel changed the title Set: allocate new slice when key does not exist internalGet: truncate capacity on return value Apr 30, 2020
zengjinzheng pushed a commit to zengjinzheng/jsonparser that referenced this issue May 15, 2020
@AllenX2018
Copy link
Collaborator

Awesome! Thanks! I prefer to restrict the capacity of the slice returned by the internalGet function!

@thempatel
Copy link
Author

Sweet!

@thempatel
Copy link
Author

I'm gonna go ahead and close this so it drops out of my list of things

AllenX2018 added a commit that referenced this issue May 28, 2020
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

No branches or pull requests

2 participants