Bad auto-detection for json-comment-optional

[frietsch]

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.

[James Burke]

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.

[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?

[Jared Jurkiewicz]

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

[Jared Jurkiewicz]

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?

[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.

[Jared Jurkiewicz]

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.

[James Burke]

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.}

[frietsch]

IMO, this should be enough...

[James Burke]

(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.