Should return null if setting is JSON and cookie contains invalid JSON?

[danielcha]

In an attempt to make sure my site doesn't break nomatter how bad people try to mess with my preference cookies I found out the library breaks if the cookie contains either no or invalid JSON.

I am now solving this with a try/catch:

try {
    prefs = $.cookie('prefs');
} catch (e) {
    nukeCookieAndSetDefaultSettings();
    return false;
}

Perhaps the library should return null if pasing the JSON fails?

[sharipov-ru]

+1

danielcha, how have you cleared invalid JSON?

[carhartl]

IMO it's never a good idea to fail silently on errors. It's hiding issues from a user. I think how you solved it is a proper solution, for your particular case.

[carhartl]

Though what is the case with "no json"? That seems to be a bug.

[carhartl]

I mean, with json=true, the plugin should handle arbitrary values nonetheless.

[danielcha]

I agree with the the fact it not a good idea to fail silently in most cases. In my code this catch should never be used. However, if a user or program alters my cookies content for whatever reason, the javascript will break and stop execution. As my site is very javascript heavy, it's an unacceptable UX as most stuff won't work. Also, being cookie based, refreshing the page won't solve the issue, leaving the average user clueless why it's not working.

Because I always try to keep in mind all possible scenarios I built in the try/catch. However, my guess would be there are plenty of programmers who don't and as soon as a cookie becomes corrupt the site breaks, with no clear solution to fix it from an average user perspective. This is why I think it should NULL as if the cookie does not exist, or another error enabling the programmer to validate the cookie.

[FagnerMartinsBrack]

IMO it's never a good idea to fail silently on errors

I dont't think null is a bad idea since jQuery also does not throw any exception on any API. Since the cookie is invalid it could be considered inexistent in the POV of a programmer that uses the json flag (which expects a valid JSON or nothing at all)
Since I never ran to this issue I am just especulating, maybe a little more feedback would be useful.

[carhartl]

What I meant was that I'd prefer not to return null in case JSON is invalid, simply because the developer would never know the reason why it is null - why it's not written that is - and thus were never able to fix the root cause of this.

But yeah, I'd probably even more prefer to be consistent with jQuery here.

[carhartl]

Regarding cookie not containing valid JSON in the first place, I can simulate like so:

$.cookie('test', 'foo') // => "test=foo"
$.cookie.json = true
$.cookie('test') // => SyntaxError: JSON Parse error: Unexpected identifier "foo"

This would usually happen with server-side written cookies I assume, because this works fine:

$.cookie.json = true
$.cookie('test', 'foo') // => "test=%22foo%22"
$.cookie('test') // => "foo"

Is there an alternative to return null? Logging a warning (could do that anyway)?

[muxa]

I think throwing an exception is perfectly valid if JSON parsing failed. And it's possible to trap it by the caller with try .. catch as @danielcha pointed out.
The problem with returning null is that there's no way to know if the cookie was non-existent or it had a value but it was not a valid JSON.

[danielcha]

Well... I would prefer the error gets logged, instead of throwing an exception. Because in most cases the cookies are not critical for the operation of a website. However, other Javascript I use is. So if the cookie is "corrupt" and the cookie has a long expiration date... javascript will break. And yes, the Try/Catch is a solution. However, less experienced programmers might forget this and it can break a complete website for users with corrupt cookies.

I would let the script either return null (a corrupt cookie needs to be removed/overwritten, thus one could argue that it is the same as not having a cookie at all). Or et it return a string with "error" and/or log a warning. Then your code could be something like. Then you could validate if the cookie is not null AND is not "error".

[muxa]

Here's another idea: how about returning the SyntaxError object itself?

[carhartl]

fwiw

// jQuery
$.parseJSON('foo') // => JSON Parse error: Unexpected identifier "foo"

[muxa]

Another option is to throw exception by default and add a config option to return null if parsing fails. E.g:

$.cookie.json = true;
$.cookie.safeJSON = true; // would make invalid JSON values return null instead of throwing exception

[danielcha]

I'm not an expert in semantics or anything, but I think that's actually a good solution :-)

[FagnerMartinsBrack]

A new property would be one more thing to document and one more thing for ppl to learn, better keep or change the behaviour based in the most intuitive use.
I still think nothing should be thrown at all, let the developer debug his code and find out why the JSON is malformed, javascript is very bad for handling exceptions in the regular developer flow side.

[kirpit]

I'm not sure if this is the same issue but seems like these versions are a bit unstable:

$.cookie.json = true;
$.cookie('dummy', 'A');
console.log($.cookie('dummy'));
// => Uncaught SyntaxError: Unexpected token A 

[carhartl]

Use json = true when you're dealing with JavaScript objects you'd like to have automatically serialized and parsed back, for example { "A": "foo" }. Don't use it for primitive (string) values, such as "A".

Whether we're throwing an error or simply return nothing for such a case of wrong usage is currently to be discussed here.

[kirpit]

I see, but still some questions here. Although you do a $.cookie('dummy', JSON.stringify('A')); with json = true, I'm not sure if the value should be stored as %22%5C%22A%5C%22%22 or as "\"A\""

I haven't check the standards but the second option what django does. Which means, if you use jquery.cookie on a django platform, you have to do decoding before json parsing on every cookie you set from client-side. However, it works vice versa that jquery.cookie can read every back-slash escaped values just fine.

On the other hand, I've read everything under this topic, not a js ninja to admit but just talking from the cocky high level; I have some values as regular strings, some values json objects, some server-side values that I can't control at all (i.e. session id). So, setting json = true or false on a global setting for every second cookie read sounds just weird for me. Again, may sound radical but how about $.cookieJSON('dummy') and $.cookieJSON('dummy', 'a value to be stringified') approach?

[FagnerMartinsBrack]

Hi @kirpit,

JSON.stringify should be used passing an Object Literal to be converted into a JSON string:
JSON.stringify({ key: "value" }); //"{"key":"value"}" and should not be used with a string like JSON.stringify('A') or JSON.stringify('{ "key": "value" }').

If you want to pass an object literal from a string to the json cookie creation you should do $.cookie( "dummy", JSON.parse( '{"key": "value"}' ) ).

For more info check http://json.org/ for JSON (format) spec and http://es5.github.com/#x15.12 for JSON (javascript Object) spec

In version 1.3.0 #143 happened and even non related cookies were throwing an error when checking if the cookie existed or not, but that was fixed in 1.3.1.

Now the discussion is to decide if the exception should be thrown when checking an invalid JSON cookie existence or return null and suppress the exception.

[kirpit]

Maybe I should open another issue, but I simply can't read $.cookie('jsonVal', { key: "value" }); with json = true from server-side without doing low-level percent decoding stuff.

[FagnerMartinsBrack]

Open a new issue describing how you are receiving the cookie values on server-side so we can discuss the proper way to handle it. I will be using jquery cookie's json feature in a huge project soon, so I am particularly interested on any issue related to it.
I believe this is not directly related to this issue.

[carhartl]

Back to the original question:

I dont't think null is a bad idea since jQuery also does not throw any exception on any API. Since the cookie is
invalid it could be considered inexistent in the POV of a programmer that uses the json flag (which expects a
valid JSON or nothing at all)

I finally tend to agree with it, for one consistent behavior trumps** and the notion "invalid == not existing" from the POV of the programmer also seems reasonable. There's just not much to do about an invalid JSON string apart from writing a new (valid) one. So let's go that route, but I believe we need to return undefined now since that had just changed in 265617e.

** (though $.jsonParse will throw an error when passing an invalid JSON string)

The only other thought I had was making json = true obsolete in the first place. Because I have the feeling juggling the json config all the time when dealing with both types of cookies will turn out not to be very practical. So when passing an object literal we would just always stringify it. When reading, we would attempt to parse the object back from string, catch any error and return the value unparsed in the error case (and thus returning the string if it was a "normal" string in the first place). The only issue with that were that we'd potentially return invalid stringified JSON like in the case mentioned by the issues reporter. I can see other questions derive from that approach: Will developers expect to have numbers and booleans converted back in the same fashion we would do with objects automatically? Were there other issues I am overlooking right now when we were handling JSON automatically under the hood?

[carhartl]

Other idea: Allow setting up converters:

// write and read converters
$.cookie.converter = [JSON.stringify, JSON.parse];
// single function becomes read converter
$.cookie.converter = function (value) { return parseInt(value); };

That could even make deprecation of json = true work (upon first read/write, JSON converters were set up when we find config.json to be true).

Converters could be build in a way that they skip values with say a particular prefix. I'm seeing much gained flexibility for developers with this approach.

[carhartl]

<- New issue

[danielcha]

tnx for the fix man! :-)