From 2977e34ed5bc88cf5ae42a1f0e8fca1129d4b854 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 2 Jan 2023 14:15:08 -0600 Subject: [PATCH] catalog-backend: log in case of multiple sortfields are passed Signed-off-by: Vincenzo Scamporlino --- .../src/service/CatalogBuilder.ts | 7 +- .../service/DefaultEntitiesCatalog.test.ts | 139 ++++++++++++++---- .../src/service/DefaultEntitiesCatalog.ts | 22 ++- 3 files changed, 127 insertions(+), 41 deletions(-) diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index 48bf6a08ef..3dfdec09e5 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -453,10 +453,11 @@ export class CatalogBuilder { legacySingleProcessorValidation: this.legacySingleProcessorValidation, }); const stitcher = new Stitcher(dbClient, logger); - const unauthorizedEntitiesCatalog = new DefaultEntitiesCatalog( - dbClient, + const unauthorizedEntitiesCatalog = new DefaultEntitiesCatalog({ + database: dbClient, + logger, stitcher, - ); + }); let permissionEvaluator: PermissionEvaluator; if ('authorizeConditional' in permissions) { diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index bedf856ff7..48ac1c4b0c 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { getVoidLogger } from '@backstage/backend-common'; import { TestDatabaseId, TestDatabases } from '@backstage/backend-test-utils'; import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; import { Knex } from 'knex'; @@ -152,7 +153,11 @@ describe('DefaultEntitiesCatalog', () => { await addEntity(parent, [{ entity: grandparent }]); await addEntity(root, [{ entity: parent }]); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const result = await catalog.entityAncestry('k:default/root'); expect(result.rootEntityRef).toEqual('k:default/root'); @@ -182,7 +187,11 @@ describe('DefaultEntitiesCatalog', () => { 'should throw error if the entity does not exist, %p', async databaseId => { await createDatabase(databaseId); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); await expect(() => catalog.entityAncestry('k:default/root'), ).rejects.toThrow('No such entity k:default/root'); @@ -225,7 +234,11 @@ describe('DefaultEntitiesCatalog', () => { await addEntity(parent2, [{ entity: grandparent }]); await addEntity(root, [{ entity: parent1 }, { entity: parent2 }]); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const result = await catalog.entityAncestry('k:default/root'); expect(result.rootEntityRef).toEqual('k:default/root'); @@ -281,7 +294,11 @@ describe('DefaultEntitiesCatalog', () => { }; await addEntityToSearch(entity1); await addEntityToSearch(entity2); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const testFilter = { key: 'spec.test', @@ -315,7 +332,11 @@ describe('DefaultEntitiesCatalog', () => { }; await addEntityToSearch(entity1); await addEntityToSearch(entity2); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const testFilter = { not: { @@ -363,7 +384,11 @@ describe('DefaultEntitiesCatalog', () => { await addEntityToSearch(entity2); await addEntityToSearch(entity3); await addEntityToSearch(entity4); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const testFilter1 = { key: 'metadata.org', @@ -419,7 +444,11 @@ describe('DefaultEntitiesCatalog', () => { }; await addEntityToSearch(entity1); await addEntityToSearch(entity2); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const testFilter1 = { key: 'metadata.org', @@ -462,7 +491,11 @@ describe('DefaultEntitiesCatalog', () => { }; await addEntityToSearch(entity1); await addEntityToSearch(entity2); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const testFilter = { key: 'kind', @@ -505,7 +538,11 @@ describe('DefaultEntitiesCatalog', () => { }, [], ); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const { entities } = await catalog.entities(); @@ -716,7 +753,11 @@ describe('DefaultEntitiesCatalog', () => { entities.concat(notFoundEntities).map(e => addEntityToSearch(e)), ); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const filter = { key: 'spec.should_include_this', @@ -866,7 +907,11 @@ describe('DefaultEntitiesCatalog', () => { entities.concat(notFoundEntities).map(e => addEntityToSearch(e)), ); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const filter = { key: 'spec.should_include_this', @@ -1017,7 +1062,11 @@ describe('DefaultEntitiesCatalog', () => { entities.concat(notFoundEntities).map(e => addEntityToSearch(e)), ); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const filter = { key: 'spec.should_include_this', @@ -1059,7 +1108,11 @@ describe('DefaultEntitiesCatalog', () => { ), ); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); const request: PaginatedEntitiesInitialRequest = { limit: 0, @@ -1092,7 +1145,10 @@ describe('DefaultEntitiesCatalog', () => { addEntityToSearch(entityFrom('DD')), ]); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + }); const limit = 2; @@ -1191,7 +1247,10 @@ describe('DefaultEntitiesCatalog', () => { addEntityToSearch(entityFrom('DD'), 'id3'), ]); - const catalog = new DefaultEntitiesCatalog(knex); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + }); const limit = 2; @@ -1320,20 +1379,26 @@ describe('DefaultEntitiesCatalog', () => { await addEntity(unrelated1, []); await addEntity(unrelated2, []); await knex('refresh_state').update({ result_hash: 'not-changed' }); - await knex('relations').insert({ - originating_entity_id: uid, - type: 't', - source_entity_ref: 'k:default/root', - target_entity_ref: 'k:default/unrelated1', - }); - await knex('relations').insert({ - originating_entity_id: uid, - type: 't', - source_entity_ref: 'k:default/unrelated2', - target_entity_ref: 'k:default/root', - }); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + // TODO(vinzscam): check whether this is needed + // await knex('relations').insert({ + // originating_entity_id: uid, + // type: 't', + // source_entity_ref: 'k:default/root', + // target_entity_ref: 'k:default/unrelated1', + // }); + // await knex('relations').insert({ + // originating_entity_id: uid, + // type: 't', + // source_entity_ref: 'k:default/unrelated2', + // target_entity_ref: 'k:default/root', + // }); + + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); await catalog.removeEntityByUid(uid); await expect( @@ -1380,7 +1445,11 @@ describe('DefaultEntitiesCatalog', () => { metadata: { name: 'two' }, spec: {}, }); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); await expect(catalog.facets({ facets: ['kind'] })).resolves.toEqual({ facets: { @@ -1419,7 +1488,11 @@ describe('DefaultEntitiesCatalog', () => { }, spec: {}, }); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); await expect( catalog.facets({ @@ -1464,7 +1537,11 @@ describe('DefaultEntitiesCatalog', () => { }, spec: {}, }); - const catalog = new DefaultEntitiesCatalog(knex, stitcher); + const catalog = new DefaultEntitiesCatalog({ + database: knex, + logger: getVoidLogger(), + stitcher, + }); await expect( catalog.facets({ diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index ba68fb9a0e..25d232958b 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -21,7 +21,8 @@ import { } from '@backstage/catalog-model'; import { InputError, NotFoundError } from '@backstage/errors'; import { Knex } from 'knex'; -import { isEqual } from 'lodash'; +import { isEqual, chunk as lodashChunk } from 'lodash'; +import { Logger } from 'winston'; import { EntitiesBatchRequest, EntitiesBatchResponse, @@ -183,10 +184,15 @@ function parseFilter( } export class DefaultEntitiesCatalog implements EntitiesCatalog { - constructor( - private readonly database: Knex, - private readonly stitcher: Stitcher, - ) {} + private readonly database: Knex; + private readonly logger: Logger; + private readonly stitcher: Stitcher; + + constructor(options: { database: Knex; logger: Logger; stitcher: Stitcher }) { + this.database = options.database; + this.logger = options.logger; + this.stitcher = options.stitcher; + } async entities(request?: EntitiesRequest): Promise { const db = this.database; @@ -298,7 +304,7 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { ): Promise { const lookup = new Map(); - for (const chunk of lodash.chunk(request.entityRefs, 200)) { + for (const chunk of lodashChunk(request.entityRefs, 200)) { let query = this.database('final_entities') .innerJoin('refresh_state', { 'refresh_state.entity_id': 'final_entities.entity_id', @@ -341,7 +347,9 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { const isFetchingBackwards = cursor.isPrevious; - // TODO(vinzscam): log if multiple sort fields are provided + if (cursor.sortFields.length > 1) { + this.logger.warn(`Only one sort field is supported, ignoring the rest`); + } const sortField: EntitySortField = { ...defaultSortField,