Wrapping does not work

[srgl]

It looks like the 8701d62 has broken the wrapping functionality. Redis adapter returns null in case there is no specified key, as a result .wrap doesn't call workload and always return null.

As a workaround, there is the isCacheableValue option.

[jeff-kilbride]

@srgl I see what you are saying. In that commit, I changed the default isCacheableValue() from this:

  self.isCacheableValue = args.isCacheableValue || function(value) {
    return value !== null && value !== undefined;
  };

to this:

  self.isCacheableValue = args.isCacheableValue || function(value) {
    return value !== undefined;
  };

Redis allows storing null as a value, so that's what we were trying to accomplish.

However, something is strange, because the default isCacheableValue() in the cache-manager code is the same as the new commit:

    if (typeof args.isCacheableValue === 'function') {
        self._isCacheableValue = args.isCacheableValue;
    } else if (typeof self.store.isCacheableValue === 'function') {
        self._isCacheableValue = self.store.isCacheableValue;
    } else {
        self._isCacheableValue = function(value) {
            return value !== undefined;
        };
    }

The default implementation only tests for undefined. So, I'm a bit confused as to how .wrap is working at all with the default settings. Maybe Redis is the only store that allows null values and returns null, rather than undefined, for missing keys. This may be a problem that needs to be fixed in cache-manager itself. I am going to post something on their issues and see what kind of answer I get.

We can always revert to testing for null in isCacheableValue(), but that would also prevent anyone from storing null as a value. The other option is to have a warning for anyone using .wrap to implement a custom isCacheableValue() that also tests for null -- and to realize that they won't be able to store null explicitly as a value.

I would like some input from @PuKoren, @mrister, and @jkernech on this, too. It's definitely a problem in 0.2.5.

[jeff-kilbride]

Ok, just looking through the issues and PRs at cache-manager, this is a known issue:

jaredwray/cacheable#25
jaredwray/cacheable#29

Somebody suggested having separate isCacheableValue() and isReturnableValue() functions to help deal with this, but that idea was never merged:

jaredwray/cacheable#41
jaredwray/cacheable#42

So, the question is, what is the best way to deal with this now? We either:

1. Allow null values to be stored by default and require anyone using .wrap to implement their own isCacheableValue().
2. Disallow null values by default and require anyone who wants to store null to implement their own isCacheableValue().

Choose your poison. Which do we think is best?

EDIT: The more I think about it, since null is a valid, storable value, the redis package should really return undefined for missing keys -- but I doubt that will ever change...

Also, option 1 (the way it is in 0.2.5) is a breaking change for anyone using .wrap and should really be a bump to 0.3.0.

[srgl]

@jeff-kilbride Simple logic: as the redis adapter returns null in case there is no specified key and we hardly can change this, the .wrap should work out-of-box, without redefining the isCacheableValue option.

[PuKoren]

This is a difficult decision, as I personally think null is a valid cache value in application (and indeed not in redis). What do you think of checking if key exists in cases where redis returns null? It will make a double-check but since in applications redis cache manager should be coupled with memory cache for better performances, it should not happen often.

What do you think about it?

[mrister]

Indeed a difficult decision. We must take into account our current behaviour regardless of .wrap so changing isCacheableValue (in this module) might be a bad option. Checking if key exists is an interesting option. IMHO we should keep it as it is now (isCacheableValue looks only for undefined also in default implementation as Jeff mentioned) and utilise the key exists call. Anybody else can adjust with isCacheableValue. I don't think we will be able to make every use case work with cache-manager out of the bo .

[jeff-kilbride]

Since Redis allows null values to be stored, IMHO we should support that as the default. The .wrap method is _extra_ functionality and I don't think it's too much of a burden to require people using it to define their own isCacheableValue. It's only one line of code. It just needs to be clearly explained with an example in the docs.

I use .wrap all the time -- and never store null values. And, honestly, I rarely use a separate memory cache unless it's really needed. I would rather define my own isCacheableValue than do any double checking, but that's just me. The exists command would have to be called for every cache miss, since redis returns null for non-existing keys. Depending on your ttl and cache hit ratio, that could be significant. Especially when initially loading a large cache. If you guys feel strongly about using exists, I'm okay with that -- but it's not my preference.

However, the change in 0.2.5 is a breaking change for anyone currently using .wrap. I really think this should be reverted and released again as 0.2.6 to prevent problems. The new implementation that allows null values, and requires a custom isCacheableValue to use .wrap, should probably be an 0.3.0 release to signify the incompatibility.

Let me know what you want to do and I can submit a PR.

[PuKoren]

If there is a breaking change, it should be tagged as major 1.0.0, otherwise every projects including ^0.x.x version will upgrade.

About the change for wrap, since redis returns null and it is a cacheable value, wrap just stopped working if redis is empty for the key - I don't think it is a breaking change, it is more likely a bug

I'm fine with reverting, but we should think about something because this means that users that wants to store null values and are redefining isCacheableValue will have a broken wrap with this module, for all currently released versions

[jeff-kilbride]

Yeah, people are already having issues. See #57.

I only called it a "breaking change" because I changed the default isCacheableValue. You're probably right that it should be called a "bug" instead.

If we revert, it shouldn't affect anyone that has already redefined isCacheableValue. If they have a custom isCacheableValue, the default version won't run.

[PuKoren]

People redefining isCacheableValue to allow null value will face the same issue as we have now - bug will not be here by default, but there is still a hidden bug (now revealed) in all current versions

@jeff-kilbride: would you like to be added to the contributors of this project? Your insights are helpful and you are an active contributor, it will allow you to work directly on this project branches

[jeff-kilbride]

I see what you're saying. I guess I'm more concerned with people using the default version, which is now broken without any warning.

[PuKoren]

You are right. Do we all agree to revert and re-release (with zlib, as 0.3.0) while we think about a fix and let this issue open? @dial-once/developers @jeff-kilbride?

Edit: wut, 3 messages in 3 seconds 👍

Below: yes, we can release zlib IMO. Ok to revert the commit on 0.2.5 and release as 0.2.6, and release develop on 0.3.0?

[jeff-kilbride]

I basically have this reverted in a new branch, but I just noticed that you merged the compression stuff into develop. Do you want to do the bugfix off of master? Or do you want to go ahead and release the compression stuff at the same time as the bugfix?

[srgl]

@jeff-kilbride agree. Spent three hours investigating why wrapped function start to return null value :)

[jeff-kilbride]

If we revert, I think it should be 0.2.6. Anybody with ~0.2.x won't pickup 0.3.0. They'll be stuck at 0.2.5 until they update their package.json. We could do a bugfix branch off master just to revert the problem as 0.2.6, then release 0.3.0 with the new default isCacheableValue and compression when we're ready.

What do you think?

[PuKoren]

Yep, totally agree with 2 releases

[jeff-kilbride]

@PuKoren I merged my upstream master into my local repo, but I don't see the v0.2.5 tag. I only have up to v0.2.4. How can I get v0.2.5 locally?

[jeff-kilbride]

Here is what I have:

[2016-11-10 03:12:39]-[jeff@JMBP]-[~/WebstormProjects/node-cache-manager-redis]-[git master]
$ git fetch upstream
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
Unpacking objects: 100% (1/1), done.
From https://github.com/dial-once/node-cache-manager-redis
   b51a6a3..bab8a57  develop    -> upstream/develop

[2016-11-10 03:13:51]-[jeff@JMBP]-[~/WebstormProjects/node-cache-manager-redis]-[git master]
$ git merge upstream/master
Updating 20c91ac..8f18b34
Fast-forward
 index.js                               |  16 ++++++-----
 package.json                           |  11 ++++----
 spec/support/jasmine.json              |   6 -----
 {spec => test}/config.json             |   0
 {spec => test}/lib/redis-store-spec.js | 208 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------
 5 files changed, 145 insertions(+), 96 deletions(-)
 delete mode 100644 spec/support/jasmine.json
 rename {spec => test}/config.json (100%)
 rename {spec => test}/lib/redis-store-spec.js (65%)

[2016-11-10 03:14:23]-[jeff@JMBP]-[~/WebstormProjects/node-cache-manager-redis]-[git master]
$ git tag
v0.1.0
v0.1.1
v0.1.10
v0.1.11
v0.1.12
v0.1.2
v0.1.3
v0.1.9
v0.2.0
v0.2.1
v0.2.2
v0.2.3
v0.2.4

[PuKoren]

@jeff-kilbride forgot to push it, should be fine now, sorry about that!

[jeff-kilbride]

Ok, I have reverted isCacheableValue to test both undefined and null. I have commented out the tests that try to store a null and I have added a test for the .wrap method. See #58.

[PuKoren]

Thanks @jeff-kilbride, we merged it and published it in 0.2.6 and above