diff --git a/middleware/index.js b/middleware/index.js index 51e7bf45f7..610e593d0e 100644 --- a/middleware/index.js +++ b/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')) diff --git a/middleware/redirects/handle-redirects.js b/middleware/redirects/handle-redirects.js index 48eea18472..ea04f02e80 100644 --- a/middleware/redirects/handle-redirects.js +++ b/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 diff --git a/middleware/search.js b/middleware/search.js deleted file mode 100644 index c3711911ab..0000000000 --- a/middleware/search.js +++ /dev/null @@ -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 diff --git a/tests/content/search.js b/tests/content/search.js deleted file mode 100644 index f99423cb7a..0000000000 --- a/tests/content/search.js +++ /dev/null @@ -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) - }) -}) diff --git a/tests/routing/redirects.js b/tests/routing/redirects.js index 2331232325..743353bb29 100644 --- a/tests/routing/redirects.js +++ b/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}`) + }) + }) })