From 53cce8663178091f8ff46c40a48db4e06ebf9159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 22 Aug 2024 16:53:28 +0200 Subject: [PATCH] fix ordered by-query call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/tough-peaches-kneel.md | 5 ++ .../service/DefaultEntitiesCatalog.test.ts | 37 +++++++++++ .../src/service/DefaultEntitiesCatalog.ts | 63 ++++++++++++++----- 3 files changed, 91 insertions(+), 14 deletions(-) create mode 100644 .changeset/tough-peaches-kneel.md diff --git a/.changeset/tough-peaches-kneel.md b/.changeset/tough-peaches-kneel.md new file mode 100644 index 0000000000..7674da8879 --- /dev/null +++ b/.changeset/tough-peaches-kneel.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Fixed an issue with the by-query call, where ordering by a field that does not exist on all entities led to not all results being returned diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index 3c98f2a8bc..92acd9e3ce 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -1648,6 +1648,43 @@ describe('DefaultEntitiesCatalog', () => { expect(response5.totalItems).toBe(6); }, ); + + it.each(databases.eachSupportedId())( + 'should sort properly for fields that do not exist on all entities, %p', + async databaseId => { + await createDatabase(databaseId); + + await Promise.all([ + addEntityToSearch(entityFrom('AA', { uid: 'id1' })), + addEntityToSearch(entityFrom('BB', { uid: 'id2', title: 'YY' })), + addEntityToSearch(entityFrom('CC', { uid: 'id3', title: 'XX' })), + ]); + + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: mockServices.logger.mock(), + stitcher, + }); + + await expect( + catalog + .queryEntities({ + orderFields: [{ field: 'metadata.title', order: 'asc' }], + credentials: mockCredentials.none(), + }) + .then(r => r.items.map(e => e.metadata.name)), + ).resolves.toEqual(['CC', 'BB', 'AA']); // 'AA' has no title, ends up last + + await expect( + catalog + .queryEntities({ + orderFields: [{ field: 'metadata.title', order: 'desc' }], + credentials: mockCredentials.none(), + }) + .then(r => r.items.map(e => e.metadata.name)), + ).resolves.toEqual(['BB', 'CC', 'AA']); // 'AA' has no title, ends up last + }, + ); }); describe('removeEntityByUid', () => { diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index 9049685d38..32abb60674 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -379,12 +379,20 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { const [prevItemOrderFieldValue, prevItemUid] = cursor.orderFieldValues || []; - const dbQuery = db('search') - .join('final_entities', 'search.entity_id', 'final_entities.entity_id') - .where('search.key', sortField.field); + const dbQuery = db('final_entities').leftOuterJoin('search', qb => + qb + .on('search.entity_id', 'final_entities.entity_id') + .andOnVal('search.key', sortField.field), + ); if (cursor.filter) { - parseFilter(cursor.filter, dbQuery, db, false, 'search.entity_id'); + parseFilter( + cursor.filter, + dbQuery, + db, + false, + 'final_entities.entity_id', + ); } const normalizedFullTextFilterTerm = cursor.fullTextFilter?.term?.trim(); @@ -410,7 +418,7 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { `%${normalizedFullTextFilterTerm.toLocaleLowerCase('en-US')}%`, ); }); - dbQuery.andWhere('search.entity_id', 'in', matchQuery); + dbQuery.andWhere('final_entities.entity_id', 'in', matchQuery); } } @@ -427,32 +435,59 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { ) .orWhere('value', '=', prevItemOrderFieldValue) .andWhere( - 'search.entity_id', + 'final_entities.entity_id', isFetchingBackwards !== isOrderingDescending ? '<' : '>', prevItemUid, ); }); } - dbQuery - .orderBy([ + if (db.client.config.client === 'pg') { + // pg correctly orders by the column value and handling nulls in one go + dbQuery.orderBy([ { - column: 'value', + column: 'search.value', + order: isFetchingBackwards + ? invertOrder(sortField.order) + : sortField.order, + nulls: 'last', + }, + { + column: 'final_entities.entity_id', + order: isFetchingBackwards + ? invertOrder(sortField.order) + : sortField.order, + }, + ]); + } else { + // sqlite and mysql translate the above statement ONLY into "order by (value is null) asc" + // no matter what the order is, for some reason, so we have to manually add back the statement + // that translates to "order by value " while avoiding to give an order + dbQuery.orderBy([ + { + column: 'search.value', + order: undefined, + nulls: 'last', + }, + { + column: 'search.value', order: isFetchingBackwards ? invertOrder(sortField.order) : sortField.order, }, { - column: 'search.entity_id', + column: 'final_entities.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); + ]); + } - countQuery.count('search.entity_id', { as: 'count' }); + // fetch an extra item to check if there are more items. + dbQuery.limit(isFetchingBackwards ? limit : limit + 1); + + countQuery.count('final_entities.entity_id', { as: 'count' }); const [rows, [{ count }]] = await Promise.all([ limit > 0 ? dbQuery : [],