Opened 11 years ago

Closed 10 years ago

#9133 closed defect (fixed)

Bad auto-detection for json-comment-optional

Reported by: frietsch Owned by: James Burke
Priority: high Milestone: 1.4
Component: Core Version: 1.3.0
Keywords: Cc:
Blocked By: Blocking:

Description

Even though I'm using JsonService? with json-prefixed server data ({}&& ), I sometimes get warnings that I shouldn't use json-commenting. In those cases, data retrieval completely fails.

It took me a while to realize that Dojo's default "handleAs" parameter for JsonService? is "json-comment-optional", implying an auto-detection if the data is commented or not.

This auto-detection is done in a very simple way, as it just checks if "/*" appears anywhere in the data. If so, only the data between the first /* and */ is evaluated. Unfortunately, my JSON data contains Strings with some of them containing "/*". I think it should pass the auto-detection nevertheless, as this is valid JSON IMO.

Maybe one should require the "/*" to appear in the beginning of the data, after stripping possible spaces? At least, I would like to be able to pass a user-defined "handleAs" to JsonService? when creating one. Currently this does not seem to be possible.

Attachments (2)

dojo_xhr.patch (516 bytes) - added by Jared Jurkiewicz 11 years ago.
Patch to trim whitespace and verify /* is the start of the data before deciding it is comment filtered.
dojo_xhr2.patch (557 bytes) - added by Jared Jurkiewicz 11 years ago.
Better patch, validates /* occurs before any { or ], and if it does, assume comment filtered. should handle the case the user reports where they put /* in a text string of their code.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 11 years ago by James Burke

Owner: changed from anonymous to Jared Jurkiewicz

json-comment-filtered is deprecated now since it does not provide robust protection. The stores probably should change the handleAs values to just be "json". Assigning to jaredj to get his take.

comment:2 Changed 11 years ago by Jared Jurkiewicz

We need to keep the behavior as in in the stores to provide backwards compabitility.

Dojo's json-comment-optional parser shouldn't be barfing on this.

Needs to be fixed in base.

Adding a user defined handle is also probably appropriate for the services/stores as well, but we need to continue to have json-comment-optional as the default for compatibility.

Who owns/manages the comment-optional checking code?

Changed 11 years ago by Jared Jurkiewicz

Attachment: dojo_xhr.patch added

Patch to trim whitespace and verify /* is the start of the data before deciding it is comment filtered.

comment:3 Changed 11 years ago by Jared Jurkiewicz

Owner: changed from Jared Jurkiewicz to James Burke

Except .... that patch doesn't work with whacky (and frankly invalid) json:

blag /*{"foo":"bar","baz":[{"thonk":"blarg"},"xyzzy!"]}*/

Which is used in one of the xhr test cases. I'm not sure we should even be allowing for that sort of forming. Need the person who added it to speak up why it was needed.

James, was that you?

comment:4 Changed 11 years ago by Jared Jurkiewicz

Here's a better patch thanks to Doug Hays!

Regexp check that /* occurs before any starting { or ], which I believe should work good enough for the intention of this test.

Changed 11 years ago by Jared Jurkiewicz

Attachment: dojo_xhr2.patch added

Better patch, validates /* occurs before any { or ], and if it does, assume comment filtered. should handle the case the user reports where they put /* in a text string of their code.

comment:5 Changed 11 years ago by James Burke

Milestone: tbd1.4
severity: majornormal

Hmm OK. Unfortunate that it still needs to be supported. I'll fix base, but I think using a regexp like /\s*\/\*/ would be good enough? I will likely just use that regexp.

comment:6 Changed 10 years ago by frietsch

IMO, this should be enough...

comment:7 Changed 10 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [17474]) Fixes #9133: improve json-comment-filtered. Thanks to doughays for the basic patch: looks like our unit tests allow for other code, like while(true); block before the start of the comment, so we need to look for explicit starts of an object or array.

Note: See TracTickets for help on using tickets.