Dynamic cache length overriding

lib/request-handler.js

@@ -89,19 +89,35 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
 
   const allowedKeys = flattenQueryParams(handlerOptions.queryParams)
   const {
-    cacheLength: serviceCacheLengthSeconds,
+    cacheLength: serviceDefaultCacheLengthSeconds,
     fetchLimitBytes,
   } = handlerOptions
 
   return (queryParams, match, end, ask) => {
     const reqTime = new Date()
 
-    setCacheHeaders({
-      cacheHeaderConfig,
-      serviceCacheLengthSeconds,
-      queryParams,
-      res: ask.res,
-    })
+    // `defaultCacheLengthSeconds` can be overridden by
+    // `serviceDefaultCacheLengthSeconds` (either by category or on a badge-
+    // by-badge basis). Then in turn that can be overridden by
+    // `serviceOverrideCacheLengthSeconds` (which we expect to be used only in
+    // the dynamic badge) but only if `serviceOverrideCacheLengthSeconds` is
+    // longer than `serviceDefaultCacheLengthSeconds` and then the `maxAge`
+    // query param can also override both of those but again only if `maxAge`
+    // is longer.
+    //
+    // When the legacy services have been rewritten, all the code in here
+    // will go away, which should achieve this goal in a simpler way.
+    //
+    // Ref: https://github.com/badges/shields/pull/2755
+    function setCacheHeadersOnResponse(res, serviceOverrideCacheLengthSeconds) {
+      setCacheHeaders({
+        cacheHeaderConfig,
+        serviceDefaultCacheLengthSeconds,
+        serviceOverrideCacheLengthSeconds,
+        queryParams,
+        res,
+      })
+    }
 
     analytics.noteRequest(queryParams, match)
 
@@ -123,6 +139,10 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
       const tooSoon = +reqTime - cached.time < cached.interval
       if (tooSoon || cached.dataChange / cached.reqs <= freqRatioMax) {
         const svg = makeBadge(cached.data.badgeData)
+        setCacheHeadersOnResponse(
+          ask.res,
+          cached.data.badgeData.cacheLengthSeconds
+        )
         makeSend(cached.data.format, ask.res, end)(svg)
         cachedVersionSent = true
         // We do not wish to call the vendor servers.
@@ -140,9 +160,13 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
         return
       }
       if (requestCache.has(cacheIndex)) {
-        const cached = requestCache.get(cacheIndex).data
-        const svg = makeBadge(cached.badgeData)
-        makeSend(cached.format, ask.res, end)(svg)
+        const cached = requestCache.get(cacheIndex)
+        const svg = makeBadge(cached.data.badgeData)
+        setCacheHeadersOnResponse(
+          ask.res,
+          cached.data.badgeData.cacheLengthSeconds
+        )
+        makeSend(cached.data.format, ask.res, end)(svg)
         return
       }
       ask.res.setHeader('Cache-Control', 'no-cache, no-store, must-revalidate')
@@ -155,6 +179,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
         extension = 'svg'
       }
       const svg = makeBadge(badgeData)
+      setCacheHeadersOnResponse(ask.res)
       makeSend(extension, ask.res, end)(svg)
     }, 25000)
 
@@ -256,6 +281,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
           requestCache.set(cacheIndex, updatedCache)
           if (!cachedVersionSent) {
             const svg = makeBadge(badgeData)
+            setCacheHeadersOnResponse(ask.res, badgeData.cacheLengthSeconds)
             makeSend(format, ask.res, end)(svg)
           }
         },

lib/request-handler.spec.js

@@ -25,6 +25,16 @@ function fakeHandler(queryParams, match, sendBadge, request) {
   sendBadge(format, badgeData)
 }
 
