From ec302a668d5af15669b79ebc7c315324c8a52b3e Mon Sep 17 00:00:00 2001 From: Peter Bengtsson Date: Mon, 22 Aug 2022 21:57:42 +0200 Subject: [PATCH] simplify and improve default cache-control headers (#30162) simply and perfect cache-control headers --- middleware/anchor-redirect.js | 5 +++++ middleware/api/search.js | 8 +++----- middleware/cache-control.js | 17 +++++++++++++++++ middleware/render-page.js | 11 +++-------- tests/content/api-search.js | 6 +++++- tests/unit/anchor-redirect.js | 13 +++++++++++++ 6 files changed, 46 insertions(+), 14 deletions(-) diff --git a/middleware/anchor-redirect.js b/middleware/anchor-redirect.js index 92300fd624ec..0bc4b2c2d6a2 100644 --- a/middleware/anchor-redirect.js +++ b/middleware/anchor-redirect.js @@ -1,5 +1,7 @@ import express from 'express' + import { readCompressedJsonFileFallbackLazily } from '../lib/read-json-file.js' +import { defaultCacheControl } from './cache-control.js' const clientSideRestAPIRedirects = readCompressedJsonFileFallbackLazily( './lib/redirects/static/client-side-rest-api-redirects.json' @@ -15,6 +17,9 @@ router.get('/', function redirects(req, res, next) { if (!req.query.hash) { return res.status(400).send("Missing 'hash' query string") } + + defaultCacheControl(res) + const redirectFrom = `${req.query.path}#${req.query.hash}` res.status(200).send({ to: clientSideRestAPIRedirects()[redirectFrom] } || null) }) diff --git a/middleware/api/search.js b/middleware/api/search.js index 93422e33c84c..3c08c4510104 100644 --- a/middleware/api/search.js +++ b/middleware/api/search.js @@ -3,7 +3,7 @@ import express from 'express' import searchVersions from '../../lib/search/versions.js' import languages from '../../lib/languages.js' import { allVersions } from '../../lib/all-versions.js' -import { cacheControlFactory } from '../cache-control.js' +import { defaultCacheControl } from '../cache-control.js' import catchMiddlewareError from '../catch-middleware-error.js' import { getSearchResults, ELASTICSEARCH_URL } from './es-search.js' @@ -13,8 +13,6 @@ const languagesSet = new Set(Object.keys(languages)) const router = express.Router() -const cacheControl = cacheControlFactory(60 * 60 * 24) - const DEFAULT_SIZE = 10 const MAX_SIZE = 50 // How much you return has a strong impact on performance const DEFAULT_PAGE = 1 @@ -137,7 +135,7 @@ router.get( } }) if (process.env.NODE_ENV !== 'development') { - cacheControl(res) + defaultCacheControl(res) } res.setHeader('x-search-legacy', 'yes') @@ -227,7 +225,7 @@ router.get( // So the only distinguishing key is the request URL. // Because of that, it's safe to allow the reverse proxy (a.k.a the CDN) // cache and hold on to this. - cacheControl(res) + defaultCacheControl(res) } // The v1 version of the output matches perfectly what comes out diff --git a/middleware/cache-control.js b/middleware/cache-control.js index e9100c1c296d..8b2aa911838c 100644 --- a/middleware/cache-control.js +++ b/middleware/cache-control.js @@ -38,3 +38,20 @@ export function cacheControlFactory( res.set(key, directives) } } + +// 24 hours for CDN, we soft-purge this with each deploy +const defaultCDNCacheControl = cacheControlFactory(60 * 60 * 24, { + key: 'surrogate-control', +}) + +// Shorter because between deployments and their (sort) purges, +// we don't want the browser to overly cache because with them we +// can't control purging. +const defaultBrowserCacheControl = cacheControlFactory(60) + +// A general default configuration that is useful to almost all responses +// that can be cached. +export function defaultCacheControl(res) { + defaultCDNCacheControl(res) + defaultBrowserCacheControl(res) +} diff --git a/middleware/render-page.js b/middleware/render-page.js index 8f4a1e054254..9edce60fba5b 100644 --- a/middleware/render-page.js +++ b/middleware/render-page.js @@ -8,12 +8,7 @@ import statsd from '../lib/statsd.js' import { allVersions } from '../lib/all-versions.js' import { isConnectionDropped } from './halt-on-dropped-connection.js' import { nextApp, nextHandleRequest } from './next.js' -import { cacheControlFactory } from './cache-control.js' - -const browserCacheControl = cacheControlFactory(60) // 1 minute for browsers -const cdnCacheControl = cacheControlFactory(60 * 60 * 24, { - key: 'surrogate-control', -}) // 24 hours for CDN, we purge this with each deploy +import { defaultCacheControl } from './cache-control.js' async function buildRenderedPage(req) { const { context } = req @@ -50,8 +45,6 @@ export default async function renderPage(req, res, next) { const { page } = context const path = req.pagePath || req.path - browserCacheControl(res) - cdnCacheControl(res) // render a 404 page if (!page) { @@ -130,5 +123,7 @@ export default async function renderPage(req, res, next) { } } + defaultCacheControl(res) + return nextHandleRequest(req, res) } diff --git a/tests/content/api-search.js b/tests/content/api-search.js index 1b326fadb512..5637df873f69 100644 --- a/tests/content/api-search.js +++ b/tests/content/api-search.js @@ -15,6 +15,7 @@ import { jest, test, expect } from '@jest/globals' import { describeIfElasticsearchURL } from '../helpers/conditional-runs.js' import { get } from '../helpers/e2etest.js' +import { SURROGATE_ENUMS } from '../../middleware/set-fastly-surrogate-key.js' if (!process.env.ELASTICSEARCH_URL) { console.warn( @@ -62,7 +63,10 @@ describeIfElasticsearchURL('search middleware', () => { // Check that it can be cached at the CDN expect(res.headers['set-cookie']).toBeUndefined() expect(res.headers['cache-control']).toContain('public') - expect(res.headers['cache-control']).toMatch(/max-age=\d+/) + expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) + expect(res.headers['surrogate-control']).toContain('public') + expect(res.headers['surrogate-control']).toMatch(/max-age=[1-9]/) + expect(res.headers['surrogate-key']).toBe(SURROGATE_ENUMS.DEFAULT) }) test('debug search', async () => { diff --git a/tests/unit/anchor-redirect.js b/tests/unit/anchor-redirect.js index f7ae7819fe3b..cc77ed0e657c 100644 --- a/tests/unit/anchor-redirect.js +++ b/tests/unit/anchor-redirect.js @@ -1,5 +1,7 @@ import { describe, expect } from '@jest/globals' + import { get } from '../helpers/e2etest.js' +import { SURROGATE_ENUMS } from '../../middleware/set-fastly-surrogate-key.js' import clientSideRedirects from '../../lib/redirects/static/client-side-rest-api-redirects.json' describe('anchor-redirect middleware', () => { @@ -41,4 +43,15 @@ describe('anchor-redirect middleware', () => { const { to } = JSON.parse(res.text) expect(to).toBe(undefined) }) + test('reasonably aggressive cache-control headers', async () => { + const sp = new URLSearchParams() + sp.set('path', 'foo') + sp.set('hash', 'bar') + const res = await get('/anchor-redirect?' + sp) + expect(res.headers['cache-control']).toContain('public') + expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/) + expect(res.headers['surrogate-control']).toContain('public') + expect(res.headers['surrogate-control']).toMatch(/max-age=[1-9]/) + expect(res.headers['surrogate-key']).toBe(SURROGATE_ENUMS.DEFAULT) + }) })