Skip to content

Commit

Permalink
Be gone old api/search/legacy (github#34839)
Browse files Browse the repository at this point in the history
  • Loading branch information
@peterbe
peterbe authored Feb 16, 2023
1 parent 3fbfe56 commit fc63361
Show file tree
Hide file tree
Showing 4 changed files with 3 additions and 176 deletions.
113 changes: 3 additions & 110 deletions middleware/api/search.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import express from 'express'

import searchVersions from '../../lib/search/versions.js'
import FailBot from '../../lib/failbot.js'
import languages from '../../lib/languages.js'
import { allVersions } from '../../lib/all-versions.js'
Expand All @@ -14,10 +13,6 @@ import {
DEFAULT_HIGHLIGHT_FIELDS,
} from './es-search.js'

// Used by the legacy search
const versions = new Set(Object.values(searchVersions))
const languagesSet = new Set(Object.keys(languages))

const router = express.Router()

const DEFAULT_SIZE = 10
Expand Down Expand Up @@ -48,16 +43,6 @@ Object.values(allVersions).forEach((info) => {
}
})

const legacyEnterpriseServerVersions = Object.fromEntries(
Object.entries(searchVersions)
.filter(([fullName]) => {
return fullName.startsWith('enterprise-server@')
})
.map(([, shortName]) => {
return [shortName, `ghes-${shortName}`]
})
)

function getIndexPrefix() {
// This logic is mirrored in the scripts we use before running tests
// In particular, see the `index-test-fixtures` npm script.
Expand All @@ -72,101 +57,9 @@ function getIndexPrefix() {
return ''
}

function convertLegacyVersionName(version) {
// In the olden days we used to use `?version=3.5&...` but we decided
// that's ambiguous and it should be `ghes-3.5` instead.
return legacyEnterpriseServerVersions[version] || version
}

router.get(
'/legacy',
catchMiddlewareError(async function legacySearch(req, res) {
const { query, version, language, filters, limit: limit_ } = req.query
const topics = []
if (filters) {
if (Array.isArray(filters)) {
topics.push(...filters)
} else {
topics.push(filters)
}
}
const limit = Math.min(parseInt(limit_, 10) || 10, 100)
if (!versions.has(version)) {
return res.status(400).json({ error: 'Unrecognized version' })
}
if (!languagesSet.has(language)) {
return res.status(400).json({ error: 'Unrecognized language' })
}
if (!query || !limit) {
return res.status(200).json([])
}

const indexName = `${getIndexPrefix()}github-docs-${convertLegacyVersionName(
version
)}-${language}`

const hits = []
const tags = ['version:legacy', `indexName:${indexName}`]
const timed = statsd.asyncTimer(getSearchResults, 'api.search', tags)
const options = {
indexName,
query,
page: 1,
sort: 'best',
size: limit,
debug: true,
includeTopics: true,
// The legacy search is used as an autocomplete. In other words,
// a debounce that sends the query before the user has had a
// chance to fully submit the search. That means if the user
// send the query 'google cl' they hope to find 'Google Cloud'
// even though they didn't type that fully.
usePrefixSearch: true,
topics,
}
try {
const { hits: hits_, meta } = await timed(options)
hits.push(...hits_)
statsd.timing('api.search.total', meta.took.total_msec, tags)
statsd.timing('api.search.query', meta.took.query_msec, tags)
} catch (error) {
// If we don't catch here, the `catchMiddlewareError()` wrapper
// will take any thrown error and pass it to `next()`.
await handleGetSearchResultsError(req, res, error, options)
return
}

// The legacy search just returned an array
const results = hits.map((hit) => {
let title = hit.title
if (hit.highlights?.title && hit.highlights?.title.length) {
title = hit.highlights.title[0]
}
let content = ''
if (hit.highlights?.content && hit.highlights?.content.length) {
content = hit.highlights.content.join('\n')
}

return {
url: hit.url,
title,
breadcrumbs: hit.breadcrumbs || '',
content,
topics: hit.topics || [],
popularity: hit.popularity || 0.0,
score: hit.score,
}
})
if (process.env.NODE_ENV !== 'development') {
searchCacheControl(res)
setFastlySurrogateKey(res, `api-search:${language}`, true)
}

res.setHeader('x-search-legacy', 'yes')

res.status(200).json(results)
})
)
router.get('/legacy', (req, res) => {
res.status(410).send('Use /api/search/v1 instead.')
})

class ValidationError extends Error {}

Expand Down
21 changes: 0 additions & 21 deletions middleware/redirects/handle-redirects.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,27 +21,6 @@ export default function handleRedirects(req, res, next) {
return res.redirect(302, `/${language}`)
}

// 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
let queryParams = req._parsedUrl.query
Expand Down
30 changes: 0 additions & 30 deletions tests/content/api-search.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,36 +256,6 @@ describeIfElasticsearchURL('search v1 middleware', () => {
})
})

describeIfElasticsearchURL('search legacy middleware', () => {
jest.setTimeout(60 * 1000)

test('basic legacy search', async () => {
const sp = new URLSearchParams()
sp.set('query', 'foo')
sp.set('language', 'en')
sp.set('version', 'dotcom')
const res = await get('/api/search/legacy?' + sp)
expect(res.statusCode).toBe(200)
const results = JSON.parse(res.text)
expect(Array.isArray(results)).toBeTruthy()
const foundURLS = results.map((result) => result.url)
expect(foundURLS.includes('/en/foo')).toBeTruthy()
})

test('basic legacy search with unknown filters', async () => {
const sp = new URLSearchParams()
sp.set('query', 'foo')
sp.set('language', 'en')
sp.set('version', 'dotcom')
sp.set('filters', 'Never heard of')
const res = await get('/api/search/legacy?' + sp)
expect(res.statusCode).toBe(200)
const results = JSON.parse(res.text)
expect(Array.isArray(results)).toBeTruthy()
expect(results.length).toBe(0)
})
})

describeIfElasticsearchURL("additional fields with 'include'", () => {
jest.setTimeout(60 * 1000)

Expand Down
15 changes: 0 additions & 15 deletions tests/routing/redirects.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,19 +471,4 @@ 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}`)
})
})
})

0 comments on commit fc63361

Please sign in to comment.