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

inf and nan encoded as invalid JSON #28

Closed
Grinnz opened this issue Nov 30, 2014 · 11 comments
Closed

inf and nan encoded as invalid JSON #28

Grinnz opened this issue Nov 30, 2014 · 11 comments
Assignees
Milestone

Comments

@Grinnz
Copy link

Grinnz commented Nov 30, 2014

Encoding a value that is represented as "inf" or "nan" results in bareword output which is not valid JSON.

Examples:

encode_json [9**9**9]

=> [inf]

encode_json [-sin 9**9**9]

=> [nan]

(In Mojo::JSON, these get encoded to the strings "inf" and "nan")

@rurban rurban self-assigned this Nov 30, 2014
@rurban
Copy link
Owner

rurban commented Nov 30, 2014

I'm not sure if we should stringify it. It's is left out on purpose: https://stackoverflow.com/questions/1423081/json-left-out-infinity-and-nan-json-status-in-ecmascript

Other libs convert it to null instead. See e.g. https://code.google.com/p/go/issues/detail?id=3480

@Grinnz
Copy link
Author

Grinnz commented Nov 30, 2014

There is no way to make roundtrips with inf or nan work, so as long as it produces valid JSON it is acceptable. I mention Mojo::JSON's behavior as a point of reference, converting to null like with blessed references is another option.

@rurban
Copy link
Owner

rurban commented Nov 30, 2014

I'm waiting how upstream JSON decides.

@Grinnz
Copy link
Author

Grinnz commented Dec 1, 2014

FYI, I have let Marc know about this issue and his response was essentially "it's a bug in your code to produce invalid JSON." So JSON::XS won't be fixing it.

@rurban
Copy link
Owner

rurban commented Dec 1, 2014

Yes, that's the easiest and probable best way to handle it. Let the user check all illegal values, similar to perl, where the user has to check for all potential security problems with no strict 'refs' by himself. javascript shouldn't see inf nor nan.
On the other hand null is the value most others used.
And stringification to "nan", "inf" as in Mojo::JSON, parrot and perl6 keeps the most information.

Let's see what makamaka thinks of it. I have no special opinion.

@rurban rurban added this to the Next Release milestone Dec 11, 2014
@rurban
Copy link
Owner

rurban commented Dec 11, 2014

Added now the testcase from makamaka/JSON-PP#10

@rurban
Copy link
Owner

rurban commented Dec 11, 2014

No response.
I'll do it like this:

  • new default: null as in most other JSON libs,
  • or optionally via -DSTRINGIFY_INFNAN "inf" and "nan"

@rurban
Copy link
Owner

rurban commented Dec 11, 2014

Now implemented with cb9007d and fc48ace for 3.0108

@Grinnz
Copy link
Author

Grinnz commented Dec 12, 2014

Win32 test reports from CPAN testers are getting odd results from inf/nan encoding: http://www.cpantesters.org/cpan/report/ebf54671-6c00-1014-9247-51fd2a825c07

#   Failed test at t/117_numbers.t line 6.
#          got: '[1.#INF]'
#     expected: '[null]'

#   Failed test at t/117_numbers.t line 7.
#          got: '[-1.#IND]'
#     expected: '[null]'

And this one report ended up with [-nan] somehow...
http://www.cpantesters.org/cpan/report/470cdb04-820f-11e4-8cdc-aa95bf9d5952

I think we need two more test cases for -inf and -nan, I just tested these and they are also outputting barewords:

is encode_json([-9**9**9]), '[null]'; # [-inf]
is encode_json([sin(9**9**9)]), '[null]'; # [-nan]
is encode_json([9**9**9/9**9**9]), '[null]'; # also [-nan]

@rurban
Copy link
Owner

rurban commented Dec 13, 2014

yeah. I haven't checked windows and the minus. Ok, I'll add these also.

@rurban rurban reopened this Dec 13, 2014
rurban pushed a commit that referenced this issue Dec 13, 2014
- Fixed detecting 1.#INF/1.#IND on windows. Also detect now -inf and -nan. GH #28
- Fixed STRINGIFY_INFNAN return string, length off by one. GH #28
@rurban
Copy link
Owner

rurban commented Dec 13, 2014

Fixed with 3.0111

@rurban rurban closed this as completed Dec 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants