Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#4888 closed enhancement (fixed)

dojo.rpc.* handling of Json should probably use: json-comment-optional

Reported by: Jared Jurkiewicz Owned by: Jared Jurkiewicz
Priority: high Milestone: 1.0
Component: RPC Version: 0.9
Keywords: Cc:
Blocked By: Blocking:

Description

dojo.rpc.* handling of Json should probably use: json-comment-optional

Dmachi,

This was brought up to me by a couple co-workers that the Rpc code in dojo.rpc didn't support running in the more secure mode of comment filtered JSON. To support this, it's an absolutely trivial change to the three files to change the handleAs to:

json-comment-optional

I'll be attaching a patch for this. If you don't mind me just applying the patch and committing the enhancement, I'll go ahead and do it. I just wanted your blessing first, since this is your module. Thanks!

-- Jared

Attachments (1)

dojorpc.patch (1.3 KB) - added by Jared Jurkiewicz 12 years ago.
Simple patch to dojo.rpc code for supporting comment filtered JSON RPC services.

Download all attachments as: .zip

Change History (9)

Changed 12 years ago by Jared Jurkiewicz

Attachment: dojorpc.patch added

Simple patch to dojo.rpc code for supporting comment filtered JSON RPC services.

comment:1 Changed 12 years ago by Dustin Machi

Owner: changed from Dustin Machi to Jared Jurkiewicz

Go right ahead, the change is fine with me.

comment:2 Changed 12 years ago by Jared Jurkiewicz

Excellent. Thanks dmachi!

comment:3 Changed 12 years ago by Jared Jurkiewicz

Owner: changed from Jared Jurkiewicz to Adam Peller

Running a few tests on it. I think a change peller made to xhr is causing a problem. He's using a regular expression to trim off the start and end comments ... but I think it's breaking on comments inside the JSON. I don't know if it's even valid to have comments inside JSON, but.

In specific, the yahoo SMD file has comments embedded in it There at:

SyntaxError?: unterminated comment message=unterminated commentbootstrap.js (line 183) Objectbootstrap.js (line 283) Run Test: Objectbootstrap.js (line 294) TypeError?: this.svc.webSearch is not a function message=this.svc.webSearch is not a functionbootstrap.js (line 263) ERROR IN: (function () {var d = new doh.Deferred;if (window.location.protocol == "file:") {var err = new Error("This Test requires a webserver and will fail intentionally if loaded from file://");d.errback(err);return d;}console.debug("Run Test: ", this.svc);var td = this.svc.webSearch({[Error: Query filter requires field and constraints separated by a "="]});td.addCallbacks(function (result) {return true;if (result.ResultSet?.Result[0].DisplayUrl? == "dojotoolkit.org/") {return true;} else {return new Error("JsonRpc_SMD_Loading_Test failed, resultant content didn't match");}}, function (result) {return new Error(result);});td.addBoth(d, "callback");return d;})bootstrap.js (line 263) FAILED test: JsonP_test

Which I think is cased by the SMD file having:

]

}

/*

{

"name":"",

"serviceURL": "",

"parameters":[

{ "name":"street", "type":"STRING" },

]

}

*/

]

}

A commented block in it.

Routing to Peller for his take, since he made the change to base xhr with the regexp.

Won't commit this till we know how this should be properly handled.

Peller,

Input? I think that regexp has another issue to deal with. Argh. :-/

comment:4 Changed 12 years ago by Adam Peller

Milestone: 1.0

Crockford said today that comments are not valid in JSON, yet his own company is using them. Clearly, we have to be tolerant of this, so I'll probably just revert the regexp change. It was an attempt to reduce the code to something more clear and efficient.

I'm a little confused as to why this is taking place in this ticket, though. We've been trying to keep Dojo trac tickets short and focused.

comment:5 Changed 12 years ago by Adam Peller

(In [11157]) I give up. Put back the indexOf searches for comments in place of regexp attempt. Refs #3961, #4829, #4888 Reverts part of change in [10834]

comment:6 Changed 12 years ago by Adam Peller

Owner: changed from Adam Peller to Jared Jurkiewicz

comment:7 Changed 12 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [11190]) Committing in minor enhancement for RPC. fixes #4888

comment:8 Changed 12 years ago by Jared Jurkiewicz

(In [11191]) After more looking, I don't think you can do jsonp comment filtered. So reverting #4888. refs #4888

Note: See TracTickets for help on using tickets.