Opened 8 years ago

Closed 5 years ago

Last modified 4 years ago

#8111 closed enhancement (fixed)

pass through to native JSON methods

Reported by: peller Owned by: kzyp
Priority: high Milestone: 1.7
Component: Core Version: 1.2.1
Keywords: Cc: alex, jburke, elazutkin
Blocked by: Blocking:

Description

IE and FF landed native JSON support for the upcoming ECMAScript 3.1 spec. We should look at using these in our fromJson/toJson implementations.

http://blogs.msdn.com/ie/archive/2008/09/10/native-json-in-ie8.aspx https://bugzilla.mozilla.org/show_bug.cgi?id=387522

Attachments (1)

json.diff (11.3 KB) - added by kzyp 5 years ago.
Add dojo/json to use native JSON parsing when available, use has() to feature detect when to use manual, dojo/_base/json uses dojo/json

Download all attachments as: .zip

Change History (35)

comment:1 Changed 8 years ago by alex

  • Cc alex jburke added

good call on filing a bug for these...they've been on my todo list but never got logged.

ISTM that the contract we use about toJson or toString methods may break in this world, so some testing is likely required for encoding. Save JSON decoding, though, is a no-brainer.

comment:2 Changed 8 years ago by peller

  • Milestone changed from tbd to 1.3

consider for 1.3

comment:3 Changed 8 years ago by peller

okay, how do we handle the toJson problem? Seems like we'd have to de-support it? Or maybe we need to create new methods like dojo.stringify (though I don't like the idea of dojo.parse) which don't advertise support for anything which is incompatible with the new standards, and only have them map through to the new native methods? Unfortunately, this just adds bloat.

comment:4 Changed 8 years ago by toonetown

It seems to me, from a *very* cursory look at the specs, that we could add support for an optional "replacer" function in dojo.toJson and an optional "reviver" function in dojo.fromJson and we should be able to support it....

However, I am completely ignorant of any of the details of the toJson/fromJson functions....so there are probably lots of gotchas that would creep up (and bloat is very likely)

comment:5 Changed 8 years ago by kzyp

  • Milestone changed from 1.3 to 1.4

All the wrappers to match our custom reviver/serializer function with spec certainly does add bloat. In addition, we have a significant backwards incompatibility issue in that we have always been allowing any JavaScript to be parsed in dojo.fromJson (since it is an eval without any JSON syntax checks). I know there are systems that are using this fact to pass in JavaScript constructs, especially dates (new Date()), so it would certainly break some sites to upgrade if dojo.fromJson used JSON.parse.

Rather than changing dojo.fromJson and dojo.toJson, we could create JSON.parse and JSON.stringify for browsers that don't have them, using our fromJson and toJson methods (with probably some private parameters indicating which revivere/serializer functions to look for). Although it seems the Dojo community is rather militantly against adding/augmenting the native/globals like that.

Anyway, this is non-trivial. I think we need to work on it for 1.4, I probably don't have to time to get it done for 1.3.

comment:6 Changed 7 years ago by jburke

Because of the backward compat issues, I am fine if we have a Core module, dojo.json, that defines dojo.json.parse() dojo.json.stringify() where we can do the right things. We can then update the docs for dojo.toJson and dojo.fromJson to mention the stricter versions in the dojo.json module.

If the implementations are not very large, maybe even go with dojo.jsonParse() and dojo.jsonStringify() in Base, but we'd have to look at the code weight cost.

comment:7 Changed 7 years ago by jburke

The Bespin folks talk about the desire to have strict JSON parsing here: http://groups.google.com/group/bespin-core/browse_thread/thread/9a353d007c7a9c6a?hl=en

In that thread, Dion suggested dojo.toJSON(data, besafeplease) but that besafeplease conflicts with our existing prettyPrint arg.

Also, looking at http://www.json.org/json2.js, that seems too big to fit into base, so I am favoring dojo.json module with a stringify and parse method.

I want to fit this in base as dojo.jsonStringify/jsonParse, and say "old browsers that do not have native JSON methods fall back to dojo.toJson/fromJson)", but that is not very consistent (we would not pay attention to the reviver/replacer arguments) and the browsers that most need this stricter JSON evaluation (IE 6/7) will not get it.

So looks like the dojo.json module wins. Perhaps use json2.js as the base implementation for non-native browsers. I believe we can use json2.js code as it is specified as public domain code? I am going to say it is OK, unless someone from the Foundation or more up on the legal stuff says it is not.

comment:8 Changed 7 years ago by kzyp

Alex wanted a CLA from Doug before committing code from json2.js to the codebase, as I was going to use the JSON sanitizer in dojox.secure to provide a secure JSON serializer. I asked Doug to sign a CLA last year, and he said he would be happy to, but it hasn't come in yet. I could bug him again about it.

One of the issues here, is that we consistently get questions from newbies thinking that Dojo should be sanitizing code on the client side which is completely wrong for the vast majority of web apps, where a same-origin server should responsible for creating safe JSON. I hate to see users lured away from dojo.fromJson for security misconceptions. Of course there are performance benefits to using the native JSON parsers.

I assume Dion had a typo in his dojo.toJSON(data, besafeplease). I don't think there is any such thing as a "safe" or "unsafe" serializer. It is the parser where the concerns lie. And I don't think we have any conflict with dojo.fromJSON(data, besafeplease).

That being said, I completely agree with you James. Strict JSON parsing does is far too heavy to go in base. We dismissed much smaller patches to add far more valuable functionality in the past. If we are going to add strict JSON parsing with delegation to native code where possible, it should go in a new module (dojo.json or dojox.secure.json).

comment:9 Changed 7 years ago by kzyp

Thinking about this a little more, I think it would best to have delegation to JSON.parse where available and simply use dojo.fromJson otherwise in dojo.json.parse, instead of having it sanitize. If the goal is performance, sanitization is counter-productive. Then we could put the sanitizing code (from json2.js) in dojox.secure (which could also delegate to JSON.parse when available).

comment:10 Changed 7 years ago by peller

In addition to a secure option, perhaps we could also continue to have something lightweight like we have in base today, documented as NOT secure, but delegating to native methods where available for performance. Perhaps we could define a useful, common subset of our existing routines and the new ES5 methods (e.g. without reviver/replacer?) Even doing this would likely require a deprecation cycle. I think the most problematic compatibility issue is our support for json/json methods.

comment:11 Changed 7 years ago by jburke

Kris, good points on the distinctions related to security.

Even though the non-native sanitizing in json2.js may be slower, I was interested in having dojo.json.parse be API/behavior compatible with the ES5 spec, complete with reviver/replacer options. So we do our normal thing of bringing the future now to developers.

I think it is fine to document that dojo.json.parse can be slow on browsers that do not support the spec (make this clear in the documentation, listing the browsers), but if the developer wants to be ES5 compatible for API and behavior, dojo.json.parse is what the developer should consider.

I am fine considering this a Core module since JSON is pretty core, stringify/parse are in ES5 and some browsers now, and we try to put in core "future is now" stuff in where we can (like the array extras in base). But Kris is very knowledgeable in this area, and may have better ideas.

On a different note, there is some sort of quirk in IE8's handling: http://tech.groups.yahoo.com/group/json/message/1268

but maybe that is just a "watch out for bugs in the native code/use a replacer" warning. Not sure we would want to try to globally fix that in our module.

comment:12 Changed 7 years ago by elazutkin

To sum it up: we need to make sure that dojo.toJson() and dojo.fromJson() are compatible with the standard, so we can use builtin implementations and be reasonable small in the base. The rest goes to the core as a separate module dojo.json:

  • Explicitly secure parser.
  • Customizable serializer.
  • Support for revivers/replacers, and more.

We already have a good set of JSON tools in dojox.json: !JSONPath-based !JSONQuery, JSON Schema, JSON Referencing. I may add some JsonML-related tools to dojox.xml, which I already used in some projects. So I think it covers all possible bases in regard of JSON-related needs of real-life projects.

comment:13 follow-up: Changed 7 years ago by kzyp

What exactly do you mean by "behavior compatible with the ES5 spec"? We already are compatible in the sense that anything that can be parsed by JSON.parse(), can also be parsed by dojo.fromJson(). Do you want to replicate JSON.parse() in terms of rejecting exactly the same strings that it rejects? Even json2.js doesn't do this. It has been discussed on the ES5 mailing list that json2.js accepts JSON that is not valid. The point of json2.js isn't to reject all invalid JSON, but to reject all insecure JSON. In order to exactly replicate the rejection pattern of JSON.parse() would probably require a char-by-char parsing (non-eval based) which can be painfully slow (much more so than the regex+eval technique of json2.js, which provides adequate security).

If your intent is that we follow ES5's API with regards to reviver/replacer, then yes, I agree, that is appropriate for dojo.json.*. Having dojo.json.* functions that delegate to JSON.* when available, and implement the reviver/replacer when JSON.* is not available sounds good to me. And I would save sanitation for dojox.json or dojox.secure.

comment:14 follow-up: Changed 7 years ago by kzyp

Also, I would argue that if you really want to give devs the "future is now", we should be creating JSON.* functions when they don't exist instead of putting them in a dojo.json.* namespace. That way when native JSON.* are sufficiently widely deployed, devs can simply remove the dojo.json module without having to change all their uses of JSON functions. But it seems most Dojo committers seem to consider any global namespace additions to be blasphemous.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by elazutkin

Replying to kzyp:

What exactly do you mean by "behavior compatible with the ES5 spec"?

Exactly I meant to compare dojo.fromJson(json) and JSON.parse(text, reviver) to see if we are different in some details (save security considerations). Do the same for dojo.toJson(it, prettyPrint, _indentStr) and JSON.stringify(value, replacer, space). The goal is to be able to use JSON.parse() in dojo.fromJson() and JSON.stringify()}} in {{{dojo.toJson() --- eventually, when all browsers implement the standard :-), we should be able to eliminate our custom code completely calling the standard library directly. If some weird behavioral problems stand in the way, we should eliminate them. The sooner we do that, the better.

All non-standard or extended stuff should go to dojo.json and dojox.json.

If your intent is that we follow ES5's API with regards to reviver/replacer, then yes, I agree, that is appropriate for dojo.json.*. Having dojo.json.* functions that delegate to JSON.* when available, and implement the reviver/replacer when JSON.* is not available sounds good to me. And I would save sanitation for dojox.json or dojox.secure.

Sounds like a plan.

comment:16 in reply to: ↑ 14 Changed 7 years ago by elazutkin

Replying to kzyp:

Also, I would argue that if you really want to give devs the "future is now"

That kind of slogans is definitely not my goal.

we should be creating JSON.* functions when they don't exist instead of putting them in a dojo.json.* namespace. That way when native JSON.* are sufficiently widely deployed, devs can simply remove the dojo.json module without having to change all their uses of JSON functions. But it seems most Dojo committers seem to consider any global namespace additions to be blasphemous.

Sounds like the venerable prototype.js. I don't want to revive the ancient discussion about modifying/substituting standard objects or polluting the global namespace. Suffice to say that all "extenders" do not play well with other libraries, and frequently inadvertently introduce unfortunate side effects.

But you don't have to take my word for it --- I was a silent reader in that discussion. Publish your question on dojo-dev to get answers straight from the horse's mouth.

comment:17 in reply to: ↑ 15 Changed 7 years ago by jburke

Replying to elazutkin:

Replying to kzyp:

If your intent is that we follow ES5's API with regards to reviver/replacer, then yes, I agree, that is appropriate for dojo.json.*. Having dojo.json.* functions that delegate to JSON.* when available, and implement the reviver/replacer when JSON.* is not available sounds good to me. And I would save sanitation for dojox.json or dojox.secure.

Sounds like a plan.

Sounds good to me too. When I earlier mentioned "behavior compatible with the ES5 spec" I thought things behaved a lot more uniform underneath, but apparently not. The big thing was matching the API.

On the other note, modifying globals (as in to create a global JSON object) is a larger discussion outside the scope for this ticket. We can always move things out globally if that philosophical position in Dojo changes.

comment:18 Changed 7 years ago by elazutkin

  • Cc elazutkin added

comment:19 Changed 7 years ago by kzyp

  • Milestone changed from 1.4 to 1.5

comment:20 Changed 6 years ago by peller

  • Milestone changed from 1.5 to 1.6

comment:21 Changed 6 years ago by kzyp

(In [22926]) Added dojox.secure.fromJson for safe JSON parsing, fixes #10333, refs #8111

Changed 5 years ago by kzyp

Add dojo/json to use native JSON parsing when available, use has() to feature detect when to use manual, dojo/_base/json uses dojo/json

comment:22 Changed 5 years ago by kzyp

Added a proposed patch for this issue. Let me know what you think.

comment:23 Changed 5 years ago by kzyp

  • Resolution set to fixed
  • Status changed from new to closed

(In [24028]) Created dojo/json as a module for JSON parsing and serialization that can map to native JSON when available, fixes #8111

comment:24 Changed 5 years ago by bill

  • Milestone changed from 1.6 to 1.7

1.6 was tagged a few weeks ago, so [24028] won't go out until 1.7.

comment:25 Changed 5 years ago by bill

(In [24029]) Fix inline API doc for dojo._escapeString() to get picked up by parser, and also typo in version number. Refs #8111.

comment:26 Changed 5 years ago by ppraveen

I noticed a difference between native JSON stringify and custom dojo.json.stringify implementation when there are NaN values.

var num;
JSON.stringify({ g: Number(num) });

returns

{"g":null}

but

var num;
dojo.json.stringify({ g: Number(num) });

returns

{"g":NaN}

I tested this in Firefox 3.6.

comment:27 Changed 5 years ago by bill

  • Resolution fixed deleted
  • Status changed from closed to reopened

Also, the [24028] checkin is causing dojo/tests/_base/xhr.html to fail, at least on IE8.

comment:28 Changed 5 years ago by kzyp

(In [24486]) Tolerate any valid JSON serialization of extended unicode characters, refs #8111 !strict

comment:29 Changed 5 years ago by kzyp

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [24487]) Fixed serialization of non-finite numbers, fixes #8111 !strict

comment:30 Changed 5 years ago by kzyp

(In [24558]) Apparently Firefox doesn't pass the key to the toJSON method, so use static test instead, refs #8111

comment:31 Changed 5 years ago by kzyp

(In [24819]) Remove debugger statement, refs #8111 !strict

comment:32 Changed 5 years ago by bill

(In [25192]) spacing fixes, refs #8111

comment:33 Changed 5 years ago by bill

In [26140]:

Apparently since [24028] the _indentStr parameter isn't used, even internally. Refs #8111 !strict.

comment:34 Changed 4 years ago by bill

In [30405]:

remove unused dependency on dojo/main, refs #8111

Note: See TracTickets for help on using tickets.