-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: handle NaN and Inf #3480
Labels
Comments
Sorry one of the first sentences is missing a few words, it should read: The RFC draft describing JSON (http://tools.ietf.org/html/rfc4627) explicitly forbids the use of NaN and (-)Inf as numbers, so does the ECMAScript Specification (regardless of version) but _it is_ pretty clear about the handling _of_ such values. |
Sorry the first two paragraphs are both missing a few words, they should read: This is a bit weird. I would't mind the error since it might be a reasonable idea to notify the user that the encoding didn't preserve _all data_, although I would prefer no error at all. The RFC draft describing JSON (http://tools.ietf.org/html/rfc4627) explicitly forbids the use of NaN and (-)Inf as numbers, so does the ECMAScript Specification (regardless of version) but _it is_ pretty clear about the handling _of_ such values. |
One workaround for now is to custom-type your floats and implement the json.Marshaler interface (json.http://golang.org/pkg/encoding/json/#Marshaler) for that type. E.g.: ============================================== type jsonFloat float32 func (value jsonFloat) MarshalJSON([]byte, error) { ... // return your own float representation here. } ============================================== This also works for floats that are parts of structs, etc. But I agree it would be nice to get this fixed in Go itself. |
Good question. I can think of two options: a) unmarshalling any "null" back into NaN seems at least somewhat sensible b) don't support unmarshalling "null" back to a float at all, only support marshalling (return an error when encountering a "null" value for a float field) But I'm not sure I can appropriately judge the implications of either option. |
In that case it should decode to NaN to keep things symmetric and because we should try to loose as little information as possible. The current implementation maps null to 0 for any number type, which means there is no way to tell "0" and "null" apart in the result, which isn't perfect anyway. Maybe decoding 'null' should fail for ints. We could of course also use strings for encoding the three special cases. And just refuse to decode 'null' for numbers, if we can live with differing a little bit from the ECMAScript behavior on the encoding side (decoding doesn't happen in ECMAScript anyway, they basically just evaluate JSON strings). |
Oh god, you're right of course. I'm very sorry about the misinformation, stupid of me. I forgot to try with differently initialized variables. I also forgot about the more general rules regarding null when unmarshalling. Encoding any float value to null would be weirdly asymmetric with the current decoding behavior I guess. I am not sure this is a good idea anymore. It's hard to match ecmascript behavior without major changes or being inconsistent about null. |
Dieterbe
added a commit
to grafana/metrictank
that referenced
this issue
Dec 3, 2015
otherwise we would see errors like `json: error calling MarshalJSON for type main.Point: invalid character 'N' looking for beginning of value` the data can contain NaN values, i verified they were not coming from or stored in the chunks, but happened in divide(), like when we did 0/0. Graphite-web has some special cases in divideSeries() but we don't need to, we can just use NaN in the data arrays and execute our logic/math as usual, this works fine in Go, we just need to encode them properly. for the record, in the official json encoder, NaN/null isn't even supported: golang/go#3480
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The text was updated successfully, but these errors were encountered: