Make redirect_from safe when old and new pages coexist (#62757)

Author: sarahs

src/redirects/lib/precompile.ts

@@ -26,6 +26,16 @@ export async function precompileRedirects(pageList: Page[]): Promise<Redirects>
     Object.assign(allRedirects, page.buildRedirects())
   }
 
+  // Remove any redirect whose source URL is also a real page permalink.
+  // This prevents redirect_from entries from clobbering live pages when a
+  // new page (versioned broadly) declares a redirect_from that overlaps
+  // with an older page that still exists in some versions.
+  for (const page of pageList.filter((xpage) => xpage.languageCode === 'en')) {
+    for (const permalink of page.permalinks) {
+      delete allRedirects[permalink.hrefWithoutLanguage]
+    }
+  }
+
   // NOTE: Exception redirects **MUST COME AFTER** pageList redirects above in order
   // to properly override them. Exception redirects are unicorn one-offs that are not
   // otherwise handled by the versionless redirect fallbacks (see lib/all-versions.ts).

src/redirects/tests/content/redirect-orphans.ts

@@ -3,39 +3,24 @@ import path from 'path'
 import { describe, expect, test, vi } from 'vitest'
 
 import { loadPages } from '@/frame/lib/page-data'
-import Permalink from '@/frame/lib/permalink'
 
 describe('redirect orphans', () => {
   // Because calling `loadPages` will trigger a warmup, this can potentially
   // be very slow in CI. So we need a timeout.
   vi.setConfig({ testTimeout: 60 * 1000 })
 
-  test('no page is a redirect in another file', async () => {
+  test('no redirect_from entry has a trailing slash', async () => {
     // Only doing English because they're the only files we do PRs for.
     const pageList = await loadPages(undefined, ['en'])
 
-    const redirectFroms = new Map()
+    const errors = []
     for (const page of pageList) {
       for (const redirectFrom of page.redirect_from || []) {
         if (redirectFrom.endsWith('/') && redirectFrom.startsWith('/')) {
-          throw new Error(
-            `In ${path.join(
-              'content',
-              page.relativePath,
-            )} redirect entry (${redirectFrom}) has a trailing slash`,
+          errors.push(
+            `In ${path.join('content', page.relativePath)} redirect entry (${redirectFrom}) has a trailing slash`,
           )
         }
-        redirectFroms.set(redirectFrom, page.relativePath)
-      }
-    }
-
-    const errors = []
-    for (const page of pageList) {
-      const asPath = Permalink.relativePathToSuffix(page.relativePath)
-      if (redirectFroms.has(asPath)) {
-        errors.push(
-          `${asPath} is a redirect_from in ${path.join('content', redirectFroms.get(asPath))}`,
-        )
       }
     }
     expect(errors.length, errors.join('\n')).toBe(0)

src/redirects/tests/unit/precompile.ts

@@ -0,0 +1,79 @@
+import { describe, expect, test, vi } from 'vitest'
+
+import type { Page } from '@/types'
+
+vi.mock('@/frame/lib/read-json-file', () => ({
+  readCompressedJsonFileFallback: () => ({}),
+}))
+
+vi.mock('../../lib/exception-redirects', () => ({
+  default: () => ({}),
+}))
+
+const { default: precompileRedirects } = await import('../../lib/precompile')
+const { default: generateRedirectsForPermalinks } = await import('../../lib/permalinks')
+
+// Minimal stand-in for a Page instance. precompileRedirects() only relies on
+// `languageCode`, `permalinks`, and `buildRedirects()`.
+function makePage(
+  languageCode: string,
+  permalinks: { pageVersion: string; hrefWithoutLanguage: string }[],
+  redirectFrom: string[],
+): Page {
+  const fullPermalinks = permalinks.map((permalink) => ({
+    languageCode,
+    title: 'Title',
+    href: `/${languageCode}${permalink.hrefWithoutLanguage}`,
+    ...permalink,
+  }))
+  return {
+    languageCode,
+    permalinks: fullPermalinks,
+    redirect_from: redirectFrom,
+    buildRedirects: () => generateRedirectsForPermalinks(fullPermalinks, redirectFrom),
+  } as unknown as Page
+}
+
+describe('precompileRedirects', () => {
+  test('removes a redirect_from-generated redirect that clobbers a live old-page permalink, but keeps it for versions where the old page is absent', async () => {
+    // The old page only exists in GHES 3.14.
+    const oldPage = makePage(
+      'en',
+      [
+        {
+          pageVersion: 'enterprise-server@3.14',
+          hrefWithoutLanguage: '/enterprise-server@3.14/foo',
+        },
+      ],
+      [],
+    )
+
+    // The replacement page exists in both 3.14 and 3.15, and declares
+    // `redirect_from: ['/foo']`, which would otherwise clobber the old
+    // page's live permalink in 3.14.
+    const newPage = makePage(
+      'en',
+      [
+        {
+          pageVersion: 'enterprise-server@3.14',
+          hrefWithoutLanguage: '/enterprise-server@3.14/bar',
+        },
+        {
+          pageVersion: 'enterprise-server@3.15',
+          hrefWithoutLanguage: '/enterprise-server@3.15/bar',
+        },
+      ],
+      ['/foo'],
+    )
+
+    const redirects = await precompileRedirects([oldPage, newPage])
+
+    // The old page's live permalink in 3.14 must not be clobbered by the
+    // replacement page's redirect_from.
+    expect(redirects['/enterprise-server@3.14/foo']).toBeUndefined()
+
+    // But in 3.15, where the old page doesn't exist, the redirect must
+    // still be there.
+    expect(redirects['/enterprise-server@3.15/foo']).toBe('/enterprise-server@3.15/bar')
+  })
+})