diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index af4bacadaa..d5619ec540 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -85,8 +85,7 @@ describe('DefaultEntitiesCatalog', () => { return id; } - async function addEntityToSearch(knex: Knex, entity: Entity) { - const id = uuid(); + async function addEntityToSearch(knex: Knex, entity: Entity, id = uuid()) { const entityRef = stringifyEntityRef(entity); const entityJson = JSON.stringify(entity); @@ -746,7 +745,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response2.entities).toEqual([entityFrom('C'), entityFrom('D')]); expect(response2.nextCursor).toBeDefined(); expect(response2.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response2.totalItems).toBe(names.length); // third request (forward) const request3: PaginatedEntitiesCursorRequest = { @@ -757,7 +756,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response3.entities).toEqual([entityFrom('E'), entityFrom('F')]); expect(response3.nextCursor).toBeDefined(); expect(response3.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response3.totalItems).toBe(names.length); // fourth request (backwards) const request4: PaginatedEntitiesCursorRequest = { @@ -768,7 +767,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response4.entities).toEqual([entityFrom('C'), entityFrom('D')]); expect(response4.nextCursor).toBeDefined(); expect(response4.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response4.totalItems).toBe(names.length); // fifth request (backwards) const request5: PaginatedEntitiesCursorRequest = { @@ -779,7 +778,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response5.entities).toEqual([entityFrom('A'), entityFrom('B')]); expect(response5.nextCursor).toBeDefined(); expect(response5.prevCursor).toBeUndefined(); - expect(response1.totalItems).toBe(names.length); + expect(response5.totalItems).toBe(names.length); // sixth request (forward) const request6: PaginatedEntitiesCursorRequest = { @@ -790,7 +789,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response6.entities).toEqual([entityFrom('C'), entityFrom('D')]); expect(response6.nextCursor).toBeDefined(); expect(response6.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response6.totalItems).toBe(names.length); // seventh request (forward) const request7: PaginatedEntitiesCursorRequest = { @@ -801,7 +800,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response7.entities).toEqual([entityFrom('E'), entityFrom('F')]); expect(response7.nextCursor).toBeDefined(); expect(response7.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response7.totalItems).toBe(names.length); // seventh.2 request (forward with a different limit) const request7bis: PaginatedEntitiesCursorRequest = { @@ -816,7 +815,7 @@ describe('DefaultEntitiesCatalog', () => { ]); expect(response7bis.nextCursor).toBeUndefined(); expect(response7bis.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response7bis.totalItems).toBe(names.length); // last request (forward) const request8: PaginatedEntitiesCursorRequest = { @@ -827,7 +826,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response8.entities).toEqual([entityFrom('G')]); expect(response8.nextCursor).toBeUndefined(); expect(response8.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response8.totalItems).toBe(names.length); }, ); @@ -898,7 +897,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response2.entities).toEqual([entityFrom('E'), entityFrom('D')]); expect(response2.nextCursor).toBeDefined(); expect(response2.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response2.totalItems).toBe(names.length); // third request (forward) const request3: PaginatedEntitiesCursorRequest = { @@ -909,7 +908,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response3.entities).toEqual([entityFrom('C'), entityFrom('B')]); expect(response3.nextCursor).toBeDefined(); expect(response3.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response3.totalItems).toBe(names.length); // fourth request (backwards) const request4: PaginatedEntitiesCursorRequest = { @@ -921,7 +920,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response4.entities).toEqual([entityFrom('E'), entityFrom('D')]); expect(response4.nextCursor).toBeDefined(); expect(response4.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response4.totalItems).toBe(names.length); // fifth request (backwards) const request5: PaginatedEntitiesCursorRequest = { @@ -932,7 +931,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response5.entities).toEqual([entityFrom('G'), entityFrom('F')]); expect(response5.nextCursor).toBeDefined(); expect(response5.prevCursor).toBeUndefined(); - expect(response1.totalItems).toBe(names.length); + expect(response5.totalItems).toBe(names.length); // sixth request (forward) const request6: PaginatedEntitiesCursorRequest = { @@ -943,7 +942,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response6.entities).toEqual([entityFrom('E'), entityFrom('D')]); expect(response6.nextCursor).toBeDefined(); expect(response6.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response6.totalItems).toBe(names.length); // seventh request (forward) const request7: PaginatedEntitiesCursorRequest = { @@ -954,7 +953,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response7.entities).toEqual([entityFrom('C'), entityFrom('B')]); expect(response7.nextCursor).toBeDefined(); expect(response7.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response7.totalItems).toBe(names.length); // seventh.2 request (forward with a different limit) const request7bis: PaginatedEntitiesCursorRequest = { @@ -969,7 +968,7 @@ describe('DefaultEntitiesCatalog', () => { ]); expect(response7bis.nextCursor).toBeUndefined(); expect(response7bis.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response7bis.totalItems).toBe(names.length); // last request (forward) const request8: PaginatedEntitiesCursorRequest = { @@ -980,7 +979,7 @@ describe('DefaultEntitiesCatalog', () => { expect(response8.entities).toEqual([entityFrom('A')]); expect(response8.nextCursor).toBeUndefined(); expect(response8.prevCursor).toBeDefined(); - expect(response1.totalItems).toBe(names.length); + expect(response8.totalItems).toBe(names.length); }, ); @@ -1073,6 +1072,105 @@ describe('DefaultEntitiesCatalog', () => { expect(response).toEqual({ totalItems: 20, entities: [] }); }, ); + + it.each(databases.eachSupportedId())( + 'should paginate results accordingly in case of clashing items, %p', + async databaseId => { + const { knex } = await createDatabase(databaseId); + + function entityFrom(name: string, namespace?: string) { + return { + apiVersion: 'a', + kind: 'k', + metadata: { name, ...(!!namespace && { namespace }) }, + spec: { should_include_this: 'yes' }, + }; + } + + await Promise.all([ + addEntityToSearch(knex, entityFrom('AA')), + addEntityToSearch(knex, entityFrom('AA', 'namespace2')), + addEntityToSearch(knex, entityFrom('AA', 'namespace3')), + addEntityToSearch(knex, entityFrom('AA', 'namespace4')), + addEntityToSearch(knex, entityFrom('CC')), + addEntityToSearch(knex, entityFrom('DD')), + ]); + + const catalog = new DefaultEntitiesCatalog(knex); + + const limit = 2; + + // initial request + const request1: PaginatedEntitiesInitialRequest = { + limit, + sortFields: [{ field: 'metadata.name' }], + }; + const response1 = await catalog.paginatedEntities(request1); + expect(response1.entities).toMatchObject([ + entityFrom('AA'), + entityFrom('AA'), + ]); + expect(response1.nextCursor).toBeDefined(); + expect(response1.prevCursor).toBeUndefined(); + expect(response1.totalItems).toBe(6); + + // second request (forward) + const request2: PaginatedEntitiesCursorRequest = { + cursor: response1.nextCursor!, + limit, + }; + const response2 = await catalog.paginatedEntities(request2); + expect(response2.entities).toMatchObject([ + entityFrom('AA'), + entityFrom('AA'), + ]); + expect(response2.nextCursor).toBeDefined(); + expect(response2.prevCursor).toBeDefined(); + expect(response2.totalItems).toBe(6); + + // third request (forward) + const request3: PaginatedEntitiesCursorRequest = { + cursor: response2.nextCursor!, + limit, + }; + const response3 = await catalog.paginatedEntities(request3); + expect(response3.entities).toEqual([ + entityFrom('CC'), + entityFrom('DD'), + ]); + expect(response3.nextCursor).toBeUndefined(); + expect(response3.prevCursor).toBeDefined(); + expect(response3.totalItems).toBe(6); + + // forth request (backward) + const request4: PaginatedEntitiesCursorRequest = { + cursor: response3.prevCursor!, + limit, + }; + const response4 = await catalog.paginatedEntities(request4); + expect(response4.entities).toMatchObject([ + entityFrom('AA'), + entityFrom('AA'), + ]); + expect(response4.nextCursor).toBeDefined(); + expect(response4.prevCursor).toBeDefined(); + expect(response4.totalItems).toBe(6); + + // fifth request (backward) + const request5: PaginatedEntitiesCursorRequest = { + cursor: response4.prevCursor!, + limit, + }; + const response5 = await catalog.paginatedEntities(request5); + expect(response5.entities).toMatchObject([ + entityFrom('AA'), + entityFrom('AA'), + ]); + expect(response5.nextCursor).toBeDefined(); + expect(response5.prevCursor).toBeUndefined(); + expect(response5.totalItems).toBe(6); + }, + ); }); describe('removeEntityByUid', () => { diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index 022313db7c..ce330eb9c1 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -20,8 +20,8 @@ import { stringifyEntityRef, } from '@backstage/catalog-model'; import { InputError, NotFoundError } from '@backstage/errors'; -import lodash from 'lodash'; import { Knex } from 'knex'; +import { isEqual } from 'lodash'; import { EntitiesBatchRequest, EntitiesBatchResponse, @@ -331,8 +331,9 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { const db = this.database; const limit = request?.limit ?? 20; - const cursor: Omit & { sortFieldIds?: string[] } = { - firstFieldId: '', + const cursor: Omit & { + sortFieldValues?: string[]; + } = { sortFields: [defaultSortField], isPrevious: false, ...parseCursorFromRequest(request), @@ -346,11 +347,11 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { ...cursor.sortFields[0], }; - const sortFieldId = cursor.sortFieldIds?.[0]; + const [sortFieldId, metadataSortFieldId] = cursor.sortFieldValues || []; const dbQuery = db('search') .join('final_entities', 'search.entity_id', 'final_entities.entity_id') - .where('key', sortField.field); + .where('search.key', sortField.field); if (cursor.filter) { parseFilter(cursor.filter, dbQuery, db, false, 'search.entity_id'); @@ -368,18 +369,36 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { const isOrderingDescending = sortField.order === 'desc'; if (sortFieldId) { - dbQuery.andWhere( - 'value', - isFetchingBackwards !== isOrderingDescending ? '<' : '>', - sortFieldId, - ); + dbQuery + .andWhere( + 'value', + isFetchingBackwards !== isOrderingDescending ? '<' : '>', + sortFieldId, + ) + .orWhere(function nested() { + this.where('value', '=', sortFieldId).andWhere( + 'search.entity_id', + isFetchingBackwards !== isOrderingDescending ? '<' : '>', + metadataSortFieldId, + ); + }); } dbQuery - .orderBy( - 'value', - isFetchingBackwards ? invertOrder(sortField.order) : sortField.order, - ) + .orderBy([ + { + column: 'value', + order: isFetchingBackwards + ? invertOrder(sortField.order) + : sortField.order, + }, + { + column: 'search.entity_id', + order: isFetchingBackwards + ? invertOrder(sortField.order) + : sortField.order, + }, + ]) // fetch an extra item to check if there are more items. .limit(isFetchingBackwards ? limit : limit + 1); @@ -409,15 +428,22 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { rows.length -= 1; } - const isInitialRequest = cursor.firstFieldId === ''; + const isInitialRequest = cursor.firstSortFieldValues === undefined; - const firstFieldId = cursor.firstFieldId || rows[0]?.value; + const firstSortFieldValues = cursor.firstSortFieldValues || [ + rows[0]?.value, + rows[0]?.entity_id, + ]; const nextCursor = hasMoreResults ? encodeCursor({ ...cursor, - sortFieldIds: [rows[rows.length - 1].value], - firstFieldId, + sortFieldValues: [ + // TODO generalize + rows[rows.length - 1].value, + rows[rows.length - 1].entity_id, + ], + firstSortFieldValues, isPrevious: false, totalItems, }) @@ -426,11 +452,12 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { const prevCursor = !isInitialRequest && rows.length > 0 && - rows[0].value !== cursor.firstFieldId + !isEqual(sortFieldsFromRow(rows[0]), cursor.firstSortFieldValues) ? encodeCursor({ ...cursor, - sortFieldIds: [rows[0].value], - firstFieldId: cursor.firstFieldId, + // TODO generalize + sortFieldValues: [rows[0].value, rows[0].entity_id], + firstSortFieldValues: cursor.firstSortFieldValues, isPrevious: true, totalItems, }) @@ -652,3 +679,7 @@ function parseCursorFromRequest( function invertOrder(order: EntitySortField['order']) { return order === 'asc' ? 'desc' : 'asc'; } + +function sortFieldsFromRow(row: DbSearchRow) { + return [row.value, row.entity_id]; +}