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

data.json: nan should not be serialized to "undefined" #958

Closed
tomykaira opened this Issue Jan 18, 2015 · 3 comments

Comments

Projects
None yet
4 participants
@tomykaira

tomykaira commented Jan 18, 2015

Problem: The serialization object including NaN is invaild JSON.

auto j = Json.emptyObject;
j["float"] = Json(double.init);
assert(j.toString == `{"float":undefined}`);  // literal "undefined" is invalid

According to existing unittests, this is intended behavior, but IMO, not acceptable as implementation of JSON serializer.
Serialization must not create invalid JSON data without error notification.

I understand it is difficult to approach from compatibility point of view, but I appreciate if one of following fixes are introduced.

  • Throw an exception because it is NaN input is not permitted [1]
  • Serialize NaN to null (as ECMAScript does [2])
  1. RFC 4627 - The application/json Media Type for JavaScript Object Notation (JSON) 2.4. Numbers
  2. PDF: Final draft Standard ECMA-262 edition 5.1, March 2011 (Rev. 6) p.208
@NfNitLoop

This comment has been minimized.

Show comment
Hide comment
@NfNitLoop

NfNitLoop Sep 14, 2015

There's a similar issue with the default undefined value in Json:

Json j;
assert(j.toString == "undefined")

These kinds of values are time bombs for JSON in D. They give neither compile-time nor runtime errors, they just result in invalid output that can't be parsed by REST clients!

NfNitLoop commented Sep 14, 2015

There's a similar issue with the default undefined value in Json:

Json j;
assert(j.toString == "undefined")

These kinds of values are time bombs for JSON in D. They give neither compile-time nor runtime errors, they just result in invalid output that can't be parsed by REST clients!

@dmonagle

This comment has been minimized.

Show comment
Hide comment
@dmonagle

dmonagle Nov 5, 2015

Contributor

Okay so when I did this, I also made another change to our local fork of vibe.d which I've had to reapply for every upgrade. I wasn't sure if people would like the behaviour, but here is my logic.

For a JSON object, a value that is not set should simply not appear in the structure. I don't particularly see why we should have 'null' in there, anymore than we should have undefined. If the key isn't set, it shouldn't exist. For a value, I agree that null is the better value to set to.

We worked with this by modifying writeJsonString. A NaN is output as null, and ignored if it is part of an object.

Now some people may not like this idea as if something is missing from the structure, it needs to be optional. But if you are going to allow NaN to be serialised, then it really is an optional value anyway.

If you guys are okay with the idea of just modifying the serialised version of the string, then I'm happy to submit my branch as a pull request.

Contributor

dmonagle commented Nov 5, 2015

Okay so when I did this, I also made another change to our local fork of vibe.d which I've had to reapply for every upgrade. I wasn't sure if people would like the behaviour, but here is my logic.

For a JSON object, a value that is not set should simply not appear in the structure. I don't particularly see why we should have 'null' in there, anymore than we should have undefined. If the key isn't set, it shouldn't exist. For a value, I agree that null is the better value to set to.

We worked with this by modifying writeJsonString. A NaN is output as null, and ignored if it is part of an object.

Now some people may not like this idea as if something is missing from the structure, it needs to be optional. But if you are going to allow NaN to be serialised, then it really is an optional value anyway.

If you guys are okay with the idea of just modifying the serialised version of the string, then I'm happy to submit my branch as a pull request.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 31, 2016

Member

Fixed by #1548

@intrica: I'm not sure if omitting null in objects is a good idea in general. After all, it still carries information and most importantly, such a change could break a lot of code. Now, NaN is of course a special case, if you'd count it as undefined instead of as null, but I'd again prefer to follow the ECMA script JSON conversion rules, which state null, AFAIR.

Member

s-ludwig commented Aug 31, 2016

Fixed by #1548

@intrica: I'm not sure if omitting null in objects is a good idea in general. After all, it still carries information and most importantly, such a change could break a lot of code. Now, NaN is of course a special case, if you'd count it as undefined instead of as null, but I'd again prefer to follow the ECMA script JSON conversion rules, which state null, AFAIR.

@s-ludwig s-ludwig closed this Aug 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment