From e6231859bccec657d12694ab139c9908cd595c40 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 26 Apr 2023 23:50:03 +0200 Subject: [PATCH 1/5] catalog-backend: fix filters clashing with pagination clause Signed-off-by: Vincenzo Scamporlino --- .../service/DefaultEntitiesCatalog.test.ts | 86 ++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index 08ae295834..18796ab24f 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -1418,6 +1418,87 @@ describe('DefaultEntitiesCatalog', () => { }, ); + it.each(databases.eachSupportedId())( + 'should exclude filtered entities when paginating, %p', + async databaseId => { + await createDatabase(databaseId); + + await Promise.all([ + addEntityToSearch(entityFrom('AA', { uid: '1' })), + addEntityToSearch( + entityFrom('AA', { + namespace: 'namespace2', + kind: 'included', + uid: '2', + }), + ), + addEntityToSearch( + entityFrom('AA', { + namespace: 'ns', + kind: 'excluded', + uid: '3', + }), + ), + addEntityToSearch( + entityFrom('AA', { + namespace: 'namespace3', + uid: '4', + kind: 'included', + }), + ), + addEntityToSearch( + entityFrom('AA', { + namespace: 'namespace4', + uid: '5', + kind: 'included', + }), + ), + addEntityToSearch(entityFrom('CC', { uid: '6', kind: 'included' })), + addEntityToSearch(entityFrom('DD', { uid: '7', kind: 'included' })), + ]); + + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); + + const limit = 2; + + // initial request + const request1: QueryEntitiesInitialRequest = { + limit, + filter: { + key: 'kind', + values: ['included'], + }, + orderFields: [{ field: 'metadata.name', order: 'asc' }], + }; + const response1 = await catalog.queryEntities(request1); + expect(response1.items).toMatchObject([ + entityFrom('AA', { uid: '1' }), + entityFrom('AA', { uid: '2' }), + ]); + expect(response1.pageInfo.nextCursor).toBeDefined(); + expect(response1.pageInfo.prevCursor).toBeUndefined(); + expect(response1.totalItems).toBe(6); + + // second request (forward) + const request2: QueryEntitiesCursorRequest = { + cursor: response1.pageInfo.nextCursor!, + limit, + }; + const response2 = await catalog.queryEntities(request2); + expect(response2.items).toMatchObject([ + entityFrom('AA', { uid: '4' }), + entityFrom('AA', { uid: '5' }), + ]); + expect(response2.pageInfo.nextCursor).toBeDefined(); + expect(response2.pageInfo.prevCursor).toBeDefined(); + expect(response2.totalItems).toBe(6); + }, + ); + it.each(databases.eachSupportedId())( 'should paginate results without sort fields, %p', async databaseId => { @@ -1754,11 +1835,12 @@ function entityFrom( uid, namespace, title, - }: { uid?: string; namespace?: string; title?: string } = {}, + kind = 'k', + }: { uid?: string; namespace?: string; title?: string; kind?: string } = {}, ) { return { apiVersion: 'a', - kind: 'k', + kind, metadata: { name, ...(!!namespace && { namespace }), From 2b99c76ef5b0a975766c44bcb85f658bcb9ef15d Mon Sep 17 00:00:00 2001 From: Ke Ma Date: Tue, 25 Apr 2023 13:36:53 +0200 Subject: [PATCH 2/5] fix: db query Signed-off-by: Ke Ma --- .../src/service/DefaultEntitiesCatalog.ts | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index ba165f96af..5fca950cac 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -413,17 +413,18 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { const isOrderingDescending = sortField.order === 'desc'; if (prevItemOrderFieldValue) { - dbQuery.andWhere( - 'value', - isFetchingBackwards !== isOrderingDescending ? '<' : '>', - prevItemOrderFieldValue, - ); - dbQuery.orWhere(function nested() { - this.where('value', '=', prevItemOrderFieldValue).andWhere( - 'search.entity_id', + dbQuery.andWhere(function nested() { + this.where( + 'value', isFetchingBackwards !== isOrderingDescending ? '<' : '>', - prevItemUid, - ); + prevItemOrderFieldValue, + ) + .orWhere('value', '=', prevItemOrderFieldValue) + .andWhere( + 'search.entity_id', + isFetchingBackwards !== isOrderingDescending ? '<' : '>', + prevItemUid, + ); }); } From 3587a968dcd78cfc0871ea57d2f5b504fc4fd552 Mon Sep 17 00:00:00 2001 From: Ke Ma Date: Tue, 25 Apr 2023 14:39:44 +0200 Subject: [PATCH 3/5] add changelog Signed-off-by: Ke Ma --- .changeset/fifty-grapes-explode.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fifty-grapes-explode.md diff --git a/.changeset/fifty-grapes-explode.md b/.changeset/fifty-grapes-explode.md new file mode 100644 index 0000000000..e717340c03 --- /dev/null +++ b/.changeset/fifty-grapes-explode.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Fix a bug in an SQL query where the AND and OR logic is incorrect. From b2d1bdb8026f58f9ff40547cfe1f10ee41a703e7 Mon Sep 17 00:00:00 2001 From: Mark Date: Thu, 27 Apr 2023 09:23:25 +0200 Subject: [PATCH 4/5] Update .changeset/fifty-grapes-explode.md Co-authored-by: Vincenzo Scamporlino Signed-off-by: Mark --- .changeset/fifty-grapes-explode.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/fifty-grapes-explode.md b/.changeset/fifty-grapes-explode.md index e717340c03..999a105c4f 100644 --- a/.changeset/fifty-grapes-explode.md +++ b/.changeset/fifty-grapes-explode.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend': patch --- -Fix a bug in an SQL query where the AND and OR logic is incorrect. +Fixed a bug in the `queryEntities` endpoint that was causing filtered entities to be included in cursor requests. From 243a4bcbceeb036123c7df272e512b83ad475d86 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 27 Apr 2023 15:07:12 +0200 Subject: [PATCH 5/5] catalog-backend: add missing kind Signed-off-by: Vincenzo Scamporlino --- .../src/service/DefaultEntitiesCatalog.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index 18796ab24f..93a37fb04a 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -1424,7 +1424,7 @@ describe('DefaultEntitiesCatalog', () => { await createDatabase(databaseId); await Promise.all([ - addEntityToSearch(entityFrom('AA', { uid: '1' })), + addEntityToSearch(entityFrom('AA', { uid: '1', kind: 'included' })), addEntityToSearch( entityFrom('AA', { namespace: 'namespace2', @@ -1476,8 +1476,8 @@ describe('DefaultEntitiesCatalog', () => { }; const response1 = await catalog.queryEntities(request1); expect(response1.items).toMatchObject([ - entityFrom('AA', { uid: '1' }), - entityFrom('AA', { uid: '2' }), + entityFrom('AA', { uid: '1', kind: 'included' }), + entityFrom('AA', { uid: '2', kind: 'included' }), ]); expect(response1.pageInfo.nextCursor).toBeDefined(); expect(response1.pageInfo.prevCursor).toBeUndefined(); @@ -1490,8 +1490,8 @@ describe('DefaultEntitiesCatalog', () => { }; const response2 = await catalog.queryEntities(request2); expect(response2.items).toMatchObject([ - entityFrom('AA', { uid: '4' }), - entityFrom('AA', { uid: '5' }), + entityFrom('AA', { uid: '4', kind: 'included' }), + entityFrom('AA', { uid: '5', kind: 'included' }), ]); expect(response2.pageInfo.nextCursor).toBeDefined(); expect(response2.pageInfo.prevCursor).toBeDefined();