make enterprise 3.0 redirects work (github#26041)

* make enterprise 3.0 redirects work

* improve tests and better variable name

Author: peterbe

lib/enterprise-server-releases.js

@@ -14,8 +14,18 @@ export const next = '3.5'
 export const nextNext = '3.6'
 
 export const supported = ['3.4', '3.3', '3.2', '3.1']
+
+// This indicates the point where we started treating redirect lookups
+// to be a *function* rather than a big *lookup object*.
+// This is important distinguish because we need to leverage that
+// when dealing with redirects specifically in these archived
+// enterprise versions.
+// When you're archiving a version, add the new archived number to this
+// array and you should never need to touch the `deprecated` array
+// on the line just below.
+export const deprecatedWithFunctionalRedirects = ['3.0']
 export const deprecated = [
-  '3.0',
+  ...deprecatedWithFunctionalRedirects,
   '2.22',
   '2.21',
   '2.20',

lib/get-redirect.js

@@ -2,21 +2,32 @@ import { languageKeys } from './languages.js'
 import nonEnterpriseDefaultVersion from './non-enterprise-default-version.js'
 
 import { allVersions } from './all-versions.js'
-import { latest, supported } from './enterprise-server-releases.js'
+import {
+  latest,
+  supported,
+  deprecatedWithFunctionalRedirects,
+} from './enterprise-server-releases.js'
 
 const languagePrefixRegex = new RegExp(`^/(${languageKeys.join('|')})/`)
 const nonEnterpriseDefaultVersionPrefix = `/${nonEnterpriseDefaultVersion}`
 
-// Return the new URI if there is one, otherwise return undefined.
-export default function getRedirect(uri, context) {
-  const { redirects, userLanguage } = context
+const supportedAndRecentlyDeprecated = [...supported, ...deprecatedWithFunctionalRedirects]
 
+export function splitPathByLanguage(uri, userLanguage) {
   let language = userLanguage || 'en'
   let withoutLanguage = uri
   if (languagePrefixRegex.test(uri)) {
     language = uri.match(languagePrefixRegex)[1]
     withoutLanguage = uri.replace(languagePrefixRegex, '/')
   }
+  return [language, withoutLanguage]
+}
+
+// Return the new URI if there is one, otherwise return undefined.
+export default function getRedirect(uri, context) {
+  const { redirects, userLanguage } = context
+
+  const [language, withoutLanguage] = splitPathByLanguage(uri, userLanguage)
 
   let destination
 
@@ -54,7 +65,6 @@ export default function getRedirect(uri, context) {
     if (withoutLanguage === '/enterprise-server') {
       return basicCorrection
     }
-    // console.log({ basicCorrection })
   } else if (withoutLanguage.startsWith('/enterprise-server@latest')) {
     // E.g. '/enterprise-server@latest' or '/enterprise-server@latest/3.3/foo'
     basicCorrection =
@@ -67,9 +77,9 @@ export default function getRedirect(uri, context) {
     }
   } else if (
     withoutLanguage.startsWith('/enterprise/') &&
-    supported.includes(withoutLanguage.split('/')[2])
+    supportedAndRecentlyDeprecated.includes(withoutLanguage.split('/')[2])
   ) {
-    // E.g. '/enterprise/3.3' or '/enterprise/3.3/foo'
+    // E.g. '/enterprise/3.3' or '/enterprise/3.3/foo' or '/enterprise/3.0/foo
 
     // If the URL is without a language, and no redirect is necessary,
     // but it has as version prefix, the language has to be there

middleware/archived-enterprise-versions.js

@@ -4,6 +4,7 @@ import statsd from '../lib/statsd.js'
 import {
   firstVersionDeprecatedOnNewSite,
   lastVersionWithoutArchivedRedirectsFile,
+  deprecatedWithFunctionalRedirects,
 } from '../lib/enterprise-server-releases.js'
 import patterns from '../lib/patterns.js'
 import versionSatisfiesRange from '../lib/version-satisfies-range.js'
@@ -13,6 +14,7 @@ import got from 'got'
 import { readCompressedJsonFileFallbackLazily } from '../lib/read-json-file.js'
 import { cacheControlFactory } from './cache-control.js'
 import { pathLanguagePrefixed, languagePrefixPathRegex } from '../lib/languages.js'
+import getRedirect, { splitPathByLanguage } from '../lib/get-redirect.js'
 
 function splitByLanguage(uri) {
   let language = null
@@ -35,6 +37,7 @@ const archivedFrontmatterFallbacks = readCompressedJsonFileFallbackLazily(
 )
 
 const cacheControl = cacheControlFactory(60 * 60 * 24 * 365)
+const noCacheControl = cacheControlFactory(0)
 
 // Combine all the things you need to make sure the response is
 // aggresively cached.
@@ -99,6 +102,39 @@ export default async function archivedEnterpriseVersions(req, res, next) {
 
   const redirectCode = pathLanguagePrefixed(req.path) ? 301 : 302
 
+  if (deprecatedWithFunctionalRedirects.includes(requestedVersion)) {
+    const redirectTo = getRedirect(req.path, req.context)
+    if (redirectTo) {
+      if (redirectCode === 301) {
+        cacheControl(res)
+      } else {
+        noCacheControl(res)
+      }
+      res.removeHeader('set-cookie')
+      return res.redirect(redirectCode, redirectTo)
+    }
+
+    const redirectJson = await getRemoteJSON(getProxyPath('redirects.json', requestedVersion), {
+      retry: retryConfiguration,
+      // This is allowed to be different compared to the other requests
+      // we make because downloading the `redirects.json` once is very
+      // useful because it caches so well.
+      // And, as of 2021 that `redirects.json` is 10MB so it's more likely
+      // to time out.
+      timeout: 1000,
+    })
+    const [language, withoutLanguage] = splitPathByLanguage(req.path, req.context.userLanguage)
+    const newRedirectTo = redirectJson[withoutLanguage]
+    if (newRedirectTo) {
+      if (redirectCode === 301) {
+        cacheControl(res)
+      } else {
+        noCacheControl(res)
+      }
+      res.removeHeader('set-cookie')
+      return res.redirect(redirectCode, `/${language}${newRedirectTo}`)
+    }
+  }
   // redirect language-prefixed URLs like /en/enterprise/2.10 -> /enterprise/2.10
   // (this only applies to versions <2.13)
   if (
@@ -135,7 +171,10 @@ export default async function archivedEnterpriseVersions(req, res, next) {
     }
   }
 
-  if (versionSatisfiesRange(requestedVersion, `>${lastVersionWithoutArchivedRedirectsFile}`)) {
+  if (
+    versionSatisfiesRange(requestedVersion, `>${lastVersionWithoutArchivedRedirectsFile}`) &&
+    !deprecatedWithFunctionalRedirects.includes(requestedVersion)
+  ) {
     const redirectJson = await getRemoteJSON(getProxyPath('redirects.json', requestedVersion), {
       retry: retryConfiguration,
       // This is allowed to be different compared to the other requests

tests/routing/deprecated-enterprise-versions.js

@@ -1,9 +1,11 @@
+import supertest from 'supertest'
+import { describe, jest, test } from '@jest/globals'
+
 import createApp from '../../lib/app.js'
 import enterpriseServerReleases from '../../lib/enterprise-server-releases.js'
 import { get, getDOM } from '../helpers/supertest.js'
 import { SURROGATE_ENUMS } from '../../middleware/set-fastly-surrogate-key.js'
-import supertest from 'supertest'
-import { jest } from '@jest/globals'
+import { PREFERRED_LOCALE_COOKIE_NAME } from '../../middleware/detect-language.js'
 
 jest.useFakeTimers('legacy')
 
@@ -89,6 +91,73 @@ describe('enterprise deprecation', () => {
   })
 })
 
+// Starting with the deprecation of 3.0, it's the first time we deprecate
+// enterprise versions since redirects is a *function* rather than a
+// lookup in a big object.
+describe('recently deprecated redirects', () => {
+  test('basic enterprise 3.0 redirects', async () => {
+    const res = await get('/enterprise/3.0')
+    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')
+  })
+  test('basic enterprise 3.0 redirects by cookie', async () => {
+    const res = await get('/enterprise/3.0', {
+      headers: {
+        Cookie: `${PREFERRED_LOCALE_COOKIE_NAME}=ja`,
+      },
+    })
+    expect(res.statusCode).toBe(302)
+    expect(res.headers.location).toBe('/ja/[email protected]')
+  })
+  test('already languaged enterprise 3.0 redirects', async () => {
+    const res = await get('/en/enterprise/3.0')
+    expect(res.statusCode).toBe(301)
+    expect(res.headers.location).toBe('/en/[email protected]')
+    // 301 redirects are safe to cache aggressively
+    expect(res.headers['set-cookie']).toBeUndefined()
+    expect(res.headers['cache-control']).toContain('public')
+    expect(res.headers['cache-control']).toMatch(/max-age=\d+/)
+  })
+  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')
+    // This is based on
+    // https://github.com/github/help-docs-archived-enterprise-versions/blob/master/3.0/redirects.json
+    expect(res.headers.location).toBe(
+      '/en/[email protected]/get-started/learning-about-github/githubs-products'
+    )
+  })
+  test('redirects enterprise-server 3.0 with actual redirect with language', async () => {
+    const res = await get(
+      '/ja/[email protected]/github/getting-started-with-github/githubs-products'
+    )
+    expect(res.statusCode).toBe(301)
+    expect(res.headers['set-cookie']).toBeUndefined()
+    expect(res.headers['cache-control']).toContain('public')
+    expect(res.headers['cache-control']).toMatch(/max-age=\d+/)
+    // This is based on
+    // https://github.com/github/help-docs-archived-enterprise-versions/blob/master/3.0/redirects.json
+    expect(res.headers.location).toBe(
+      '/ja/[email protected]/get-started/learning-about-github/githubs-products'
+    )
+  })
+  test('follow redirects enterprise-server 3.0 with actual redirect without language', async () => {
+    const res = await get(
+      '/[email protected]/github/getting-started-with-github/githubs-products',
+      { followAllRedirects: true }
+    )
+    expect(res.statusCode).toBe(200)
+  })
+})
+
 describe('deprecation banner', () => {
   test('renders a deprecation warning banner on oldest supported Enterprise version', async () => {
     const $ = await getDOM(`/en/enterprise/${enterpriseServerReleases.oldestSupported}`)

tests/unit/get-redirect.js

@@ -1,6 +1,36 @@
-import getRedirect from '../../lib/get-redirect.js'
+import { describe, expect, test } from '@jest/globals'
+
+import getRedirect, { splitPathByLanguage } from '../../lib/get-redirect.js'
 import { latest } from '../../lib/enterprise-server-releases.js'
 
+describe('splitPathByLanguage', () => {
+  test('basic', () => {
+    const [language, withoutLanguage] = splitPathByLanguage('/foo/')
+    expect(language).toBe('en')
+    expect(withoutLanguage).toBe('/foo/')
+  })
+  test('already has /en in it', () => {
+    const [language, withoutLanguage] = splitPathByLanguage('/en/foo/')
+    expect(language).toBe('en')
+    expect(withoutLanguage).toBe('/foo/')
+  })
+  test('basic with different fallback', () => {
+    const [language, withoutLanguage] = splitPathByLanguage('/foo/', 'ja')
+    expect(language).toBe('ja')
+    expect(withoutLanguage).toBe('/foo/')
+  })
+  test('already has /en different fallback', () => {
+    const [language, withoutLanguage] = splitPathByLanguage('/en/foo/', 'ja')
+    expect(language).toBe('en')
+    expect(withoutLanguage).toBe('/foo/')
+  })
+  test('unrecognized prefix is ignored', () => {
+    const [language, withoutLanguage] = splitPathByLanguage('/sv/foo/')
+    expect(language).toBe('en')
+    expect(withoutLanguage).toBe('/sv/foo/')
+  })
+})
+
 describe('getRedirect basics', () => {
   it('should sometimes not correct the version prefix', () => {
     // This essentially tests legacy entries that come from the
@@ -162,4 +192,15 @@ describe('getRedirect basics', () => {
     ctx.userLanguage = null
     expect(getRedirect('/foo', ctx)).toBe(`/en/bar`)
   })
+
+  it('should work for some deprecated enterprise-server URLs too', () => {
+    // Starting with enterprise-server 3.0, we have made redirects become
+    // a *function* rather than a lookup on a massive object.
+    const ctx = {
+      pages: {},
+      redirects: {},
+    }
+    expect(getRedirect('/enterprise/3.0', ctx)).toBe('/en/[email protected]')
+    expect(getRedirect('/enterprise/3.0/foo', ctx)).toBe('/en/[email protected]/foo')
+  })
 })