Merge branch 'main' into repo-sync

middleware/index.js

@@ -26,7 +26,6 @@ import findPage from './find-page.js'
 import blockRobots from './block-robots.js'
 import archivedEnterpriseVersionsAssets from './archived-enterprise-versions-assets.js'
 import api from './api/index.js'
-import search from './search.js'
 import healthz from './healthz.js'
 import anchorRedirect from './anchor-redirect.js'
 import remoteIP from './remote-ip.js'
@@ -231,7 +230,6 @@ export default function (app) {
 
   // *** Rendering, 2xx responses ***
   app.use('/api', instrument(api, './api'))
-  app.use('/search', instrument(search, './search')) // The old search API
   app.use('/healthz', instrument(healthz, './healthz'))
   app.use('/anchor-redirect', instrument(anchorRedirect, './anchor-redirect'))
   app.get('/_ip', instrument(remoteIP, './remoteIP'))

middleware/redirects/handle-redirects.js

@@ -24,16 +24,26 @@ export default function handleRedirects(req, res, next) {
     return res.redirect(302, `/${language}`)
   }
 
-  // Don't try to redirect if the URL is `/search` which is the XHR
-  // endpoint. It should not become `/en/search`.
-  // It's unfortunate and looks a bit needlessly complicated. But
-  // it comes from the legacy that the JSON API endpoint was and needs to
-  // continue to be `/search` when it would have been more neat if it
-  // was something like `/api/search`.
-  // If someone types in `/search?query=foo` manually, they'll get JSON.
-  // Maybe sometime in 2023 we remove `/search` as an endpoint for the
-  // JSON.
-  if (req.path === '/search') return next()
+  // The URL `/search` was the old JSON API. We no longer use it anywhere
+  // and neither does support.github.com any more.
+  // But there could be legacy third-party integrators which we don't know
+  // about.
+  // In the future we might want to re-use this for our dedicated search
+  // result page which is `/$lang/search` but until we're certain all
+  // third-party search apps have noticed, we can't do that. Perhaps
+  // some time in mid to late 2023.
+  if (req.path === '/search') {
+    let url = '/api/search/legacy'
+    if (Object.keys(req.query).length) {
+      url += `?${new URLSearchParams(req.query)}`
+    }
+    // This is a 302 redirect.
+    // Why not a 301? Because permanent redirects tend to get very stuck
+    // in client caches (e.g. browsers) which would make it hard to one
+    // day turn this redirect into a redirect to `/en/search` which is
+    // how all pages work when typed in without a language prefix.
+    return res.redirect(url)
+  }
 
   // begin redirect handling
   let redirect = req.path

middleware/search.js

@@ -1,57 +0,0 @@
-import express from 'express'
-import libLanguages from '../lib/languages.js'
-import searchVersions from '../lib/search/versions.js'
-import loadLunrResults, { QueryTermError } from '../lib/search/lunr-search.js'
-import { cacheControlFactory } from './cache-control.js'
-import catchMiddlewareError from './catch-middleware-error.js'
-
-const languages = new Set(Object.keys(libLanguages))
-const versions = new Set(Object.values(searchVersions))
-const router = express.Router()
-const cacheControl = cacheControlFactory(60 * 60 * 24)
-const noCacheControl = cacheControlFactory(0)
-
-router.get(
-  '/',
-  catchMiddlewareError(async function getSearch(req, res) {
-    const { query, version, language, filters, limit: limit_ } = req.query
-    const limit = Math.min(parseInt(limit_, 10) || 10, 100)
-    if (!versions.has(version)) {
-      return res.status(400).json({ error: 'Unrecognized version' })
-    }
-    if (!languages.has(language)) {
-      return res.status(400).json({ error: 'Unrecognized language' })
-    }
-    if (!query || !limit) {
-      return res.status(200).json([])
-    }
-
-    try {
-      const results = await loadLunrResults({
-        version,
-        language,
-        query: `${query} ${filters || ''}`,
-        limit,
-      })
-      // Only reply if the headers have not been sent and the request was not aborted...
-      if (!res.headersSent && !req.aborted) {
-        cacheControl(res)
-        return res.status(200).json(results)
-      }
-    } catch (err) {
-      if (err instanceof QueryTermError) {
-        // Handled as an not entirely unexpected potential error
-        return res.status(400).json({ error: err.toString() })
-      }
-
-      console.error(err)
-      // Only reply if the headers have not been sent and the request was not aborted...
-      if (!res.headersSent && !req.aborted) {
-        noCacheControl(res)
-        return res.status(400).json({ error: err.toString() })
-      }
-    }
-  })
-)
-
-export default router

tests/content/search.js

@@ -1,114 +0,0 @@
-import { jest, describe, expect } from '@jest/globals'
-
-import { dates, supported } from '../../lib/enterprise-server-releases.js'
-import libLanguages from '../../lib/languages.js'
-import { namePrefix } from '../../lib/search/config.js'
-import lunrIndexNames from '../../script/search/lunr-get-index-names.js'
-import { get } from '../helpers/e2etest.js'
-
-const languageCodes = Object.keys(libLanguages)
-
-describe('search', () => {
-  test('has Lunr index for every language for every supported GHE version', () => {
-    expect(supported.length).toBeGreaterThan(1)
-    supported.forEach((version) => {
-      languageCodes.forEach((languageCode) => {
-        const indexName = `${namePrefix}-${version}-${languageCode}`
-        const indexRecordName = `${indexName}-records`
-
-        // workaround for GHES release branches not in production yet
-        if (!lunrIndexNames.includes(indexName)) {
-          const today = getDate()
-          const releaseDate = getDate(dates[version].releaseDate)
-          // if the release date is in the future or today, ignore this version;
-          // this means if the new index is not uploaded at the time of the release,
-          // the test will not fail until the following day.
-          if (releaseDate >= today) return
-        }
-
-        expect(lunrIndexNames.includes(indexName)).toBe(true)
-        expect(lunrIndexNames.includes(indexRecordName)).toBe(true)
-      })
-    })
-  })
-})
-
-function getDate(date) {
-  const dateObj = date ? new Date(date) : new Date()
-  return dateObj.toISOString().slice(0, 10)
-}
-
-describe('search middleware', () => {
-  jest.setTimeout(60 * 1000)
-
-  test('basic search', async () => {
-    const sp = new URLSearchParams()
-    sp.set('query', 'stuff')
-    sp.set('language', 'en')
-    sp.set('version', 'dotcom')
-    const res = await get('/search?' + sp)
-    expect(res.statusCode).toBe(200)
-    const results = JSON.parse(res.text)
-    expect(Array.isArray(results)).toBeTruthy()
-
-    // 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+/)
-  })
-
-  test('limit search', async () => {
-    const sp = new URLSearchParams()
-    sp.set('query', 'github') // will yield lots of results
-    sp.set('language', 'en')
-    sp.set('version', 'dotcom')
-    sp.set('limit', '1')
-    const res = await get('/search?' + sp)
-    expect(res.statusCode).toBe(200)
-    const results = JSON.parse(res.text)
-    expect(Array.isArray(results)).toBeTruthy()
-    expect(results.length).toBe(1)
-  })
-
-  test('invalid search version', async () => {
-    const sp = new URLSearchParams()
-    sp.set('query', 'stuff')
-    sp.set('language', 'en')
-    sp.set('version', 'NEVERHEARDOF')
-    const res = await get('/search?' + sp)
-    expect(res.statusCode).toBe(400)
-    const { error } = JSON.parse(res.text)
-    expect(error).toContain('Unrecognized version')
-  })
-
-  test('invalid search language', async () => {
-    const sp = new URLSearchParams()
-    sp.set('query', 'stuff')
-    sp.set('language', 'NEVERHEARDOF')
-    sp.set('version', 'dotcom')
-    const res = await get('/search?' + sp)
-    expect(res.statusCode).toBe(400)
-    const { error } = JSON.parse(res.text)
-    expect(error).toContain('Unrecognized language')
-  })
-
-  test('only certain keywords are supported', async () => {
-    const sp = new URLSearchParams()
-    sp.set('language', 'en')
-    sp.set('version', 'dotcom')
-
-    sp.set('query', 'title: Security')
-    let res = await get('/search?' + sp)
-    expect(res.statusCode).toBe(200)
-
-    // and...
-    sp.set('query', 'topics: actions')
-    res = await get('/search?' + sp)
-    expect(res.statusCode).toBe(200)
-
-    // but...
-    sp.set('query', 'anyword: bla')
-    res = await get('/search?' + sp)
-    expect(res.statusCode).toBe(400)
-  })
-})

tests/routing/redirects.js

@@ -486,4 +486,19 @@ describe('redirects', () => {
       expect(res.headers.location).toBe(`/en`)
     })
   })
+
+  describe('redirects from old Lunr search to ES legacy search', () => {
+    test('redirects even without query string', async () => {
+      const res = await get(`/search`, { followRedirects: false })
+      expect(res.statusCode).toBe(302)
+      expect(res.headers.location).toBe(`/api/search/legacy`)
+    })
+
+    test('redirects with query string', async () => {
+      const params = new URLSearchParams({ foo: 'bar' })
+      const res = await get(`/search?${params}`, { followRedirects: false })
+      expect(res.statusCode).toBe(302)
+      expect(res.headers.location).toBe(`/api/search/legacy?${params}`)
+    })
+  })
 })