From 37a22edbe9b25bfd0b411aab162c662cb5620b53 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Wed, 22 Apr 2020 17:27:43 +0100 Subject: [PATCH] Refactored cache-control mw to remove dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - cache-control had some logic in it for private blogging + similar logic exists for members in site/app - having it in 2 places is weird, and having it inside the mw makes the mw less generic/reusable - instead of requiring config inside the middleware, we pass config in for the one case where this is used - fixed tests that didn't test anything 🙈 --- .../web/shared/middlewares/cache-control.js | 15 +++------ core/server/web/site/app.js | 10 +++--- .../unit/web/middleware/cache-control_spec.js | 31 +++++++++---------- 3 files changed, 23 insertions(+), 33 deletions(-) diff --git a/core/server/web/shared/middlewares/cache-control.js b/core/server/web/shared/middlewares/cache-control.js index f04cdfe9ba..d4c7e7f4dd 100644 --- a/core/server/web/shared/middlewares/cache-control.js +++ b/core/server/web/shared/middlewares/cache-control.js @@ -7,27 +7,22 @@ // Allows each app to declare its own default caching rules const isString = require('lodash/isString'); -const config = require('../../../config'); -const cacheControl = (options) => { +const cacheControl = (profile, options = {maxAge: 0}) => { const profiles = { - public: 'public, max-age=' + config.get('caching:frontend:maxAge'), + public: `public, max-age=${options.maxAge}`, private: 'no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0' }; let output; - if (isString(options) && Object.prototype.hasOwnProperty.call(profiles, options)) { - output = profiles[options]; + if (isString(profile) && Object.prototype.hasOwnProperty.call(profiles, profile)) { + output = profiles[profile]; } return function cacheControlHeaders(req, res, next) { if (output) { - if (res.isPrivateBlog) { - res.set({'Cache-Control': profiles.private}); - } else { - res.set({'Cache-Control': output}); - } + res.set({'Cache-Control': output}); } next(); }; diff --git a/core/server/web/site/app.js b/core/server/web/site/app.js index ecc10203a2..50135b7a96 100644 --- a/core/server/web/site/app.js +++ b/core/server/web/site/app.js @@ -183,14 +183,12 @@ module.exports = function setupSiteApp(options = {}) { siteApp.use(shared.middlewares.prettyUrls); // ### Caching - // Site frontend is cacheable UNLESS request made by a member - const publicCacheControl = shared.middlewares.cacheControl('public'); - const privateCacheControl = shared.middlewares.cacheControl('private'); siteApp.use(function (req, res, next) { - if (req.member) { - return privateCacheControl(req, res, next); + // Site frontend is cacheable UNLESS request made by a member or blog is in private mode + if (req.member || res.isPrivateBlog) { + return shared.middlewares.cacheControl('private')(req, res, next); } else { - return publicCacheControl(req, res, next); + return shared.middlewares.cacheControl('public', {maxAge: config.get('caching:frontend:maxAge')})(req, res, next); } }); diff --git a/test/unit/web/middleware/cache-control_spec.js b/test/unit/web/middleware/cache-control_spec.js index 479b17ae38..e8e3746970 100644 --- a/test/unit/web/middleware/cache-control_spec.js +++ b/test/unit/web/middleware/cache-control_spec.js @@ -19,7 +19,16 @@ describe('Middleware: cacheControl', function () { cacheControl('public')(null, res, function (a) { should.not.exist(a); res.set.calledOnce.should.be.true(); - res.set.calledWith({'Cache-Control': 'public, max-age=0'}); + res.set.calledWith({'Cache-Control': 'public, max-age=0'}).should.be.true(); + done(); + }); + }); + + it('correctly sets the public profile headers with custom maxAge', function (done) { + cacheControl('public', {maxAge: 123456})(null, res, function (a) { + should.not.exist(a); + res.set.calledOnce.should.be.true(); + res.set.calledWith({'Cache-Control': 'public, max-age=123456'}).should.be.true(); done(); }); }); @@ -30,7 +39,7 @@ describe('Middleware: cacheControl', function () { res.set.calledOnce.should.be.true(); res.set.calledWith({ 'Cache-Control': 'no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0' - }); + }).should.be.true(); done(); }); }); @@ -49,13 +58,13 @@ describe('Middleware: cacheControl', function () { publicCC(null, res, function () { res.set.calledOnce.should.be.true(); - res.set.calledWith({'Cache-Control': 'public, max-age=0'}); + res.set.calledWith({'Cache-Control': 'public, max-age=0'}).should.be.true(); privateCC(null, res, function () { res.set.calledTwice.should.be.true(); res.set.calledWith({ 'Cache-Control': 'no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0' - }); + }).should.be.true(); publicCC(null, res, function () { res.set.calledThrice.should.be.true(); @@ -64,23 +73,11 @@ describe('Middleware: cacheControl', function () { privateCC(null, res, function () { res.set.calledWith({ 'Cache-Control': 'no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0' - }); + }).should.be.true(); done(); }); }); }); }); }); - - it('will override public with private for private blogs', function (done) { - res.isPrivateBlog = true; - cacheControl('public')(null, res, function (a) { - should.not.exist(a); - res.set.calledOnce.should.be.true(); - res.set.calledWith({ - 'Cache-Control': 'no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0' - }); - done(); - }); - }); });