Custom isCacheableValue tests may not be working correctly

[jeff-kilbride]

I'm working on the tests for gzip functionality and I noticed a few things. Since you have changed the way undefined values are being handled in the set method, the custom isCacheableValue function in your tests may not be working correctly anymore in the get method. It's a little confusing, so I will try to be clear.

In the set method, you now have:

var val = JSON.stringify(value) || 'undefined';

which means all undefined values are being stored as the string 'undefined'. However, in your custom isCacheableValue test (redis-store-spec.js: 25-29, 119-138), you are only testing for the value undefined, not the string.

if (val === undefined) return true;

This works correctly when you call the custom isCacheableValue function from the set method, before undefined has been converted to a string. However, when you call the custom isCacheableValue function from the get method, after undefined has been converted to a string, this test returns false. It then falls back to the default redisCache.store.isCacheableValue function -- which returns true.

return value !== null && value !== undefined;

Ultimately, this works, but it's a bit confusing and I'm not sure this is what you intended. I had to step through the test code with a debugger to see what was actually happening. It would be clearer if the custom isCacheableValue function explicitly tested for both undefined and 'undefined'.

if (val === undefined || val === 'undefined') return true;

This would work correctly from both the set and get methods without falling back to the default implementation. (See below for a different possible solution)

---

Also, I don't think this statement in the set method is working correctly:

    if (!value && !self.isCacheableValue(value)) {
      return cb(new Error('value cannot be ' + value));
    }

This will work when value is undefined, but not for other cases. In the above, any time value is truthy the entire test becomes false, and self.isCacheableValue won't be called due to short-circuiting. For example, if I don't want to store the string value 'FooBar' and I create a custom isCacheableValue function that returns false for 'FooBar', it won't work. My custom isCacheableValue function will never get called. The string 'FooBar' is truthy and !'FooBar' causes the entire if statement to evaluate false.

---

Finally, I believe if you store undefined values as a JSON parseable string in the set method, like this:

var val = JSON.stringify(value) || '"undefined"';

or using JSON.stringify like this:

var val = JSON.stringify(value) || JSON.stringify('undefined'); (same result as above...)

you can simplify the logic in the handleResponse function and get rid of this test:

if(! ( (result === undefined || result === 'undefined') && typeof args.isCacheableValue === 'function' && args.isCacheableValue(result)))

This if test guards against JSON.parse errors in the next line of code. However, since all undefined values are now being converted to strings, there should be no need to test for undefined explicitly (result === undefined). All other values being stored are being sent through JSON.stringify, so the only value you are guarding against is 'undefined'. Right now, JSON.parse will throw an error if you remove this test, because the string 'undefined' is not valid JSON (not surrounded in double-quotes). If you store it as '"undefined"', JSON.parse will not throw an error and you shouldn't need this test. (As a bonus, it also gets rid of the call to isCacheableValue from the get method, which eliminates the first issue above...)

Sorry for the length of this! Hopefully, it all makes sense. I didn't want to address these issues in my current PR, since they have nothing to do with the gzip implementation.

[mrister]

@jeff-kilbride Thank you very much for analyses of code. Would you mind making another PR with the changes you think should be in there so we can review and test ? Thanks again.

[jeff-kilbride]

No problem. However, I just realized that the if test in the get method is also guarding against null return values for invalid keys. So, getting rid of that if statement entirely probably won't work. But, I still think it can be simplified.

I will work on a different PR for this. Some of this affects my gzip code, so I would like to get this resolved first.

EDIT: Actually, it did work. I was able to remove that if statement. JSON.parse(null) is just null...

[mrister]

Whichever you prefer and we are all up for any simplifications and code readability improvements you can find and are willing to implement.

[jeff-kilbride]

Fixed by #48.