From eb81a5fdfbadc53e7f779840b7be82cfdcc41c0f Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Mon, 16 Aug 2021 09:30:32 +0200 Subject: [PATCH 01/12] Implement `offset` and `limit` based paging in search API Signed-off-by: Oliver Sand --- packages/search-common/src/types.ts | 4 +++- plugins/search-backend/src/service/router.ts | 12 +++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/search-common/src/types.ts b/packages/search-common/src/types.ts index 2a4447efac..0bd12fbd42 100644 --- a/packages/search-common/src/types.ts +++ b/packages/search-common/src/types.ts @@ -19,7 +19,8 @@ export interface SearchQuery { term: string; filters?: JsonObject; types?: string[]; - pageCursor: string; + offset?: number; + limit?: number; } export interface SearchResult { @@ -29,6 +30,7 @@ export interface SearchResult { export interface SearchResultSet { results: SearchResult[]; + totalCount: number; } /** diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index 3a0a43522e..5a87fb1734 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -36,15 +36,21 @@ export async function createRouter({ req: express.Request, res: express.Response, ) => { - const { term, filters = {}, pageCursor = '' } = req.query; + const { term, filters = {}, types, offset, limit } = req.query; logger.info( `Search request received: term="${term}", filters=${JSON.stringify( filters, - )}, ${pageCursor}`, + )}, offset=${offset ?? ''}, limit=${limit ?? ''}`, ); try { - const results = await engine?.query(req.query); + const results = await engine?.query({ + term, + types, + filters, + offset: offset ? Number(offset) : undefined, + limit: limit ? Number(limit) : undefined, + }); res.send(results); } catch (err) { throw new Error( From 3abef1177e833d59e1c2f4377d4d98ee241e0aeb Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Mon, 16 Aug 2021 09:30:55 +0200 Subject: [PATCH 02/12] Implement `offset` and `limit` based paging in Lunr search engine Signed-off-by: Oliver Sand --- .../src/engines/LunrSearchEngine.test.ts | 123 +++++++++++++++--- .../src/engines/LunrSearchEngine.ts | 11 +- 2 files changed, 111 insertions(+), 23 deletions(-) diff --git a/plugins/search-backend-node/src/engines/LunrSearchEngine.test.ts b/plugins/search-backend-node/src/engines/LunrSearchEngine.test.ts index 3233c3adae..71c2b95a29 100644 --- a/plugins/search-backend-node/src/engines/LunrSearchEngine.test.ts +++ b/plugins/search-backend-node/src/engines/LunrSearchEngine.test.ts @@ -48,14 +48,12 @@ describe('LunrSearchEngine', () => { await testLunrSearchEngine.query({ term: 'testTerm', filters: {}, - pageCursor: '', }); // Then: the translator is invoked with expected args. expect(translatorSpy).toHaveBeenCalledWith({ term: 'testTerm', filters: {}, - pageCursor: '', }); }); @@ -68,12 +66,101 @@ describe('LunrSearchEngine', () => { const actualTranslatedQuery = translatorUnderTest({ term: 'testTerm', filters: {}, - pageCursor: '', + offset: 0, + limit: 50, }) as ConcreteLunrQuery; expect(actualTranslatedQuery).toMatchObject({ documentTypes: undefined, lunrQueryBuilder: expect.any(Function), + offset: 0, + limit: 50, + }); + + const query: jest.Mocked = { + allFields: [], + clauses: [], + term: jest.fn(), + clause: jest.fn(), + }; + + actualTranslatedQuery.lunrQueryBuilder.bind(query)(query); + + expect(query.term).toBeCalledWith(lunr.tokenizer('testTerm'), { + boost: 100, + usePipeline: true, + }); + expect(query.term).toBeCalledWith(lunr.tokenizer('testTerm'), { + boost: 10, + usePipeline: false, + wildcard: lunr.Query.wildcard.TRAILING, + }); + expect(query.term).toBeCalledWith(lunr.tokenizer('testTerm'), { + boost: 1, + usePipeline: false, + editDistance: 2, + }); + }); + + it('should have default offset and limit', async () => { + const inspectableSearchEngine = new LunrSearchEngineForTranslatorTests({ + logger: getVoidLogger(), + }); + const translatorUnderTest = inspectableSearchEngine.getTranslator(); + + const actualTranslatedQuery = translatorUnderTest({ + term: 'testTerm', + }) as ConcreteLunrQuery; + + expect(actualTranslatedQuery).toMatchObject({ + documentTypes: undefined, + lunrQueryBuilder: expect.any(Function), + offset: 0, + limit: 25, + }); + + const query: jest.Mocked = { + allFields: [], + clauses: [], + term: jest.fn(), + clause: jest.fn(), + }; + + actualTranslatedQuery.lunrQueryBuilder.bind(query)(query); + + expect(query.term).toBeCalledWith(lunr.tokenizer('testTerm'), { + boost: 100, + usePipeline: true, + }); + expect(query.term).toBeCalledWith(lunr.tokenizer('testTerm'), { + boost: 10, + usePipeline: false, + wildcard: lunr.Query.wildcard.TRAILING, + }); + expect(query.term).toBeCalledWith(lunr.tokenizer('testTerm'), { + boost: 1, + usePipeline: false, + editDistance: 2, + }); + }); + + it('should have maximum limit', async () => { + const inspectableSearchEngine = new LunrSearchEngineForTranslatorTests({ + logger: getVoidLogger(), + }); + const translatorUnderTest = inspectableSearchEngine.getTranslator(); + + const actualTranslatedQuery = translatorUnderTest({ + term: 'testTerm', + offset: 0, + limit: 1000, + }) as ConcreteLunrQuery; + + expect(actualTranslatedQuery).toMatchObject({ + documentTypes: undefined, + lunrQueryBuilder: expect.any(Function), + offset: 0, + limit: 100, }); const query: jest.Mocked = { @@ -110,7 +197,6 @@ describe('LunrSearchEngine', () => { const actualTranslatedQuery = translatorUnderTest({ term: 'testTerm', filters: { kind: 'testKind' }, - pageCursor: '', }) as ConcreteLunrQuery; expect(actualTranslatedQuery).toMatchObject({ @@ -156,7 +242,6 @@ describe('LunrSearchEngine', () => { const actualTranslatedQuery = translatorUnderTest({ term: 'testTerm', filters: { kind: 'testKind', namespace: 'testNameSpace' }, - pageCursor: '', }) as ConcreteLunrQuery; expect(actualTranslatedQuery).toMatchObject({ @@ -206,7 +291,6 @@ describe('LunrSearchEngine', () => { const actualTranslatedQuery = translatorUnderTest({ term: 'testTerm', filters: { kind: 'testKind' }, - pageCursor: '', }) as ConcreteLunrQuery; expect(actualTranslatedQuery).toMatchObject({ @@ -235,18 +319,16 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'testTerm', filters: {}, - pageCursor: '', }); expect(querySpy).toHaveBeenCalled(); expect(querySpy).toHaveBeenCalledWith({ term: 'testTerm', filters: {}, - pageCursor: '', }); // Should return 0 results as nothing is indexed here - expect(mockedSearchResult).toMatchObject({ results: [] }); + expect(mockedSearchResult).toMatchObject({ results: [], totalCount: 0 }); }); it('should perform search query and return 0 results on no match', async () => { @@ -265,11 +347,10 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'unknown', filters: {}, - pageCursor: '', }); // Should return 0 results as we are mocking the indexing of 1 document but with no match on the fields - expect(mockedSearchResult).toMatchObject({ results: [] }); + expect(mockedSearchResult).toMatchObject({ results: [], totalCount: 0 }); }); it('should perform search query and return all results on empty term', async () => { @@ -288,7 +369,6 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: '', filters: {}, - pageCursor: '', }); expect(mockedSearchResult).toMatchObject({ @@ -302,6 +382,7 @@ describe('LunrSearchEngine', () => { type: 'test-index', }, ], + totalCount: 1, }); }); @@ -321,7 +402,6 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'testTitle', filters: {}, - pageCursor: '', }); expect(mockedSearchResult).toMatchObject({ @@ -334,6 +414,7 @@ describe('LunrSearchEngine', () => { }, }, ], + totalCount: 1, }); }); @@ -353,7 +434,6 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'testTitle', filters: {}, - pageCursor: '', }); expect(mockedSearchResult).toMatchObject({ @@ -366,6 +446,7 @@ describe('LunrSearchEngine', () => { }, }, ], + totalCount: 1, }); }); @@ -385,7 +466,6 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'testTitel', // Intentional typo filters: {}, - pageCursor: '', }); // Should return 1 result as we are mocking the indexing of 1 document with match on the title field @@ -399,6 +479,7 @@ describe('LunrSearchEngine', () => { }, }, ], + totalCount: 1, }); }); @@ -418,7 +499,6 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'World', filters: {}, - pageCursor: '', }); // Should return 1 result as we are mocking the indexing of 1 document with match on the title field @@ -432,6 +512,7 @@ describe('LunrSearchEngine', () => { }, }, ], + totalCount: 1, }); }); @@ -451,7 +532,6 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'Search', filters: {}, - pageCursor: '', }); // Should return 1 result as we are mocking the indexing of 1 document with match on the title field @@ -465,6 +545,7 @@ describe('LunrSearchEngine', () => { }, }, ], + totalCount: 1, }); }); @@ -489,7 +570,6 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'testTitle', filters: { location: 'test/location2' }, - pageCursor: '', }); // Should return 1 of 2 results as we are @@ -505,6 +585,7 @@ describe('LunrSearchEngine', () => { }, }, ], + totalCount: 1, }); }); @@ -532,7 +613,6 @@ describe('LunrSearchEngine', () => { // Perform search query scoped to "test-index-2" with a filter on the field "extraField" const mockedSearchResult = await testLunrSearchEngine.query({ term: 'testTitle', - pageCursor: '', filters: { extraField: 'testExtraField' }, }); @@ -547,6 +627,7 @@ describe('LunrSearchEngine', () => { }, }, ], + totalCount: 1, }); }); @@ -571,7 +652,6 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'testTitle', filters: { location: 'test:location2' }, - pageCursor: '', }); // Should return 1 of 2 results as we are @@ -587,6 +667,7 @@ describe('LunrSearchEngine', () => { }, }, ], + totalCount: 1, }); }); @@ -625,7 +706,6 @@ describe('LunrSearchEngine', () => { const mockedSearchResult = await testLunrSearchEngine.query({ term: 'testTitle', types: ['test-index-2'], - pageCursor: '', }); expect(mockedSearchResult).toMatchObject({ @@ -645,6 +725,7 @@ describe('LunrSearchEngine', () => { }, }, ], + totalCount: 2, }); }); }); diff --git a/plugins/search-backend-node/src/engines/LunrSearchEngine.ts b/plugins/search-backend-node/src/engines/LunrSearchEngine.ts index 0cb4fd8336..75e5fdc625 100644 --- a/plugins/search-backend-node/src/engines/LunrSearchEngine.ts +++ b/plugins/search-backend-node/src/engines/LunrSearchEngine.ts @@ -27,6 +27,8 @@ import { Logger } from 'winston'; export type ConcreteLunrQuery = { lunrQueryBuilder: lunr.Index.QueryBuilder; documentTypes?: string[]; + offset: number; + limit: number; }; type LunrResultEnvelope = { @@ -50,6 +52,8 @@ export class LunrSearchEngine implements SearchEngine { term, filters, types, + offset, + limit, }: SearchQuery): ConcreteLunrQuery => { return { lunrQueryBuilder: q => { @@ -107,6 +111,8 @@ export class LunrSearchEngine implements SearchEngine { } }, documentTypes: types, + offset: offset ?? 0, + limit: Math.min(limit ?? 25, 100), }; }; @@ -141,7 +147,7 @@ export class LunrSearchEngine implements SearchEngine { } async query(query: SearchQuery): Promise { - const { lunrQueryBuilder, documentTypes } = this.translator( + const { lunrQueryBuilder, documentTypes, offset, limit } = this.translator( query, ) as ConcreteLunrQuery; @@ -179,9 +185,10 @@ export class LunrSearchEngine implements SearchEngine { // Translate results into SearchResultSet const realResultSet: SearchResultSet = { - results: results.map(d => { + results: results.slice(offset, offset + limit).map(d => { return { type: d.type, document: this.docStore[d.result.ref] }; }), + totalCount: results.length, }; return realResultSet; From 81cf103cf65ca925a0244962f4dcb1d6e2d46bcd Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Mon, 16 Aug 2021 09:31:10 +0200 Subject: [PATCH 03/12] Implement `offset` and `limit` based paging in elastic search engine Signed-off-by: Oliver Sand --- .../engines/ElasticSearchSearchEngine.test.ts | 100 +++++++++++++++--- .../src/engines/ElasticSearchSearchEngine.ts | 8 +- 2 files changed, 89 insertions(+), 19 deletions(-) diff --git a/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.test.ts b/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.test.ts index b4bba9de9c..18ee1314ce 100644 --- a/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.test.ts +++ b/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.test.ts @@ -87,13 +87,11 @@ describe('ElasticSearchSearchEngine', () => { await testSearchEngine.query({ term: 'testTerm', filters: {}, - pageCursor: '', }); expect(translatorSpy).toHaveBeenCalledWith({ term: 'testTerm', filters: {}, - pageCursor: '', }); }); @@ -104,7 +102,6 @@ describe('ElasticSearchSearchEngine', () => { types: ['indexName'], term: 'testTerm', filters: { kind: 'testKind' }, - pageCursor: '', }) as ConcreteElasticSearchQuery; expect(actualTranslatedQuery).toMatchObject({ @@ -132,6 +129,79 @@ describe('ElasticSearchSearchEngine', () => { }, }, }, + from: 0, + size: 25, + }); + }); + + it('should pass offset and limit', async () => { + const translatorUnderTest = inspectableSearchEngine.getTranslator(); + + const actualTranslatedQuery = translatorUnderTest({ + types: ['indexName'], + term: 'testTerm', + offset: 25, + limit: 50, + }) as ConcreteElasticSearchQuery; + + expect(actualTranslatedQuery).toMatchObject({ + documentTypes: ['indexName'], + elasticSearchQuery: expect.any(Object), + }); + + const queryBody = actualTranslatedQuery.elasticSearchQuery; + + expect(queryBody).toEqual({ + query: { + bool: { + filter: [], + must: { + multi_match: { + query: 'testTerm', + fields: ['*'], + fuzziness: 'auto', + minimum_should_match: 1, + }, + }, + }, + }, + from: 25, + size: 50, + }); + }); + + it('should have maximum limit of 100', async () => { + const translatorUnderTest = inspectableSearchEngine.getTranslator(); + + const actualTranslatedQuery = translatorUnderTest({ + types: ['indexName'], + term: 'testTerm', + offset: 25, + limit: 500, + }) as ConcreteElasticSearchQuery; + + expect(actualTranslatedQuery).toMatchObject({ + documentTypes: ['indexName'], + elasticSearchQuery: expect.any(Object), + }); + + const queryBody = actualTranslatedQuery.elasticSearchQuery; + + expect(queryBody).toEqual({ + query: { + bool: { + filter: [], + must: { + multi_match: { + query: 'testTerm', + fields: ['*'], + fuzziness: 'auto', + minimum_should_match: 1, + }, + }, + }, + }, + from: 25, size: 100, }); }); @@ -143,7 +213,6 @@ describe('ElasticSearchSearchEngine', () => { types: ['indexName'], term: 'testTerm', filters: { kind: 'testKind', namespace: 'testNameSpace' }, - pageCursor: '', }) as ConcreteElasticSearchQuery; expect(actualTranslatedQuery).toMatchObject({ @@ -178,7 +247,8 @@ describe('ElasticSearchSearchEngine', () => { ], }, }, - size: 100, + from: 0, + size: 25, }); }); @@ -189,7 +259,6 @@ describe('ElasticSearchSearchEngine', () => { types: ['indexName'], term: 'testTerm', filters: { kind: ['testKind', 'kastTeint'] }, - pageCursor: '', }) as ConcreteElasticSearchQuery; expect(actualTranslatedQuery).toMatchObject({ @@ -228,7 +297,8 @@ describe('ElasticSearchSearchEngine', () => { }, }, }, - size: 100, + from: 0, + size: 25, }); }); @@ -239,7 +309,6 @@ describe('ElasticSearchSearchEngine', () => { types: ['indexName'], term: 'testTerm', filters: { kind: { a: 'b' } }, - pageCursor: '', }) as ConcreteElasticSearchQuery; expect(actualTranslatedQuery).toThrow(); }); @@ -315,11 +384,10 @@ describe('ElasticSearchSearchEngine', () => { const mockedSearchResult = await testSearchEngine.query({ term: 'testTerm', filters: {}, - pageCursor: '', }); // Should return 0 results as nothing is indexed here - expect(mockedSearchResult).toMatchObject({ results: [] }); + expect(mockedSearchResult).toMatchObject({ results: [], totalCount: 0 }); }); it('should handle index/search type filtering correctly', async () => { @@ -327,7 +395,6 @@ describe('ElasticSearchSearchEngine', () => { await testSearchEngine.query({ term: 'testTerm', filters: {}, - pageCursor: '', }); expect(elasticSearchQuerySpy).toHaveBeenCalled(); @@ -346,7 +413,8 @@ describe('ElasticSearchSearchEngine', () => { filter: [], }, }, - size: 100, + from: 0, + size: 25, }, index: '*__search', }); @@ -359,7 +427,6 @@ describe('ElasticSearchSearchEngine', () => { await testSearchEngine.query({ term: '', filters: {}, - pageCursor: '', }); expect(elasticSearchQuerySpy).toHaveBeenCalled(); @@ -373,7 +440,8 @@ describe('ElasticSearchSearchEngine', () => { filter: [], }, }, - size: 100, + from: 0, + size: 25, }, index: '*__search', }); @@ -386,7 +454,6 @@ describe('ElasticSearchSearchEngine', () => { await testSearchEngine.query({ term: '', filters: {}, - pageCursor: '', types: ['test-type'], }); @@ -401,7 +468,8 @@ describe('ElasticSearchSearchEngine', () => { filter: [], }, }, - size: 100, + from: 0, + size: 25, }, index: ['test-type__search'], }); diff --git a/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.ts b/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.ts index d1e831b2ca..c1c959778b 100644 --- a/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.ts +++ b/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.ts @@ -140,6 +140,8 @@ export class ElasticSearchSearchEngine implements SearchEngine { term, filters = {}, types, + offset, + limit, }: SearchQuery): ConcreteElasticSearchQuery { const filter = Object.entries(filters) .filter(([_, value]) => Boolean(value)) @@ -172,9 +174,8 @@ export class ElasticSearchSearchEngine implements SearchEngine { elasticSearchQuery: esb .requestBodySearch() .query(esb.boolQuery().filter(filter).must([query])) - // TODO: Replace size limit with page cursor after pagination approach decided - // See: https://github.com/backstage/backstage/issues/6062 - .size(100) + .from(offset ?? 0) + .size(Math.min(limit ?? 25, 100)) .toJSON(), documentTypes: types, }; @@ -262,6 +263,7 @@ export class ElasticSearchSearchEngine implements SearchEngine { type: this.getTypeFromIndex(d._index), document: d._source, })), + totalCount: result.body.hits.total.value, }; } catch (e) { this.logger.error( From 487b84b4a0079dafc90fa5f61078c95006d25ccb Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Mon, 16 Aug 2021 09:31:23 +0200 Subject: [PATCH 04/12] Implement `offset` and `limit` based paging in pg search engine Signed-off-by: Oliver Sand --- .../src/PgSearchEngine/PgSearchEngine.test.ts | 53 +++++++++-- .../src/PgSearchEngine/PgSearchEngine.ts | 15 ++- .../database/DatabaseDocumentStore.test.ts | 94 ++++++++++++++++++- .../src/database/DatabaseDocumentStore.ts | 44 ++++++--- .../src/database/types.ts | 3 + 5 files changed, 185 insertions(+), 24 deletions(-) diff --git a/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.test.ts b/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.test.ts index 7bedee6dea..0940e4d550 100644 --- a/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.test.ts +++ b/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.test.ts @@ -27,6 +27,7 @@ describe('PgSearchEngine', () => { transaction: jest.fn(), insertDocuments: jest.fn(), query: jest.fn(), + count: jest.fn(), completeInsert: jest.fn(), prepareInsert: jest.fn(), }; @@ -44,24 +45,55 @@ describe('PgSearchEngine', () => { await searchEngine.query({ term: 'testTerm', filters: {}, - pageCursor: '', + offset: 25, + limit: 50, }); expect(translatorSpy).toHaveBeenCalledWith({ term: 'testTerm', filters: {}, - pageCursor: '', + offset: 25, + limit: 50, + }); + }); + + it('should pass offset and limit', async () => { + const actualTranslatedQuery = searchEngine.translator({ + term: 'Hello', + offset: 25, + limit: 50, + }) as PgSearchQuery; + + expect(actualTranslatedQuery).toMatchObject({ + pgTerm: '("Hello" | "Hello":*)', + offset: 25, + limit: 50, + }); + }); + + it('should have maximum limit of 100', async () => { + const actualTranslatedQuery = searchEngine.translator({ + term: 'Hello', + offset: 25, + limit: 1000, + }) as PgSearchQuery; + + expect(actualTranslatedQuery).toMatchObject({ + pgTerm: '("Hello" | "Hello":*)', + offset: 25, + limit: 100, }); }); it('should return translated query term', async () => { const actualTranslatedQuery = searchEngine.translator({ term: 'Hello World', - pageCursor: '', }) as PgSearchQuery; expect(actualTranslatedQuery).toMatchObject({ pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', + offset: 0, + limit: 25, }); }); @@ -81,13 +113,14 @@ describe('PgSearchEngine', () => { term: 'testTerm', filters: { kind: 'testKind' }, types: ['my-filter'], - pageCursor: '', }) as PgSearchQuery; expect(actualTranslatedQuery).toMatchObject({ pgTerm: '("testTerm" | "testTerm":*)', fields: { kind: 'testKind' }, types: ['my-filter'], + offset: 0, + limit: 25, }); }); }); @@ -138,6 +171,7 @@ describe('PgSearchEngine', () => { describe('query', () => { it('should perform query', async () => { database.transaction.mockImplementation(fn => fn(tx)); + database.count.mockResolvedValue(1337); database.query.mockResolvedValue([ { document: { @@ -151,7 +185,6 @@ describe('PgSearchEngine', () => { const results = await searchEngine.query({ term: 'Hello World', - pageCursor: '', }); expect(results).toEqual({ @@ -165,10 +198,18 @@ describe('PgSearchEngine', () => { type: 'my-type', }, ], + totalCount: 1337, }); - expect(database.transaction).toHaveBeenCalledTimes(1); + expect(database.transaction).toHaveBeenCalledTimes(2); expect(database.query).toHaveBeenCalledWith(tx, { pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', + offset: 0, + limit: 25, + }); + expect(database.count).toHaveBeenCalledWith(tx, { + pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', + offset: 0, + limit: 25, }); }); }); diff --git a/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.ts b/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.ts index 74a1e09010..d5cf498cc1 100644 --- a/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.ts +++ b/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.ts @@ -56,6 +56,8 @@ export class PgSearchEngine implements SearchEngine { .join('&'), fields: query.filters as Record, types: query.types, + offset: query.offset ?? 0, + limit: Math.min(query.limit ?? 25, 100), }; } @@ -79,14 +81,19 @@ export class PgSearchEngine implements SearchEngine { async query(query: SearchQuery): Promise { const pgQuery = this.translator(query); - const rows = await this.databaseStore.transaction(async tx => - this.databaseStore.query(tx, pgQuery), - ); + const [rows, totalCount] = await Promise.all([ + this.databaseStore.transaction(async tx => + this.databaseStore.query(tx, pgQuery), + ), + this.databaseStore.transaction(async tx => + this.databaseStore.count(tx, pgQuery), + ), + ]); const results = rows.map(({ type, document }) => ({ type, document, })); - return { results }; + return { results, totalCount }; } } diff --git a/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.test.ts b/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.test.ts index b860faca6a..a456d0caf5 100644 --- a/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.test.ts +++ b/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.test.ts @@ -192,6 +192,83 @@ describe('DatabaseDocumentStore', () => { 60_000, ); + it.each(databases.eachSupportedId())( + 'should return requested range, %p', + async databaseId => { + const { store } = await createStore(databaseId); + + await store.transaction(async tx => { + await store.prepareInsert(tx); + await store.insertDocuments(tx, 'test', [ + { + title: 'Lorem Ipsum', + text: 'Hello World', + location: 'LOCATION-1', + }, + { + title: 'Hello World', + text: 'Around the world', + location: 'LOCATION-1', + }, + { + title: 'Another one', + text: 'From the next page', + location: 'LOCATION-1', + }, + ]); + await store.completeInsert(tx, 'test'); + }); + + const rows = await store.transaction(tx => + store.query(tx, { pgTerm: 'Hello & World', offset: 1, limit: 1 }), + ); + + expect(rows).toEqual([ + { + document: { + location: 'LOCATION-1', + text: 'Hello World', + title: 'Lorem Ipsum', + }, + rank: expect.any(Number), + type: 'test', + }, + ]); + }, + 60_000, + ); + + it.each(databases.eachSupportedId())( + 'count by term, %p', + async databaseId => { + const { store } = await createStore(databaseId); + + await store.transaction(async tx => { + await store.prepareInsert(tx); + await store.insertDocuments(tx, 'test', [ + { + title: 'Lorem Ipsum', + text: 'Hello World', + location: 'LOCATION-1', + }, + { + title: 'Hello World', + text: 'Around the world', + location: 'LOCATION-1', + }, + ]); + await store.completeInsert(tx, 'test'); + }); + + const totalCount = await store.transaction(tx => + store.count(tx, { pgTerm: 'Hello & World', offset: 0, limit: 25 }), + ); + + expect(totalCount).toEqual(2); + }, + 60_000, + ); + it.each(databases.eachSupportedId())( 'query by term, %p', async databaseId => { @@ -215,7 +292,7 @@ describe('DatabaseDocumentStore', () => { }); const rows = await store.transaction(tx => - store.query(tx, { pgTerm: 'Hello & World' }), + store.query(tx, { pgTerm: 'Hello & World', offset: 0, limit: 25 }), ); expect(rows).toEqual([ @@ -271,7 +348,12 @@ describe('DatabaseDocumentStore', () => { }); const rows = await store.transaction(tx => - store.query(tx, { pgTerm: 'Hello & World', types: ['my-type'] }), + store.query(tx, { + pgTerm: 'Hello & World', + types: ['my-type'], + offset: 0, + limit: 25, + }), ); expect(rows).toEqual([ @@ -322,6 +404,8 @@ describe('DatabaseDocumentStore', () => { store.query(tx, { pgTerm: 'Hello & World', fields: { myField: 'this' }, + offset: 0, + limit: 25, }), ); @@ -374,6 +458,8 @@ describe('DatabaseDocumentStore', () => { store.query(tx, { pgTerm: 'Hello & World', fields: { myField: ['this', 'that'] }, + offset: 0, + limit: 25, }), ); @@ -433,6 +519,8 @@ describe('DatabaseDocumentStore', () => { store.query(tx, { pgTerm: 'Hello & World', fields: { myField: 'this', otherField: 'another' }, + offset: 0, + limit: 25, }), ); @@ -480,6 +568,8 @@ describe('DatabaseDocumentStore', () => { const rows = await store.transaction(tx => store.query(tx, { fields: { myField: 'this' }, + offset: 0, + limit: 25, }), ); diff --git a/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.ts b/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.ts index 77f15746a6..80398ec707 100644 --- a/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.ts +++ b/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.ts @@ -128,7 +128,7 @@ export class DatabaseDocumentStore implements DatabaseStore { async query( tx: Knex.Transaction, - { types, pgTerm, fields }: PgSearchQuery, + searchQuery: PgSearchQuery, ): Promise { // Builds a query like: // SELECT ts_rank_cd(body, query) AS rank, type, document @@ -136,6 +136,36 @@ export class DatabaseDocumentStore implements DatabaseStore { // WHERE query @@ body AND (document @> '{"kind": "API"}') // ORDER BY rank DESC // LIMIT 10; + const query = this.buildQuery(tx, searchQuery); + + query.select('type', 'document'); + + const { pgTerm, limit, offset } = searchQuery; + + if (pgTerm) { + query + .select(tx.raw('ts_rank_cd(body, query) AS "rank"')) + .orderBy('rank', 'desc'); + } else { + query.select(tx.raw('1 as rank')); + } + + return await query.offset(offset).limit(limit); + } + + async count( + tx: Knex.Transaction, + searchQuery: PgSearchQuery, + ): Promise { + const query = this.buildQuery(tx, searchQuery); + const [row] = await query.count(); + return Number(row.count); + } + + private buildQuery( + tx: Knex.Transaction, + { types, pgTerm, fields }: PgSearchQuery, + ) { const query = tx('documents'); if (pgTerm) { @@ -164,16 +194,6 @@ export class DatabaseDocumentStore implements DatabaseStore { }); } - query.select('type', 'document'); - - if (pgTerm) { - query - .select(tx.raw('ts_rank_cd(body, query) AS "rank"')) - .orderBy('rank', 'desc'); - } else { - query.select(tx.raw('1 as rank')); - } - - return await query.limit(100); + return query; } } diff --git a/plugins/search-backend-module-pg/src/database/types.ts b/plugins/search-backend-module-pg/src/database/types.ts index 8c98628caf..f8425c41b6 100644 --- a/plugins/search-backend-module-pg/src/database/types.ts +++ b/plugins/search-backend-module-pg/src/database/types.ts @@ -20,6 +20,8 @@ export interface PgSearchQuery { fields?: Record; types?: string[]; pgTerm?: string; + offset: number; + limit: number; } export interface DatabaseStore { @@ -35,6 +37,7 @@ export interface DatabaseStore { tx: Knex.Transaction, pgQuery: PgSearchQuery, ): Promise; + count(tx: Knex.Transaction, pgQuery: PgSearchQuery): Promise; } export interface RawDocumentRow { From 6876c48423987812c5c13b5120eeff5ff6bb628d Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Mon, 16 Aug 2021 11:17:32 +0200 Subject: [PATCH 05/12] Implement `offset` and `limit` based paging in frontend Signed-off-by: Oliver Sand --- .../app/src/components/search/SearchPage.tsx | 93 +++++++------------ plugins/search/src/apis.test.ts | 6 +- .../components/SearchBar/SearchBar.test.tsx | 2 +- .../SearchContext/SearchContext.test.tsx | 42 ++++++--- .../SearchContext/SearchContext.tsx | 47 +++++----- .../SearchFilter/SearchFilter.test.tsx | 2 +- .../components/SearchPage/SearchPage.test.tsx | 24 +++-- .../SearchResult/SearchResult.test.tsx | 66 ++++++++++++- .../components/SearchResult/SearchResult.tsx | 26 +++++- .../components/SearchType/SearchType.test.tsx | 2 +- 10 files changed, 193 insertions(+), 117 deletions(-) diff --git a/packages/app/src/components/search/SearchPage.tsx b/packages/app/src/components/search/SearchPage.tsx index 9b0ae7eecf..1c8d750e08 100644 --- a/packages/app/src/components/search/SearchPage.tsx +++ b/packages/app/src/components/search/SearchPage.tsx @@ -14,20 +14,18 @@ * limitations under the License. */ -import React, { useState } from 'react'; -import { makeStyles, Theme, Grid, List, Paper } from '@material-ui/core'; -import Pagination from '@material-ui/lab/Pagination'; +import { Content, Header, Lifecycle, Page } from '@backstage/core-components'; import { CatalogResultListItem } from '@backstage/plugin-catalog'; import { + DefaultResultListItem, SearchBar, SearchFilter, SearchResult, SearchType, - DefaultResultListItem, } from '@backstage/plugin-search'; -import { Content, Header, Lifecycle, Page } from '@backstage/core-components'; import { DocsResultListItem } from '@backstage/plugin-techdocs'; -import { SearchResultSet } from '@backstage/search-common'; +import { Grid, List, makeStyles, Paper, Theme } from '@material-ui/core'; +import React from 'react'; const useStyles = makeStyles((theme: Theme) => ({ bar: { @@ -43,59 +41,6 @@ const useStyles = makeStyles((theme: Theme) => ({ }, })); -// TODO: Move this into the search plugin once pagination is natively supported. -// See: https://github.com/backstage/backstage/issues/6062 -const SearchResultList = ({ results }: SearchResultSet) => { - const pageSize = 10; - const [page, setPage] = useState(1); - const changePage = (_: any, pageIndex: number) => { - setPage(pageIndex); - }; - const pageAmount = Math.ceil((results.length || 0) / pageSize); - return ( - <> - - {results - .slice(pageSize * (page - 1), pageSize * page) - .map(({ type, document }) => { - switch (type) { - case 'software-catalog': - return ( - - ); - case 'techdocs': - return ( - - ); - default: - return ( - - ); - } - })} - - {pageAmount > 1 && ( - - )} - - ); -}; - const SearchPage = () => { const classes = useStyles(); return ( @@ -129,7 +74,35 @@ const SearchPage = () => { - {({ results }) => } + {({ results }) => ( + + {results.map(({ type, document }) => { + switch (type) { + case 'software-catalog': + return ( + + ); + case 'techdocs': + return ( + + ); + default: + return ( + + ); + } + })} + + )} diff --git a/plugins/search/src/apis.test.ts b/plugins/search/src/apis.test.ts index cc59ca1a77..38f34bb91b 100644 --- a/plugins/search/src/apis.test.ts +++ b/plugins/search/src/apis.test.ts @@ -21,7 +21,7 @@ describe('apis', () => { term: '', filters: {}, types: [], - pageCursor: '', + page: {}, }; const baseUrl = 'https://base-url.com/'; @@ -53,7 +53,7 @@ describe('apis', () => { it('Fetch is called with expected URL (including stringified Q params)', async () => { await client.query(query); expect(getBaseUrl).toHaveBeenLastCalledWith('search/query'); - expect(fetch).toHaveBeenLastCalledWith(`${baseUrl}?term=&pageCursor=`, { + expect(fetch).toHaveBeenLastCalledWith(`${baseUrl}?term=`, { headers: {}, }); }); @@ -65,7 +65,7 @@ describe('apis', () => { }); await authedClient.query(query); expect(getBaseUrl).toHaveBeenLastCalledWith('search/query'); - expect(fetch).toHaveBeenLastCalledWith(`${baseUrl}?term=&pageCursor=`, { + expect(fetch).toHaveBeenLastCalledWith(`${baseUrl}?term=`, { headers: { Authorization: `Bearer ${token}` }, }); }); diff --git a/plugins/search/src/components/SearchBar/SearchBar.test.tsx b/plugins/search/src/components/SearchBar/SearchBar.test.tsx index b99b375252..acfeb2cb8b 100644 --- a/plugins/search/src/components/SearchBar/SearchBar.test.tsx +++ b/plugins/search/src/components/SearchBar/SearchBar.test.tsx @@ -30,7 +30,7 @@ jest.mock('@backstage/core-plugin-api', () => ({ describe('SearchBar', () => { const initialState = { term: '', - pageCursor: '', + page: {}, filters: {}, types: ['*'], }; diff --git a/plugins/search/src/components/SearchContext/SearchContext.test.tsx b/plugins/search/src/components/SearchContext/SearchContext.test.tsx index 4de2a8f0a6..c1618be5ec 100644 --- a/plugins/search/src/components/SearchContext/SearchContext.test.tsx +++ b/plugins/search/src/components/SearchContext/SearchContext.test.tsx @@ -38,7 +38,7 @@ describe('SearchContext', () => { const initialState = { term: '', - pageCursor: '', + page: {}, filters: {}, types: ['*'], }; @@ -93,22 +93,26 @@ describe('SearchContext', () => { initialProps: { initialState: { ...initialState, - pageCursor: '1', + page: { offset: 0, limit: 25 }, }, }, }); await waitForNextUpdate(); - expect(result.current.pageCursor).toBe('1'); + expect(result.current.page).toEqual({ offset: 0, limit: 25 }); act(() => { result.current.setTerm('first term'); }); + act(() => { + result.current.setPage({ offset: 75, limit: 25 }); + }); + await waitForNextUpdate(); - expect(result.current.pageCursor).toBe('1'); + expect(result.current.page).toEqual({ offset: 75, limit: 25 }); act(() => { result.current.setTerm('second term'); @@ -116,7 +120,7 @@ describe('SearchContext', () => { await waitForNextUpdate(); - expect(result.current.pageCursor).toBe(''); + expect(result.current.page).toEqual({ offset: 0, limit: 25 }); }); describe('Performs search (and sets results)', () => { @@ -139,7 +143,10 @@ describe('SearchContext', () => { await waitForNextUpdate(); expect(query).toHaveBeenLastCalledWith({ - ...initialState, + filters: {}, + types: ['*'], + limit: undefined, + offset: undefined, term, }); }); @@ -163,12 +170,15 @@ describe('SearchContext', () => { await waitForNextUpdate(); expect(query).toHaveBeenLastCalledWith({ - ...initialState, filters, + types: ['*'], + limit: undefined, + offset: undefined, + term: '', }); }); - it('When pageCursor is set', async () => { + it('When page is set', async () => { const { result, waitForNextUpdate } = renderHook(() => useSearch(), { wrapper, initialProps: { @@ -178,17 +188,20 @@ describe('SearchContext', () => { await waitForNextUpdate(); - const pageCursor = 'pageCursor'; + const page = { offset: 25, limit: 50 }; act(() => { - result.current.setPageCursor(pageCursor); + result.current.setPage(page); }); await waitForNextUpdate(); expect(query).toHaveBeenLastCalledWith({ - ...initialState, - pageCursor, + filters: {}, + types: ['*'], + limit: 50, + offset: 25, + term: '', }); }); @@ -211,8 +224,11 @@ describe('SearchContext', () => { await waitForNextUpdate(); expect(query).toHaveBeenLastCalledWith({ - ...initialState, types, + filters: {}, + limit: undefined, + offset: undefined, + term: '', }); }); }); diff --git a/plugins/search/src/components/SearchContext/SearchContext.tsx b/plugins/search/src/components/SearchContext/SearchContext.tsx index 45a74f5355..018496876b 100644 --- a/plugins/search/src/components/SearchContext/SearchContext.tsx +++ b/plugins/search/src/components/SearchContext/SearchContext.tsx @@ -14,19 +14,21 @@ * limitations under the License. */ -import React, { - PropsWithChildren, - createContext, - useContext, - useState, - useEffect, -} from 'react'; -import { useAsync, usePrevious } from 'react-use'; -import { SearchResultSet } from '@backstage/search-common'; -import { searchApiRef } from '../../apis'; -import { AsyncState } from 'react-use/lib/useAsync'; import { JsonObject } from '@backstage/config'; import { useApi } from '@backstage/core-plugin-api'; +import { SearchResultSet } from '@backstage/search-common'; +import React, { + createContext, + PropsWithChildren, + useContext, + useEffect, + useState, +} from 'react'; +import { useAsync, usePrevious } from 'react-use'; +import { AsyncState } from 'react-use/lib/useAsync'; +import { searchApiRef } from '../../apis'; + +type Page = { limit?: number; offset?: number }; type SearchContextValue = { result: AsyncState; @@ -36,13 +38,13 @@ type SearchContextValue = { setTypes: React.Dispatch>; filters: JsonObject; setFilters: React.Dispatch>; - pageCursor: string; - setPageCursor: React.Dispatch>; + page: Page; + setPage: React.Dispatch>; }; type SettableSearchContext = Omit< SearchContextValue, - 'result' | 'setTerm' | 'setTypes' | 'setFilters' | 'setPageCursor' + 'result' | 'setTerm' | 'setTypes' | 'setFilters' | 'setPage' >; export const SearchContext = createContext( @@ -52,14 +54,14 @@ export const SearchContext = createContext( export const SearchContextProvider = ({ initialState = { term: '', - pageCursor: '', + page: {}, filters: {}, types: [], }, children, }: PropsWithChildren<{ initialState?: SettableSearchContext }>) => { const searchApi = useApi(searchApiRef); - const [pageCursor, setPageCursor] = useState(initialState.pageCursor); + const [page, setPage] = useState(initialState.page); const [filters, setFilters] = useState(initialState.filters); const [term, setTerm] = useState(initialState.term); const [types, setTypes] = useState(initialState.types); @@ -70,18 +72,19 @@ export const SearchContextProvider = ({ searchApi.query({ term, filters, - pageCursor, + offset: page?.offset, + limit: page?.limit, types, }), - [term, filters, types, pageCursor], + [term, filters, types, page], ); useEffect(() => { // Any time a term is reset, we want to start from page 0. if (term && prevTerm && term !== prevTerm) { - setPageCursor(''); + setPage(initialState.page); } - }, [term, prevTerm]); + }, [term, prevTerm, initialState.page]); const value: SearchContextValue = { result, @@ -91,8 +94,8 @@ export const SearchContextProvider = ({ setTerm, types, setTypes, - pageCursor, - setPageCursor, + page, + setPage, }; return ; diff --git a/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx b/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx index e8c2f048b9..b5dd56bfb6 100644 --- a/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx +++ b/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx @@ -32,7 +32,7 @@ describe('SearchFilter', () => { term: '', filters: {}, types: [], - pageCursor: '', + page: {}, }; const name = 'field'; diff --git a/plugins/search/src/components/SearchPage/SearchPage.test.tsx b/plugins/search/src/components/SearchPage/SearchPage.test.tsx index c387d57b60..91568cddbe 100644 --- a/plugins/search/src/components/SearchPage/SearchPage.test.tsx +++ b/plugins/search/src/components/SearchPage/SearchPage.test.tsx @@ -74,21 +74,25 @@ describe('SearchPage', () => { const expectedTerm = 'justin bieber'; const expectedTypes = ['software-catalog']; const expectedFilters = { [expectedFilterField]: expectedFilterValue }; - const expectedPageCursor = 'page2-or-something'; + const expectedOffset = 25; + const expectedLimit = 50; - // e.g. ?query=petstore&pageCursor=1&filters[lifecycle][]=experimental&filters[kind]=Component + // e.g. ?query=petstore&offset=25&limit=50&filters[lifecycle][]=experimental&filters[kind]=Component (useLocation as jest.Mock).mockReturnValueOnce({ - search: `?query=${expectedTerm}&types[]=${expectedTypes[0]}&filters[${expectedFilterField}]=${expectedFilterValue}&pageCursor=${expectedPageCursor}`, + search: `?query=${expectedTerm}&types[]=${expectedTypes[0]}&filters[${expectedFilterField}]=${expectedFilterValue}&offset=${expectedOffset}&limit=${expectedLimit}`, }); // When we render the page... await renderInTestApp(); - // Then search context should be set with these values... - expect(setTermMock).toHaveBeenCalledWith(expectedTerm); - expect(setTypesMock).toHaveBeenCalledWith(expectedTypes); - expect(setPageCursorMock).toHaveBeenCalledWith(expectedPageCursor); - expect(setFiltersMock).toHaveBeenCalledWith(expectedFilters); + // Then search context should be initialized with these values... + const calls = (SearchContextProvider as jest.Mock).mock.calls[0]; + const actualInitialState = calls[0].initialState; + expect(actualInitialState.term).toEqual(expectedTerm); + expect(actualInitialState.types).toEqual(expectedTypes); + expect(actualInitialState.page.limit).toEqual(expectedLimit); + expect(actualInitialState.page.offset).toEqual(expectedOffset); + expect(actualInitialState.filters).toStrictEqual(expectedFilters); }); it('renders provided router element', async () => { @@ -108,7 +112,7 @@ describe('SearchPage', () => { (useSearch as jest.Mock).mockReturnValueOnce({ term: 'bieber', types: ['software-catalog'], - pageCursor: 'page2-or-something', + page: { offset: 25, limit: 50 }, filters: { anyKey: 'anyValue' }, setTerm: setTermMock, setTypes: setTypesMock, @@ -116,7 +120,7 @@ describe('SearchPage', () => { setPageCursor: setPageCursorMock, }); const expectedLocation = encodeURI( - '?query=bieber&types[]=software-catalog&pageCursor=page2-or-something&filters[anyKey]=anyValue', + '?query=bieber&types[]=software-catalog&offset=25&limit=50&filters[anyKey]=anyValue', ); await renderInTestApp(); diff --git a/plugins/search/src/components/SearchResult/SearchResult.test.tsx b/plugins/search/src/components/SearchResult/SearchResult.test.tsx index bb8f393d67..d28bcc9136 100644 --- a/plugins/search/src/components/SearchResult/SearchResult.test.tsx +++ b/plugins/search/src/components/SearchResult/SearchResult.test.tsx @@ -24,6 +24,7 @@ jest.mock('../SearchContext', () => ({ ...jest.requireActual('../SearchContext'), useSearch: jest.fn().mockReturnValue({ result: {}, + page: {}, }), })); @@ -93,16 +94,73 @@ describe('SearchResult', () => { it('Calls children with results set to result.value', async () => { (useSearch as jest.Mock).mockReturnValueOnce({ - result: { loading: false, error: '', value: { results: [] } }, + result: { + loading: false, + error: '', + value: { + totalCount: 1, + results: [ + { + type: 'some-type', + document: { + title: 'some-title', + text: 'some-text', + location: 'some-location', + }, + }, + ], + }, + }, + page: {}, }); - await renderInTestApp( + const { getByText } = await renderInTestApp( {({ results }) => { - expect(results).toEqual([]); - return <>; + return <>Results {results.length}; }} , ); + + expect(getByText('Results 1')).toBeInTheDocument(); + }); + + it('Starts on initial page if no offset is set', async () => { + (useSearch as jest.Mock).mockReturnValueOnce({ + page: {}, + result: { + loading: false, + error: '', + value: { results: [{}], totalCount: 100 }, + }, + }); + + const { getByLabelText } = await renderInTestApp( + {({}) => <>}, + ); + + expect(getByLabelText('page 1')).toHaveAttribute('aria-current', 'true'); + expect(getByLabelText('Go to page 2')).toBeInTheDocument(); + expect(getByLabelText('Go to page 3')).toBeInTheDocument(); + expect(getByLabelText('Go to page 4')).toBeInTheDocument(); + }); + + it('Shows the right page', async () => { + (useSearch as jest.Mock).mockReturnValueOnce({ + page: { offset: 25 }, + result: { + loading: false, + error: '', + value: { results: [{}], totalCount: 63 }, + }, + }); + + const { getByLabelText } = await renderInTestApp( + {({}) => <>}, + ); + + expect(getByLabelText('Go to page 1')).toBeInTheDocument(); + expect(getByLabelText('page 2')).toHaveAttribute('aria-current', 'true'); + expect(getByLabelText('Go to page 3')).toBeInTheDocument(); }); }); diff --git a/plugins/search/src/components/SearchResult/SearchResult.tsx b/plugins/search/src/components/SearchResult/SearchResult.tsx index 26a2484a86..5ac2c86a79 100644 --- a/plugins/search/src/components/SearchResult/SearchResult.tsx +++ b/plugins/search/src/components/SearchResult/SearchResult.tsx @@ -20,16 +20,20 @@ import { ResponseErrorPanel, } from '@backstage/core-components'; import { SearchResult } from '@backstage/search-common'; +import { Pagination } from '@material-ui/lab'; import React from 'react'; import { useSearch } from '../SearchContext'; type Props = { children: (results: { results: SearchResult[] }) => JSX.Element; + initialPageSize?: number; }; -const SearchResultComponent = ({ children }: Props) => { +const SearchResultComponent = ({ children, initialPageSize = 25 }: Props) => { const { result: { loading, error, value }, + page, + setPage, } = useSearch(); if (loading) { @@ -48,7 +52,25 @@ const SearchResultComponent = ({ children }: Props) => { return ; } - return children({ results: value.results }); + const pageSize = page.limit ?? initialPageSize; + const totalPages = Math.ceil(value.totalCount / pageSize); + const currentPage = page.offset ? Math.floor(page.offset / pageSize) + 1 : 1; + + const handlePageChange = (_: React.ChangeEvent, pageNum: number) => { + setPage({ offset: (pageNum - 1) * pageSize, limit: pageSize }); + window.scrollTo({ top: 0, left: 0, behavior: 'smooth' }); + }; + + return ( + <> + {children({ results: value.results })} + + + ); }; export { SearchResultComponent as SearchResult }; diff --git a/plugins/search/src/components/SearchType/SearchType.test.tsx b/plugins/search/src/components/SearchType/SearchType.test.tsx index b8bf59d13e..f8171b0779 100644 --- a/plugins/search/src/components/SearchType/SearchType.test.tsx +++ b/plugins/search/src/components/SearchType/SearchType.test.tsx @@ -31,7 +31,7 @@ describe('SearchType', () => { term: '', filters: {}, types: [], - pageCursor: '', + page: {}, }; const name = 'field'; From cd40f01ffd794013a306c9d921e6e8a8df49c143 Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Mon, 16 Aug 2021 11:22:16 +0200 Subject: [PATCH 06/12] Fix e2e test for search Signed-off-by: Oliver Sand --- .../app/cypress/integration/components/search/SearchPage.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/app/cypress/integration/components/search/SearchPage.js b/packages/app/cypress/integration/components/search/SearchPage.js index 4db7acf4fd..6a18d16198 100644 --- a/packages/app/cypress/integration/components/search/SearchPage.js +++ b/packages/app/cypress/integration/components/search/SearchPage.js @@ -34,7 +34,7 @@ describe('SearchPage', () => { cy.visit('/search-next', { onBeforeLoad(win) { cy.stub(win, 'fetch') - .withArgs(`${API_ENDPOINT}?term=&pageCursor=`) + .withArgs(`${API_ENDPOINT}?term=`) .resolves({ ok: true, json: () => ({ results }), @@ -56,7 +56,7 @@ describe('SearchPage', () => { onBeforeLoad(win) { cy.stub(win, 'fetch') .withArgs( - `${API_ENDPOINT}?term=&filters%5Bkind%5D=Component&filters%5Blifecycle%5D%5B0%5D=experimental&pageCursor=`, + `${API_ENDPOINT}?term=&filters%5Bkind%5D=Component&filters%5Blifecycle%5D%5B0%5D=experimental`, ) .resolves({ ok: true, @@ -102,7 +102,7 @@ describe('SearchPage', () => { cy.visit('/search-next?query=backstage', { onBeforeLoad(win) { cy.stub(win, 'fetch') - .withArgs(`${API_ENDPOINT}?term=backstage&pageCursor=`) + .withArgs(`${API_ENDPOINT}?term=backstage`) .resolves({ ok: true, json: () => ({ results: [] }), From 56e79326291eeb75b7f8c0a9ca8478fd5af15abb Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Mon, 16 Aug 2021 11:29:50 +0200 Subject: [PATCH 07/12] Update API reports Signed-off-by: Oliver Sand --- packages/search-common/api-report.md | 6 +++++- .../search-backend-module-elasticsearch/api-report.md | 2 ++ plugins/search-backend-module-pg/api-report.md | 10 +++++++++- plugins/search/api-report.md | 4 +++- 4 files changed, 19 insertions(+), 3 deletions(-) diff --git a/packages/search-common/api-report.md b/packages/search-common/api-report.md index 055f2d152f..b4fcac22ca 100644 --- a/packages/search-common/api-report.md +++ b/packages/search-common/api-report.md @@ -53,7 +53,9 @@ export interface SearchQuery { // (undocumented) filters?: JsonObject; // (undocumented) - pageCursor: string; + limit?: number; + // (undocumented) + offset?: number; // (undocumented) term: string; // (undocumented) @@ -76,6 +78,8 @@ export interface SearchResult { export interface SearchResultSet { // (undocumented) results: SearchResult[]; + // (undocumented) + totalCount: number; } // (No @packageDocumentation comment for this package) diff --git a/plugins/search-backend-module-elasticsearch/api-report.md b/plugins/search-backend-module-elasticsearch/api-report.md index eac76a30b9..e538201bc0 100644 --- a/plugins/search-backend-module-elasticsearch/api-report.md +++ b/plugins/search-backend-module-elasticsearch/api-report.md @@ -45,6 +45,8 @@ export class ElasticSearchSearchEngine implements SearchEngine { term, filters, types, + offset, + limit, }: SearchQuery): ConcreteElasticSearchQuery; } diff --git a/plugins/search-backend-module-pg/api-report.md b/plugins/search-backend-module-pg/api-report.md index 23a1f3ecbb..4879e64d06 100644 --- a/plugins/search-backend-module-pg/api-report.md +++ b/plugins/search-backend-module-pg/api-report.md @@ -18,6 +18,8 @@ export class DatabaseDocumentStore implements DatabaseStore { // (undocumented) completeInsert(tx: Knex.Transaction, type: string): Promise; // (undocumented) + count(tx: Knex.Transaction, searchQuery: PgSearchQuery): Promise; + // (undocumented) static create(knex: Knex): Promise; // (undocumented) insertDocuments( @@ -32,7 +34,7 @@ export class DatabaseDocumentStore implements DatabaseStore { // (undocumented) query( tx: Knex.Transaction, - { types, pgTerm, fields }: PgSearchQuery, + searchQuery: PgSearchQuery, ): Promise; // (undocumented) static supported(knex: Knex): Promise; @@ -47,6 +49,8 @@ export interface DatabaseStore { // (undocumented) completeInsert(tx: Knex.Transaction, type: string): Promise; // (undocumented) + count(tx: Knex.Transaction, pgQuery: PgSearchQuery): Promise; + // (undocumented) insertDocuments( tx: Knex.Transaction, type: string, @@ -93,6 +97,10 @@ export interface PgSearchQuery { // (undocumented) fields?: Record; // (undocumented) + limit: number; + // (undocumented) + offset: number; + // (undocumented) pgTerm?: string; // (undocumented) types?: string[]; diff --git a/plugins/search/api-report.md b/plugins/search/api-report.md index e3075a448d..616c6f2cae 100644 --- a/plugins/search/api-report.md +++ b/plugins/search/api-report.md @@ -139,8 +139,10 @@ export { searchPlugin }; // @public (undocumented) export const SearchResult: ({ children, + initialPageSize, }: { children: (results: { results: SearchResult_2[] }) => JSX.Element; + initialPageSize?: number | undefined; }) => JSX.Element; // Warning: (ae-forgotten-export) The symbol "SearchTypeProps" needs to be exported by the entry point index.d.ts @@ -167,7 +169,7 @@ export const useSearch: () => SearchContextValue; // Warnings were encountered during analysis: // -// src/components/SearchContext/SearchContext.d.ts:19:5 - (ae-forgotten-export) The symbol "SettableSearchContext" needs to be exported by the entry point index.d.ts +// src/components/SearchContext/SearchContext.d.ts:23:5 - (ae-forgotten-export) The symbol "SettableSearchContext" needs to be exported by the entry point index.d.ts // src/components/SearchFilter/SearchFilter.d.ts:13:5 - (ae-forgotten-export) The symbol "Props" needs to be exported by the entry point index.d.ts // src/components/SearchFilter/SearchFilter.d.ts:14:5 - (ae-forgotten-export) The symbol "Component" needs to be exported by the entry point index.d.ts From a13f21cdc99c539f1492e7281bfb101c39ffe0f5 Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Mon, 16 Aug 2021 11:29:58 +0200 Subject: [PATCH 08/12] Update changeset Signed-off-by: Oliver Sand --- .changeset/search-happy-owls-sneeze.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .changeset/search-happy-owls-sneeze.md diff --git a/.changeset/search-happy-owls-sneeze.md b/.changeset/search-happy-owls-sneeze.md new file mode 100644 index 0000000000..7c2959d020 --- /dev/null +++ b/.changeset/search-happy-owls-sneeze.md @@ -0,0 +1,10 @@ +--- +'@backstage/search-common': minor +'@backstage/plugin-search': patch +'@backstage/plugin-search-backend': patch +'@backstage/plugin-search-backend-module-elasticsearch': patch +'@backstage/plugin-search-backend-module-pg': minor +'@backstage/plugin-search-backend-node': patch +--- + +Implement `offset` and `limit` based paging in search. From 532b4cc6563621ec5cf76cdf573689ffb8a30146 Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Thu, 19 Aug 2021 17:57:11 +0200 Subject: [PATCH 09/12] Rework search paging based on cursors Signed-off-by: Oliver Sand --- .changeset/search-happy-owls-sneeze.md | 2 +- packages/search-common/api-report.md | 10 +- packages/search-common/src/types.ts | 6 +- .../api-report.md | 3 +- .../engines/ElasticSearchSearchEngine.test.ts | 167 +++++++++++----- .../src/engines/ElasticSearchSearchEngine.ts | 63 ++++-- .../search-backend-module-pg/api-report.md | 20 +- .../src/PgSearchEngine/PgSearchEngine.test.ts | 180 +++++++++++++----- .../src/PgSearchEngine/PgSearchEngine.ts | 85 ++++++--- .../src/PgSearchEngine/index.ts | 1 + .../database/DatabaseDocumentStore.test.ts | 31 --- .../src/database/DatabaseDocumentStore.ts | 44 ++--- .../src/database/types.ts | 1 - .../src/engines/LunrSearchEngine.test.ts | 173 +++++++++++------ .../src/engines/LunrSearchEngine.ts | 43 ++++- plugins/search-backend/src/service/router.ts | 14 +- plugins/search/api-report.md | 4 +- plugins/search/src/apis.test.ts | 1 - .../components/SearchBar/SearchBar.test.tsx | 1 - .../SearchContext/SearchContext.test.tsx | 99 +++++++--- .../SearchContext/SearchContext.tsx | 55 ++++-- .../SearchFilter/SearchFilter.test.tsx | 1 - .../components/SearchPage/SearchPage.test.tsx | 27 ++- .../SearchResult/SearchResult.test.tsx | 41 ---- .../components/SearchResult/SearchResult.tsx | 22 +-- .../SearchResultPager.test.tsx | 59 ++++++ .../SearchResultPager/SearchResultPager.tsx | 64 +++++++ .../src/components/SearchResultPager/index.ts | 17 ++ .../components/SearchType/SearchType.test.tsx | 1 - plugins/search/src/components/index.tsx | 9 +- 30 files changed, 826 insertions(+), 418 deletions(-) create mode 100644 plugins/search/src/components/SearchResultPager/SearchResultPager.test.tsx create mode 100644 plugins/search/src/components/SearchResultPager/SearchResultPager.tsx create mode 100644 plugins/search/src/components/SearchResultPager/index.ts diff --git a/.changeset/search-happy-owls-sneeze.md b/.changeset/search-happy-owls-sneeze.md index 7c2959d020..4740c767d2 100644 --- a/.changeset/search-happy-owls-sneeze.md +++ b/.changeset/search-happy-owls-sneeze.md @@ -7,4 +7,4 @@ '@backstage/plugin-search-backend-node': patch --- -Implement `offset` and `limit` based paging in search. +Implement optional `pageCursor` based paging in search. diff --git a/packages/search-common/api-report.md b/packages/search-common/api-report.md index b4fcac22ca..f00beb6830 100644 --- a/packages/search-common/api-report.md +++ b/packages/search-common/api-report.md @@ -53,9 +53,7 @@ export interface SearchQuery { // (undocumented) filters?: JsonObject; // (undocumented) - limit?: number; - // (undocumented) - offset?: number; + pageCursor?: string; // (undocumented) term: string; // (undocumented) @@ -77,9 +75,11 @@ export interface SearchResult { // @public (undocumented) export interface SearchResultSet { // (undocumented) - results: SearchResult[]; + nextPageCursor?: string; // (undocumented) - totalCount: number; + previousPageCursor?: string; + // (undocumented) + results: SearchResult[]; } // (No @packageDocumentation comment for this package) diff --git a/packages/search-common/src/types.ts b/packages/search-common/src/types.ts index 0bd12fbd42..063e0cb96c 100644 --- a/packages/search-common/src/types.ts +++ b/packages/search-common/src/types.ts @@ -19,8 +19,7 @@ export interface SearchQuery { term: string; filters?: JsonObject; types?: string[]; - offset?: number; - limit?: number; + pageCursor?: string; } export interface SearchResult { @@ -30,7 +29,8 @@ export interface SearchResult { export interface SearchResultSet { results: SearchResult[]; - totalCount: number; + nextPageCursor?: string; + previousPageCursor?: string; } /** diff --git a/plugins/search-backend-module-elasticsearch/api-report.md b/plugins/search-backend-module-elasticsearch/api-report.md index e538201bc0..1644ce65ea 100644 --- a/plugins/search-backend-module-elasticsearch/api-report.md +++ b/plugins/search-backend-module-elasticsearch/api-report.md @@ -45,8 +45,7 @@ export class ElasticSearchSearchEngine implements SearchEngine { term, filters, types, - offset, - limit, + pageCursor, }: SearchQuery): ConcreteElasticSearchQuery; } diff --git a/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.test.ts b/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.test.ts index 18ee1314ce..36fb2e56c6 100644 --- a/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.test.ts +++ b/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.test.ts @@ -16,12 +16,14 @@ import { getVoidLogger } from '@backstage/backend-common'; import { SearchEngine } from '@backstage/search-common'; -import { - ConcreteElasticSearchQuery, - ElasticSearchSearchEngine, -} from './ElasticSearchSearchEngine'; import { Client } from '@elastic/elasticsearch'; import Mock from '@elastic/elasticsearch-mock'; +import { + ConcreteElasticSearchQuery, + decodePageCursor, + ElasticSearchSearchEngine, + encodePageCursor, +} from './ElasticSearchSearchEngine'; class ElasticSearchSearchEngineForTranslatorTests extends ElasticSearchSearchEngine { getTranslator() { @@ -134,14 +136,13 @@ describe('ElasticSearchSearchEngine', () => { }); }); - it('should pass offset and limit', async () => { + it('should pass page cursor', async () => { const translatorUnderTest = inspectableSearchEngine.getTranslator(); const actualTranslatedQuery = translatorUnderTest({ types: ['indexName'], term: 'testTerm', - offset: 25, - limit: 50, + pageCursor: 'MQ==', }) as ConcreteElasticSearchQuery; expect(actualTranslatedQuery).toMatchObject({ @@ -166,43 +167,7 @@ describe('ElasticSearchSearchEngine', () => { }, }, from: 25, - size: 50, - }); - }); - - it('should have maximum limit of 100', async () => { - const translatorUnderTest = inspectableSearchEngine.getTranslator(); - - const actualTranslatedQuery = translatorUnderTest({ - types: ['indexName'], - term: 'testTerm', - offset: 25, - limit: 500, - }) as ConcreteElasticSearchQuery; - - expect(actualTranslatedQuery).toMatchObject({ - documentTypes: ['indexName'], - elasticSearchQuery: expect.any(Object), - }); - - const queryBody = actualTranslatedQuery.elasticSearchQuery; - - expect(queryBody).toEqual({ - query: { - bool: { - filter: [], - must: { - multi_match: { - query: 'testTerm', - fields: ['*'], - fuzziness: 'auto', - minimum_should_match: 1, - }, - }, - }, - }, - from: 25, - size: 100, + size: 25, }); }); @@ -387,7 +352,103 @@ describe('ElasticSearchSearchEngine', () => { }); // Should return 0 results as nothing is indexed here - expect(mockedSearchResult).toMatchObject({ results: [], totalCount: 0 }); + expect(mockedSearchResult).toMatchObject({ + results: [], + nextPageCursor: undefined, + }); + }); + + it('should perform search query with more results than one page', async () => { + mock.clear({ + method: 'POST', + path: '/*__search/_search', + }); + mock.add( + { + method: 'POST', + path: '/*__search/_search', + }, + () => { + return { + hits: { + total: { value: 30, relation: 'eq' }, + hits: Array(25) + .fill(null) + .map((_, i) => ({ + _index: 'mytype-index__', + _source: { + value: `${i}`, + }, + })), + }, + }; + }, + ); + + const mockedSearchResult = await testSearchEngine.query({ + term: 'testTerm', + filters: {}, + }); + + expect(mockedSearchResult).toMatchObject({ + results: expect.arrayContaining( + Array(25) + .fill(null) + .map((_, i) => ({ + type: 'mytype', + document: { value: `${i}` }, + })), + ), + nextPageCursor: 'MQ==', + }); + }); + + it('should perform search query for second page', async () => { + mock.clear({ + method: 'POST', + path: '/*__search/_search', + }); + mock.add( + { + method: 'POST', + path: '/*__search/_search', + }, + () => { + return { + hits: { + total: { value: 30, relation: 'eq' }, + hits: Array(30) + .fill(null) + .map((_, i) => ({ + _index: 'mytype-index__', + _source: { + value: `${i}`, + }, + })) + .slice(25), + }, + }; + }, + ); + + const mockedSearchResult = await testSearchEngine.query({ + term: 'testTerm', + filters: {}, + pageCursor: 'MQ==', + }); + + expect(mockedSearchResult).toMatchObject({ + results: expect.arrayContaining( + Array(30) + .fill(null) + .map((_, i) => ({ + type: 'mytype', + document: { value: `${i}` }, + })) + .slice(25), + ), + previousPageCursor: 'MA==', + }); }); it('should handle index/search type filtering correctly', async () => { @@ -498,3 +559,19 @@ describe('ElasticSearchSearchEngine', () => { }); }); }); + +describe('decodePageCursor', () => { + test('should decode page', () => { + expect(decodePageCursor('MQ==')).toEqual({ page: 1 }); + }); + + test('should fallback to first page if empty', () => { + expect(decodePageCursor()).toEqual({ page: 0 }); + }); +}); + +describe('encodePageCursor', () => { + test('should encode page', () => { + expect(encodePageCursor({ page: 1 })).toEqual('MQ=='); + }); +}); diff --git a/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.ts b/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.ts index c1c959778b..a12daef282 100644 --- a/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.ts +++ b/plugins/search-backend-module-elasticsearch/src/engines/ElasticSearchSearchEngine.ts @@ -15,24 +15,25 @@ */ import { - IndexableDocument, - SearchQuery, - SearchResultSet, - SearchEngine, -} from '@backstage/search-common'; -import { Logger } from 'winston'; -import esb from 'elastic-builder'; -import { Client } from '@elastic/elasticsearch'; + awsGetCredentials, + createAWSConnection, +} from '@acuris/aws-es-connection'; import { Config } from '@backstage/config'; import { - createAWSConnection, - awsGetCredentials, -} from '@acuris/aws-es-connection'; + IndexableDocument, + SearchEngine, + SearchQuery, + SearchResultSet, +} from '@backstage/search-common'; +import { Client } from '@elastic/elasticsearch'; +import esb from 'elastic-builder'; import { isEmpty, isNaN as nan, isNumber } from 'lodash'; +import { Logger } from 'winston'; export type ConcreteElasticSearchQuery = { documentTypes?: string[]; elasticSearchQuery: Object; + pageSize: number; }; type ElasticSearchQueryTranslator = ( @@ -140,8 +141,7 @@ export class ElasticSearchSearchEngine implements SearchEngine { term, filters = {}, types, - offset, - limit, + pageCursor, }: SearchQuery): ConcreteElasticSearchQuery { const filter = Object.entries(filters) .filter(([_, value]) => Boolean(value)) @@ -169,15 +169,18 @@ export class ElasticSearchSearchEngine implements SearchEngine { .multiMatchQuery(['*'], term) .fuzziness('auto') .minimumShouldMatch(1); + const pageSize = 25; + const { page } = decodePageCursor(pageCursor); return { elasticSearchQuery: esb .requestBodySearch() .query(esb.boolQuery().filter(filter).must([query])) - .from(offset ?? 0) - .size(Math.min(limit ?? 25, 100)) + .from(page * pageSize) + .size(pageSize) .toJSON(), documentTypes: types, + pageSize, }; } @@ -249,7 +252,8 @@ export class ElasticSearchSearchEngine implements SearchEngine { } async query(query: SearchQuery): Promise { - const { elasticSearchQuery, documentTypes } = this.translator(query); + const { elasticSearchQuery, documentTypes, pageSize } = + this.translator(query); const queryIndices = documentTypes ? documentTypes.map(it => this.constructSearchAlias(it)) : this.constructSearchAlias('*'); @@ -258,12 +262,23 @@ export class ElasticSearchSearchEngine implements SearchEngine { index: queryIndices, body: elasticSearchQuery, }); + const { page } = decodePageCursor(query.pageCursor); + const hasNextPage = result.body.hits.total.value > page * pageSize; + const hasPreviousPage = page > 0; + const nextPageCursor = hasNextPage + ? encodePageCursor({ page: page + 1 }) + : undefined; + const previousPageCursor = hasPreviousPage + ? encodePageCursor({ page: page - 1 }) + : undefined; + return { results: result.body.hits.hits.map((d: ElasticSearchResult) => ({ type: this.getTypeFromIndex(d._index), document: d._source, })), - totalCount: result.body.hits.total.value, + nextPageCursor, + previousPageCursor, }; } catch (e) { this.logger.error( @@ -291,3 +306,17 @@ export class ElasticSearchSearchEngine implements SearchEngine { return `${this.indexPrefix}${type}${postFix}`; } } + +export function decodePageCursor(pageCursor?: string): { page: number } { + if (!pageCursor) { + return { page: 0 }; + } + + return { + page: Number(Buffer.from(pageCursor, 'base64').toString('utf-8')), + }; +} + +export function encodePageCursor({ page }: { page: number }): string { + return Buffer.from(`${page}`, 'utf-8').toString('base64'); +} diff --git a/plugins/search-backend-module-pg/api-report.md b/plugins/search-backend-module-pg/api-report.md index 4879e64d06..6a56daa8d7 100644 --- a/plugins/search-backend-module-pg/api-report.md +++ b/plugins/search-backend-module-pg/api-report.md @@ -10,6 +10,14 @@ import { SearchEngine } from '@backstage/plugin-search-backend-node'; import { SearchQuery } from '@backstage/search-common'; import { SearchResultSet } from '@backstage/search-common'; +// Warning: (ae-missing-release-tag) "ConcretePgSearchQuery" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export type ConcretePgSearchQuery = { + pgQuery: PgSearchQuery; + pageSize: number; +}; + // Warning: (ae-missing-release-tag) "DatabaseDocumentStore" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) @@ -18,8 +26,6 @@ export class DatabaseDocumentStore implements DatabaseStore { // (undocumented) completeInsert(tx: Knex.Transaction, type: string): Promise; // (undocumented) - count(tx: Knex.Transaction, searchQuery: PgSearchQuery): Promise; - // (undocumented) static create(knex: Knex): Promise; // (undocumented) insertDocuments( @@ -34,7 +40,7 @@ export class DatabaseDocumentStore implements DatabaseStore { // (undocumented) query( tx: Knex.Transaction, - searchQuery: PgSearchQuery, + { types, pgTerm, fields, offset, limit }: PgSearchQuery, ): Promise; // (undocumented) static supported(knex: Knex): Promise; @@ -49,8 +55,6 @@ export interface DatabaseStore { // (undocumented) completeInsert(tx: Knex.Transaction, type: string): Promise; // (undocumented) - count(tx: Knex.Transaction, pgQuery: PgSearchQuery): Promise; - // (undocumented) insertDocuments( tx: Knex.Transaction, type: string, @@ -83,11 +87,13 @@ export class PgSearchEngine implements SearchEngine { // (undocumented) query(query: SearchQuery): Promise; // (undocumented) - setTranslator(translator: (query: SearchQuery) => PgSearchQuery): void; + setTranslator( + translator: (query: SearchQuery) => ConcretePgSearchQuery, + ): void; // (undocumented) static supported(database: PluginDatabaseManager): Promise; // (undocumented) - translator(query: SearchQuery): PgSearchQuery; + translator(query: SearchQuery): ConcretePgSearchQuery; } // Warning: (ae-missing-release-tag) "PgSearchQuery" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) diff --git a/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.test.ts b/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.test.ts index 0940e4d550..56a3a6e643 100644 --- a/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.test.ts +++ b/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.test.ts @@ -14,8 +14,13 @@ * limitations under the License. */ import { range } from 'lodash'; -import { DatabaseStore, PgSearchQuery } from '../database'; -import { PgSearchEngine } from './PgSearchEngine'; +import { DatabaseStore } from '../database'; +import { + ConcretePgSearchQuery, + decodePageCursor, + encodePageCursor, + PgSearchEngine, +} from './PgSearchEngine'; describe('PgSearchEngine', () => { const tx: any = {} as any; @@ -27,7 +32,6 @@ describe('PgSearchEngine', () => { transaction: jest.fn(), insertDocuments: jest.fn(), query: jest.fn(), - count: jest.fn(), completeInsert: jest.fn(), prepareInsert: jest.fn(), }; @@ -45,55 +49,42 @@ describe('PgSearchEngine', () => { await searchEngine.query({ term: 'testTerm', filters: {}, - offset: 25, - limit: 50, }); expect(translatorSpy).toHaveBeenCalledWith({ term: 'testTerm', filters: {}, - offset: 25, - limit: 50, }); }); - it('should pass offset and limit', async () => { + it('should pass page cursor', async () => { const actualTranslatedQuery = searchEngine.translator({ term: 'Hello', - offset: 25, - limit: 50, - }) as PgSearchQuery; - - expect(actualTranslatedQuery).toMatchObject({ - pgTerm: '("Hello" | "Hello":*)', - offset: 25, - limit: 50, + pageCursor: 'MQ==', }); - }); - - it('should have maximum limit of 100', async () => { - const actualTranslatedQuery = searchEngine.translator({ - term: 'Hello', - offset: 25, - limit: 1000, - }) as PgSearchQuery; expect(actualTranslatedQuery).toMatchObject({ - pgTerm: '("Hello" | "Hello":*)', - offset: 25, - limit: 100, + pgQuery: { + pgTerm: '("Hello" | "Hello":*)', + offset: 25, + limit: 26, + }, + pageSize: 25, }); }); it('should return translated query term', async () => { const actualTranslatedQuery = searchEngine.translator({ term: 'Hello World', - }) as PgSearchQuery; + }); expect(actualTranslatedQuery).toMatchObject({ - pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', - offset: 0, - limit: 25, + pgQuery: { + pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', + offset: 0, + limit: 26, + }, + pageSize: 25, }); }); @@ -101,10 +92,13 @@ describe('PgSearchEngine', () => { const actualTranslatedQuery = searchEngine.translator({ term: 'H&e|l!l*o W\0o(r)l:d', pageCursor: '', - }) as PgSearchQuery; + }) as ConcretePgSearchQuery; expect(actualTranslatedQuery).toMatchObject({ - pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', + pgQuery: { + pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', + }, + pageSize: 25, }); }); @@ -113,14 +107,17 @@ describe('PgSearchEngine', () => { term: 'testTerm', filters: { kind: 'testKind' }, types: ['my-filter'], - }) as PgSearchQuery; + }); expect(actualTranslatedQuery).toMatchObject({ - pgTerm: '("testTerm" | "testTerm":*)', - fields: { kind: 'testKind' }, - types: ['my-filter'], - offset: 0, - limit: 25, + pgQuery: { + pgTerm: '("testTerm" | "testTerm":*)', + fields: { kind: 'testKind' }, + types: ['my-filter'], + offset: 0, + limit: 26, + }, + pageSize: 25, }); }); }); @@ -171,7 +168,6 @@ describe('PgSearchEngine', () => { describe('query', () => { it('should perform query', async () => { database.transaction.mockImplementation(fn => fn(tx)); - database.count.mockResolvedValue(1337); database.query.mockResolvedValue([ { document: { @@ -198,19 +194,113 @@ describe('PgSearchEngine', () => { type: 'my-type', }, ], - totalCount: 1337, + nextPageCursor: undefined, }); - expect(database.transaction).toHaveBeenCalledTimes(2); + expect(database.transaction).toHaveBeenCalledTimes(1); expect(database.query).toHaveBeenCalledWith(tx, { pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', offset: 0, - limit: 25, + limit: 26, }); - expect(database.count).toHaveBeenCalledWith(tx, { + }); + + it('should include next page cursor if results exceed page size', async () => { + database.transaction.mockImplementation(fn => fn(tx)); + database.query.mockResolvedValue( + Array(30) + .fill(0) + .map((_, i) => ({ + document: { + title: 'Hello World', + text: 'Lorem Ipsum', + location: `location-${i}`, + }, + type: 'my-type', + })), + ); + + const results = await searchEngine.query({ + term: 'Hello World', + }); + + expect(results).toEqual({ + results: Array(25) + .fill(0) + .map((_, i) => ({ + document: { + title: 'Hello World', + text: 'Lorem Ipsum', + location: `location-${i}`, + }, + type: 'my-type', + })), + nextPageCursor: 'MQ==', + }); + expect(database.transaction).toHaveBeenCalledTimes(1); + expect(database.query).toHaveBeenCalledWith(tx, { pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', offset: 0, - limit: 25, + limit: 26, + }); + }); + + it('should include previous page cursor if on another page', async () => { + database.transaction.mockImplementation(fn => fn(tx)); + database.query.mockResolvedValue( + Array(30) + .fill(0) + .map((_, i) => ({ + document: { + title: 'Hello World', + text: 'Lorem Ipsum', + location: `location-${i}`, + }, + type: 'my-type', + })) + .slice(25), + ); + + const results = await searchEngine.query({ + term: 'Hello World', + pageCursor: 'MQ==', + }); + + expect(results).toEqual({ + results: Array(30) + .fill(0) + .map((_, i) => ({ + document: { + title: 'Hello World', + text: 'Lorem Ipsum', + location: `location-${i}`, + }, + type: 'my-type', + })) + .slice(25), + previousPageCursor: 'MA==', + }); + expect(database.transaction).toHaveBeenCalledTimes(1); + expect(database.query).toHaveBeenCalledWith(tx, { + pgTerm: '("Hello" | "Hello":*)&("World" | "World":*)', + offset: 25, + limit: 26, }); }); }); }); + +describe('decodePageCursor', () => { + test('should decode page', () => { + expect(decodePageCursor('MQ==')).toEqual({ page: 1 }); + }); + + test('should fallback to first page if empty', () => { + expect(decodePageCursor()).toEqual({ page: 0 }); + }); +}); + +describe('encodePageCursor', () => { + test('should encode page', () => { + expect(encodePageCursor({ page: 1 })).toEqual('MQ=='); + }); +}); diff --git a/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.ts b/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.ts index d5cf498cc1..a032dd783e 100644 --- a/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.ts +++ b/plugins/search-backend-module-pg/src/PgSearchEngine/PgSearchEngine.ts @@ -27,7 +27,10 @@ import { PgSearchQuery, } from '../database'; -// TODO: Support paging using page cursor (return cursor and parse cursor) +export type ConcretePgSearchQuery = { + pgQuery: PgSearchQuery; + pageSize: number; +}; export class PgSearchEngine implements SearchEngine { constructor(private readonly databaseStore: DatabaseStore) {} @@ -46,22 +49,33 @@ export class PgSearchEngine implements SearchEngine { return await DatabaseDocumentStore.supported(await database.getClient()); } - translator(query: SearchQuery): PgSearchQuery { + translator(query: SearchQuery): ConcretePgSearchQuery { + const pageSize = 25; + const { page } = decodePageCursor(query.pageCursor); + const offset = page * pageSize; + // We request more result to know whether there is another page + const limit = pageSize + 1; + return { - pgTerm: query.term - .split(/\s/) - .map(p => p.replace(/[\0()|&:*!]/g, '').trim()) - .filter(p => p !== '') - .map(p => `(${JSON.stringify(p)} | ${JSON.stringify(p)}:*)`) - .join('&'), - fields: query.filters as Record, - types: query.types, - offset: query.offset ?? 0, - limit: Math.min(query.limit ?? 25, 100), + pgQuery: { + pgTerm: query.term + .split(/\s/) + .map(p => p.replace(/[\0()|&:*!]/g, '').trim()) + .filter(p => p !== '') + .map(p => `(${JSON.stringify(p)} | ${JSON.stringify(p)}:*)`) + .join('&'), + fields: query.filters as Record, + types: query.types, + offset, + limit, + }, + pageSize, }; } - setTranslator(translator: (query: SearchQuery) => PgSearchQuery): void { + setTranslator( + translator: (query: SearchQuery) => ConcretePgSearchQuery, + ): void { this.translator = translator; } @@ -79,21 +93,44 @@ export class PgSearchEngine implements SearchEngine { } async query(query: SearchQuery): Promise { - const pgQuery = this.translator(query); + const { pgQuery, pageSize } = this.translator(query); - const [rows, totalCount] = await Promise.all([ - this.databaseStore.transaction(async tx => - this.databaseStore.query(tx, pgQuery), - ), - this.databaseStore.transaction(async tx => - this.databaseStore.count(tx, pgQuery), - ), - ]); - const results = rows.map(({ type, document }) => ({ + const rows = await this.databaseStore.transaction(async tx => + this.databaseStore.query(tx, pgQuery), + ); + + // We requested one result more than the page size to know whether there is + // another page. + const { page } = decodePageCursor(query.pageCursor); + const hasNextPage = rows.length > pageSize; + const hasPreviousPage = page > 0; + const pageRows = rows.slice(0, pageSize); + const nextPageCursor = hasNextPage + ? encodePageCursor({ page: page + 1 }) + : undefined; + const previousPageCursor = hasPreviousPage + ? encodePageCursor({ page: page - 1 }) + : undefined; + + const results = pageRows.map(({ type, document }) => ({ type, document, })); - return { results, totalCount }; + return { results, nextPageCursor, previousPageCursor }; } } + +export function decodePageCursor(pageCursor?: string): { page: number } { + if (!pageCursor) { + return { page: 0 }; + } + + return { + page: Number(Buffer.from(pageCursor, 'base64').toString('utf-8')), + }; +} + +export function encodePageCursor({ page }: { page: number }): string { + return Buffer.from(`${page}`, 'utf-8').toString('base64'); +} diff --git a/plugins/search-backend-module-pg/src/PgSearchEngine/index.ts b/plugins/search-backend-module-pg/src/PgSearchEngine/index.ts index b28e03ee5c..7994998baf 100644 --- a/plugins/search-backend-module-pg/src/PgSearchEngine/index.ts +++ b/plugins/search-backend-module-pg/src/PgSearchEngine/index.ts @@ -14,3 +14,4 @@ * limitations under the License. */ export { PgSearchEngine } from './PgSearchEngine'; +export type { ConcretePgSearchQuery } from './PgSearchEngine'; diff --git a/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.test.ts b/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.test.ts index a456d0caf5..71c012ec00 100644 --- a/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.test.ts +++ b/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.test.ts @@ -238,37 +238,6 @@ describe('DatabaseDocumentStore', () => { 60_000, ); - it.each(databases.eachSupportedId())( - 'count by term, %p', - async databaseId => { - const { store } = await createStore(databaseId); - - await store.transaction(async tx => { - await store.prepareInsert(tx); - await store.insertDocuments(tx, 'test', [ - { - title: 'Lorem Ipsum', - text: 'Hello World', - location: 'LOCATION-1', - }, - { - title: 'Hello World', - text: 'Around the world', - location: 'LOCATION-1', - }, - ]); - await store.completeInsert(tx, 'test'); - }); - - const totalCount = await store.transaction(tx => - store.count(tx, { pgTerm: 'Hello & World', offset: 0, limit: 25 }), - ); - - expect(totalCount).toEqual(2); - }, - 60_000, - ); - it.each(databases.eachSupportedId())( 'query by term, %p', async databaseId => { diff --git a/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.ts b/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.ts index 80398ec707..8c180ea019 100644 --- a/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.ts +++ b/plugins/search-backend-module-pg/src/database/DatabaseDocumentStore.ts @@ -128,7 +128,7 @@ export class DatabaseDocumentStore implements DatabaseStore { async query( tx: Knex.Transaction, - searchQuery: PgSearchQuery, + { types, pgTerm, fields, offset, limit }: PgSearchQuery, ): Promise { // Builds a query like: // SELECT ts_rank_cd(body, query) AS rank, type, document @@ -136,36 +136,6 @@ export class DatabaseDocumentStore implements DatabaseStore { // WHERE query @@ body AND (document @> '{"kind": "API"}') // ORDER BY rank DESC // LIMIT 10; - const query = this.buildQuery(tx, searchQuery); - - query.select('type', 'document'); - - const { pgTerm, limit, offset } = searchQuery; - - if (pgTerm) { - query - .select(tx.raw('ts_rank_cd(body, query) AS "rank"')) - .orderBy('rank', 'desc'); - } else { - query.select(tx.raw('1 as rank')); - } - - return await query.offset(offset).limit(limit); - } - - async count( - tx: Knex.Transaction, - searchQuery: PgSearchQuery, - ): Promise { - const query = this.buildQuery(tx, searchQuery); - const [row] = await query.count(); - return Number(row.count); - } - - private buildQuery( - tx: Knex.Transaction, - { types, pgTerm, fields }: PgSearchQuery, - ) { const query = tx('documents'); if (pgTerm) { @@ -194,6 +164,16 @@ export class DatabaseDocumentStore implements DatabaseStore { }); } - return query; + query.select('type', 'document'); + + if (pgTerm) { + query + .select(tx.raw('ts_rank_cd(body, query) AS "rank"')) + .orderBy('rank', 'desc'); + } else { + query.select(tx.raw('1 as rank')); + } + + return await query.offset(offset).limit(limit); } } diff --git a/plugins/search-backend-module-pg/src/database/types.ts b/plugins/search-backend-module-pg/src/database/types.ts index f8425c41b6..0c0596160e 100644 --- a/plugins/search-backend-module-pg/src/database/types.ts +++ b/plugins/search-backend-module-pg/src/database/types.ts @@ -37,7 +37,6 @@ export interface DatabaseStore { tx: Knex.Transaction, pgQuery: PgSearchQuery, ): Promise; - count(tx: Knex.Transaction, pgQuery: PgSearchQuery): Promise; } export interface RawDocumentRow { diff --git a/plugins/search-backend-node/src/engines/LunrSearchEngine.test.ts b/plugins/search-backend-node/src/engines/LunrSearchEngine.test.ts index 71c2b95a29..a2cbb4ecc3 100644 --- a/plugins/search-backend-node/src/engines/LunrSearchEngine.test.ts +++ b/plugins/search-backend-node/src/engines/LunrSearchEngine.test.ts @@ -17,7 +17,12 @@ import { getVoidLogger } from '@backstage/backend-common'; import lunr from 'lunr'; import { SearchEngine } from '@backstage/search-common'; -import { ConcreteLunrQuery, LunrSearchEngine } from './LunrSearchEngine'; +import { + ConcreteLunrQuery, + LunrSearchEngine, + decodePageCursor, + encodePageCursor, +} from './LunrSearchEngine'; /** * Just used to test the default translator shipped with LunrSearchEngine. @@ -48,12 +53,14 @@ describe('LunrSearchEngine', () => { await testLunrSearchEngine.query({ term: 'testTerm', filters: {}, + pageCursor: 'MQ==', }); // Then: the translator is invoked with expected args. expect(translatorSpy).toHaveBeenCalledWith({ term: 'testTerm', filters: {}, + pageCursor: 'MQ==', }); }); @@ -66,15 +73,12 @@ describe('LunrSearchEngine', () => { const actualTranslatedQuery = translatorUnderTest({ term: 'testTerm', filters: {}, - offset: 0, - limit: 50, }) as ConcreteLunrQuery; expect(actualTranslatedQuery).toMatchObject({ documentTypes: undefined, lunrQueryBuilder: expect.any(Function), - offset: 0, - limit: 50, + pageSize: 25, }); const query: jest.Mocked = { @@ -115,52 +119,7 @@ describe('LunrSearchEngine', () => { expect(actualTranslatedQuery).toMatchObject({ documentTypes: undefined, lunrQueryBuilder: expect.any(Function), - offset: 0, - limit: 25, - }); - - const query: jest.Mocked = { - allFields: [], - clauses: [], - term: jest.fn(), - clause: jest.fn(), - }; - - actualTranslatedQuery.lunrQueryBuilder.bind(query)(query); - - expect(query.term).toBeCalledWith(lunr.tokenizer('testTerm'), { - boost: 100, - usePipeline: true, - }); - expect(query.term).toBeCalledWith(lunr.tokenizer('testTerm'), { - boost: 10, - usePipeline: false, - wildcard: lunr.Query.wildcard.TRAILING, - }); - expect(query.term).toBeCalledWith(lunr.tokenizer('testTerm'), { - boost: 1, - usePipeline: false, - editDistance: 2, - }); - }); - - it('should have maximum limit', async () => { - const inspectableSearchEngine = new LunrSearchEngineForTranslatorTests({ - logger: getVoidLogger(), - }); - const translatorUnderTest = inspectableSearchEngine.getTranslator(); - - const actualTranslatedQuery = translatorUnderTest({ - term: 'testTerm', - offset: 0, - limit: 1000, - }) as ConcreteLunrQuery; - - expect(actualTranslatedQuery).toMatchObject({ - documentTypes: undefined, - lunrQueryBuilder: expect.any(Function), - offset: 0, - limit: 100, + pageSize: 25, }); const query: jest.Mocked = { @@ -328,7 +287,10 @@ describe('LunrSearchEngine', () => { }); // Should return 0 results as nothing is indexed here - expect(mockedSearchResult).toMatchObject({ results: [], totalCount: 0 }); + expect(mockedSearchResult).toMatchObject({ + results: [], + nextPageCursor: undefined, + }); }); it('should perform search query and return 0 results on no match', async () => { @@ -350,7 +312,10 @@ describe('LunrSearchEngine', () => { }); // Should return 0 results as we are mocking the indexing of 1 document but with no match on the fields - expect(mockedSearchResult).toMatchObject({ results: [], totalCount: 0 }); + expect(mockedSearchResult).toMatchObject({ + results: [], + nextPageCursor: undefined, + }); }); it('should perform search query and return all results on empty term', async () => { @@ -382,7 +347,7 @@ describe('LunrSearchEngine', () => { type: 'test-index', }, ], - totalCount: 1, + nextPageCursor: undefined, }); }); @@ -414,7 +379,7 @@ describe('LunrSearchEngine', () => { }, }, ], - totalCount: 1, + nextPageCursor: undefined, }); }); @@ -446,7 +411,7 @@ describe('LunrSearchEngine', () => { }, }, ], - totalCount: 1, + nextPageCursor: undefined, }); }); @@ -479,7 +444,7 @@ describe('LunrSearchEngine', () => { }, }, ], - totalCount: 1, + nextPageCursor: undefined, }); }); @@ -512,7 +477,7 @@ describe('LunrSearchEngine', () => { }, }, ], - totalCount: 1, + nextPageCursor: undefined, }); }); @@ -545,7 +510,7 @@ describe('LunrSearchEngine', () => { }, }, ], - totalCount: 1, + nextPageCursor: undefined, }); }); @@ -585,7 +550,7 @@ describe('LunrSearchEngine', () => { }, }, ], - totalCount: 1, + nextPageCursor: undefined, }); }); @@ -627,7 +592,7 @@ describe('LunrSearchEngine', () => { }, }, ], - totalCount: 1, + nextPageCursor: undefined, }); }); @@ -667,7 +632,7 @@ describe('LunrSearchEngine', () => { }, }, ], - totalCount: 1, + nextPageCursor: undefined, }); }); @@ -725,9 +690,75 @@ describe('LunrSearchEngine', () => { }, }, ], - totalCount: 2, + nextPageCursor: undefined, }); }); + + it('should return next page cursor if results exceed page size', async () => { + const mockDocuments = Array(30) + .fill(0) + .map((_, i) => ({ + title: 'testTitle', + text: 'testText', + location: `test/location/${i}`, + })); + + await testLunrSearchEngine.index('test-index', mockDocuments); + + const mockedSearchResult = await testLunrSearchEngine.query({ + term: 'testTitle', + types: ['test-index'], + }); + + expect(mockedSearchResult).toMatchObject({ + results: Array(25) + .fill(0) + .map((_, i) => ({ + document: { + title: 'testTitle', + text: 'testText', + location: `test/location/${i}`, + }, + type: 'test-index', + })), + nextPageCursor: 'MQ==', + previousPageCursor: undefined, + }); + }); + }); + + it('should return previous page cursor if on another page', async () => { + const mockDocuments = Array(30) + .fill(0) + .map((_, i) => ({ + title: 'testTitle', + text: 'testText', + location: `test/location/${i}`, + })); + + await testLunrSearchEngine.index('test-index', mockDocuments); + + const mockedSearchResult = await testLunrSearchEngine.query({ + term: 'testTitle', + types: ['test-index'], + pageCursor: 'MQ==', + }); + + expect(mockedSearchResult).toMatchObject({ + results: Array(30) + .fill(0) + .map((_, i) => ({ + document: { + title: 'testTitle', + text: 'testText', + location: `test/location/${i}`, + }, + type: 'test-index', + })) + .slice(25), + nextPageCursor: undefined, + previousPageCursor: 'MA==', + }); }); describe('index', () => { @@ -750,3 +781,19 @@ describe('LunrSearchEngine', () => { }); }); }); + +describe('decodePageCursor', () => { + test('should decode page', () => { + expect(decodePageCursor('MQ==')).toEqual({ page: 1 }); + }); + + test('should fallback to first page if empty', () => { + expect(decodePageCursor()).toEqual({ page: 0 }); + }); +}); + +describe('encodePageCursor', () => { + test('should encode page', () => { + expect(encodePageCursor({ page: 1 })).toEqual('MQ=='); + }); +}); diff --git a/plugins/search-backend-node/src/engines/LunrSearchEngine.ts b/plugins/search-backend-node/src/engines/LunrSearchEngine.ts index 75e5fdc625..f22762cb9f 100644 --- a/plugins/search-backend-node/src/engines/LunrSearchEngine.ts +++ b/plugins/search-backend-node/src/engines/LunrSearchEngine.ts @@ -27,8 +27,7 @@ import { Logger } from 'winston'; export type ConcreteLunrQuery = { lunrQueryBuilder: lunr.Index.QueryBuilder; documentTypes?: string[]; - offset: number; - limit: number; + pageSize: number; }; type LunrResultEnvelope = { @@ -52,9 +51,9 @@ export class LunrSearchEngine implements SearchEngine { term, filters, types, - offset, - limit, }: SearchQuery): ConcreteLunrQuery => { + const pageSize = 25; + return { lunrQueryBuilder: q => { const termToken = lunr.tokenizer(term); @@ -111,8 +110,7 @@ export class LunrSearchEngine implements SearchEngine { } }, documentTypes: types, - offset: offset ?? 0, - limit: Math.min(limit ?? 25, 100), + pageSize, }; }; @@ -147,7 +145,7 @@ export class LunrSearchEngine implements SearchEngine { } async query(query: SearchQuery): Promise { - const { lunrQueryBuilder, documentTypes, offset, limit } = this.translator( + const { lunrQueryBuilder, documentTypes, pageSize } = this.translator( query, ) as ConcreteLunrQuery; @@ -183,14 +181,41 @@ export class LunrSearchEngine implements SearchEngine { return doc2.result.score - doc1.result.score; }); + // Perform paging + const { page } = decodePageCursor(query.pageCursor); + const offset = page * pageSize; + const hasPreviousPage = page > 0; + const hasNextPage = results.length > offset + pageSize; + const nextPageCursor = hasNextPage + ? encodePageCursor({ page: page + 1 }) + : undefined; + const previousPageCursor = hasPreviousPage + ? encodePageCursor({ page: page - 1 }) + : undefined; + // Translate results into SearchResultSet const realResultSet: SearchResultSet = { - results: results.slice(offset, offset + limit).map(d => { + results: results.slice(offset, offset + pageSize).map(d => { return { type: d.type, document: this.docStore[d.result.ref] }; }), - totalCount: results.length, + nextPageCursor, + previousPageCursor, }; return realResultSet; } } + +export function decodePageCursor(pageCursor?: string): { page: number } { + if (!pageCursor) { + return { page: 0 }; + } + + return { + page: Number(Buffer.from(pageCursor, 'base64').toString('utf-8')), + }; +} + +export function encodePageCursor({ page }: { page: number }): string { + return Buffer.from(`${page}`, 'utf-8').toString('base64'); +} diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index 5a87fb1734..6df39ca1eb 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -36,21 +36,17 @@ export async function createRouter({ req: express.Request, res: express.Response, ) => { - const { term, filters = {}, types, offset, limit } = req.query; + const { term, filters = {}, types, pageCursor } = req.query; logger.info( `Search request received: term="${term}", filters=${JSON.stringify( filters, - )}, offset=${offset ?? ''}, limit=${limit ?? ''}`, + )}, types=${types ? types.join(',') : ''}, pageCursor=${ + pageCursor ?? '' + }`, ); try { - const results = await engine?.query({ - term, - types, - filters, - offset: offset ? Number(offset) : undefined, - limit: limit ? Number(limit) : undefined, - }); + const results = await engine?.query(req.query); res.send(results); } catch (err) { throw new Error( diff --git a/plugins/search/api-report.md b/plugins/search/api-report.md index 616c6f2cae..2cdac79cad 100644 --- a/plugins/search/api-report.md +++ b/plugins/search/api-report.md @@ -139,10 +139,8 @@ export { searchPlugin }; // @public (undocumented) export const SearchResult: ({ children, - initialPageSize, }: { children: (results: { results: SearchResult_2[] }) => JSX.Element; - initialPageSize?: number | undefined; }) => JSX.Element; // Warning: (ae-forgotten-export) The symbol "SearchTypeProps" needs to be exported by the entry point index.d.ts @@ -169,7 +167,7 @@ export const useSearch: () => SearchContextValue; // Warnings were encountered during analysis: // -// src/components/SearchContext/SearchContext.d.ts:23:5 - (ae-forgotten-export) The symbol "SettableSearchContext" needs to be exported by the entry point index.d.ts +// src/components/SearchContext/SearchContext.d.ts:21:5 - (ae-forgotten-export) The symbol "SettableSearchContext" needs to be exported by the entry point index.d.ts // src/components/SearchFilter/SearchFilter.d.ts:13:5 - (ae-forgotten-export) The symbol "Props" needs to be exported by the entry point index.d.ts // src/components/SearchFilter/SearchFilter.d.ts:14:5 - (ae-forgotten-export) The symbol "Component" needs to be exported by the entry point index.d.ts diff --git a/plugins/search/src/apis.test.ts b/plugins/search/src/apis.test.ts index 38f34bb91b..91395d1f0b 100644 --- a/plugins/search/src/apis.test.ts +++ b/plugins/search/src/apis.test.ts @@ -21,7 +21,6 @@ describe('apis', () => { term: '', filters: {}, types: [], - page: {}, }; const baseUrl = 'https://base-url.com/'; diff --git a/plugins/search/src/components/SearchBar/SearchBar.test.tsx b/plugins/search/src/components/SearchBar/SearchBar.test.tsx index acfeb2cb8b..188ffb279b 100644 --- a/plugins/search/src/components/SearchBar/SearchBar.test.tsx +++ b/plugins/search/src/components/SearchBar/SearchBar.test.tsx @@ -30,7 +30,6 @@ jest.mock('@backstage/core-plugin-api', () => ({ describe('SearchBar', () => { const initialState = { term: '', - page: {}, filters: {}, types: ['*'], }; diff --git a/plugins/search/src/components/SearchContext/SearchContext.test.tsx b/plugins/search/src/components/SearchContext/SearchContext.test.tsx index c1618be5ec..d4153e8e4b 100644 --- a/plugins/search/src/components/SearchContext/SearchContext.test.tsx +++ b/plugins/search/src/components/SearchContext/SearchContext.test.tsx @@ -14,13 +14,11 @@ * limitations under the License. */ -import React from 'react'; -import { render, screen, waitFor } from '@testing-library/react'; -import { renderHook, act } from '@testing-library/react-hooks'; - -import { useSearch, SearchContextProvider } from './SearchContext'; - import { useApi } from '@backstage/core-plugin-api'; +import { render, screen, waitFor } from '@testing-library/react'; +import { act, renderHook } from '@testing-library/react-hooks'; +import React from 'react'; +import { SearchContextProvider, useSearch } from './SearchContext'; jest.mock('@backstage/core-plugin-api', () => ({ ...jest.requireActual('@backstage/core-plugin-api'), @@ -38,7 +36,6 @@ describe('SearchContext', () => { const initialState = { term: '', - page: {}, filters: {}, types: ['*'], }; @@ -46,6 +43,7 @@ describe('SearchContext', () => { beforeEach(() => { query.mockResolvedValue({}); (useApi as jest.Mock).mockReturnValue({ query: query }); + window.scrollTo = jest.fn(); }); afterAll(() => { @@ -93,26 +91,26 @@ describe('SearchContext', () => { initialProps: { initialState: { ...initialState, - page: { offset: 0, limit: 25 }, + pageCursor: 'SOMEPAGE', }, }, }); await waitForNextUpdate(); - expect(result.current.page).toEqual({ offset: 0, limit: 25 }); + expect(result.current.pageCursor).toEqual('SOMEPAGE'); act(() => { result.current.setTerm('first term'); }); act(() => { - result.current.setPage({ offset: 75, limit: 25 }); + result.current.setPageCursor('OTHERPAGE'); }); await waitForNextUpdate(); - expect(result.current.page).toEqual({ offset: 75, limit: 25 }); + expect(result.current.pageCursor).toEqual('OTHERPAGE'); act(() => { result.current.setTerm('second term'); @@ -120,7 +118,7 @@ describe('SearchContext', () => { await waitForNextUpdate(); - expect(result.current.page).toEqual({ offset: 0, limit: 25 }); + expect(result.current.pageCursor).toEqual(undefined); }); describe('Performs search (and sets results)', () => { @@ -145,8 +143,6 @@ describe('SearchContext', () => { expect(query).toHaveBeenLastCalledWith({ filters: {}, types: ['*'], - limit: undefined, - offset: undefined, term, }); }); @@ -172,8 +168,6 @@ describe('SearchContext', () => { expect(query).toHaveBeenLastCalledWith({ filters, types: ['*'], - limit: undefined, - offset: undefined, term: '', }); }); @@ -188,10 +182,8 @@ describe('SearchContext', () => { await waitForNextUpdate(); - const page = { offset: 25, limit: 50 }; - act(() => { - result.current.setPage(page); + result.current.setPageCursor('SOMEPAGE'); }); await waitForNextUpdate(); @@ -199,8 +191,7 @@ describe('SearchContext', () => { expect(query).toHaveBeenLastCalledWith({ filters: {}, types: ['*'], - limit: 50, - offset: 25, + pageCursor: 'SOMEPAGE', term: '', }); }); @@ -226,10 +217,72 @@ describe('SearchContext', () => { expect(query).toHaveBeenLastCalledWith({ types, filters: {}, - limit: undefined, - offset: undefined, term: '', }); }); + + it('provides function for fetch the next page', async () => { + query.mockResolvedValue({ + results: [], + nextPageCursor: 'NEXT', + }); + + const { result, waitForNextUpdate } = renderHook(() => useSearch(), { + wrapper, + initialProps: { + initialState, + }, + }); + + await waitForNextUpdate(); + + expect(result.current.fetchNextPage).toBeDefined(); + expect(result.current.fetchPreviousPage).toBeUndefined(); + + act(() => { + result.current.fetchNextPage!(); + }); + + await waitForNextUpdate(); + + expect(query).toHaveBeenLastCalledWith({ + types: ['*'], + filters: {}, + term: '', + pageCursor: 'NEXT', + }); + }); + + it('provides function for fetch the previous page', async () => { + query.mockResolvedValue({ + results: [], + previousPageCursor: 'PREVIOUS', + }); + + const { result, waitForNextUpdate } = renderHook(() => useSearch(), { + wrapper, + initialProps: { + initialState, + }, + }); + + await waitForNextUpdate(); + + expect(result.current.fetchNextPage).toBeUndefined(); + expect(result.current.fetchPreviousPage).toBeDefined(); + + act(() => { + result.current.fetchPreviousPage!(); + }); + + await waitForNextUpdate(); + + expect(query).toHaveBeenLastCalledWith({ + types: ['*'], + filters: {}, + term: '', + pageCursor: 'PREVIOUS', + }); + }); }); }); diff --git a/plugins/search/src/components/SearchContext/SearchContext.tsx b/plugins/search/src/components/SearchContext/SearchContext.tsx index 018496876b..0badc7fd20 100644 --- a/plugins/search/src/components/SearchContext/SearchContext.tsx +++ b/plugins/search/src/components/SearchContext/SearchContext.tsx @@ -20,6 +20,7 @@ import { SearchResultSet } from '@backstage/search-common'; import React, { createContext, PropsWithChildren, + useCallback, useContext, useEffect, useState, @@ -28,8 +29,6 @@ import { useAsync, usePrevious } from 'react-use'; import { AsyncState } from 'react-use/lib/useAsync'; import { searchApiRef } from '../../apis'; -type Page = { limit?: number; offset?: number }; - type SearchContextValue = { result: AsyncState; term: string; @@ -38,13 +37,21 @@ type SearchContextValue = { setTypes: React.Dispatch>; filters: JsonObject; setFilters: React.Dispatch>; - page: Page; - setPage: React.Dispatch>; + pageCursor?: string; + setPageCursor: React.Dispatch>; + fetchNextPage?: React.DispatchWithoutAction; + fetchPreviousPage?: React.DispatchWithoutAction; }; type SettableSearchContext = Omit< SearchContextValue, - 'result' | 'setTerm' | 'setTypes' | 'setFilters' | 'setPage' + | 'result' + | 'setTerm' + | 'setTypes' + | 'setFilters' + | 'setPageCursor' + | 'fetchNextPage' + | 'fetchPreviousPage' >; export const SearchContext = createContext( @@ -54,14 +61,16 @@ export const SearchContext = createContext( export const SearchContextProvider = ({ initialState = { term: '', - page: {}, + pageCursor: undefined, filters: {}, types: [], }, children, }: PropsWithChildren<{ initialState?: SettableSearchContext }>) => { const searchApi = useApi(searchApiRef); - const [page, setPage] = useState(initialState.page); + const [pageCursor, setPageCursor] = useState( + initialState.pageCursor, + ); const [filters, setFilters] = useState(initialState.filters); const [term, setTerm] = useState(initialState.term); const [types, setTypes] = useState(initialState.types); @@ -72,19 +81,31 @@ export const SearchContextProvider = ({ searchApi.query({ term, filters, - offset: page?.offset, - limit: page?.limit, + pageCursor: pageCursor, types, }), - [term, filters, types, page], + [term, filters, types, pageCursor], ); + const hasNextPage = + !result.loading && !result.error && result.value?.nextPageCursor; + const hasPreviousPage = + !result.loading && !result.error && result.value?.previousPageCursor; + const fetchNextPage = useCallback(() => { + setPageCursor(result.value?.nextPageCursor); + resetScrollPosition(); + }, [result.value?.nextPageCursor]); + const fetchPreviousPage = useCallback(() => { + setPageCursor(result.value?.previousPageCursor); + resetScrollPosition(); + }, [result.value?.previousPageCursor]); + useEffect(() => { // Any time a term is reset, we want to start from page 0. if (term && prevTerm && term !== prevTerm) { - setPage(initialState.page); + setPageCursor(undefined); } - }, [term, prevTerm, initialState.page]); + }, [term, prevTerm, initialState.pageCursor]); const value: SearchContextValue = { result, @@ -94,8 +115,10 @@ export const SearchContextProvider = ({ setTerm, types, setTypes, - page, - setPage, + pageCursor, + setPageCursor, + fetchNextPage: hasNextPage ? fetchNextPage : undefined, + fetchPreviousPage: hasPreviousPage ? fetchPreviousPage : undefined, }; return ; @@ -108,3 +131,7 @@ export const useSearch = () => { } return context; }; + +function resetScrollPosition() { + window.scrollTo({ top: 0, left: 0, behavior: 'smooth' }); +} diff --git a/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx b/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx index b5dd56bfb6..8f716caf4b 100644 --- a/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx +++ b/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx @@ -32,7 +32,6 @@ describe('SearchFilter', () => { term: '', filters: {}, types: [], - page: {}, }; const name = 'field'; diff --git a/plugins/search/src/components/SearchPage/SearchPage.test.tsx b/plugins/search/src/components/SearchPage/SearchPage.test.tsx index 91568cddbe..5ff61a3dbe 100644 --- a/plugins/search/src/components/SearchPage/SearchPage.test.tsx +++ b/plugins/search/src/components/SearchPage/SearchPage.test.tsx @@ -14,10 +14,9 @@ * limitations under the License. */ -import React from 'react'; import { renderInTestApp } from '@backstage/test-utils'; +import React from 'react'; import { useLocation, useOutlet } from 'react-router'; - import { useSearch } from '../SearchContext'; import { SearchPage } from './'; @@ -74,25 +73,21 @@ describe('SearchPage', () => { const expectedTerm = 'justin bieber'; const expectedTypes = ['software-catalog']; const expectedFilters = { [expectedFilterField]: expectedFilterValue }; - const expectedOffset = 25; - const expectedLimit = 50; + const expectedPageCursor = 'SOMEPAGE'; - // e.g. ?query=petstore&offset=25&limit=50&filters[lifecycle][]=experimental&filters[kind]=Component + // e.g. ?query=petstore&pageCursor=SOMEPAGE&filters[lifecycle][]=experimental&filters[kind]=Component (useLocation as jest.Mock).mockReturnValueOnce({ - search: `?query=${expectedTerm}&types[]=${expectedTypes[0]}&filters[${expectedFilterField}]=${expectedFilterValue}&offset=${expectedOffset}&limit=${expectedLimit}`, + search: `?query=${expectedTerm}&types[]=${expectedTypes[0]}&filters[${expectedFilterField}]=${expectedFilterValue}&pageCursor=${expectedPageCursor}`, }); // When we render the page... await renderInTestApp(); - // Then search context should be initialized with these values... - const calls = (SearchContextProvider as jest.Mock).mock.calls[0]; - const actualInitialState = calls[0].initialState; - expect(actualInitialState.term).toEqual(expectedTerm); - expect(actualInitialState.types).toEqual(expectedTypes); - expect(actualInitialState.page.limit).toEqual(expectedLimit); - expect(actualInitialState.page.offset).toEqual(expectedOffset); - expect(actualInitialState.filters).toStrictEqual(expectedFilters); + // Then search context should be set with these values... + expect(setTermMock).toHaveBeenCalledWith(expectedTerm); + expect(setTypesMock).toHaveBeenCalledWith(expectedTypes); + expect(setPageCursorMock).toHaveBeenCalledWith(expectedPageCursor); + expect(setFiltersMock).toHaveBeenCalledWith(expectedFilters); }); it('renders provided router element', async () => { @@ -112,7 +107,7 @@ describe('SearchPage', () => { (useSearch as jest.Mock).mockReturnValueOnce({ term: 'bieber', types: ['software-catalog'], - page: { offset: 25, limit: 50 }, + pageCursor: 'SOMEPAGE', filters: { anyKey: 'anyValue' }, setTerm: setTermMock, setTypes: setTypesMock, @@ -120,7 +115,7 @@ describe('SearchPage', () => { setPageCursor: setPageCursorMock, }); const expectedLocation = encodeURI( - '?query=bieber&types[]=software-catalog&offset=25&limit=50&filters[anyKey]=anyValue', + '?query=bieber&types[]=software-catalog&pageCursor=SOMEPAGE&filters[anyKey]=anyValue', ); await renderInTestApp(); diff --git a/plugins/search/src/components/SearchResult/SearchResult.test.tsx b/plugins/search/src/components/SearchResult/SearchResult.test.tsx index d28bcc9136..43a9cdb022 100644 --- a/plugins/search/src/components/SearchResult/SearchResult.test.tsx +++ b/plugins/search/src/components/SearchResult/SearchResult.test.tsx @@ -24,7 +24,6 @@ jest.mock('../SearchContext', () => ({ ...jest.requireActual('../SearchContext'), useSearch: jest.fn().mockReturnValue({ result: {}, - page: {}, }), })); @@ -111,7 +110,6 @@ describe('SearchResult', () => { ], }, }, - page: {}, }); const { getByText } = await renderInTestApp( @@ -124,43 +122,4 @@ describe('SearchResult', () => { expect(getByText('Results 1')).toBeInTheDocument(); }); - - it('Starts on initial page if no offset is set', async () => { - (useSearch as jest.Mock).mockReturnValueOnce({ - page: {}, - result: { - loading: false, - error: '', - value: { results: [{}], totalCount: 100 }, - }, - }); - - const { getByLabelText } = await renderInTestApp( - {({}) => <>}, - ); - - expect(getByLabelText('page 1')).toHaveAttribute('aria-current', 'true'); - expect(getByLabelText('Go to page 2')).toBeInTheDocument(); - expect(getByLabelText('Go to page 3')).toBeInTheDocument(); - expect(getByLabelText('Go to page 4')).toBeInTheDocument(); - }); - - it('Shows the right page', async () => { - (useSearch as jest.Mock).mockReturnValueOnce({ - page: { offset: 25 }, - result: { - loading: false, - error: '', - value: { results: [{}], totalCount: 63 }, - }, - }); - - const { getByLabelText } = await renderInTestApp( - {({}) => <>}, - ); - - expect(getByLabelText('Go to page 1')).toBeInTheDocument(); - expect(getByLabelText('page 2')).toHaveAttribute('aria-current', 'true'); - expect(getByLabelText('Go to page 3')).toBeInTheDocument(); - }); }); diff --git a/plugins/search/src/components/SearchResult/SearchResult.tsx b/plugins/search/src/components/SearchResult/SearchResult.tsx index 5ac2c86a79..ff01e96e74 100644 --- a/plugins/search/src/components/SearchResult/SearchResult.tsx +++ b/plugins/search/src/components/SearchResult/SearchResult.tsx @@ -20,20 +20,17 @@ import { ResponseErrorPanel, } from '@backstage/core-components'; import { SearchResult } from '@backstage/search-common'; -import { Pagination } from '@material-ui/lab'; import React from 'react'; import { useSearch } from '../SearchContext'; +import { SearchResultPager } from '../SearchResultPager'; type Props = { children: (results: { results: SearchResult[] }) => JSX.Element; - initialPageSize?: number; }; -const SearchResultComponent = ({ children, initialPageSize = 25 }: Props) => { +export const SearchResultComponent = ({ children }: Props) => { const { result: { loading, error, value }, - page, - setPage, } = useSearch(); if (loading) { @@ -52,23 +49,10 @@ const SearchResultComponent = ({ children, initialPageSize = 25 }: Props) => { return ; } - const pageSize = page.limit ?? initialPageSize; - const totalPages = Math.ceil(value.totalCount / pageSize); - const currentPage = page.offset ? Math.floor(page.offset / pageSize) + 1 : 1; - - const handlePageChange = (_: React.ChangeEvent, pageNum: number) => { - setPage({ offset: (pageNum - 1) * pageSize, limit: pageSize }); - window.scrollTo({ top: 0, left: 0, behavior: 'smooth' }); - }; - return ( <> {children({ results: value.results })} - + ); }; diff --git a/plugins/search/src/components/SearchResultPager/SearchResultPager.test.tsx b/plugins/search/src/components/SearchResultPager/SearchResultPager.test.tsx new file mode 100644 index 0000000000..ab46d77938 --- /dev/null +++ b/plugins/search/src/components/SearchResultPager/SearchResultPager.test.tsx @@ -0,0 +1,59 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { renderInTestApp } from '@backstage/test-utils'; +import { waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import React from 'react'; +import { useSearch } from '../SearchContext'; +import { SearchResultPager } from './SearchResultPager'; + +jest.mock('../SearchContext', () => ({ + ...jest.requireActual('../SearchContext'), + useSearch: jest.fn().mockReturnValue({ + result: {}, + }), +})); + +describe('SearchResultPager', () => { + it('renders pager buttons', async () => { + const fetchNextPage = jest.fn(); + const fetchPreviousPage = jest.fn(); + (useSearch as jest.Mock).mockReturnValueOnce({ + result: { loading: false, value: [] }, + fetchNextPage, + fetchPreviousPage, + }); + + const { getByLabelText } = await renderInTestApp(); + + await waitFor(() => { + expect(getByLabelText('previous page')).toBeInTheDocument(); + + userEvent.click(getByLabelText('previous page')); + }); + + expect(fetchPreviousPage).toBeCalled(); + + await waitFor(() => { + expect(getByLabelText('next page')).toBeInTheDocument(); + + userEvent.click(getByLabelText('next page')); + }); + + expect(fetchNextPage).toBeCalled(); + }); +}); diff --git a/plugins/search/src/components/SearchResultPager/SearchResultPager.tsx b/plugins/search/src/components/SearchResultPager/SearchResultPager.tsx new file mode 100644 index 0000000000..97fa0048e3 --- /dev/null +++ b/plugins/search/src/components/SearchResultPager/SearchResultPager.tsx @@ -0,0 +1,64 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Button, makeStyles } from '@material-ui/core'; +import ArrowBackIosIcon from '@material-ui/icons/ArrowBackIos'; +import ArrowForwardIosIcon from '@material-ui/icons/ArrowForwardIos'; +import React from 'react'; +import { useSearch } from '../SearchContext'; + +const useStyles = makeStyles(theme => ({ + root: { + display: 'flex', + justifyContent: 'center', + gap: theme.spacing(2), + marginTop: theme.spacing(1), + marginBottom: theme.spacing(1), + }, +})); + +export const SearchResultPager = () => { + const { fetchNextPage, fetchPreviousPage } = useSearch(); + const classes = useStyles(); + + if (!fetchNextPage && !fetchPreviousPage) { + return <>; + } + + return ( + + ); +}; diff --git a/plugins/search/src/components/SearchResultPager/index.ts b/plugins/search/src/components/SearchResultPager/index.ts new file mode 100644 index 0000000000..39b72ec71f --- /dev/null +++ b/plugins/search/src/components/SearchResultPager/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { SearchResultPager } from './SearchResultPager'; diff --git a/plugins/search/src/components/SearchType/SearchType.test.tsx b/plugins/search/src/components/SearchType/SearchType.test.tsx index f8171b0779..d3792bb9f4 100644 --- a/plugins/search/src/components/SearchType/SearchType.test.tsx +++ b/plugins/search/src/components/SearchType/SearchType.test.tsx @@ -31,7 +31,6 @@ describe('SearchType', () => { term: '', filters: {}, types: [], - page: {}, }; const name = 'field'; diff --git a/plugins/search/src/components/index.tsx b/plugins/search/src/components/index.tsx index 889eb36334..8b6493ae9b 100644 --- a/plugins/search/src/components/index.tsx +++ b/plugins/search/src/components/index.tsx @@ -14,12 +14,13 @@ * limitations under the License. */ +export * from './DefaultResultListItem'; export * from './Filters'; -export * from './SearchFilter'; -export * from './SearchType'; export * from './SearchBar'; +export * from './SearchContext'; +export * from './SearchFilter'; export * from './SearchPage'; export * from './SearchResult'; -export * from './DefaultResultListItem'; +export * from './SearchResultPager'; +export * from './SearchType'; export * from './SidebarSearch'; -export * from './SearchContext'; From 66be8145796626e19b28f580190a6ccfb80f9cb4 Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Thu, 2 Sep 2021 10:17:22 +0200 Subject: [PATCH 10/12] Remove scrolling behavior Signed-off-by: Oliver Sand --- .../src/components/SearchContext/SearchContext.test.tsx | 1 - .../search/src/components/SearchContext/SearchContext.tsx | 6 ------ 2 files changed, 7 deletions(-) diff --git a/plugins/search/src/components/SearchContext/SearchContext.test.tsx b/plugins/search/src/components/SearchContext/SearchContext.test.tsx index d4153e8e4b..6513dc21bb 100644 --- a/plugins/search/src/components/SearchContext/SearchContext.test.tsx +++ b/plugins/search/src/components/SearchContext/SearchContext.test.tsx @@ -43,7 +43,6 @@ describe('SearchContext', () => { beforeEach(() => { query.mockResolvedValue({}); (useApi as jest.Mock).mockReturnValue({ query: query }); - window.scrollTo = jest.fn(); }); afterAll(() => { diff --git a/plugins/search/src/components/SearchContext/SearchContext.tsx b/plugins/search/src/components/SearchContext/SearchContext.tsx index 0badc7fd20..16d1371964 100644 --- a/plugins/search/src/components/SearchContext/SearchContext.tsx +++ b/plugins/search/src/components/SearchContext/SearchContext.tsx @@ -93,11 +93,9 @@ export const SearchContextProvider = ({ !result.loading && !result.error && result.value?.previousPageCursor; const fetchNextPage = useCallback(() => { setPageCursor(result.value?.nextPageCursor); - resetScrollPosition(); }, [result.value?.nextPageCursor]); const fetchPreviousPage = useCallback(() => { setPageCursor(result.value?.previousPageCursor); - resetScrollPosition(); }, [result.value?.previousPageCursor]); useEffect(() => { @@ -131,7 +129,3 @@ export const useSearch = () => { } return context; }; - -function resetScrollPosition() { - window.scrollTo({ top: 0, left: 0, behavior: 'smooth' }); -} From f8546dadbffa9de509426e5d2045d60fca78599f Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Thu, 2 Sep 2021 10:19:08 +0200 Subject: [PATCH 11/12] Force adopters to include the search result pager on their own Signed-off-by: Oliver Sand --- .changeset/search-happy-owls-sneeze.md | 3 +++ .../app/src/components/search/SearchPage.tsx | 2 ++ .../components/SearchResult/SearchResult.tsx | 8 +----- .../SearchResultPager/SearchResultPager.tsx | 5 ++-- plugins/search/src/index.ts | 25 ++++++++++--------- 5 files changed, 21 insertions(+), 22 deletions(-) diff --git a/.changeset/search-happy-owls-sneeze.md b/.changeset/search-happy-owls-sneeze.md index 4740c767d2..1f3cfd5dd1 100644 --- a/.changeset/search-happy-owls-sneeze.md +++ b/.changeset/search-happy-owls-sneeze.md @@ -8,3 +8,6 @@ --- Implement optional `pageCursor` based paging in search. + +To use paging in your app, add a `` to your +`SearchPage.tsx`. diff --git a/packages/app/src/components/search/SearchPage.tsx b/packages/app/src/components/search/SearchPage.tsx index 1c8d750e08..54f59d1a6e 100644 --- a/packages/app/src/components/search/SearchPage.tsx +++ b/packages/app/src/components/search/SearchPage.tsx @@ -21,6 +21,7 @@ import { SearchBar, SearchFilter, SearchResult, + SearchResultPager, SearchType, } from '@backstage/plugin-search'; import { DocsResultListItem } from '@backstage/plugin-techdocs'; @@ -104,6 +105,7 @@ const SearchPage = () => { )} + diff --git a/plugins/search/src/components/SearchResult/SearchResult.tsx b/plugins/search/src/components/SearchResult/SearchResult.tsx index ff01e96e74..34b6840f21 100644 --- a/plugins/search/src/components/SearchResult/SearchResult.tsx +++ b/plugins/search/src/components/SearchResult/SearchResult.tsx @@ -22,7 +22,6 @@ import { import { SearchResult } from '@backstage/search-common'; import React from 'react'; import { useSearch } from '../SearchContext'; -import { SearchResultPager } from '../SearchResultPager'; type Props = { children: (results: { results: SearchResult[] }) => JSX.Element; @@ -49,12 +48,7 @@ export const SearchResultComponent = ({ children }: Props) => { return ; } - return ( - <> - {children({ results: value.results })} - - - ); + return <>{children({ results: value.results })}; }; export { SearchResultComponent as SearchResult }; diff --git a/plugins/search/src/components/SearchResultPager/SearchResultPager.tsx b/plugins/search/src/components/SearchResultPager/SearchResultPager.tsx index 97fa0048e3..f1850772bb 100644 --- a/plugins/search/src/components/SearchResultPager/SearchResultPager.tsx +++ b/plugins/search/src/components/SearchResultPager/SearchResultPager.tsx @@ -23,10 +23,9 @@ import { useSearch } from '../SearchContext'; const useStyles = makeStyles(theme => ({ root: { display: 'flex', - justifyContent: 'center', + justifyContent: 'space-between', gap: theme.spacing(2), - marginTop: theme.spacing(1), - marginBottom: theme.spacing(1), + margin: theme.spacing(4), }, })); diff --git a/plugins/search/src/index.ts b/plugins/search/src/index.ts index f09ba739bd..d178329ab4 100644 --- a/plugins/search/src/index.ts +++ b/plugins/search/src/index.ts @@ -15,25 +15,26 @@ */ export { searchApiRef } from './apis'; -export { - searchPlugin, - searchPlugin as plugin, - SearchPage, - SearchPageNext, - SearchBarNext, - SearchResult, - DefaultResultListItem, -} from './plugin'; export { Filters, FiltersButton, SearchBar, SearchContextProvider, - useSearch, - SearchPage as Router, SearchFilter, - SearchType, SearchFilterNext, + SearchPage as Router, + SearchResultPager, + SearchType, SidebarSearch, + useSearch, } from './components'; export type { FiltersState } from './components'; +export { + DefaultResultListItem, + SearchBarNext, + SearchPage, + SearchPageNext, + searchPlugin as plugin, + searchPlugin, + SearchResult, +} from './plugin'; From dba58584c381bab05aa6dd9c6985f06e591c7466 Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Thu, 2 Sep 2021 10:50:39 +0200 Subject: [PATCH 12/12] Update api report Signed-off-by: Oliver Sand --- plugins/search/api-report.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/plugins/search/api-report.md b/plugins/search/api-report.md index 2cdac79cad..03c24609e2 100644 --- a/plugins/search/api-report.md +++ b/plugins/search/api-report.md @@ -143,6 +143,11 @@ export const SearchResult: ({ children: (results: { results: SearchResult_2[] }) => JSX.Element; }) => JSX.Element; +// Warning: (ae-missing-release-tag) "SearchResultPager" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export const SearchResultPager: () => JSX.Element; + // Warning: (ae-forgotten-export) The symbol "SearchTypeProps" needs to be exported by the entry point index.d.ts // Warning: (ae-missing-release-tag) "SearchType" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) //