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

Fix an issue with JSON encoding of "Infinity" and "NaN" values #26933

Merged
merged 1 commit into from Nov 13, 2016

Conversation

@prathamesh-sonpatki
Copy link
Member

prathamesh-sonpatki commented Oct 30, 2016

Summary

  • When as_json returns Infinity or NaN as the value of any of the key,
    we don't used to call as_json on it as it was treated as primitive.
  • This used to pass Infinity or NaN to JSON.generate and Ruby used
    to throw an error for Infinity/NaN not allowed in JSON.
  • This patch changes the code to call as_json on these primitives so
    that they are converted to proper values before being passed to
    JSON.generate.
  • Fixes #26877.

r? @chancancode

- When `as_json` returns `Infinity` or `NaN` as the value of any of the key,
  we don't used to call `as_json` on it as it was treated as primitive.
- This used to pass `Infinity` or `NaN` to `JSON.generate` and Ruby used
  to throw an error for `Infinity/NaN not allowed in JSON.`
- This patch changes the code to call `as_json` on these primitives so
  that they are converted to proper values before being passed to
  `JSON.generate`.
- Fixes #26877.
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:fix-26877 branch to 8776f15 Oct 30, 2016
@sergey-alekseev
Copy link
Contributor

sergey-alekseev commented Nov 12, 2016

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Nov 13, 2016

@pixeltrix I had a discussion about this issue with @matthewd at Rubyconf and he suggested that we merge and backport this change and after that deprecate the behvior from #2532 and #6096 on master, and eventually remove it in next to next release. I will open deprecation PR once this gets merged.

@pixeltrix
Copy link
Member

pixeltrix commented Nov 13, 2016

@prathamesh-sonpatki sounds like a plan 😄

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Nov 13, 2016

@pixeltrix So can we merge this 😄

@pixeltrix pixeltrix merged commit c85d305 into rails:master Nov 13, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate Code Climate didn't find any new or fixed issues.
Details
@pixeltrix
Copy link
Member

pixeltrix commented Nov 13, 2016

Done 👍

@@ -432,6 +432,28 @@ def test_exception_to_json
assert_equal '"foo"', ActiveSupport::JSON.encode(exception)
end

class InfiniteNumber
def as_json(options = nil)
{ "number" => 1.0 / 0 }

This comment has been minimized.

@simi

simi Nov 13, 2016 Contributor

What about to use Float::INFINITY directly here?

This comment has been minimized.

@pixeltrix

pixeltrix Nov 13, 2016 Member

Changed in d2890f7 - thanks @simi

Copy link
Member

pixeltrix left a comment

Merged 👍

def test_to_json_works_when_as_json_returns_infinite_number
expected = { number: nil }.to_json
assert_equal expected, InfiniteNumber.new.to_json
end

This comment has been minimized.

@pixeltrix

pixeltrix Nov 13, 2016 Member

I wouldn't use to_json to generate expected values here since that's the method under test - better to use literal values, e.g:

assert_equal '{"number":null}' InfiniteNumber.new.to_json
def test_to_json_works_when_as_json_returns_NaN_number
expected = { number: nil }.to_json
assert_equal expected, NaNNumber.new.to_json
end

This comment has been minimized.

@pixeltrix

pixeltrix Nov 13, 2016 Member

Ditto here - use a literal for the expected value.

@@ -432,6 +432,28 @@ def test_exception_to_json
assert_equal '"foo"', ActiveSupport::JSON.encode(exception)
end

class InfiniteNumber
def as_json(options = nil)
{ "number" => 1.0 / 0 }

This comment has been minimized.

@pixeltrix

pixeltrix Nov 13, 2016 Member

Changed in d2890f7 - thanks @simi


class NaNNumber
def as_json(options = nil)
{ "number" => 0.0 / 0 }

This comment has been minimized.

@simi

simi Nov 13, 2016 Contributor

0.0 / 0 => Float:NAN

Too late :(, is it worth to open PR for this?

This comment has been minimized.

@pixeltrix

pixeltrix Nov 13, 2016 Member

No, I'll fix it

This comment has been minimized.

@pixeltrix

pixeltrix Nov 13, 2016 Member

Two commits later I've fixed it - more haste, less speed 😄

@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Nov 13, 2016

Thanks @simi and @pixeltrix!

@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:fix-26877 branch Nov 13, 2016
@prathamesh-sonpatki
Copy link
Member Author

prathamesh-sonpatki commented Nov 13, 2016

@pixeltrix Please backport this and other related commits as well.

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Nov 18, 2016
Fix an issue with JSON encoding of "Infinity" and "NaN" values
@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 4, 2017

Backported in 2e421ad

rafaelfranca added a commit that referenced this pull request Jan 4, 2017
Fix an issue with JSON encoding of "Infinity" and "NaN" values
eileencodes added a commit that referenced this pull request Nov 30, 2018
Removing `as_json` here revealed that these tests failed because they
return a non-json compliant value of NaNNumber and InfinitNumber.

While we've supported this for awhile one of the issues with this
support is that once we hit `jsonify` all the values _should_ already be
json-compliant values. These tests I removed were added in #26933 - but
in the comments on that PR they were supposed to be deprecated and
removed in the next cycle - but that work never happened. See
#26933 (comment)

We want to remove this `as_json` here because Rails JSON is very slow.
One of the reasons it's slow is because in some cases we actually loop
over the JSON we're parsing 3 times!

Here are the passes:

1. Call as_json on a tree of objects
  1a. Special objects pass the options down. These are
  Object, Struct, Enumerable, Array, Hash
  1b. This first pass eliminates Structs, Enumerables, and Objects
  Structs replaced with hashes, Enumerables -> Arrays, Objects -> Hash
  1c. New tree contains only: String, Number, Hash, Array, nil, true,
  false
2. Tree is walked again
  2a. Convert Strings to EscapedStrings
  2b. Can retun bad values from as_json (like the infinity and nan)
3. Convert to json

By removing this `as_json` we reduce one of the passes for this case.
The argument here is that the first pass should never have returned a
bad value so we shouldn't have to walk those values a second time - the
value should be `nil` (which it what it will be on pass 3).

[Eileen M. Uchitelle & Aaron Patterson]
@DaniG2k
Copy link

DaniG2k commented Jun 6, 2019

I am still experiencing this issue in Rails 4.2.11.1. See this StackOverflow thread. Has the issue been reintroduced?

@eileencodes
Copy link
Member

eileencodes commented Jun 6, 2019

@DaniG2k - Rails 4.2 hasn't been supported for bug fixes in a long time. It's only supported for security releases. If you look at the commit c85d305 you can see it's included in tags 5.1.0.beta1 and up. This change required a deprecation before it was merged and was a change in behavior so we couldn't backport it at the time to 4.2 and definitely can't now.

Just in case it's not on your radar, Rails 4.2 will soon no longer be supported for even security fixes so we recommend you upgraded ASAP. Also gotta say Rails 6.0 is prettty nice. 😄

@DaniG2k
Copy link

DaniG2k commented Jun 6, 2019

OK I'll find a workaround in the meanwhile 🙂 thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants
You can’t perform that action at this time.