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

encoding/json: Encode results in invalid json if json.Number is not a valid number #10281

Closed
erikdubbelboer opened this issue Mar 28, 2015 · 7 comments

Comments

@erikdubbelboer
Copy link
Contributor

When encoding a struct with a json.Number containing an invalid number the resulting json is invalid.
See: http://play.golang.org/p/Xx4GjIgL_X

This is because of the exception made at: https://github.com/golang/go/blob/master/src/encoding/json/encode.go#L533

If this is intentional I think it should be documented in the json.Number documentation. If not it should be fixed by trying the number.Float64() method first and if it fails writing the result as a String enclosed in quotes.

@rsc rsc added this to the Go1.5Maybe milestone Apr 10, 2015
@rsc
Copy link
Contributor

rsc commented Jul 15, 2015

Interesting. Too late for Go 1.5 but worth looking into early in Go 1.6.

Probably there needs to be a separate routine to validate the form of
JSON number syntax. It's not exactly what strconv.ParseFloat requires,
so using Float64 is only an approximation.

Also, probably both Marshal and Unmarshal should use it.
Unmarshaling "invalid" into a json.Number should not store 'invalid'.
It should be an error.

I think.

@rsc rsc modified the milestones: Go1.6Early, Go1.5Maybe Jul 15, 2015
@erikdubbelboer
Copy link
Contributor Author

I wrote a simple Valid() method for the Number type: http://play.golang.org/p/USf7fIxih7

This method could be used at decode.go#L783 to validate the number before setting it. I guess it's best to return an error when it's not valid.

The same goes for encode.go#L532.

If you want I can submit a diff for this using the Contribution Guidelines?

The function is based on this official JSON diagram:

JSON Number

It is quite a lot faster than using a regexp based method:

BenchmarkNumberValid        50000000            37.5 ns/op
BenchmarkNumberValidRegexp   2000000           846 ns/op

@josharian
Copy link
Contributor

Yes, please mail your change, thanks. Note that the tree is frozen for Go 1.5, so a reviewer will mostly likely mark the change as being for Go 1.6, and it won't be reviewed until the tree reopens.

erikdubbelboer added a commit to erikdubbelboer/go that referenced this issue Jul 15, 2015
json.Number is a special case which didn't have any checks and could result in invalid JSON.

Fixes golang#10281

Change-Id: Ie3e726e4d6bf6a6aba535d36f6107013ceac913a
@erikdubbelboer
Copy link
Contributor Author

@gopherbot
Copy link

CL https://golang.org/cl/12250 mentions this issue.

erikdubbelboer added a commit to erikdubbelboer/go that referenced this issue Jul 16, 2015
json.Number is a special case which didn't have any checks and could result in invalid JSON.

Fixes golang#10281

Change-Id: Ie3e726e4d6bf6a6aba535d36f6107013ceac913a
erikdubbelboer added a commit to erikdubbelboer/go that referenced this issue Aug 10, 2015
json.Number is a special case which didn't have any checks and could result in invalid JSON.

Fixes golang#10281

Change-Id: Ie3e726e4d6bf6a6aba535d36f6107013ceac913a
erikdubbelboer added a commit to erikdubbelboer/go that referenced this issue Aug 10, 2015
json.Number is a special case which didn't have any checks and could result in invalid JSON.

Fixes golang#10281

Change-Id: Ie3e726e4d6bf6a6aba535d36f6107013ceac913a
@erikdubbelboer
Copy link
Contributor Author

While fuzzy testing I found some edge cases and fixed them.

I also added a regexp benchmark function to show the speed increase from not using the regexp:

BenchmarkNumberIsValid-8        50000000            27.8 ns/op
BenchmarkNumberIsValidRegexp-8   2000000           881 ns/op

@gopherbot
Copy link

CL https://golang.org/cl/17199 mentions this issue.

rsc added a commit that referenced this issue Dec 4, 2015
Followup to CL 12250.

For #10281.

Change-Id: If25d9cac92f10327bb355f2d11b00c625b464661
Reviewed-on: https://go-review.googlesource.com/17199
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Nov 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants