Opened 12 years ago
Closed 12 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)
Change History (9)
comment:1 Changed 12 years ago by
Owner: | changed from anonymous to Jared Jurkiewicz |
---|
comment:2 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
Milestone: | tbd → 1.4 |
---|---|
severity: | major → normal |
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:7 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.