TTL-cache CMS (WordPress) reads — removes the ~3s per-request floor

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

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=
 

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() {

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')
+})