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

Infinity handling differs to python native json #80

Closed
telegraphic opened this issue Apr 19, 2013 · 10 comments · Fixed by #562
Closed

Infinity handling differs to python native json #80

telegraphic opened this issue Apr 19, 2013 · 10 comments · Fixed by #562
Labels
bug Something isn't working

Comments

@telegraphic
Copy link

It looks like ujson handles infinity different to the native json encoder:

import ujson, json, numpy

a = np.array([1,2,3,4,numpy.inf])
b = json.dumps({"test" : a.tolist()})
# outputs '{"test": [1,2,3,4,5, Infinity]}'
b= ujson.dumps({"test" : a.tolist()})
# raises OverflowError: Invalid Inf value when encoding double

Similarly, an Overflow error is raised when NaN is encountered. It seems the relevant lines are 499-508 in ultrajsonenc.c:

    if (value == HUGE_VAL || value == -HUGE_VAL)
    {
        SetError (obj, enc, "Invalid Inf value when encoding double");
        return FALSE;
    }
    if (! (value == value)) 
    {
        SetError (obj, enc, "Invalid Nan value when encoding double");
        return FALSE;
    }

In the interests of keeping ujson as a drop-in replacement for json, may I suggest this is changed so that it converts the NaN / Infinity to strings (like json) and doesn't raise an error? The decoder would likely need to be changed too...

Cheers
Dan

@jskorpan
Copy link

The specification on json.org is very clear about numeric types only being numbers or exponents.
//JT

