Cache language redirects (github#33028)

Author: heiskr

lib/languages.js

@@ -1,4 +1,7 @@
-// see also languages-schema.js
+// See also languages-schema.js
+// Nota bene: If you are adding a new language,
+// change accept-language handling in CDN config as well.
+
 import dotenv from 'dotenv'
 
 import { TRANSLATIONS_ROOT } from './constants.js'

middleware/archived-enterprise-versions.js

@@ -13,7 +13,7 @@ import isArchivedVersion from '../lib/is-archived-version.js'
 import { setFastlySurrogateKey, SURROGATE_ENUMS } from './set-fastly-surrogate-key.js'
 import got from 'got'
 import { readCompressedJsonFileFallbackLazily } from '../lib/read-json-file.js'
-import { archivedCacheControl, noCacheControl } from './cache-control.js'
+import { archivedCacheControl, languageCacheControl } from './cache-control.js'
 import { pathLanguagePrefixed, languagePrefixPathRegex } from '../lib/languages.js'
 import getRedirect, { splitPathByLanguage } from '../lib/get-redirect.js'
 
@@ -100,11 +100,10 @@ export default async function archivedEnterpriseVersions(req, res, next) {
   if (deprecatedWithFunctionalRedirects.includes(requestedVersion)) {
     const redirectTo = getRedirect(req.path, req.context)
     if (redirectTo) {
-      if (redirectCode === 301) {
-        archivedCacheControl(res)
-      } else {
-        noCacheControl(res)
+      if (redirectCode === 302) {
+        languageCacheControl(res) // call first to get `vary`
       }
+      archivedCacheControl(res) // call second to extend duration
       return res.redirect(redirectCode, redirectTo)
     }
 
@@ -120,11 +119,10 @@ export default async function archivedEnterpriseVersions(req, res, next) {
     const [language, withoutLanguage] = splitPathByLanguage(req.path, req.context.userLanguage)
     const newRedirectTo = redirectJson[withoutLanguage]
     if (newRedirectTo) {
-      if (redirectCode === 301) {
-        archivedCacheControl(res)
-      } else {
-        noCacheControl(res)
+      if (redirectCode === 302) {
+        languageCacheControl(res) // call first to get `vary`
       }
+      archivedCacheControl(res) // call second to extend duration
       return res.redirect(redirectCode, `/${language}${newRedirectTo}`)
     }
   }

middleware/cache-control.js

@@ -55,3 +55,12 @@ export const noCacheControl = cacheControlFactory(0)
 
 // Long caching for archived pages and assets
 export const archivedCacheControl = cacheControlFactory(60 * 60 * 24 * 365)
+
+// Vary on language when needed
+// x-user-language is a custom request header derived from req.cookie:user_language
+// accept-language is truncated to one of our available languages
+// https://bit.ly/3u5UeRN
+export function languageCacheControl(res) {
+  defaultCacheControl(res)
+  res.set('vary', 'accept-language, x-user-language')
+}

middleware/fast-root-redirect.js

@@ -56,7 +56,6 @@ import favicons from './favicons.js'
 import setStaticAssetCaching from './static-asset-caching.js'
 import fastHead from './fast-head.js'
 import fastlyCacheTest from './fastly-cache-test.js'
-import fastRootRedirect from './fast-root-redirect.js'
 import trailingSlashes from './trailing-slashes.js'
 import fastlyBehavior from './fastly-behavior.js'
 
@@ -189,7 +188,6 @@ export default function (app) {
   }
 
   // *** Early exits ***
-  app.get('/', fastRootRedirect)
   app.use(instrument(handleInvalidPaths, './handle-invalid-paths'))
   app.use(instrument(handleNextDataPath, './handle-next-data-path'))
 

middleware/index.js

@@ -3,10 +3,7 @@ import { URL } from 'url'
 import { pathLanguagePrefixed } from '../../lib/languages.js'
 import { deprecatedWithFunctionalRedirects } from '../../lib/enterprise-server-releases.js'
 import getRedirect from '../../lib/get-redirect.js'
-import { cacheControlFactory } from '../cache-control.js'
-
-const cacheControl = cacheControlFactory(60 * 60 * 24) // one day
-const noCacheControl = cacheControlFactory(0)
+import { defaultCacheControl, languageCacheControl } from '../cache-control.js'
 
 export default function handleRedirects(req, res, next) {
   // never redirect assets
@@ -20,7 +17,7 @@ export default function handleRedirects(req, res, next) {
   // blanket redirects for languageless homepage
   if (req.path === '/') {
     const language = getLanguage(req)
-    noCacheControl(res)
+    languageCacheControl(res)
     return res.redirect(302, `/${language}`)
   }
 
@@ -154,9 +151,9 @@ export default function handleRedirects(req, res, next) {
 
   // do the redirect if the from-URL already had a language in it
   if (pathLanguagePrefixed(req.path) || redirect.includes('://')) {
-    cacheControl(res)
+    defaultCacheControl(res)
   } else {
-    noCacheControl(res)
+    languageCacheControl(res)
   }
 
   const permanent = redirect.includes('://') || usePermanentRedirect(req)

middleware/redirects/handle-redirects.js

@@ -609,8 +609,11 @@ describe('server', () => {
       const res = await get('/articles/deleting-a-team', { followRedirects: false })
       expect(res.statusCode).toBe(302)
       expect(res.headers['set-cookie']).toBeUndefined()
-      // no cache control because a language prefix had to be injected
-      expect(res.headers['cache-control']).toBe('private, no-store')
+      // language specific caching
+      expect(res.headers['cache-control']).toContain('public')
+      expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/)
+      expect(res.headers.vary).toContain('accept-language')
+      expect(res.headers.vary).toContain('x-user-language')
     })
 
     test('redirects old articles to their slugified URL', async () => {
@@ -625,8 +628,12 @@ describe('server', () => {
       const res = await get('/')
       expect(res.statusCode).toBe(302)
       expect(res.headers.location).toBe('/en')
-      expect(res.headers['cache-control']).toBe('private, no-store')
       expect(res.headers['set-cookie']).toBeUndefined()
+      // language specific caching
+      expect(res.headers['cache-control']).toContain('public')
+      expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/)
+      expect(res.headers.vary).toContain('accept-language')
+      expect(res.headers.vary).toContain('x-user-language')
     })
 
     // This test exists because in a previous life, our NextJS used to
@@ -644,8 +651,12 @@ describe('server', () => {
 
       expect(res.statusCode).toBe(302)
       expect(res.headers.location).toBe('/en')
-      expect(res.headers['cache-control']).toBe('private, no-store')
       expect(res.headers['set-cookie']).toBeUndefined()
+      // language specific caching
+      expect(res.headers['cache-control']).toContain('public')
+      expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/)
+      expect(res.headers.vary).toContain('accept-language')
+      expect(res.headers.vary).toContain('x-user-language')
     })
 
     test('redirects / to /en when unsupported language preference is specified', async () => {
@@ -658,17 +669,24 @@ describe('server', () => {
       })
       expect(res.statusCode).toBe(302)
       expect(res.headers.location).toBe('/en')
-      expect(res.headers['cache-control']).toBe('private, no-store')
       expect(res.headers['set-cookie']).toBeUndefined()
+      // language specific caching
+      expect(res.headers['cache-control']).toContain('public')
+      expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/)
+      expect(res.headers.vary).toContain('accept-language')
+      expect(res.headers.vary).toContain('x-user-language')
     })
 
     test('adds English prefix to old article URLs', async () => {
       const res = await get('/articles/deleting-a-team')
       expect(res.statusCode).toBe(302)
       expect(res.headers.location.startsWith('/en/')).toBe(true)
       expect(res.headers['set-cookie']).toBeUndefined()
-      // no cache control because a language prefix had to be injected
-      expect(res.headers['cache-control']).toBe('private, no-store')
+      // language specific caching
+      expect(res.headers['cache-control']).toContain('public')
+      expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/)
+      expect(res.headers.vary).toContain('accept-language')
+      expect(res.headers.vary).toContain('x-user-language')
     })
 
     test('redirects that not only injects /en/ should have cache-control', async () => {

tests/rendering/server.js

@@ -95,8 +95,11 @@ describe('recently deprecated redirects', () => {
     expect(res.statusCode).toBe(302)
     expect(res.headers.location).toBe('/en/[email protected]')
     expect(res.headers['set-cookie']).toBeUndefined()
-    // Deliberately no cache control because it is user-dependent
-    expect(res.headers['cache-control']).toBe('private, no-store')
+    // language specific caching
+    expect(res.headers['cache-control']).toContain('public')
+    expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/)
+    expect(res.headers.vary).toContain('accept-language')
+    expect(res.headers.vary).toContain('x-user-language')
   })
 
   test('already languaged enterprise 3.0 redirects', async () => {
@@ -108,14 +111,18 @@ describe('recently deprecated redirects', () => {
     expect(res.headers['cache-control']).toContain('public')
     expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/)
   })
+
   test('redirects enterprise-server 3.0 with actual redirect without language', async () => {
     const res = await get(
       '/[email protected]/github/getting-started-with-github/githubs-products'
     )
     expect(res.statusCode).toBe(302)
     expect(res.headers['set-cookie']).toBeUndefined()
-    // Deliberately no cache control because it is user-dependent
-    expect(res.headers['cache-control']).toBe('private, no-store')
+    // language specific caching
+    expect(res.headers['cache-control']).toContain('public')
+    expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/)
+    expect(res.headers.vary).toContain('accept-language')
+    expect(res.headers.vary).toContain('x-user-language')
     // This is based on
     // https://github.com/github/help-docs-archived-enterprise-versions/blob/master/3.0/redirects.json
     expect(res.headers.location).toBe(

tests/routing/deprecated-enterprise-versions.js

@@ -126,10 +126,14 @@ describe('redirects', () => {
       const res = await get('/')
       expect(res.statusCode).toBe(302)
       expect(res.headers.location).toBe('/en')
-      expect(res.headers['cache-control']).toBe('private, no-store')
+      // language specific caching
+      expect(res.headers['cache-control']).toContain('public')
+      expect(res.headers['cache-control']).toMatch(/max-age=\d+/)
+      expect(res.headers.vary).toContain('accept-language')
+      expect(res.headers.vary).toContain('x-user-language')
     })
 
-    test('trailing slash on languaged homepage should permantently redirect', async () => {
+    test('trailing slash on languaged homepage should permanently redirect', async () => {
       const res = await get('/en/')
       expect(res.statusCode).toBe(301)
       expect(res.headers.location).toBe('/en')