24h cache all internal redirects without language prefix (#23354)

Part of #1271

Author: Peter Bengtsson

middleware/cache-control.js

@@ -8,6 +8,19 @@
 //    cacheControlYear(res)
 //    res.send(body)
 //
+// Or, if you want to make it definitely not cache:
+//
+//    const noCacheControl = getCacheControl(0) // you can use `false` too
+//    ...
+//    noControlYear(res)
+//    res.send(body)
+//
 export function cacheControlFactory(maxAge = 60 * 60, public_ = true) {
-  return (res) => res.set('cache-control', `${public_ ? 'public, ' : ''}max-age=${maxAge}`)
+  return (res) => {
+    if (maxAge) {
+      res.set('cache-control', `${public_ ? 'public, ' : ''}max-age=${maxAge}`)
+    } else {
+      res.set('cache-control', 'private, no-store')
+    }
+  }
 }

middleware/redirects/handle-redirects.js

@@ -1,6 +1,12 @@
 import patterns from '../../lib/patterns.js'
 import { URL } from 'url'
-import languages from '../../lib/languages.js'
+import languages, { languageKeys } from '../../lib/languages.js'
+import { cacheControlFactory } from '../cache-control.js'
+
+const cacheControl = cacheControlFactory(60 * 60 * 24) // one day
+const noCacheControl = cacheControlFactory(0)
+
+const languagePrefixURIRegex = new RegExp(`^/(${languageKeys.join('|')})/`)
 
 export default function handleRedirects(req, res, next) {
   // never redirect assets
@@ -19,7 +25,7 @@ export default function handleRedirects(req, res, next) {
       language = req.context.userLanguage
     }
 
-    res.set('cache-control', 'private, no-store')
+    noCacheControl(res)
     return res.redirect(302, `/${language}`)
   }
 
@@ -63,7 +69,11 @@ export default function handleRedirects(req, res, next) {
     return next()
   }
 
-  // do the redirect!
+  // do the redirect if the from-URL already had a language in it
+  if (languagePrefixURIRegex.test(req.path)) {
+    cacheControl(res)
+  }
+
   return res.redirect(301, redirect)
 }
 

tests/rendering/server.js

@@ -609,6 +609,8 @@ describe('server', () => {
     test('redirects old articles to their English URL', async () => {
       const res = await get('/articles/deleting-a-team')
       expect(res.statusCode).toBe(301)
+      // no cache control because a language prefix had to be injected
+      expect(res.headers['cache-control']).toBeUndefined()
     })
 
     test('redirects old articles to their slugified URL', async () => {
@@ -629,12 +631,21 @@ describe('server', () => {
       const res = await get('/articles/deleting-a-team')
       expect(res.statusCode).toBe(301)
       expect(res.headers.location.startsWith('/en/')).toBe(true)
+      expect(res.headers['cache-control']).toBeUndefined()
+    })
+
+    test('redirects that not only injects /en/ should have cache-control', async () => {
+      const res = await get('/en/articles/deleting-a-team')
+      expect(res.statusCode).toBe(301)
+      expect(res.headers['cache-control']).toContain('public')
+      expect(res.headers['cache-control']).toMatch(/max-age=\d+/)
     })
 
     test('redirects Desktop Classic paths to desktop.github.com', async () => {
       const res = await get('/desktop-classic')
       expect(res.statusCode).toBe(301)
       expect(res.headers.location).toBe('https://desktop.github.com')
+      expect(res.headers['cache-control']).toBeUndefined()
     })
 
     // this oneoff redirect is temporarily disabled because it introduces too much complexity
@@ -646,6 +657,8 @@ describe('server', () => {
       expect(res.headers.location).toBe(
         '/en/github/managing-subscriptions-and-notifications-on-github'
       )
+      expect(res.headers['cache-control']).toContain('public')
+      expect(res.headers['cache-control']).toMatch(/max-age=\d+/)
     })
   })