From 625dd0a766001ee370e7af47d241384f00c26524 Mon Sep 17 00:00:00 2001 From: "Kozba, Waldek - Externer Mitarbeiter" Date: Wed, 2 Aug 2017 00:23:07 +0200 Subject: [PATCH 1/8] CORE-440 Respect Server response Cache related headers. - adopted tests - additional test cases - support for client request cache directive headers - README.md updated --- README.md | 18 ++- index.js | 51 ++++++-- package.json | 5 +- test/server/spec.js | 279 ++++++++++++++++++++++++++++++++++---------- utils.js | 44 ++++++- 5 files changed, 316 insertions(+), 81 deletions(-) diff --git a/README.md b/README.md index 303f004..592bbf1 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ Require and instantiate superagent-cache-plugin as follows to get the [default c ```javascript // Require and instantiate a cache module var cacheModule = require('cache-service-cache-module'); -var cache = new cacheModule({storage: 'session', defaultExpiration: 60}); +var cache = new cacheModule({storage: 'session'}); // Require superagent-cache-plugin and pass your cache module var superagentCache = require('superagent-cache-plugin')(cache); @@ -101,6 +101,22 @@ All options that can be passed to the `defaults` `require` param can be overwrit * doQuery * forceUpdate +# The `Cache-Control` request/response headers related behavior + +* Setting the request header `Cache-Control: maxe-age=X` is an alternative to the `.expiration(X)` API method call. +* Setting the request header `Cache-Control: only-if-cached` is an alternative to the `.doQuery(false)` API method call. +* Setting the `Cache-Control`request header value to one of `maxe-age=0`, `no-cache`, `no-store`switches of caching of the response. + +**NOTE** The plugin respects the server response cache related headers (`Cache-Control`, `Pragma: no-cache`, `Expires`) and calculates proper TTL for cached responses, with the respect to following: + +* When the `expiration` option is unspecified, the default behavior will be no caching, unless the server response `Cache-Control` header specifies otherwise. +* The `expiration=0` specified in options will switch off caching for any request, even when the server specifies that the response is cacheable and provides non-zero TTL via eg. the `Cache-Control: max-age=X` header. +* The non-zero `expiration` option value will narrow down any of TTL value specified via server response `Cache-Control` header. +* When the `expiration` option value is greater than the TTL specified via server response `Cache-Control` header, the later wins. + +## `ETag` and `Last-Modified` support. +The `ETag` and `Last-Modified` related cache behavior is supported with sending the associated request headers for cached response revalidation and proper handling of the `304 Not Modified` response, which results in serving the cached response instead `304` one. + # Supported Caches #### cache-service-cache-module diff --git a/index.js b/index.js index e5ab9a9..f9bc513 100644 --- a/index.js +++ b/index.js @@ -1,3 +1,4 @@ +const CachePolicy = require('http-cache-semantics'); var utils = require('./utils'); /** @@ -67,7 +68,7 @@ module.exports = function(cache, defaults){ */ Request.expiration = function(expiration){ props.expiration = expiration; - return Request; + return Request.set('cache-control', 'max-age=' + expiration); } /** @@ -89,7 +90,7 @@ module.exports = function(cache, defaults){ } /** - * Save the exisitng .end() value ("namespaced" in case of other plugins) + * Save the existing .end() value ("namespaced" in case of other plugins) * so that we can provide our customized .end() and then call through to * the underlying implementation. */ @@ -100,37 +101,65 @@ module.exports = function(cache, defaults){ * @param {function} cb */ Request.end = function(cb){ + utils.handleReqCacheHeaders(Request, props); Request.scRedirectsList = Request.scRedirectsList || []; Request.scRedirectsList = Request.scRedirectsList.concat(Request._redirectList); if(~supportedMethods.indexOf(Request.method.toUpperCase())){ var _Request = Request; var key = utils.keygen(Request, props); if(~cacheableMethods.indexOf(Request.method.toUpperCase())){ - cache.get(key, function (err, response){ - if(!err && response && !props.forceUpdate){ - utils.callbackExecutor(cb, err, response, key); + cache.get(key, function (err, entry) { + const cachedResponse = entry ? entry.response : undefined; + var policy = entry && entry.policy ? CachePolicy.fromObject(entry.policy) : undefined; + if(!err && cachedResponse && policy + && policy.satisfiesWithoutRevalidation(Request.toJSON()) && !props.forceUpdate) { + cachedResponse.headers = policy.responseHeaders(); + utils.callbackExecutor(cb, err, cachedResponse, key); } else{ if(props.doQuery){ + if (policy) { + const headers = policy.revalidationHeaders(Request.toJSON()); + Object.keys(headers).forEach(function(key) { + Request = Request.set(key, headers[key]); + }); + } end.call(Request, function (err, response){ if(err){ + if (err.status === 304 && cachedResponse) { + return utils.callbackExecutor(cb, err, cachedResponse, key); + } return utils.callbackExecutor(cb, err, response, key); } - else if(!err && response){ + else if(response){ + // just in case that superagent would stop handling '304 - Not Modified' as an Error. + if (response.status === 304 && cachedResponse) { + return utils.callbackExecutor(cb, err, cachedResponse, key); + } response.redirects = _Request.scRedirectsList; + policy = new CachePolicy(Request.toJSON(), utils.gutResponse(response)); if(props.prune){ response = props.prune(response); } - else if(props.responseProp){ + else if(props.responseProp) { response = response[props.responseProp] || null; } else{ response = utils.gutResponse(response); } - if(!utils.isEmpty(response) || props.cacheWhenEmpty){ - cache.set(key, response, props.expiration, function (){ + if ((0 !== props.expiration) && (!utils.isEmpty(response) || props.cacheWhenEmpty)) { + if (policy.storable() && policy.timeToLive() > 0) { + const expiration = props.expiration + ? Math.min(props.expiration, Math.round(policy.timeToLive()/1000, 0)) + : Math.round(policy.timeToLive()/1000, 0); + const entry = { policy: policy.toObject() , response: response }; + cache.set(key, entry, expiration , function () { + return utils.callbackExecutor(cb, err, response, key); + }); + } + else { return utils.callbackExecutor(cb, err, response, key); - }); + } } else{ return utils.callbackExecutor(cb, err, response, key); @@ -166,6 +195,6 @@ module.exports = function(cache, defaults){ } } - return Request; + return props.expiration !== undefined ? Request.set('Cache-control', 'max-age=' + props.expiration) : Request; } } diff --git a/package.json b/package.json index 128ac51..01c1042 100644 --- a/package.json +++ b/package.json @@ -34,5 +34,8 @@ "plugin", "browser", "node" - ] + ], + "dependencies": { + "http-cache-semantics": "~3.7.3" + } } diff --git a/test/server/spec.js b/test/server/spec.js index 4edf8c3..439fa7d 100644 --- a/test/server/spec.js +++ b/test/server/spec.js @@ -3,66 +3,93 @@ var expect = require('expect'); var express = require('express'); var cModule = require('cache-service-cache-module'); var cache = new cModule(); -var superagentCache = require('../../index')(cache); +var superagentCacheModule = require('../../index'); +var superagentCache = superagentCacheModule(cache, { expiration: 1 }); var app = express(); +const DEFAULT_CACHE_CONTROL = 'max-age=1'; + +function setResponseCacheControl(req, res) { + res.set('Cache-Control', req.get('cache-control') || DEFAULT_CACHE_CONTROL); +}; + app.get('/one', function(req, res){ + setResponseCacheControl(req, res); res.send(200, {key: 'one'}); }); app.post('/one', function(req, res){ + setResponseCacheControl(req, res); res.send(200, {key: 'post'}); }); app.put('/one', function(req, res){ + setResponseCacheControl(req, res); res.send(200, {key: 'put'}); }); app.patch('/one', function(req, res){ + setResponseCacheControl(req, res); res.send(200, {key: 'patch'}); }); app.delete('/one', function(req, res){ + setResponseCacheControl(req, res); res.send(200, {key: 'delete'}); }); app.get('/redirect', function(req, res){ + setResponseCacheControl(req, res); res.redirect('/one'); }); app.get('/false', function(req, res){ + setResponseCacheControl(req, res); res.send(200, {key: false}); }); app.get('/params', function(req, res){ + setResponseCacheControl(req, res); res.send(200, {pruneQuery: req.query.pruneQuery, otherParams: req.query.otherParams}); }); app.get('/options', function(req, res){ + setResponseCacheControl(req, res); res.send(200, {pruneHeader: req.get('pruneHeader'), otherOptions: req.get('otherOptions')}); }); app.get('/four', function(req, res){ + setResponseCacheControl(req, res); res.send(400, {key: 'one'}); }); var count = 0; app.get('/count', function(req, res){ count++; + setResponseCacheControl(req, res); + if (req.query.etag) { + res.set('ETag', '12345'); + } + if (req.query.lastmodified) { + res.set('Last-Modified', 'Wed, 21 Oct 2015 07:28:00 GMT'); + } res.send(200, {count: count}); }); app.listen(3000); -describe('superagentCache', function(){ +describe('superagentCache', function() { describe('API tests', function () { it('.end() should not require the \'err\' callback param', function (done) { superagent .get('localhost:3000/one') + // .set('Cache-Control', 'max-age=0') + // .set('Expires', 'Wed, 1 Aug 2017 10:22:00 GMT') .use(superagentCache) + .expiration(5) .end(function (response){ expect(response.body.key).toBe('one'); done(); @@ -101,18 +128,13 @@ describe('superagentCache', function(){ superagent .get('localhost:3000/one') .use(superagentCache) - .expiration(0.001) + .expiration(0) .end(function (err, response, key){ expect(response.body.key).toBe('one'); - cache.get(key, function (err, result){ - expect(result.body.key).toBe('one'); + cache.get(key, function (err, entry){ + expect(entry).toBe(null); + done(); }); - setTimeout(function(){ - cache.get(key, function (err, result){ - expect(result).toBe(null); - done(); - }); - }, 20); } ); }); @@ -125,10 +147,11 @@ describe('superagentCache', function(){ .get('localhost:3000/false') .use(superagentCache) .prune(prune) + .cacheWhenEmpty(true) .end(function (err, response, key){ expect(response).toBe(false); - cache.get(key, function (err, response){ - expect(response).toBe(false); + cache.get(key, function (err, entry){ + expect(entry.response).toBe(false); done(); }); } @@ -146,8 +169,8 @@ describe('superagentCache', function(){ .cacheWhenEmpty(false) .end(function (err, response, key){ expect(response).toBe(false); - cache.get(key, function (err, response){ - expect(response).toBe(null); + cache.get(key, function (err, entry){ + expect(entry).toBe(null); done(); }); } @@ -210,7 +233,6 @@ describe('superagentCache', function(){ .set({pruneHeader: true, otherOptions: false}) .pruneHeader(['pruneHeader']) .end(function (err, response, key){ - //console.log(key); expect(response.body.pruneHeader).toBe('true'); expect(response.body.otherOptions).toBe('false'); //Before superagent 1.7.0, superagent converts headers to lower case. To be backwards compatible, @@ -235,7 +257,7 @@ describe('superagentCache', function(){ expect(response).toBe(null); done(); } - ); + ) }); it('.end() should not set \'err\' callback param on error', function (done) { @@ -263,16 +285,16 @@ describe('superagentCache', function(){ .cacheWhenEmpty(false) .prune(prune) .end(function (err, response, key) { - cache.get(key, function (err, response){ - expect(response).toBe(null); + cache.get(key, function (err, entry){ + expect(entry).toBe(null); superagent .get('localhost:3000/one') .use(superagentCache) .cacheWhenEmpty(false) .prune(prune) .end(function (err, response, key) { - cache.get(key, function (err, response){ - expect(response).toBe(200); + cache.get(key, function (err, entry){ + expect(entry.response).toBe(200); done(); }); } @@ -292,8 +314,8 @@ describe('superagentCache', function(){ .use(superagentCache) .end(function (err, response, key){ expect(response.body.key).toBe('one'); - cache.get(key, function (err, response){ - expect(response.body.key).toBe('one'); + cache.get(key, function (err, entry){ + expect(entry.response.body.key).toBe('one'); done(); }); } @@ -306,8 +328,8 @@ describe('superagentCache', function(){ .use(superagentCache) .end(function (err, response, key){ expect(response.body.key).toBe('post'); - cache.get(key, function (err, response) { - expect(response).toBe(null); + cache.get(key, function (err, entry) { + expect(entry).toBe(null); done(); }); } @@ -321,8 +343,8 @@ describe('superagentCache', function(){ .end(function (err, response, key){ expect(key).toBe('{"method":"GET","uri":"http://localhost:3000/redirect","params":null,"options":{}}'); expect(response.body.key).toBe('one'); - cache.get(key, function (err, response) { - expect(response.body.key).toBe('one'); + cache.get(key, function (err, entry) { + expect(entry.response.body.key).toBe('one'); done(); }); } @@ -335,15 +357,15 @@ describe('superagentCache', function(){ .use(superagentCache) .end(function (err, response, key){ expect(response.body.key).toBe('one'); - cache.get(key, function (err, response) { - expect(response.body.key).toBe('one'); + cache.get(key, function (err, entry) { + expect(entry.response.body.key).toBe('one'); superagent .put('localhost:3000/one') .use(superagentCache) .end(function (err, response, key){ expect(response.body.key).toBe('put'); - cache.get(key, function (err, response) { - expect(response).toBe(null); + cache.get(key, function (err, entry) { + expect(entry).toBe(null); done(); }); } @@ -359,15 +381,15 @@ describe('superagentCache', function(){ .use(superagentCache) .end(function (err, response, key){ expect(response.body.key).toBe('one'); - cache.get(key, function (err, response) { - expect(response.body.key).toBe('one'); + cache.get(key, function (err, entry) { + expect(entry.response.body.key).toBe('one'); superagent .patch('localhost:3000/one') .use(superagentCache) .end(function (err, response, key){ expect(response.body.key).toBe('patch'); - cache.get(key, function (err, response) { - expect(response).toBe(null); + cache.get(key, function (err, entry) { + expect(entry).toBe(null); done(); }); } @@ -383,15 +405,15 @@ describe('superagentCache', function(){ .use(superagentCache) .end(function (err, response, key){ expect(response.body.key).toBe('one'); - cache.get(key, function (err, response){ - expect(response.body.key).toBe('one'); + cache.get(key, function (err, entry){ + expect(entry.response.body.key).toBe('one'); superagent .del('localhost:3000/one') .use(superagentCache) .end(function (err, response, key){ expect(response.body.key).toBe('delete'); - cache.get(key, function (err, response){ - expect(response).toBe(null); + cache.get(key, function (err, entry){ + expect(entry).toBe(null); done(); }); } @@ -406,7 +428,7 @@ describe('superagentCache', function(){ describe('configurability tests', function () { it('Should be able to configure global settings: doQuery', function (done) { - superagentCache.defaults = {doQuery: false, expiration: 1}; + superagentCache = superagentCacheModule(cache, {doQuery: false, expiration: 1}); superagent .get('localhost:3000/one') .use(superagentCache) @@ -420,15 +442,15 @@ describe('superagentCache', function(){ }); it('Global settings should be locally overwritten by chainables: doQuery', function (done) { - superagentCache.defaults = {doQuery: false, expiration: 1}; + superagentCache = superagentCacheModule(cache, {doQuery: false, expiration: 1}); superagent .get('localhost:3000/one') .use(superagentCache) .doQuery(true) .end(function (err, response, key){ - cache.get(key, function (err, response) { - expect(response).toNotBe(null); - expect(response.body.key).toBe('one'); + cache.get(key, function (err, entry) { + expect(entry.response).toNotBe(null); + expect(entry.response.body.key).toBe('one'); done(); }); } @@ -436,51 +458,51 @@ describe('superagentCache', function(){ }); it('Should be able to configure global settings: expiration', function (done) { - superagentCache.defaults = {doQuery: false, expiration: 1}; + superagentCache = superagentCacheModule(cache, { expiration: 1 }); superagent .get('localhost:3000/one') .use(superagentCache) - .doQuery(true) .end(function (err, response, key){ - cache.get(key, function (err, response) { - expect(response).toNotBe(null); - expect(response.body.key).toBe('one'); + cache.get(key, function (err, entry) { + expect(entry.response).toNotBe(null); + expect(entry.response.body.key).toBe('one'); setTimeout(function(){ superagent .get('localhost:3000/one') .use(superagentCache) + .doQuery(false) .end(function (err, response, key){ - cache.get(key, function (err, response) { - expect(response).toBe(null); + cache.get(key, function (err, entry) { + expect(entry).toBe(null); done(); }); } ); - }, 1000); + }, 2000); }); } ); }); it('Global settings should be locally overwritten by chainables: expiration', function (done) { - superagentCache.defaults = {doQuery: false, expiration: 1}; + superagentCache = superagentCacheModule(cache, {doQuery: false, expiration: 1}); superagent .get('localhost:3000/one') .use(superagentCache) .doQuery(true) - .expiration(2) + .expiration(5) .end(function (err, response, key){ - cache.get(key, function (err, response) { - expect(response).toNotBe(null); - expect(response.body.key).toBe('one'); + cache.get(key, function (err, entry) { + expect(entry.response).toNotBe(null); + expect(entry.response.body.key).toBe('one'); setTimeout(function(){ superagent .get('localhost:3000/one') .use(superagentCache) .end(function (err, response, key){ - cache.get(key, function (err, response) { - expect(response).toNotBe(null); - expect(response.body.key).toBe('one'); + cache.get(key, function (err, entry) { + expect(entry.response).toNotBe(null); + expect(entry.response.body.key).toBe('one'); done(); }); } @@ -491,17 +513,150 @@ describe('superagentCache', function(){ ); }); + it('Global TTL settings should be overwritten by the one calculated from the response cache-control header', function (done) { + superagentCache = superagentCacheModule(cache, {doQuery: true, expiration: 1}); + count = 0; + superagent + .get('localhost:3000/count') + .use(superagentCache) + .set('Cache-Control', 'max-age=3') + .end(function (err, response, key){ + expect(response).toNotBe(null); + expect(response.body.count).toBe(1); + setTimeout(function(){ + superagent + .get('localhost:3000/count') + .use(superagentCache) + .set('Cache-Control', 'max-age=3') + .end(function (err, response, key){ + expect(response).toNotBe(null); + expect(response.body.count).toBe(1); + done(); + } + ); + }, 1000); + } + ); + }); + + it('Cache should be switched off by \'cache-control: no-cache\' header', function (done) { + superagentCache = superagentCacheModule(cache, {doQuery: true, expiration: 1}); + count = 0; + superagent + .get('localhost:3000/count') + .use(superagentCache) + .set('Cache-Control', 'no-cache') + .end(function (err, response, key){ + cache.get(key, function (err, entry) { + expect(entry).toBe(null); + done(); + }); + } + ); + }); + + it('Cache should be switched off by \'cache-control: no-store\' header', function (done) { + superagentCache = superagentCacheModule(cache, {doQuery: true, expiration: 1}); + count = 0; + superagent + .get('localhost:3000/count') + .use(superagentCache) + .set('Cache-Control', 'no-store') + .end(function (err, response, key){ + cache.get(key, function (err, entry) { + expect(entry).toBe(null); + done(); + }); + } + ); + }); + + it('Cache should be switched off by \'cache-control: max-age=0\' header', function (done) { + superagentCache = superagentCacheModule(cache, {doQuery: true, expiration: 1}); + count = 0; + superagent + .get('localhost:3000/count') + .use(superagentCache) + .set('Cache-Control', 'max-age=0') + .end(function (err, response, key){ + cache.get(key, function (err, entry) { + expect(entry).toBe(null); + done(); + }); + } + ); + }); + + it('Should return cached response, when response is cacheable and Etag doesn\'t change', function (done) { + superagentCache = superagentCacheModule(cache, {doQuery: true, expiration: 1}); + count = 0; + superagent + .get('localhost:3000/count') + .use(superagentCache) + .query({ etag: true }) + .pruneQuery(['etag']) + .end(function (err, response, key){ + expect(response).toNotBe(null); + expect(response.body.count).toBe(1); + setTimeout(function(){ + superagent + .get('localhost:3000/count') + .use(superagentCache) + .query({ etag: true }) + .pruneQuery(['etag']) + .end(function (err, response, key){ + expect(response).toNotBe(null); + expect(response.body.count).toBe(1); + done(); + } + ); + }, 990); + } + ); + }); + + it('Should return cached response, when response is cacheable and Last-Modified doesn\'t change', function (done) { + superagentCache = superagentCacheModule(cache, {doQuery: true, expiration: 1}); + count = 0; + superagent + .get('localhost:3000/count') + .use(superagentCache) + .query({ lastmodified: true }) + .pruneQuery(['lastmodified']) + .end(function (err, response, key){ + expect(response).toNotBe(null); + expect(response.body.count).toBe(1); + setTimeout(function(){ + superagent + .get('localhost:3000/count') + .use(superagentCache) + .query({ lastmodified: true }) + .pruneQuery(['lastmodified']) + .end(function (err, response, key){ + expect(response).toNotBe(null); + expect(response.body.count).toBe(1); + done(); + } + ); + }, 990); + } + ); + }); + }); describe('forceUpdate tests', function () { it('.forceUpdate() should prevent the module from hitting the cache', function (done) { + superagentCache = superagentCacheModule(cache, {doQuery: true, expiration: 2}); + count = 0; + cache.flush(); superagent .get('localhost:3000/count') .use(superagentCache) .end(function (err, response, key){ - cache.get(key, function (err, response){ - expect(response.body.count).toBe(1); + cache.get(key, function (err, entry){ + expect(entry.response.body.count).toBe(1); superagent .get('localhost:3000/count') .use(superagentCache) diff --git a/utils.js b/utils.js index 4eab82b..c8bae38 100644 --- a/utils.js +++ b/utils.js @@ -1,3 +1,4 @@ +const CachePolicy = require('http-cache-semantics'); module.exports = { /** * Generate a cache key unique to this query @@ -46,16 +47,23 @@ module.exports = { * Find and extract headers * @param {object} reg */ - getHeaderOptions: function(req){ - //I have to remove the User-Agent header ever since superagent 1.7.0 - if(req && req._header){ - return this.pruneObj(req._header, ['User-Agent', 'user-agent']); + getHeaderOptions: function(_req){ + // I have to remove the User-Agent header ever since superagent 1.7.0 + // The cache-control header must also be removed. + // Clone the request first, as we don't want to remove the headers from the original one, do we? + const req = JSON.parse(JSON.stringify(_req)); + const headersToPrune = ['User-Agent', 'user-agent', 'Cache-Control', 'cache-control']; + if(req && req.headers){ + return this.pruneObj(req.headers, headersToPrune); + } + else if(req && req._header){ + return this.pruneObj(req._header, headersToPrune); } else if(req && req.req && req.req._headers){ - return this.pruneObj(req.req._headers, ['User-Agent', 'user-agent']); + return this.pruneObj(req.req._headers, headersToPrune); } else if(req && req.header){ - return this.pruneObj(req.header, ['User-Agent', 'user-agent']); + return this.pruneObj(req.header, headersToPrune); } return null; }, @@ -191,5 +199,29 @@ module.exports = { else{ throw new Error('UnsupportedCallbackException: Your .end() callback must pass at least one argument.'); } + }, + + /** + * Handles the request cache headers and eventually modifies the per request properties affecting caching. + * This method is called in early stage, before any attempt to execute the request against the HTTP server. + * + * @param {object} req - The request object. + * @param {object} props - The request-basis properties, which affect cache behavior. + * @returns {object} The modified properties. + */ + handleReqCacheHeaders: function(req, props) { + const cacheControl = req.get('cache-control'); + if (typeof cacheControl === 'string' && cacheControl.toLowerCase().indexOf('only-if-cached') !== -1) { + props.doQuery = false; + } + // We cheat the policy a bit here, giving the request instead of response (we don't have it at this stage), + // as we want to parse the request headers for the caching control related values, + // which could override the 'props' values. + const policy = new CachePolicy(req.toJSON(), req.toJSON()); + // The 'no-store' will be checked here. + // Note: The default 'policy.timeToLive()' is '0' (means when there's no Expires or max-age specified). + // The legacy method 'expiration()' will set the policy TTL value via the Cache-Control max-age value, + // so no conflicts here. + props.expiration = policy.storable() ? Math.round(policy.timeToLive()/1000, 0) : 0; } } From 6cee1f49df5c12a9942e07b2c428e4463ab492ea Mon Sep 17 00:00:00 2001 From: "Kozba, Waldek - Externer Mitarbeiter" Date: Wed, 2 Aug 2017 10:10:09 +0200 Subject: [PATCH 2/8] Post review fixes. --- index.js | 4 ++-- test/server/spec.js | 2 +- utils.js | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index f9bc513..e3cc01c 100644 --- a/index.js +++ b/index.js @@ -150,8 +150,8 @@ module.exports = function(cache, defaults){ if ((0 !== props.expiration) && (!utils.isEmpty(response) || props.cacheWhenEmpty)) { if (policy.storable() && policy.timeToLive() > 0) { const expiration = props.expiration - ? Math.min(props.expiration, Math.round(policy.timeToLive()/1000, 0)) - : Math.round(policy.timeToLive()/1000, 0); + ? Math.min(props.expiration, Math.round(policy.timeToLive() / 1000)) + : Math.round(policy.timeToLive() / 1000); const entry = { policy: policy.toObject() , response: response }; cache.set(key, entry, expiration , function () { return utils.callbackExecutor(cb, err, response, key); diff --git a/test/server/spec.js b/test/server/spec.js index 439fa7d..44f3340 100644 --- a/test/server/spec.js +++ b/test/server/spec.js @@ -534,7 +534,7 @@ describe('superagentCache', function() { done(); } ); - }, 1000); + }, 1200); } ); }); diff --git a/utils.js b/utils.js index c8bae38..f05d073 100644 --- a/utils.js +++ b/utils.js @@ -222,6 +222,6 @@ module.exports = { // Note: The default 'policy.timeToLive()' is '0' (means when there's no Expires or max-age specified). // The legacy method 'expiration()' will set the policy TTL value via the Cache-Control max-age value, // so no conflicts here. - props.expiration = policy.storable() ? Math.round(policy.timeToLive()/1000, 0) : 0; + props.expiration = policy.storable() ? Math.round(policy.timeToLive() / 1000) : 0; } } From 2f1655bf428bcb6db85ea3422409821692994555 Mon Sep 17 00:00:00 2001 From: "Kozba, Waldek - Externer Mitarbeiter" Date: Wed, 2 Aug 2017 10:28:43 +0200 Subject: [PATCH 3/8] Improved 'pruneObj' method. --- utils.js | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/utils.js b/utils.js index f05d073..11fb5b3 100644 --- a/utils.js +++ b/utils.js @@ -52,7 +52,7 @@ module.exports = { // The cache-control header must also be removed. // Clone the request first, as we don't want to remove the headers from the original one, do we? const req = JSON.parse(JSON.stringify(_req)); - const headersToPrune = ['User-Agent', 'user-agent', 'Cache-Control', 'cache-control']; + const headersToPrune = ['user-agent', 'cache-control']; if(req && req.headers){ return this.pruneObj(req.headers, headersToPrune); } @@ -116,13 +116,17 @@ module.exports = { * @param {boolean} isOptions */ pruneObj: function(obj, props, isOptions){ - for(var i = 0; i < props.length; i++){ - var prop = props[i]; - if(isOptions){ - delete obj[prop.toLowerCase()]; + const lowerCasedProps = props.map(function (item) { + return item.toLowerCase(); + }); + Object.keys(obj).forEach(function (key) { + if (lowerCasedProps.indexOf(key.toLowerCase()) !== -1) { + if(isOptions){ + delete obj[key.toLowerCase()]; + } + delete obj[key]; } - delete obj[prop]; - } + }); return obj; }, From b243b88840b3b7f3c7c2b0e2b346b378829accf4 Mon Sep 17 00:00:00 2001 From: "Kozba, Waldek - Externer Mitarbeiter" Date: Mon, 7 Aug 2017 00:24:12 +0200 Subject: [PATCH 4/8] Improved cache entry revalidation, 'bypassHeaders' function. --- index.js | 68 +++++++++++++++++------- test/server/spec.js | 34 +++++++++++- utils.js | 123 +++++++++++++++++++++++++++++++++++++------- 3 files changed, 188 insertions(+), 37 deletions(-) diff --git a/index.js b/index.js index e3cc01c..c83b4bd 100644 --- a/index.js +++ b/index.js @@ -89,6 +89,40 @@ module.exports = function(cache, defaults){ return Request; } + /** + * Array of header names, which should be bypassed from a request to a response. + * This is useful for eg. some correlation ID, which binds a request with a response + * and could be an issue when returning the cached response. + * Note, that bypassed headers are copied only to cached responses. + * + * @param {string|string[]} bypassHeaders + */ + Request.bypassHeaders = function(bypassHeaders){ + props.bypassHeaders = (typeof bypassHeaders === 'string') ? [bypassHeaders] : bypassHeaders; + return Request; + } + + var cachedEntry; + + // Special handling for the '304 Not Modified' case, which will only come out + // in case of the server responses with 'ETag' and 'Modification-Date'. + Request.on('response', function (res) { + if (res.status === 304 && cachedEntry) { + res.status = cachedEntry.response.status; + res.header = CachePolicy.fromObject(cachedEntry.policy).responseHeaders(); + utils.setResponseHeader(res, 'x-cache', 'HIT'); + utils.copyBypassHeaders(res, Request, props); + res.body = cachedEntry.response.body; + res.text = cachedEntry.response.text; + // update the cache entry + const key = utils.keygen(Request, props); + const policy = CachePolicy.fromObject(cachedEntry.policy).revalidatedPolicy(Request.toJSON(), res).policy; + cachedEntry.policy = policy.toObject(); + cache.set(key, cachedEntry, utils.getExpiration(props, policy)); + cachedEntry = undefined; + } + }); + /** * Save the existing .end() value ("namespaced" in case of other plugins) * so that we can provide our customized .end() and then call through to @@ -109,12 +143,17 @@ module.exports = function(cache, defaults){ var key = utils.keygen(Request, props); if(~cacheableMethods.indexOf(Request.method.toUpperCase())){ cache.get(key, function (err, entry) { + cachedEntry = entry; const cachedResponse = entry ? entry.response : undefined; var policy = entry && entry.policy ? CachePolicy.fromObject(entry.policy) : undefined; + if (cachedResponse && policy) { + cachedResponse.header = policy.responseHeaders(); + utils.setResponseHeader(cachedResponse, 'x-cache', 'HIT'); + utils.copyBypassHeaders(cachedResponse, Request, props); + } if(!err && cachedResponse && policy && policy.satisfiesWithoutRevalidation(Request.toJSON()) && !props.forceUpdate) { - cachedResponse.headers = policy.responseHeaders(); - utils.callbackExecutor(cb, err, cachedResponse, key); + return utils.callbackExecutor(cb, null, cachedResponse, key, Request); } else{ if(props.doQuery){ @@ -126,18 +165,11 @@ module.exports = function(cache, defaults){ } end.call(Request, function (err, response){ if(err){ - if (err.status === 304 && cachedResponse) { - return utils.callbackExecutor(cb, err, cachedResponse, key); - } return utils.callbackExecutor(cb, err, response, key); } else if(response){ - // just in case that superagent would stop handling '304 - Not Modified' as an Error. - if (response.status === 304 && cachedResponse) { - return utils.callbackExecutor(cb, err, cachedResponse, key); - } response.redirects = _Request.scRedirectsList; - policy = new CachePolicy(Request.toJSON(), utils.gutResponse(response)); + policy = new CachePolicy(Request.toJSON(), utils.gutResponse(response, Request)); if(props.prune){ response = props.prune(response); } @@ -145,24 +177,25 @@ module.exports = function(cache, defaults){ response = response[props.responseProp] || null; } else{ - response = utils.gutResponse(response); + response = utils.gutResponse(response, Request); } + utils.setResponseHeader(response, 'x-cache', 'MISS'); if ((0 !== props.expiration) && (!utils.isEmpty(response) || props.cacheWhenEmpty)) { if (policy.storable() && policy.timeToLive() > 0) { - const expiration = props.expiration - ? Math.min(props.expiration, Math.round(policy.timeToLive() / 1000)) - : Math.round(policy.timeToLive() / 1000); + // The TTL in underlying caches will be policy TTL x 2, as we want to allow for + // further serving of the stale objects (when the policy allows for that). + const expiration = utils.getExpiration(props, policy); const entry = { policy: policy.toObject() , response: response }; cache.set(key, entry, expiration , function () { - return utils.callbackExecutor(cb, err, response, key); + return utils.callbackExecutor(cb, null, response, key); }); } else { - return utils.callbackExecutor(cb, err, response, key); + return utils.callbackExecutor(cb, null, response, key); } } else{ - return utils.callbackExecutor(cb, err, response, key); + return utils.callbackExecutor(cb, null, response, key); } } }); @@ -194,7 +227,6 @@ module.exports = function(cache, defaults){ }); } } - return props.expiration !== undefined ? Request.set('Cache-control', 'max-age=' + props.expiration) : Request; } } diff --git a/test/server/spec.js b/test/server/spec.js index 44f3340..bab7eb8 100644 --- a/test/server/spec.js +++ b/test/server/spec.js @@ -427,6 +427,24 @@ describe('superagentCache', function() { describe('configurability tests', function () { + beforeEach(function (done) { + cache.flush(); + done(); + }); + + it('Should return null response when \'only-if-cached\' is set in the request header.', function (done) { + superagentCache = superagentCacheModule(cache, {doQuery: true, expiration: 1}); + superagent + .get('localhost:3000/one') + .use(superagentCache) + .set('cache-control', 'only-if-cached') + .end(function (err, response, key){ + expect(response).toBe(null); + done(); + } + ); + }); + it('Should be able to configure global settings: doQuery', function (done) { superagentCache = superagentCacheModule(cache, {doQuery: false, expiration: 1}); superagent @@ -598,6 +616,7 @@ describe('superagentCache', function() { .end(function (err, response, key){ expect(response).toNotBe(null); expect(response.body.count).toBe(1); + // should response with 304 and refresh the policy. setTimeout(function(){ superagent .get('localhost:3000/count') @@ -607,7 +626,20 @@ describe('superagentCache', function() { .end(function (err, response, key){ expect(response).toNotBe(null); expect(response.body.count).toBe(1); - done(); + // should serve cached response according to refreshed policy. + setTimeout(function(){ + superagent + .get('localhost:3000/count') + .use(superagentCache) + .query({ etag: true }) + .pruneQuery(['etag']) + .end(function (err, response, key){ + expect(response).toNotBe(null); + expect(response.body.count).toBe(1); + done(); + } + ); + }, 100); } ); }, 990); diff --git a/utils.js b/utils.js index 11fb5b3..d545a06 100644 --- a/utils.js +++ b/utils.js @@ -11,6 +11,7 @@ module.exports = { var cleanOptions = null; var params = this.getQueryParams(req); var options = this.getHeaderOptions(req); + props.pruneHeader = ['if-none-match', 'if-modified-since'].concat(props.pruneHeader || []); if(props.pruneQuery || props.pruneHeader){ cleanParams = (props.pruneQuery) ? this.pruneObj(this.cloneObject(params), props.pruneQuery) : params; cleanOptions = (props.pruneHeader) ? this.pruneObj(this.cloneObject(options), props.pruneHeader, true) : options; @@ -47,23 +48,23 @@ module.exports = { * Find and extract headers * @param {object} reg */ - getHeaderOptions: function(_req){ + getHeaderOptions: function(req){ // I have to remove the User-Agent header ever since superagent 1.7.0 // The cache-control header must also be removed. // Clone the request first, as we don't want to remove the headers from the original one, do we? - const req = JSON.parse(JSON.stringify(_req)); + const _req = req ? JSON.parse(JSON.stringify(req)) : req; const headersToPrune = ['user-agent', 'cache-control']; - if(req && req.headers){ - return this.pruneObj(req.headers, headersToPrune); + if(_req && _req.headers){ + return this.pruneObj(_req.headers, headersToPrune); } - else if(req && req._header){ - return this.pruneObj(req._header, headersToPrune); + else if(_req && _req._header){ + return this.pruneObj(_req._header, headersToPrune); } - else if(req && req.req && req.req._headers){ - return this.pruneObj(req.req._headers, headersToPrune); + else if(_req && _req.req && _req.req._headers){ + return this.pruneObj(_req.req._headers, headersToPrune); } - else if(req && req.header){ - return this.pruneObj(req.header, headersToPrune); + else if(_req && _req.header){ + return this.pruneObj(_req.header, headersToPrune); } return null; }, @@ -132,13 +133,17 @@ module.exports = { /** * Simplify superagent's http response object - * @param {object} r + * @param {object} r - The response. + * @param {object} Request - The superagent's Request instance. + * @param {object} props - The request properties. */ - gutResponse: function(r){ + gutResponse: function(r, Request, props){ var newResponse = {}; + newResponse.req = Request.toJSON(); newResponse.body = r.body; newResponse.text = r.text; - newResponse.headers = r.headers; + newResponse.header = r.header; + newResponse.headers = r.header; newResponse.statusCode = r.statusCode; newResponse.status = r.status; newResponse.ok = r.ok; @@ -182,7 +187,8 @@ module.exports = { expiration: d.expiration, forceUpdate: d.forceUpdate, preventDuplicateCalls: d.preventDuplicateCalls, - backgroundRefresh: d.backgroundRefresh + backgroundRefresh: d.backgroundRefresh, + bypassHeaders: d.bypassHeaders }; }, @@ -192,8 +198,22 @@ module.exports = { * @param {object} err * @param {object} response * @param {string} key + * @param {object} [Request] - Superagent Request instance. When provided it will emit the events. + * @param {object} props - The request internal properties. */ - callbackExecutor: function(cb, err, response, key){ + callbackExecutor: function(cb, err, response, key, Request){ + if (response) { + // Superagent response should bear only the 'header' attribute, this was only needed for the policy. + delete response.headers; + if (Request) { + Request.emit('request', Request); + if (err) { + Request.emit('error', err); + } else { + Request.emit('response', response); + } + } + } if(cb.length === 1){ cb(response); } @@ -213,10 +233,17 @@ module.exports = { * @param {object} props - The request-basis properties, which affect cache behavior. * @returns {object} The modified properties. */ - handleReqCacheHeaders: function(req, props) { + handleReqCacheHeaders: function (req, props) { const cacheControl = req.get('cache-control'); - if (typeof cacheControl === 'string' && cacheControl.toLowerCase().indexOf('only-if-cached') !== -1) { - props.doQuery = false; + if (typeof cacheControl === 'string') { + if (cacheControl.toLowerCase().indexOf('only-if-cached') !== -1) { + props.doQuery = false; + } + // the expiration can also be set via the Request header. + const maxAgeMatch = cacheControl.toLowerCase().match(/^(.*max-age=)(\d*).*$/); + if (maxAgeMatch) { + props.expiration = parseInt(maxAgeMatch[2]); + } } // We cheat the policy a bit here, giving the request instead of response (we don't have it at this stage), // as we want to parse the request headers for the caching control related values, @@ -227,5 +254,65 @@ module.exports = { // The legacy method 'expiration()' will set the policy TTL value via the Cache-Control max-age value, // so no conflicts here. props.expiration = policy.storable() ? Math.round(policy.timeToLive() / 1000) : 0; + }, + + /** + * Returns the `expiration` (TTL) value calculated as a minimum of the `props.expiration` and `policy.timeToLive`. + * The resulting value is multiplied by `2`due to enable further handling of the stale cache entries, + * (when the policy allows for that). + * The resulting value is to be used for underlying cache implementation. + * + * @param {object} props - The request-basis properties, which affect cache behavior. + * @param {object} policy - The cache policy. + * @returns {number} The expiration (TTL) time in seconds. + */ + getExpiration: function (props, policy) { + return props.expiration + ? Math.min(props.expiration * 2, Math.round(policy.timeToLive() * 2 / 1000)) + : Math.round(policy.timeToLive() * 2 / 1000); + }, + + /** + * Sets the response header value. + * + * @param {object} response - The response instance. + * @param {string} name - The header name. + * @param {string} value - The header value. + * @returns {object} The incoming modified response. + */ + setResponseHeader: function (response, name, value) { + // both need to be checked as someone could do strange things with 'prune' or 'responseProp'. + if (response) { + if (response.header) { + response.header[name] = value; + } + if (response.headers) { + response.headers[name] = value; + } + } + return response; + }, + + /** + * Copies the header values declared with the `bypassHeaders` option from current request + * to a cached response headers and its bound request headers. + * + * @param {object} response - The response instance. + * @param {object} req - The request object. + * @param {object} props - The request-basis properties, which affect cache behavior. + * @returns {object} The incoming modified response. + */ + copyBypassHeaders: function (response, req, props) { + const self = this; + if (props.bypassHeaders && props.bypassHeaders.forEach) { + props.bypassHeaders.forEach(function (name) { + const value = req.get(name); + self.setResponseHeader(response, name, value); + if (response.req && response.req.headers) { + response.req.headers[name] = value; + } + }); + } + return response; } } From 81dc3320f29f4e0442e0912ddc0483af9ab68c56 Mon Sep 17 00:00:00 2001 From: "Kozba, Waldek - Externer Mitarbeiter" Date: Mon, 7 Aug 2017 00:38:38 +0200 Subject: [PATCH 5/8] Wrong header name in comment. --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index c83b4bd..c9348ff 100644 --- a/index.js +++ b/index.js @@ -105,7 +105,7 @@ module.exports = function(cache, defaults){ var cachedEntry; // Special handling for the '304 Not Modified' case, which will only come out - // in case of the server responses with 'ETag' and 'Modification-Date'. + // in case of server responses with 'ETag' and/or 'Last-Modified' headers. Request.on('response', function (res) { if (res.status === 304 && cachedEntry) { res.status = cachedEntry.response.status; From 9fdb2b0e09e27ae6f9410fffd5283fb6fb667ce0 Mon Sep 17 00:00:00 2001 From: "Kozba, Waldek - Externer Mitarbeiter" Date: Mon, 7 Aug 2017 10:23:01 +0200 Subject: [PATCH 6/8] Post review fixes. --- utils.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/utils.js b/utils.js index d545a06..269e039 100644 --- a/utils.js +++ b/utils.js @@ -241,7 +241,7 @@ module.exports = { } // the expiration can also be set via the Request header. const maxAgeMatch = cacheControl.toLowerCase().match(/^(.*max-age=)(\d*).*$/); - if (maxAgeMatch) { + if (maxAgeMatch && maxAgeMatch.length > 2 && maxAgeMatch[2] !== '') { props.expiration = parseInt(maxAgeMatch[2]); } } @@ -267,9 +267,7 @@ module.exports = { * @returns {number} The expiration (TTL) time in seconds. */ getExpiration: function (props, policy) { - return props.expiration - ? Math.min(props.expiration * 2, Math.round(policy.timeToLive() * 2 / 1000)) - : Math.round(policy.timeToLive() * 2 / 1000); + return Math.min(props.expiration * 2 || Number.MAX_VALUE, Math.round(policy.timeToLive() * 2 / 1000)); }, /** From 19a08ac645235634b218db837e53d724e8a148a1 Mon Sep 17 00:00:00 2001 From: "Kozba, Waldek - Externer Mitarbeiter" Date: Mon, 7 Aug 2017 11:44:27 +0200 Subject: [PATCH 7/8] - README updated, - added 'bypassHeaders' to 'pruneHeader' automatically, - test case for the 'bypassHeaders'. --- README.md | 38 ++++++++++++++++++++++++++++++++++++++ test/server/spec.js | 31 +++++++++++++++++++++++++++++++ utils.js | 6 +++++- 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 592bbf1..4db4a4b 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,7 @@ All options that can be passed to the `defaults` `require` param can be overwrit * cacheWhenEmpty * doQuery * forceUpdate +* bypassHeaders # The `Cache-Control` request/response headers related behavior @@ -296,6 +297,43 @@ Tells `superagent-cache-plugin` to perform an ajax call regardless of whether th * bool: boolean, default: false +## .bypassHeaders(headerNames) + +Tells `superagent-cache-plugin` to copy given headers from the current executing request to the response. +This is useful for eg. some correlation ID, which binds a request with a response and could be an issue when returning a cached response. +**Note** Bypassed headers are copied only to cached responses. + +#### Arguments + +* headerNames: string or array of strings + +#### Example + +```javascript +//the superagent query will be executed with all headers +//but the key used to store the superagent response will be generated without the 'bypassHeaders' header keys +//and the response will have those keys set to the values from the request headers, when served from a cache. +var correlationId = 0; +superagent + .get(uri) + .use(superagentCache) + .expiration(1) + .bypassHeaders(['x-correlation-id']) + .set('x-correlation-id', correlationId++) + .end(function (error, response){ + superagent + .get(uri) + .use(superagentCache) + .bypassHeaders(['x-correlation-id']) + .set('x-correlation-id', correlationId++) + .end(function (error, response){ + expect(response.header['x-cache']).toBe('HIT'); + expect(response.header['x-correlation-id']).toBe(1); + }); + } +); +``` + ## superagentCache.cache This is the first constructor param you handed in when you instantiated `superagent-cache-plugin`. diff --git a/test/server/spec.js b/test/server/spec.js index bab7eb8..becb3bb 100644 --- a/test/server/spec.js +++ b/test/server/spec.js @@ -675,6 +675,37 @@ describe('superagentCache', function() { ); }); + it('Should return cached response, with proper handling of bypassed headers', function (done) { + superagentCache = superagentCacheModule(cache, { + doQuery: true, + expiration: 1, + bypassHeaders: ['x-correlation-id'] + }); + count = 0; + var correlationId = 0; + superagent + .get('localhost:3000/count') + .use(superagentCache) + .set('x-correlation-id', correlationId++) + .end(function (err, response, key){ + expect(response).toNotBe(null); + expect(response.body.count).toBe(1); + superagent + .get('localhost:3000/count') + .use(superagentCache) + .set('x-correlation-id', correlationId++) + .end(function (err, response, key){ + expect(response).toNotBe(null); + expect(response.body.count).toBe(1); + expect(response.header['x-cache']).toBe('HIT'); + expect(response.header['x-correlation-id']).toBe(1); + done(); + } + ); + } + ); + }); + }); describe('forceUpdate tests', function () { diff --git a/utils.js b/utils.js index 269e039..b9c71e6 100644 --- a/utils.js +++ b/utils.js @@ -11,7 +11,11 @@ module.exports = { var cleanOptions = null; var params = this.getQueryParams(req); var options = this.getHeaderOptions(req); - props.pruneHeader = ['if-none-match', 'if-modified-since'].concat(props.pruneHeader || []); + // prune headers together with revalidation headers added internally by superagent + // and optional 'bypassHeaders' which are likely changing per request and should not + // be used to calculate the cache key. + props.pruneHeader = ['if-none-match', 'if-modified-since'] + .concat(props.pruneHeader || [], props.bypassHeaders || []); if(props.pruneQuery || props.pruneHeader){ cleanParams = (props.pruneQuery) ? this.pruneObj(this.cloneObject(params), props.pruneQuery) : params; cleanOptions = (props.pruneHeader) ? this.pruneObj(this.cloneObject(options), props.pruneHeader, true) : options; From 472c02592ff878e78ee76b33b84d9f47f78a12a0 Mon Sep 17 00:00:00 2001 From: "Kozba, Waldek - Externer Mitarbeiter" Date: Mon, 14 Aug 2017 10:51:38 +0200 Subject: [PATCH 8/8] CORE-450 - RFC2616 compliant handling of the `only-if-cached` case. - returning cloned response from a cache. - optimized 304 handling. --- index.js | 27 +++++++++++++++++++-------- test/server/spec.js | 9 +++++---- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/index.js b/index.js index c9348ff..ad11464 100644 --- a/index.js +++ b/index.js @@ -108,17 +108,20 @@ module.exports = function(cache, defaults){ // in case of server responses with 'ETag' and/or 'Last-Modified' headers. Request.on('response', function (res) { if (res.status === 304 && cachedEntry) { - res.status = cachedEntry.response.status; - res.header = CachePolicy.fromObject(cachedEntry.policy).responseHeaders(); - utils.setResponseHeader(res, 'x-cache', 'HIT'); - utils.copyBypassHeaders(res, Request, props); - res.body = cachedEntry.response.body; - res.text = cachedEntry.response.text; // update the cache entry const key = utils.keygen(Request, props); const policy = CachePolicy.fromObject(cachedEntry.policy).revalidatedPolicy(Request.toJSON(), res).policy; cachedEntry.policy = policy.toObject(); cache.set(key, cachedEntry, utils.getExpiration(props, policy)); + // modify response + res.status = cachedEntry.response.status; + res.statusCode = cachedEntry.response.statusCode; + res.header = policy.responseHeaders(); + utils.setResponseHeader(res, 'x-cache', 'HIT'); + utils.copyBypassHeaders(res, Request, props); + res.body = cachedEntry.response.body; + res.text = cachedEntry.response.text; + // cleanup cachedEntry = undefined; } }); @@ -153,7 +156,8 @@ module.exports = function(cache, defaults){ } if(!err && cachedResponse && policy && policy.satisfiesWithoutRevalidation(Request.toJSON()) && !props.forceUpdate) { - return utils.callbackExecutor(cb, null, cachedResponse, key, Request); + // Return the clone of the cached response. + return utils.callbackExecutor(cb, null, JSON.parse(JSON.stringify(cachedResponse)), key, Request); } else{ if(props.doQuery){ @@ -201,7 +205,14 @@ module.exports = function(cache, defaults){ }); } else{ - return utils.callbackExecutor(cb, null, null, key); + // This is actually the 'only-if-cached' condition + // (doQuery=false is exactly the same intention). + // Returning the response status 504 as the RFC2616 states about the 'only-if-cached'. + // See: https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.4. + return utils.callbackExecutor(cb, null, { + status: 504, + header: {}, + }, key); } } }); diff --git a/test/server/spec.js b/test/server/spec.js index becb3bb..851f7f3 100644 --- a/test/server/spec.js +++ b/test/server/spec.js @@ -254,7 +254,7 @@ describe('superagentCache', function() { .use(superagentCache) .doQuery(false) .end(function (err, response, key){ - expect(response).toBe(null); + expect(response.status).toBe(504); done(); } ) @@ -432,14 +432,14 @@ describe('superagentCache', function() { done(); }); - it('Should return null response when \'only-if-cached\' is set in the request header.', function (done) { + it('Should return 504 response when \'only-if-cached\' is set in the request header.', function (done) { superagentCache = superagentCacheModule(cache, {doQuery: true, expiration: 1}); superagent .get('localhost:3000/one') .use(superagentCache) .set('cache-control', 'only-if-cached') .end(function (err, response, key){ - expect(response).toBe(null); + expect(response.status).toBe(504); done(); } ); @@ -451,6 +451,7 @@ describe('superagentCache', function() { .get('localhost:3000/one') .use(superagentCache) .end(function (err, response, key){ + expect(response.status).toBe(504); cache.get(key, function (err, response) { expect(response).toBe(null); done(); @@ -639,7 +640,7 @@ describe('superagentCache', function() { done(); } ); - }, 100); + }, 50); } ); }, 990);