[PATCH] lots of JSON goodness

Colin Mollenhour suggested that JSON from the response body should not override the message from X-JSON header for consistency. His suggestion is to write to the json property of the transport (xhr) object that is being passed to the callbacks as the first argument.

myCallback = function(transport, json) {
  json           // -> from X-JSON header
  transport.json // -> from response body
}

While the consistency is good, I can't help to think something is bad with this approach. I'm particularly worried about mixing the two up, which could happen even to experienced scripters. Also, I hope Safari or other browsers don't mind writing to the xhr instance

The only thing that bothers me about this solution (auto-eval depending on the mime-type) is that it prevents you from handling security issues (through the use of parseJSON), and implies evaluating the content of the response body twice if you want to do some filtering on the returned string before evaluating it (a common use is to convert dates to Date objects).

Come to think about it, shouldn't we just add a String.prototype.evalJSON and/or a String.prototype.parseJSON (if license issues permit it - it only says 'open-source')?

Seriously, isn't that enough:

myCallback = function(transport) {
  var data = transport.responseText.evalJSON();
}

evalJSON could just be something like this:

String.prototype.evalJSON = function() {
  return eval('(' + this + ')');
};

Note: John Wang posted an article on his blog a while ago about failing JSON requests in IE6 for large data sets. Comments confirm this and indicate similar issues with Firefox 1.5.0.7.

I've emailed Douglas Crockford and he confirmed that there is no license whatsoever attached to his JSON parser.

I've toyed with his implementation a little bit and I don't think we need something so powerfull. Here's what I suggest (it just adds an optional regex to check the string for valid JSON).

  String.prototype.evalJSON = function(sanitize){
    try {
      if (!sanitize || (/^("(\\.|[^"\\\n\r])*?"|[,:{}\[\]0-9.\-+Eaeflnr-u \n\r\t])+?$/.test(this)))
        return eval('(' + this + ')');
    } catch (e) { return null }
  }

sanitize is an optional param which uses a regex to check and reject malicious attempts. I've left it off by default for speed considerations (You'll be able to see the benchmarks in the attached tests) but that decision is questionable.

I've added automatic eval for 'application/json' mime-type and added boolean evalJSON (if headers aren't set) and sanitizeJSON (for security freaks) options. It needs testing, and I don't really see how we can simulate HTTP headers in the testing environment we are using.

I'm having some weird issues, so I'll post this patch tomorrow.

OK, There you go.

There's some issues in IE which I haven't fixed yet (Not sure if the code is the issue or the testing).

The tests currently contain references to some php files to create the HTTP headers, The files are not included in the diff. (If anybody has a bright idea on how we could test this in a more agnostic way, please speak up!)

I've put the Ajax and String unit tests online: Ajax tests and string tests.

! tests contain ref to some php files which aren't included.

About testing: I have 2 things in plan to enhance Ajax testing. First is the XHR mock object which is a highly-reflective class that simulates XHR; the second is teaching Webrick some new tricks. With these two we should be able to test absolutely every aspect of Ajax, we should be even able to fix latency issues!

I don't like the silent reject of malicious attempts. Couldn't an error be thrown or at least something? Also, the method returns 3 types of data: an object, null and undefined. I suggest only object or undefined.

I don't like the silent reject of malicious attempts. Couldn't an error be thrown or at least something? Also, the method returns 3 types of data: an object, null and undefined. I suggest only object or undefined.

good points.

I'm just unsure how to do that and keep evalJSON as a method of String without using a second try... catch block inside Ajax. I would like to keep evalJSON as a method of String though, because I think that's where it belongs.

Regarding what it should return, you are right, that's an omission, sorry. I'm wandering whether it should return null or undefined though. The previous implementation returned null so I was trying to keep it consistent.

! tests contain ref to some php files which aren't included.

OK, updated String.prototype.evalJSON to actually spit out some errors when the string is either badly formated or doesn't pass sanitization. Ajax now handles those exceptions through the onException callback.

All the tests now pass correctly in IE6, Firefox, Safari and Opera.

There's still that testing issue though...

OK... well, there's the news... and they're not good ones.

IE's transport object does not allow new properties to be set to it... nor does it allow existing properties to be modified.

There's an easy work-around (cloning the transport object if the response is JSON and the transport is not an instance of XMLHttpRequest), but I'm worried about the possible side-effects.

Any input on that particular issue would be welcomed.

Hi again, here's a cross-browser diff that adds the above mentioned hack (cloning transport) to deal with IE6 (& IE7) 'features'.

Again, I'm a little uneasy with this proposal - so your input is welcome.

Is there anything we might want/need to access from the response object which cloning would make impossible?

full cross-browser support through cloning the transport object when necessary

I feel uneasy thinking how the transport object gets cloned every time. I think we should keep the functionality of the native XHR instance intact and keep the extra functionality layer (properties, methods) on the Ajax.Request class.

Mislav,

The transport object only gets cloned if all the following conditions are met:

• request state == 'Complete' (no cloning otherwise)
• the browser is either IE6 or in IE7 with the XMLHttpRequest option turned off in browser preferences.
• the server is sending JSON data (in which case cloning is certainly not going to be the performance bottleneck, eval is).

However, I'm not a 100% satisfied with this solution.

I think it's an alright fix for now, but we should seriously start thinking about developing a custom cross-browser response object (a suggestion Sam made to me a while ago).

I'm going to start looking more seriously into this asap.

I too don't like having to clone the transport object in those certain cases even though I think that could still be a viable solution. However, I think I have a better solution which just dawned on me.. Why not eval the X-JSON header, then, if Content-type is 'application/json', eval it as well and Object.extend the X-JSON eval'ed response! This is more similar to your original suggestion, only rather than replacing it you would extend it.

This solution has the following advantages:

• Backwards compatibility with X-JSON response being the second argument to onXXX
• Won't break cases where bind was used to add additional arguments to onXXX
• Doesn't require writing to the transport object (or cloning)
• Keeps the API simple by using one object argument to handle both cases
• Users can migrate their code to Content-type without changing the JS code
• No data is lost unless properties in the Content-type object override properties in the X-JSON object. This is the only danger, but I think it is acceptable.

Colin

Hi Colin,

Thanks for the suggestion.

I'm in the middle of creating an Ajax.Response object which will - I hope - solve these issues and bring along much other niceties.

I hope to have it available soon.

features a new Ajax.Response object

(In [7018]) * Add Ajax.Response object which supports the following methods: responseJSON, headerJSON, getHeader, getAllHeaders and handles browser discrepancies in the other response methods. Add sanitizeJSON, evalJS and evalJSON to Ajax.Request. References #8122, #8006, #7295. * Add an isRunningFromRake property to unit tests. * Add support for Opera browser in jstest.rb. * know issues: file: protocol doesn't work at all in IE7 (#8259), solving (#4267) is nightmarish.

(In [7265]) prototype: Merge -r7016:HEAD from ../branches/ajax. Add Ajax.Response object which supports the following methods: responseJSON, headerJSON, getHeader, getAllHeaders and handles browser discrepancies in the other response methods. Add sanitizeJSON, evalJS and evalJSON to Ajax.Request. Closes #8122, #8006, #7295.