Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#7817 closed enhancement (fixed)

Create DojoX validating JSON module

Reported by: Matt Sgarlata Owned by: Kris Zyp
Priority: high Milestone: 1.3
Component: General Version: 1.2beta
Keywords: Cc: development-staff@…
Blocked By: Blocking:

Description

There is no explicit error handling code in dojo.fromJson, so if you have some invalid JSON then you could be in for a nasty surprise. In FF I got "missing ) in parenthetical" and in IE I got "Line: 4, Error: Expected ')'". This can be very difficult to debug, and it would be immensely helpful if dojo could provide some help. Here is a revised version of the function with some explicit error handling. I am also including a test page where you can compare the current dojo behavior with the behavior of this improved function

dojo.fromJson = function(/*String*/ json){
	// summary:
	// 		Parses a [JSON](http://json.org) string to return a JavaScript object.  Throws for invalid JSON strings.
	// json: 
	//		a string literal of a JSON item, for instance:
	//			`'{ "foo": [ "bar", 1, { "baz": "thud" } ] }'`

	try {
		return eval("(" + json + ")"); // Object
	}
	catch (e) {
		console.warn("Invalid JSON: '" + json + "'");
		throw Error("Invalid JSON: " + e.message);
	}
}

Attachments (2)

dojoBug7817Test.html (779 bytes) - added by Matt Sgarlata 11 years ago.
Example page comparing existing and improved dojo.fromJson functions
Picture 3.png (201.0 KB) - added by Matt Sgarlata 11 years ago.
Screenshot of this error in an actual app. It's very difficult to see where the error is coming from

Download all attachments as: .zip

Change History (16)

Changed 11 years ago by Matt Sgarlata

Attachment: dojoBug7817Test.html added

Example page comparing existing and improved dojo.fromJson functions

comment:1 Changed 11 years ago by Matt Sgarlata

BTW, I think this would have alleviated a lot of the pain and suffering from the user who submitted bug 7082: Dojo 1.0.2 to Dojo 1.1/1.1.1 migration - dojo.fromJson

http://bugs.dojotoolkit.org/ticket/7082

Changed 11 years ago by Matt Sgarlata

Attachment: Picture 3.png added

Screenshot of this error in an actual app. It's very difficult to see where the error is coming from

comment:2 Changed 11 years ago by dante

keep in mind, try{}catch(){} is an expensive function, and likely not something (me personally at least) that should be used for convenience functions, but rather to actually trap occasional exceptions in edge cases.

comment:3 Changed 11 years ago by Matt Sgarlata

I am no expert here, but according to the site below, try/catch is only expensive if the catch is executed.

http://dev.opera.com/articles/view/efficient-javascript/?page=2

So here the catch would only be expensive if there was in fact an error. So this would only be a concern if you were doing lots and lots of dojo.fromJson calls and lots and lots of them had errors.

I think you kind of hit the nail on the head there about convenience functions: they are to provide convenience to the user. In this case the convenience would be an error message letting the programmer know what's going on as opposed to just passing through a lower-level error.

comment:4 Changed 11 years ago by Adam Peller

code size is another reason why we don't want try/catch/log, especially in base. Perhaps this method could be overridden / attached to provide this extra behavior in debug mode? Throwing an exception, though, is about as explicit as you can get when it comes to debugging. Unless the caller does something like squelch the exception, which is bad programming practice IMO, shouldn't this be easy to debug as-is?

comment:5 Changed 11 years ago by Les

I agree this would be a helpful feature. I slightly modified the code as in Prototype.js:

dojo.fromJson = function(/*String*/ json){
	// summary:
	// 		Parses a [JSON](http://json.org) string to return a JavaScript object.  Throws for invalid JSON strings.
	// json: 
	//		a string literal of a JSON item, for instance:
	//			`'{ "foo": [ "bar", 1, { "baz": "thud" } ] }'`

	try {
		return eval("(" + json + ")"); // Object
	}
	catch (e) { }
	throw new SyntaxError("Invalid JSON string: " + json);

}

comment:6 in reply to:  4 ; Changed 11 years ago by Matt Sgarlata

Replying to peller:

Perhaps this method could be overridden / attached to provide this extra behavior in debug mode?

That would work for me

Throwing an exception, though, is about as explicit as you can get when it comes to debugging. Unless the caller does something like squelch the exception, which is bad programming practice IMO, shouldn't this be easy to debug as-is?

I agree 100% throwing an exception is what should be done here. However, IMO exceptions should be appropriate to the layer in which they originate and higher layers should not simply propagate low level exceptions all the way up the stack. For example, if there is a low level network I/O problem that caused a database connection to fail, I would expect to receive a message along the lines of "database connection failed" not the low level I/O error. dojo.fromJson has a very specific semantic meaning, so if there is a problem with what is passed into that method then I believe dojo should make that clear by throwing a new exception that includes information about the original exception.

Particularly in this case, and in the case mentioned in bug 7082, the error messages provided from the browser are not specific enough to be meaningful. When the browser reports "missing ) in parenthetical" I know that means that I have a syntax error somewhere, but considering my entire application is JavaScript? this message really doesn't tell me a darn thing. Firefox might as well report "error somewhere in your application. have fun figuring out what it might be!"

Another advantage of this approach is that it will make the behavior of dojo.fromJson more uniform across browsers because right now the exceptions thrown by dojo.fromJson are browser dependent. With the new approach they will be a consistent type of error that can be counted on regardless of browser.

comment:7 in reply to:  6 Changed 11 years ago by Adam Peller

Yes, the error message is inconsistent and cryptic, but that's not unique to fromJson -- try finding a syntax error in the loader, for example (and it doesn't stop there) I feel this is consistent with our coding style to avoid bounds checking, rethrowing exceptions, logging, etc. A debugger *could* catch the exception as it's thrown, though I agree a debug option to rethrow could be very helpful. I'd prefer we not add code like this to base (non-debug), however.

comment:8 Changed 11 years ago by dante

can we mark this wontfix? Seems the rule is to keep try{}'s out of base and just let errors fly ...

comment:9 Changed 11 years ago by bill

+1 from me. I hate try/catch blocks in our code as they both add bloat and make things harder to debug (in firebug etc.) by masking where the real error happened.

comment:10 Changed 11 years ago by Eugene Lazutkin

I think that the original problem in the description of the ticket should be addressed head on: we should provide a validating JSON parser somewhere outside of the Base (in DojoX?) specifically for detecting server-side problems. While it usually solved by generating JSON using numerous freely available helpers, I saw these problems in the wild on several occasions.

comment:11 Changed 11 years ago by dante

Milestone: tbdfuture
Owner: changed from anonymous to Kris Zyp
Summary: Improve error handling in dojo.fromJsonCreate DojoX validating JSON module

@kris - comments?

comment:12 Changed 11 years ago by Kris Zyp

I agree with Matt, this is real issue. The argument about performance with try/catch is irrelevant in this context, try/catch does have a performance hit, but it is orders of magnitude smaller than the eval cost. But, I do understand the concern for keeping base's size small. I would be glad to add this functionality to dojox.json.ref.fromJson, the *other* JSON parser in Dojo. Does that count?

comment:13 Changed 11 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

You can now use dojox.json.ref.fromJson to do JSON parsing with the try/catch.

comment:14 Changed 11 years ago by Adam Peller

Milestone: future1.3

batch move of tickets marked 'future' in the 1.3 timeframe

Note: See TracTickets for help on using tickets.