diff --git a/config/index.js b/config/index.js index 4bd5c531..d5160dfa 100644 --- a/config/index.js +++ b/config/index.js @@ -38,6 +38,9 @@ nconf.defaults({ API_CACHE_TTL: process.env.API_CACHE_TTL || 0, API_CACHE_ACTIONS: process.env.API_CACHE_ACTIONS || 'organization_list,group_list', + // Seconds to cache CMS (WordPress) reads. The theme awaits one on every + // non-static request, so this removes a per-request round trip. 0 disables. + CMS_CACHE_TTL: process.env.CMS_CACHE_TTL || 0, // dashboard and maps PLUGIN GIT_BASE_URL: process.env.GIT_BASE_URL || 'https://raw.githubusercontent.com', // carto plugin diff --git a/env.template b/env.template index fd5b9bdd..549b15b5 100644 --- a/env.template +++ b/env.template @@ -26,6 +26,10 @@ CKAN_PAGES_URL= # take seconds; see datopian/tech-devops#668. API_CACHE_TTL= API_CACHE_ACTIONS=organization_list,group_list + +# Seconds to cache CMS (WordPress) reads (0 = off). The theme awaits a CMS +# call in global middleware on every non-static request. +CMS_CACHE_TTL= GIT_BASE_URL= GIT_OWNER= diff --git a/plugins/wp/cms.js b/plugins/wp/cms.js index bab86229..608aded0 100644 --- a/plugins/wp/cms.js +++ b/plugins/wp/cms.js @@ -1,60 +1,80 @@ 'use strict' const config = require('../../config') +const TtlCache = require('../../lib/cache') const wpcom = require('wpcom')(config.get('WP_TOKEN')) +// Shared across CmsModel instances: the plugins each construct their own model, +// and they should not each keep a private copy of the same blog content. +const cmsCache = new TtlCache() class CmsModel { - constructor() { + // `cache` is injectable so tests get isolation; at runtime every model shares + // the module-level instance, which is the point - the plugins each build their + // own model and should not each keep a private copy of the same blog content. + constructor(cache = cmsCache) { + this.cache = cache this.blog = wpcom.site(config.get('WP_URL')) this.baseQuery = { status: `publish${eval(config.get('WP_SHOW_DRAFT')) ? ',draft' : ''}` } + this.cacheTtl = Number(config.get('CMS_CACHE_TTL')) || 0 } + // Every method here is a read of slow-changing blog content, and the theme + // awaits one of them in global middleware on every non-static request, which + // put a ~3s floor on the whole site. Caching is opt-in via CMS_CACHE_TTL so + // portals that have not asked for it are unaffected. + cached(method, params, load) { + if (!(this.cacheTtl > 0)) { + return load() + } + return this.cache.wrap(TtlCache.key(`wp:${method}`, params), this.cacheTtl, load) + } async getPost({slug, id, parentSlug, parentId}={}) { - - return new Promise(async (resolve, reject) => { - - // type any will request both pages and posts - let query = Object.assign({type: 'any'}, this.baseQuery) - - if (parentSlug || parentId) { - try { - const parentQuery = {slug: parentSlug} - if (parentId) { - parentQuery.id = parentId + return this.cached('getPost', {slug, id, parentSlug, parentId}, () => + + new Promise(async (resolve, reject) => { + + // type any will request both pages and posts + let query = Object.assign({type: 'any'}, this.baseQuery) + + if (parentSlug || parentId) { + try { + const parentQuery = {slug: parentSlug} + if (parentId) { + parentQuery.id = parentId + } + let parent = await (await this.blog.post(Object.assign(parentQuery, this.baseQuery))).get() + query.parent_id = parent.ID + let posts = (await this.blog.postsList(query)).posts + let post = posts.find(post => post.slug == slug) + resolve(post) + } catch (e) { + reject(e) } - let parent = await (await this.blog.post(Object.assign(parentQuery, this.baseQuery))).get() - query.parent_id = parent.ID - let posts = (await this.blog.postsList(query)).posts - let post = posts.find(post => post.slug == slug) - resolve(post) - } catch (e) { - reject(e) - } - } else { - if (id) { - query.id = id - } - query.slug = slug - this.blog.post(query).get((err, data) => { - if (err) { - reject(err) - } else { - resolve(data) + } else { + if (id) { + query.id = id } - }) - } - }) + query.slug = slug + this.blog.post(query).get((err, data) => { + if (err) { + reject(err) + } else { + resolve(data) + } + }) + } + }) + ) } async getListOfPages(query={}) { - query.type = "page" - const result = await this.getListOfPostsWithMeta(query) + const result = await this.getListOfPostsWithMeta(Object.assign({}, query, {type: 'page'})) return result.posts } @@ -64,15 +84,19 @@ class CmsModel { } async getListOfPostsWithMeta(query) { - return await this.blog.postsList(Object.assign(query, this.baseQuery)) + // Object.assign({}, ...) rather than mutating `query`: callers passing a + // reused object would otherwise have `status` written into it, which also + // changed its cache key between the first and second call. + const full = Object.assign({}, query, this.baseQuery) + return this.cached('getListOfPostsWithMeta', full, () => this.blog.postsList(full)) } async getCategories() { - return await this.blog.categoriesList() + return this.cached('getCategories', {}, () => this.blog.categoriesList()) } async getSiteInfo() { - return await this.blog.get() + return this.cached('getSiteInfo', {}, () => this.blog.get()) } api() { diff --git a/tests/plugins/wp-cache.test.js b/tests/plugins/wp-cache.test.js new file mode 100644 index 00000000..47a8f9f0 --- /dev/null +++ b/tests/plugins/wp-cache.test.js @@ -0,0 +1,100 @@ +const test = require('ava') +const wp = require('../../plugins/wp/cms') +const TtlCache = require('../../lib/cache') + +// Build a model with a stubbed blog, so these tests exercise the caching layer +// rather than wpcom or the network. +function model(ttl) { + const m = new wp.CmsModel(new TtlCache()) + m.cacheTtl = ttl + const calls = { postsList: 0, get: 0, categoriesList: 0 } + m.blog = { + postsList: async q => { calls.postsList += 1; return { posts: [{ slug: 'a', q }] } }, + get: async () => { calls.get += 1; return { description: 'site' } }, + categoriesList: async () => { calls.categoriesList += 1; return ['cat'] } + } + return { m, calls } +} + +test('getListOfPosts is cached - the per-request middleware call', async t => { + const { m, calls } = model(60) + await m.getListOfPosts({ type: 'page' }) + await m.getListOfPosts({ type: 'page' }) + await m.getListOfPosts({ type: 'page' }) + t.is(calls.postsList, 1, 'three identical reads must hit WordPress once') +}) + +test('different queries are cached separately', async t => { + const { m, calls } = model(60) + await m.getListOfPosts({ type: 'page' }) + await m.getListOfPosts({ tag: 'featured', number: 5 }) + t.is(calls.postsList, 2) +}) + +test('getSiteInfo and getCategories are cached', async t => { + const { m, calls } = model(60) + await m.getSiteInfo(); await m.getSiteInfo() + await m.getCategories(); await m.getCategories() + t.is(calls.get, 1) + t.is(calls.categoriesList, 1) +}) + +test('caching is off when CMS_CACHE_TTL is 0', async t => { + const { m, calls } = model(0) + await m.getListOfPosts({ type: 'page' }) + await m.getListOfPosts({ type: 'page' }) + t.is(calls.postsList, 2, 'must be a no-op for portals that have not opted in') +}) + +test('concurrent identical reads collapse into one upstream call', async t => { + const m = new wp.CmsModel(new TtlCache()) + m.cacheTtl = 60 + let calls = 0 + m.blog = { + postsList: async () => { + calls += 1 + await new Promise(r => setTimeout(r, 20)) + return { posts: [] } + } + } + await Promise.all([ + m.getListOfPosts({ type: 'page' }), + m.getListOfPosts({ type: 'page' }), + m.getListOfPosts({ type: 'page' }) + ]) + t.is(calls, 1, 'a cold cache under concurrency must not fan out') +}) + +test('a failed refresh serves the last good value', async t => { + const m = new wp.CmsModel(new TtlCache()) + // A short positive TTL, then wait it out: `cached()` only engages the cache + // when cacheTtl > 0, so a negative sentinel would bypass caching entirely. + m.cacheTtl = 0.05 + let calls = 0 + m.blog = { + postsList: async () => { + calls += 1 + if (calls === 1) return { posts: [{ slug: 'good' }] } + throw new Error('WordPress is down') + } + } + const first = await m.getListOfPosts({ type: 'page' }) + t.is(first[0].slug, 'good') + await new Promise(r => setTimeout(r, 80)) // let the entry go stale + const second = await m.getListOfPosts({ type: 'page' }) + t.is(second[0].slug, 'good', 'a WP outage must not blank the navbar') +}) + +test('getListOfPostsWithMeta does not mutate the query it is given', async t => { + const { m } = model(0) + const query = { type: 'page' } + await m.getListOfPostsWithMeta(query) + t.deepEqual(query, { type: 'page' }, 'status must not leak into the caller object') +}) + +test('getListOfPages does not mutate the query it is given', async t => { + const { m } = model(0) + const query = {} + await m.getListOfPages(query) + t.deepEqual(query, {}, 'type must not leak into the caller object') +})