WIP: Don't convert json to Infinity/NaN

[andyneff]

Problem

By design, json.dumps extends json

>>> json.dumps({'x': float('inf')})
'{"x": Infinity}’

However when you tell it to disable allow_nan, it throws an exception instead

>>> json.dumps({'x': float('inf')}, allow_nan=False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/json/__init__.py", line 251, in dumps
    sort_keys=sort_keys, **kw).encode(obj)
  File "/usr/lib/python2.7/json/encoder.py", line 207, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib/python2.7/json/encoder.py", line 270, in iterencode
    return _iterencode(o, 0)
ValueError: Out of range float values are not JSON compliant

Desired result

I choose the Same as Chrome/Firefox route, so,

>>> json.dumps({'x': float('inf')}, cls=rest_framework.utils.encoder.JSONEncoder)
'{"x": null}’

Importance

It is mentioned in getsentry/sentry#1979, but from my experience I used ajax to GET on a DRF endpoint, and it fails because there was an invalid. this possible outcome or an unhandled exception with allow_nan=False means that a valid infinity in the django database becomes an invalid ajax call.

Solution

After looking at python's json documentation, there is no good/easy way to change exactly how it handles floats to handle exactly what we'd want. "Don't create invalid JSON or throw an exception." Similar SO Most of these involve monkey patching which could easily have unforeseen consequences.

I went ahead and just overrode the iterencode function by combining encoder.py from 2.7, 3.2 and 3.6 to make a version that should support all supported of python without difference.

Other possibility

Instead of returning null every time, it should also be possible to return the string "Infinity" instead of trying to return the number Infinity. I have no grips with this solution, I just went with the "How does Firefox/Chrome do it"

q={x: 1e309, y:-1e309, z:0/0}
Object { x: Infinity, y: -Infinity, z: NaN }
JSON.stringify(q)
"{"x":null,"y":null,"z":null}"

But I believe that it should be ok to say

>>> json.dumps({'x': float('inf')}, cls=rest_framework.utils.encoder.JSONEncoder)
'{"x": "Infinity"}’

If you would rather do that, where the quotes will make it just a normal string that will never be reinterpreted as a number again (using the standards).

[tomchristie]

Python decided to extend json

json.dumps({'x': float('inf')})
'{"x": Infinity}’

cold hard stare

[tomchristie]

I feel above all, this is the second worst possible outcome (second to an unhandled exception)

I'm rather inclined to think that an unhandled exception would be the best possible outcome, with an explicit override available for setting it to None or "Infinity" if desired.

[xordoquy]

Python decided to extend json

Please avoid unfounded passive aggressive statement.

The behavior is explained in the Python documentation

If 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.
If allow_nan is true, their JavaScript equivalents (NaN, Infinity, -Infinity) will be used.

It's easy to work this around.

[xordoquy]

On a side note, I don't see how converting Infinity to a None value is better.
I think raising the exception is still the better option and let the application handle the issue

[andyneff]

@xordoquy You're right, sorry. Fixed

[andyneff]

So which default behavior do we want DRF to have when it encounters what may be a valid Infinity in a float field from the database?

1. Exactly what it already does today, {"x": Infinity}
2. Thrown an exception
3. Turn it into a string, {"x": "Infinity"}
4. Turn it into null, {"x": null}

I'm partial to option 3

[rpkilby]

I'm rather inclined to think that an unhandled exception would be the best possible outcome, with an explicit override available for setting it to None or "Infinity" if desired.

This makes the most sense to me, which is a default of option two, but configurable to become option three in @andyneff's list.

Also, is there a better way to alter the handling for NaN/infinity? Right now, the PR is duplicating a lot of the JSONEncoder's code (maybe inspired by this SO answer?), however it seems to be a little out of date.

[tomchristie]

I would probably prefer an unhandled exception, with the ability for the developer to switch that instead to an explicit override of eg. None or "Infinity" if desired. (Eg. an class attribute on JSONRenderer that can be set.)

It's a better situation to loudly fail and force developers to be aware if they've got an edge case that needs dealing with, rather than quietly coercing the value.

It is a surprising design choice in the Python JSON lib, but there we are.

[xordoquy]

It is a surprising design choice in the Python JSON lib, but there we are.

Although practicality beats purity ;)

[andyneff]

I totally agree @rpkilby , I had to duplicate way more code than I'd like to. However because of how the floatstr function is buried, this was the minimum amount of duplication I could get away with (I think, feel free to point me in another direction!)

[tomchristie]

Given the amount of overriding required I'd suggest that we'd simply move towards allow_nan=False as the default, but allow that to be overridden.

Unless there's a simpler way to achieve this result I don't think it's worth the additional complexity.

[andyneff]

@tomchristie Ok, I can set allow_nan=False by default. How would you like me to design this?

(Eg. an class attribute on JSONRenderer that can be set.)

1. Multiple classes with the different flags set, JSONRendererSpecialFloatNull JSONRendererSpecialFloatString, for example? And so when a developer wants to use that class he just add this to settings.py

REST_FRAMEWORK = {
    'DEFAULT_RENDERER_CLASSES': (
        'rest_framework.renderers.JSONRendererSpecialFloatNull',
        'rest_framework.renderers.BrowsableAPIRenderer')
}

2. Some other REST_FRAMEWORK setting?
3. Just let the developer override the flags in their own versions of the class, and essentially the same settings.py

REST_FRAMEWORK = {
    'DEFAULT_RENDERER_CLASSES': (
        'my_module.MyJSONRenderer',
        'rest_framework.renderers.BrowsableAPIRenderer')
}

4. Another way

[tomchristie]

I guess making allow_nan be an attribute on the JSONRenderer class, that then gets used.
Similar case to the existing ensure_ascii.

https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/renderers.py#L61

[Michael-J-Ward]

Hey Everyone,

I'm interested in seeing this feature merged in. What can I do to help get it ready?

[andyneff]

@Michael-J-Ward just yell at me? Lol

I haven't forgotten about this, I just keep not being able to find a weekend to work on it.

Right now it's still just a POC. We need to

• add the right options we discussed here and wire those up.
• Then we need to gate the code to disable/enable depending on that option.
  -Add additional unit tests to test those options.

Any assistance would be appreciated