+function createFakeHandlerWithCacheLength(cacheLengthSeconds) {
+  return function fakeHandler(queryParams, match, sendBadge, request) {
+    const [, someValue, format] = match
+    const badgeData = getBadgeData('testing', queryParams)
+    badgeData.text[1] = someValue
+    badgeData.cacheLengthSeconds = cacheLengthSeconds
+    sendBadge(format, badgeData)
+  }
+}
+
 function fakeHandlerWithNetworkIo(queryParams, match, sendBadge, request) {
   const [, someValue, format] = match
   request('https://www.google.com/foo/bar', (err, res, buffer) => {
@@ -198,6 +208,62 @@ describe('The request handler', function() {
         expect(res.headers.get('cache-control')).to.equal('max-age=900')
       })
 
+      it('should set the expected cache headers on cached responses', async function() {
+        register({ cacheHeaderConfig: { defaultCacheLengthSeconds: 900 } })
+
+        // Make first request.
+        await fetch(`${baseUrl}/testing/123.json`)
+
+        const res = await fetch(`${baseUrl}/testing/123.json`)
+        const expectedExpiry = new Date(
+          +new Date(res.headers.get('date')) + 900000
+        ).toGMTString()
+        expect(res.headers.get('expires')).to.equal(expectedExpiry)
+        expect(res.headers.get('cache-control')).to.equal('max-age=900')
+      })
+
+      it('should let live service data override the default cache headers with longer value', async function() {
+        camp.route(
+          /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/,
+          handleRequest(
+            { defaultCacheLengthSeconds: 300 },
+            (queryParams, match, sendBadge, request) => {
+              ++handlerCallCount
+              createFakeHandlerWithCacheLength(400)(
+                queryParams,
+                match,
+                sendBadge,
+                request
+              )
+            }
+          )
+        )
+
+        const res = await fetch(`${baseUrl}/testing/123.json`)
+        expect(res.headers.get('cache-control')).to.equal('max-age=400')
+      })
+
+      it('should not let live service data override the default cache headers with shorter value', async function() {
+        camp.route(
+          /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/,
+          handleRequest(
+            { defaultCacheLengthSeconds: 300 },
+            (queryParams, match, sendBadge, request) => {
+              ++handlerCallCount
+              createFakeHandlerWithCacheLength(200)(
+                queryParams,
+                match,
+                sendBadge,
+                request
+              )
+            }
+          )
+        )
+
+        const res = await fetch(`${baseUrl}/testing/123.json`)
+        expect(res.headers.get('cache-control')).to.equal('max-age=300')
+      })
+
       it('should set the expires header to current time + maxAge', async function() {
         register({ cacheHeaderConfig: { defaultCacheLengthSeconds: 0 } })
         const res = await fetch(`${baseUrl}/testing/123.json?maxAge=3600`)

services/base-non-memory-caching.js

@@ -24,7 +24,7 @@ const { setCacheHeaders } = require('./cache-headers')
 module.exports = class NonMemoryCachingBaseService extends BaseService {
   static register({ camp }, serviceConfig) {
     const { cacheHeaders: cacheHeaderConfig } = serviceConfig
-    const { _cacheLength: serviceCacheLengthSeconds } = this
+    const { _cacheLength: serviceDefaultCacheLengthSeconds } = this
 
     camp.route(this._regex, async (queryParams, match, end, ask) => {
       const namedParams = this._namedParamsForMatch(match)
@@ -44,7 +44,7 @@ module.exports = class NonMemoryCachingBaseService extends BaseService {
 
       setCacheHeaders({
         cacheHeaderConfig,
-        serviceCacheLengthSeconds,
+        serviceDefaultCacheLengthSeconds,
         queryParams,
         res: ask.res,
       })

services/base.js

@@ -43,6 +43,9 @@ const serviceDataSchema = Joi.object({
   // Generally services should not use these options, which are provided to
   // support the Endpoint badge.
   labelColor: Joi.string(),
+  cacheLengthSeconds: Joi.number()
+    .integer()
+    .min(0),
 }).required()
 
 class BaseService {
@@ -347,6 +350,7 @@ class BaseService {
       color: serviceColor,
       labelColor: serviceLabelColor,
       link: serviceLink,
+      cacheLengthSeconds: serviceCacheLengthSeconds,
     } = serviceData
 
     const {
@@ -355,6 +359,7 @@ class BaseService {
       label: defaultLabel,
       labelColor: defaultLabelColor,
     } = this.defaultBadgeData
+    const defaultCacheLengthSeconds = this._cacheLength
 
     let color, labelColor
     if (isError) {
@@ -385,6 +390,10 @@ class BaseService {
       logoWidth: +overrideLogoWidth,
       links: toArray(overrideLink || serviceLink),
       colorA: makeColor(labelColor),
+      cacheLengthSeconds: coalesce(
+        serviceCacheLengthSeconds,
+        defaultCacheLengthSeconds
+      ),
     }
 
     setBadgeColor(badgeData, color)

services/base.spec.js

@@ -448,6 +448,14 @@ describe('BaseService', function() {
         const badgeData = DummyService._makeBadgeData({ style: 'pill' }, {})
         expect(badgeData.template).to.equal('pill')
       })
+
+      it('overrides the cache length', function() {
+        const badgeData = DummyService._makeBadgeData(
+          { style: 'pill' },
+          { cacheLengthSeconds: 123 }
+        )
+        expect(badgeData.cacheLengthSeconds).to.equal(123)
+      })
     })
 
     describe('Service data', function() {
@@ -544,6 +552,7 @@ describe('BaseService', function() {
         logoWidth: NaN,
         links: [],
         colorA: undefined,
+        cacheLengthSeconds: undefined,
       })
     })
   })

services/cache-headers.js

@@ -14,33 +14,43 @@ const queryParamSchema = Joi.object({
     .min(0),
 }).required()
 
-function coalesceCacheLength(
+function overrideCacheLengthFromQueryParams(queryParams) {
+  try {
+    const { maxAge: overrideCacheLength } = Joi.attempt(
+      queryParams,
+      queryParamSchema,
+      { allowUnknown: true }
+    )
+    return overrideCacheLength
+  } catch (e) {
+    return undefined
+  }
+}
+
+function coalesceCacheLength({
   cacheHeaderConfig,
-  serviceCacheLengthSeconds,
-  queryParams
-) {
+  serviceDefaultCacheLengthSeconds,
+  serviceOverrideCacheLengthSeconds,
+  queryParams,
+}) {
   const { defaultCacheLengthSeconds } = cacheHeaderConfig
-  // The config returns a number so this would only fail if we break the
-  // wiring. Better to fail obviously than silently.
+  // The config returns a number so this should never happen. But this logic
+  // would be completely broken if it did.
   assert(defaultCacheLengthSeconds !== undefined)
 
-  const ourCacheLength = coalesce(
-    serviceCacheLengthSeconds,
+  const cacheLength = coalesce(
+    serviceDefaultCacheLengthSeconds,
     defaultCacheLengthSeconds
   )
 
-  const { value: { maxAge: overrideCacheLength } = {}, error } = Joi.validate(
-    queryParams,
-    queryParamSchema,
-    { allowUnknown: true }
-  )
+  // Overrides can apply _more_ caching, but not less. Query param overriding
+  // can request more overriding than service override, but not less.
+  const candidateOverrides = [
+    serviceOverrideCacheLengthSeconds,
+    overrideCacheLengthFromQueryParams(queryParams),
+  ].filter(x => x !== undefined)
 
-  if (!error && overrideCacheLength !== undefined) {
-    // The user can request _more_ caching, but not less.
-    return Math.max(overrideCacheLength, ourCacheLength)
-  } else {
-    return ourCacheLength
-  }
+  return Math.max(cacheLength, ...candidateOverrides)
 }
 
 function setHeadersForCacheLength(res, cacheLengthSeconds) {
@@ -66,15 +76,17 @@ function setHeadersForCacheLength(res, cacheLengthSeconds) {
 
 function setCacheHeaders({
   cacheHeaderConfig,
-  serviceCacheLengthSeconds,
+  serviceDefaultCacheLengthSeconds,
+  serviceOverrideCacheLengthSeconds,
   queryParams,
   res,
 }) {
-  const cacheLengthSeconds = coalesceCacheLength(
+  const cacheLengthSeconds = coalesceCacheLength({
     cacheHeaderConfig,
-    serviceCacheLengthSeconds,
-    queryParams
-  )
+    serviceDefaultCacheLengthSeconds,
+    serviceOverrideCacheLengthSeconds,
+    queryParams,
+  })
   setHeadersForCacheLength(res, cacheLengthSeconds)
 }