From: Danny Price [mailto:notifications@github.com]
Sent: den 19 april 2013 07:59
To: esnme/ultrajson
Subject: [ultrajson] Infinity handling differs to python native json (#80)

It looks like ujson handles infinity different to the native json encoder:

import ujson, json, numpy

a = np.array([1,2,3,4,numpy.inf])

b = json.dumps({"test" : a.tolist()})

outputs '{"test": [1,2,3,4,5, Infinity]}'

b= ujson.dumps({"test" : a.tolist()})

raises OverflowError: Invalid Inf value when encoding double

Similarly, an Overflow error is raised when NaN is encountered. It seems the relevant lines are 499-508 in ultrajsonenc.chttps://github.com/esnme/ultrajson/blob/master/lib/ultrajsonenc.c#L499:

if (value == HUGE_VAL || value == -HUGE_VAL)

{

    SetError (obj, enc, "Invalid Inf value when encoding double");

    return FALSE;

}

if (! (value == value))

{

    SetError (obj, enc, "Invalid Nan value when encoding double");

    return FALSE;

}

In the interests of keeping ujson as a drop-in replacement for json, may I suggest this is changed so that it converts the NaN / Infinity to strings (like json) and doesn't raise an error? The decoder would likely need to be changed too...

Cheers
Dan


Reply to this email directly or view it on GitHubhttps://github.com//issues/80.

@telegraphic
Copy link
Author

Good point - for anyone else interested there's a discussion on StackOverflow of exactly this. I find it bemusing that JSON cannot fully represent IEE754 floating point numbers!

However, if ujson is intended as a drop-in replacement for python's json, I maintain that handling inf/nans in the exact same way would be preferable to raising an exception. So, may I instead request for an argument allow_nan for loading/dumping:
allow_nan is False (default: True), then it will be a ValueError to serialize out of range float values (nan, inf, -inf) in strict compliance of the JSON specification, instead of using the JavaScript equivalents (NaN, Infinity, -Infinity).

  • Dan

@mthurlin
Copy link

I think this should be solved in a more generic way by a allowing a custom encoder/decoder that handles unknown values.

Adding an option for every special case where someone is abusing the JSON-spec is not the way to go IMO.

@telegraphic
Copy link
Author

I would argue that it's not "abuse" of the JSON spec, but is instead adding extra functionality and compatibility -- already included in simplejson, json, and cjson. Having an identical featureset to the standard json would be fantastic, and I really think handling IEEE floating point is a pretty solid move.

@mthurlin
Copy link

Well, you are asking a JSON encoder to output invalid JSON...

Also:
http://en.wikipedia.org/wiki/Robustness_principle

@telegraphic
Copy link
Author

So following the robustness principle, uJson should accept nan and inf on the decode ("code that receives input should accept non-conformant input as long as the meaning is clear"), but shouldn't encode it ("code that sends commands or data to other machines should conform completely to the specifications")?

While this is obviously not what I would prefer, I do see the advantages of following design principles. I'll leave the ball in your court on this one...

@jskorpan
Copy link

Feel free to contribute a patch towards the robustness principle. Closing this as an issue

@hsk81
Copy link

hsk81 commented Jun 9, 2013

+1 for encoding inf and nan to JavaScript values Infinity and NaN .. it seems to me that the JSON specification is simply insufficient by omitting the full range of IEE754 floating point numbers. Following specs religiously is not the way to go for practical programming.

@Joshuaalbert
Copy link

Have to agree strongly with @hsk81, and the robustness principle is clear on this too. You SHOULD decode things when the meaning is clear, and you SHOULD NOT encode things against the spec. In programming the meaning of SHOULD and MUST are distinct. (And now I shall use it in practice). You SHOULD NOT follow a rule containing the word SHOULD, when it introduces unnecessary complexity into a fundamental part of many people's code.

@bwoodsend
Copy link
Collaborator

We do appear to have a bit of a deviation here. ujson writes infinity as Inf rather than Infinity which mismatches JavaScript, Python's json and ujson's own parser so it can't even read back its own JSON!

>>> import math
>>> import ujson
>>> import json

>>> json.dumps([math.nan, math.inf, -math.inf])
'[NaN, Infinity, -Infinity]'
>>> ujson.dumps([math.nan, math.inf, -math.inf])
'[NaN,Inf,-Inf]'

>>> ujson.loads('[NaN,Inf,-Inf]')
ujson.JSONDecodeError: Unexpected character found when decoding 'Infinity'
>>> ujson.loads('[NaN, Infinity, -Infinity]')
[nan, inf, -inf]

And on the topic of JSON's inclusion/exclusion of non finite floats, one of the original JSON authors said that he didn't add it to the spec only because he didn't expect anyone to need it and that if anyone found a good use for them then he would consider his own argument to be moot.

@bwoodsend bwoodsend reopened this Aug 6, 2022
@bwoodsend bwoodsend added the bug Something isn't working label Aug 6, 2022
bwoodsend added a commit to bwoodsend/ultrajson that referenced this issue Aug 6, 2022
Infinity was being encoded as 'Inf' which, whilst the JSON spec doesn't include
any non-finite floats, differs from the conventions in other JSON libraries,
JavaScript of using 'Infinity'. It also differs from what `ujson.loads()`
expects so that `ujson.loads(ujson.dumps(math.inf))` raises an exception.

Closes ultrajson#80.
@bwoodsend bwoodsend linked a pull request Aug 6, 2022 that will close this issue
bwoodsend added a commit to bwoodsend/ultrajson that referenced this issue Aug 7, 2022
Infinity was being encoded as 'Inf' which, whilst the JSON spec doesn't include
any non-finite floats, differs from the conventions in other JSON libraries,
JavaScript of using 'Infinity'. It also differs from what `ujson.loads()`
expects so that `ujson.loads(ujson.dumps(math.inf))` raises an exception.

Closes ultrajson#80.
bwoodsend added a commit to bwoodsend/ultrajson that referenced this issue Aug 7, 2022
Infinity was being encoded as 'Inf' which, whilst the JSON spec doesn't include
any non-finite floats, differs from the conventions in other JSON libraries,
JavaScript of using 'Infinity'. It also differs from what `ujson.loads()`
expects so that `ujson.loads(ujson.dumps(math.inf))` raises an exception.

Closes ultrajson#80.
bwoodsend added a commit that referenced this issue Aug 8, 2022
Infinity was being encoded as 'Inf' which, whilst the JSON spec doesn't include
any non-finite floats, differs from the conventions in other JSON libraries,
JavaScript of using 'Infinity'. It also differs from what `ujson.loads()`
expects so that `ujson.loads(ujson.dumps(math.inf))` raises an exception.

Closes #80.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants