From a803db415b1b2d3b9f7e890d8af2bb495a31298a Mon Sep 17 00:00:00 2001 From: Bryan Donovan Date: Thu, 8 Oct 2015 16:28:06 -0700 Subject: [PATCH 1/3] multi-caching: using underlying store's isCacheableValue function when it exists --- lib/multi_caching.js | 38 +++++++++++++++++--- test/multi_caching.unit.js | 71 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 5 deletions(-) diff --git a/lib/multi_caching.js b/lib/multi_caching.js index df499a3..7a0b8ba 100644 --- a/lib/multi_caching.js +++ b/lib/multi_caching.js @@ -12,6 +12,9 @@ var CallbackFiller = require('./callback_filler'); * with every value returned from cache or from a wrapped function. This lets you specify * which values should and should not be cached. If the function returns true, it will be * stored in cache. By default it caches everything except undefined. + * + * If an underlying cache specifies its own isCacheableValue function, that function will + * be used instead of the multiCaching's _isCacheableValue function. */ var multiCaching = function(caches, options) { var self = {}; @@ -31,6 +34,19 @@ var multiCaching = function(caches, options) { }; } + /** + * If the underlying cache specifies its own isCacheableValue function (such + * as how node-cache-manager-redis does), use that function, otherwise use + * self._isCacheableValue function. + */ + function getIsCacheableValueFunction(cache) { + if (cache.store && typeof cache.store.isCacheableValue === 'function') { + return cache.store.isCacheableValue; + } else { + return self._isCacheableValue; + } + } + function getFromHighestPriorityCache(key, options, cb) { if (typeof options === 'function') { cb = options; @@ -43,7 +59,10 @@ var multiCaching = function(caches, options) { if (err) { return next(err); } - if (self._isCacheableValue(result)) { + + var _isCacheableValue = getIsCacheableValueFunction(cache); + + if (_isCacheableValue(result)) { // break out of async loop. return cb(err, result, i); } @@ -59,7 +78,13 @@ var multiCaching = function(caches, options) { function setInMultipleCaches(caches, opts, cb) { opts.options = opts.options || {}; async.each(caches, function(cache, next) { - cache.store.set(opts.key, opts.value, opts.options, next); + var _isCacheableValue = getIsCacheableValueFunction(cache); + + if (_isCacheableValue(opts.value)) { + cache.store.set(opts.key, opts.value, opts.options, next); + } else { + next(); + } }, cb); } @@ -78,11 +103,14 @@ var multiCaching = function(caches, options) { cb(err, result); - if (self._isCacheableValue(result) && index) { + if (index) { var cachesToUpdate = caches.slice(0, index); async.each(cachesToUpdate, function(cache, next) { - // We rely on the cache module's default TTL - cache.set(key, result, next); + var _isCacheableValue = getIsCacheableValueFunction(cache); + if (_isCacheableValue(result)) { + // We rely on the cache module's default TTL + cache.set(key, result, next); + } }); } }); diff --git a/test/multi_caching.unit.js b/test/multi_caching.unit.js index 185ef76..dbe5c6d 100644 --- a/test/multi_caching.unit.js +++ b/test/multi_caching.unit.js @@ -534,6 +534,77 @@ describe("multiCaching", function() { }); }); }); + + context("when an underlying store has its own isCacheableValue function", function() { + var memoryCache4; + + var testCallbacks = { + isCacheableValue: function(value) { + var x = value !== 'do_not_store_this' && value !== undefined; + return x; + } + }; + + function getValue(name, cb) { + process.nextTick(function() { + if (name === 'foo') { + cb(null, 'store_this'); + } else { + cb(null, 'do_not_store_this'); + } + }); + } + + function getCachedValue(name, cb) { + multiCache.wrap(key, function(cacheCb) { + getValue(name, function(err, result) { + cacheCb(err, result); + }); + }, cb); + } + + beforeEach(function() { + sinon.spy(testCallbacks, 'isCacheableValue'); + memoryCache4 = caching({ + store: 'memory', + ttl: memoryTtl + }); + memoryCache4.store.isCacheableValue = testCallbacks.isCacheableValue; + multiCache = multiCaching([memoryCache4]); + sinon.spy(memoryCache4.store, 'set'); + }); + + afterEach(function() { + memoryCache4.store.set.restore(); + testCallbacks.isCacheableValue.restore(); + }); + + it("stores allowed values", function(done) { + var name = 'foo'; + + getCachedValue(name, function(err) { + checkErr(err); + assert.ok(memoryCache4.store.set.called); + + assert.ok(testCallbacks.isCacheableValue.called); + + getCachedValue(name, function(err) { + checkErr(err); + done(); + }); + }); + }); + + it("does not store non-allowed values", function(done) { + var name = 'bar'; + + getCachedValue(name, function(err) { + checkErr(err); + assert.ok(memoryCache4.store.set.notCalled); + done(); + }); + }); + }); }); describe("using two cache stores", function() { From e439a5ba9a844e010b329df1df6c0b16e27c9933 Mon Sep 17 00:00:00 2001 From: Bryan Donovan Date: Thu, 8 Oct 2015 16:31:28 -0700 Subject: [PATCH 2/3] unit test note --- test/multi_caching.unit.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/multi_caching.unit.js b/test/multi_caching.unit.js index dbe5c6d..f33f70d 100644 --- a/test/multi_caching.unit.js +++ b/test/multi_caching.unit.js @@ -569,7 +569,11 @@ describe("multiCaching", function() { store: 'memory', ttl: memoryTtl }); + + // This simulates how node-cache-manager-redis sets its + // isCacheableValue function: memoryCache4.store.isCacheableValue = testCallbacks.isCacheableValue; + multiCache = multiCaching([memoryCache4]); sinon.spy(memoryCache4.store, 'set'); }); From 5af7664d979d3401a05d52046e48cc71d6d72dae Mon Sep 17 00:00:00 2001 From: Bryan Donovan Date: Sat, 17 Oct 2015 09:54:44 -0700 Subject: [PATCH 3/3] 1.2.1 --- History.md | 3 +++ package.json | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/History.md b/History.md index 4862ed3..6534e4a 100644 --- a/History.md +++ b/History.md @@ -1,3 +1,6 @@ +- 1.2.1 2015-10-17 + - Bugfix: multi-caching: using underlying store's isCacheableValue function when it exists (#34). + - 1.2.0 2015-10-07 - using `isCacheableValue` in `getFromHighestPriorityCache` and `getAndPassUp` (#32). diff --git a/package.json b/package.json index f5617c1..250ac23 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "cache-manager", - "version": "1.2.0", + "version": "1.2.1", "description": "Cache module for Node.js", "main": "index.js", "scripts": {