Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snapd,snapctl: decode json using Number #3569
Conversation
stolowski
added some commits
Jul 5, 2017
All checks have passed
Some checks were not successful
codecov-io
commented
Jul 6, 2017
•
Edited 1 time
-
codecov-io
Jul 10, 2017
-
Jul 10, 2017
Codecov Report
@@ Coverage Diff @@
## master #3569 +/- ##
==========================================
+ Coverage 75.85% 75.85% +<.01%
==========================================
Files 399 400 +1
Lines 34662 34663 +1
==========================================
+ Hits 26294 26295 +1
Misses 6496 6496
Partials 1872 1872
Continue to review full report at Codecov.
|
| @@ -256,7 +258,9 @@ func (client *Client) doSync(method, path string, query url.Values, headers map[ | ||
| } | ||
| if v != nil { | ||
| - if err := json.Unmarshal(rsp.Result, v); err != nil { | ||
| + dec := json.NewDecoder(strings.NewReader(string(rsp.Result))) |
chipaca
Jul 9, 2017
Member
the string here forces a spurious copy of rsp.Result. I think you should use a bytes.NewReader here instead.
You've got a lot of them; all instances of strings.NewReader(string(thing) where the thing is a []byte (o a json.RawMessage, same thing) should be a bytes.NewReader(thing) directly. As far as I can tell that's all instances of strings.Newreader(string(thing)) in this code.
All checks have passed
|
A bit of a meta-question: would it make sense to have a helper like |
|
@mvo5 as discussed in the HO, let's not do the helper at this point, we can do it later if there is a compelling reason. |
niemeyer
requested changes
Jul 12, 2017
The changes themselves LGTM, but can we have more unittests ensuring that this logic is in place so we'll be able to tell if something breaks?
I also wonder if some of those places that are today doing Decode and not checking for More are actually broken. My guess is that yes, it's broken.
| @@ -227,6 +227,7 @@ func (client *Client) do(method, path string, query url.Values, headers map[stri | ||
| if v != nil { | ||
| dec := json.NewDecoder(rsp.Body) | ||
| + dec.UseNumber() |
niemeyer
Jul 12, 2017
Contributor
Can we have a unit test that breaks without those changes? Otherwise this is likely to be dropped at some point and we won't notice.
| + dec := json.NewDecoder(strings.NewReader(parts[1])) | ||
| + dec.UseNumber() | ||
| + err := dec.Decode(&value) | ||
| + if err == nil && !dec.More() { |
| @@ -1552,6 +1552,7 @@ func setSnapConf(c *Command, r *http.Request, user *auth.UserState) Response { | ||
| var patchValues map[string]interface{} | ||
| decoder := json.NewDecoder(r.Body) | ||
| + decoder.UseNumber() |
| - err := json.Unmarshal([]byte(*config), &configm) | ||
| + dec := json.NewDecoder(bytes.NewReader(*config)) | ||
| + dec.UseNumber() | ||
| + err := dec.Decode(&configm) |
|
I think the helper would be appropriate here, and perhaps avoid a little the need for so many unit tests. Also, the helper would do the More bit which is currently lacking in many of those cases. That may be a good solution to the above review question. |
stolowski
added some commits
Jul 13, 2017
All checks have passed
|
@niemeyer Thanks for the review. I've pushed the decoding helper. Working on more tests, so it's not ready for re-review yet. |
stolowski
added some commits
Jul 13, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
All checks have passed
|
The branch is ready for re-review; I've reverted the changes to state, task, snapstate as they were not needed after all, also added the helper and improved unit tests and the spread test. |
mvo5
reviewed
Jul 24, 2017
Thanks for this, I like the helper approach, looks good to me, some nitpick/questions inside. Also thanks for the amount of tests in there :)
| @@ -256,7 +257,8 @@ func (client *Client) doSync(method, path string, query url.Values, headers map[ | ||
| } | ||
| if v != nil { | ||
| - if err := json.Unmarshal(rsp.Result, v); err != nil { | ||
| + err := util.DecodeJsonWithNumbers(bytes.NewReader(rsp.Result), v) |
mvo5
Jul 24, 2017
Collaborator
(nitpick) this can be written in a single line: if err := util.Decode...; err != nil {
| @@ -98,7 +100,9 @@ func GetFromChange(snapName string, subkeys []string, pos int, config map[string | ||
| if !ok { | ||
| raw = jsonRaw(value) | ||
| } | ||
| - err := json.Unmarshal([]byte(*raw), result) | ||
| + dec := json.NewDecoder(bytes.NewReader(*raw)) |
mvo5
Jul 24, 2017
Collaborator
pardon my ignorance please, but why is this not using util.DecodeJsonWithNumbers() ? Maybe a small comment with the reason behind this?
| @@ -112,7 +116,9 @@ func GetFromChange(snapName string, subkeys []string, pos int, config map[string | ||
| if !ok { | ||
| raw = jsonRaw(value) | ||
| } | ||
| - err := json.Unmarshal([]byte(*raw), &configm) | ||
| + dec := json.NewDecoder(bytes.NewReader(*raw)) |
| @@ -62,7 +64,9 @@ func (op setGetOp) args() map[string]interface{} { | ||
| } | ||
| kv := strings.SplitN(pair, "=", 2) | ||
| var v interface{} | ||
| - err := json.Unmarshal([]byte(kv[1]), &v) | ||
| + dec := json.NewDecoder(strings.NewReader(kv[1])) |
| - err := json.Unmarshal([]byte(kv[1]), &v) | ||
| + dec := json.NewDecoder(strings.NewReader(kv[1])) | ||
| + dec.UseNumber() | ||
| + err := dec.Decode(&v) |
mvo5
Jul 24, 2017
Collaborator
Not related to the PR, but this could be written in a single line: if err := dec.Decode(&v); err != nil {
| @@ -154,7 +154,9 @@ func (s *setCommand) setInterfaceSetting(context *hookstate.Context, plugOrSlot | ||
| } | ||
| var value interface{} | ||
| - err := json.Unmarshal([]byte(parts[1]), &value) | ||
| + dec := json.NewDecoder(strings.NewReader(parts[1])) |
mvo5
Jul 24, 2017
Collaborator
Would it make sense to use the helper here? If not, maybe a small comment why not?
| @@ -0,0 +1,40 @@ | ||
| +// -*- Mode: Go; indent-tabs-mode: t -*- |
mvo5
Jul 24, 2017
Collaborator
I'm not sure about util as a package name, it seems very generic. Maybe jsonutil ? I know it only carries a single helper for now, but I am concerned that it will become a bit of a grab-all package if we scoope it so wide.
stolowski
Jul 31, 2017
Contributor
Fair point. I agree, let's not let it become "grab-all package". Changed to jsonutil.
stolowski
added some commits
Jul 31, 2017
All checks have passed
niemeyer
changed the title from
snapd, snapctl: use json Decoder instead of Unmarshall
to
snapd,snapctl: decode json using Number
Aug 16, 2017
| + "io" | ||
| +) | ||
| + | ||
| +// DecodeJsonWithNumbers decodes input data using json.Decoder, ensuring numbers are preserved |
niemeyer
Aug 16, 2017
Contributor
This seems better as DecodeWithNumber, the "Json" part is implied as this is inside jsonutil, and the "Number" here refers to the type name, json.Number, rather than to "numbers" in general.
| + return err | ||
| + } | ||
| + if dec.More() { | ||
| + return fmt.Errorf("excess data found, not a valid json") |
niemeyer
Aug 16, 2017
Contributor
People won't associate "excess data" as the reason why, say, 15:45 was not accepted (which excess data?). We ourselves were confused by that when debugging the problem.
I suggest simply saying "cannot parse json value".
stolowski
added some commits
Aug 18, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
mvo5
added this to the 2.28 milestone
Aug 23, 2017
stolowski commentedJul 6, 2017
Use json Decoder to avoid issues with big integers mentioned here: https://forum.snapcraft.io/t/snap-set-digits-as-string/1099 .