From 8866b62f3ddb2a9e1cd842feea7002b30b5276bb Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Thu, 18 Nov 2021 17:42:17 +0000 Subject: [PATCH 1/4] Fail when adding duplicate locations using the dry run flag This change here adds in logic to the DefaultLocationService that will prematurely terminate the dry run addition of locations if there is a duplicate entity in the unprocessed entities. Adding this logic here will avoid an infinite loop if a monorepo arch is used (for example if a target references itself). It will also enforce some best practices. I have also added tests. You can test this out using this catalog-info.yaml: https://github.com/RoadieHQ/monorepo-infinte-loop/blob/main/catalog-info.yaml Signed-off-by: Nicolas Arnold --- .changeset/thirty-fireants-occur.md | 5 ++ .../service/DefaultLocationService.test.ts | 51 +++++++++++++++++++ .../src/service/DefaultLocationService.ts | 50 +++++++++++------- 3 files changed, 88 insertions(+), 18 deletions(-) create mode 100644 .changeset/thirty-fireants-occur.md diff --git a/.changeset/thirty-fireants-occur.md b/.changeset/thirty-fireants-occur.md new file mode 100644 index 0000000000..dc9348ac6e --- /dev/null +++ b/.changeset/thirty-fireants-occur.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Detect a duplicate entities when adding locations through dry run diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts index a089989d20..e2b82ad2c0 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts @@ -148,6 +148,57 @@ describe('DefaultLocationServiceTest', () => { expect(result.exists).toBe(true); }); + it('should fail when there are duplicate entities using dry run', async () => { + store.listLocations.mockResolvedValueOnce([]); + orchestrator.process.mockResolvedValueOnce({ + ok: true, + state: {}, + completedEntity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Location', + metadata: { + name: 'foo', + }, + }, + deferredEntities: [ + { + entity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Location', + metadata: { + name: 'foo', + }, + }, + locationKey: 'file:///tmp/mock.yaml', + }, + ], + relations: [], + errors: [], + }); + + orchestrator.process.mockResolvedValueOnce({ + ok: true, + state: {}, + completedEntity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Location', + metadata: { + name: 'foo', + }, + }, + deferredEntities: [], + relations: [], + errors: [], + }); + + await expect( + locationService.createLocation( + { type: 'url', target: 'https://backstage.io/catalog-info.yaml' }, + true, + ), + ).rejects.toThrowError('Duplicate nested entity: foo'); + }); + it('should return exists false when the location does not exist beforehand', async () => { orchestrator.process.mockResolvedValueOnce({ ok: true, diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.ts b/plugins/catalog-backend/src/service/DefaultLocationService.ts index 2166341935..d339064d33 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.ts @@ -54,6 +54,34 @@ export class DefaultLocationService implements LocationService { return this.store.deleteLocation(id); } + private async getUnprocessedEntities( + unprocessed: DeferredEntity[], + entities: Entity[], + ): Promise { + const currentEntity = unprocessed.pop(); + if (!currentEntity) { + return []; + } + const processed = await this.orchestrator.process({ + entity: currentEntity.entity, + state: {}, // we process without the existing cache + }); + + if (processed.ok) { + const { metadata, kind } = processed.completedEntity; + if ( + entities.some(e => e.metadata.name === metadata.name && e.kind === kind) + ) { + throw new Error(`Duplicate nested entity: ${metadata.name}`); + } + return await this.getUnprocessedEntities( + [...unprocessed, ...processed.deferredEntities], + [...entities, processed.completedEntity], + ); + } + throw Error(processed.errors.map(String).join(', ')); + } + private async dryRunCreateLocation( spec: LocationSpec, ): Promise<{ location: Location; entities: Entity[]; exists?: boolean }> { @@ -86,24 +114,10 @@ export class DefaultLocationService implements LocationService { const unprocessedEntities: DeferredEntity[] = [ { entity, locationKey: `${spec.type}:${spec.target}` }, ]; - const entities: Entity[] = []; - while (unprocessedEntities.length) { - const currentEntity = unprocessedEntities.pop(); - if (!currentEntity) { - continue; - } - const processed = await this.orchestrator.process({ - entity: currentEntity.entity, - state: {}, // we process without the existing cache - }); - - if (processed.ok) { - unprocessedEntities.push(...processed.deferredEntities); - entities.push(processed.completedEntity); - } else { - throw Error(processed.errors.map(String).join(', ')); - } - } + const entities: Entity[] = await this.getUnprocessedEntities( + unprocessedEntities, + [], + ); return { exists: await existsPromise, From 3c6141c37ab0af1cba9d7a803b0d058b33370f9d Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Fri, 19 Nov 2021 11:09:37 +0000 Subject: [PATCH 2/4] Fix return type Signed-off-by: Nicolas Arnold --- plugins/catalog-backend/src/service/DefaultLocationService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.ts b/plugins/catalog-backend/src/service/DefaultLocationService.ts index d339064d33..0e5f2380f8 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.ts @@ -60,7 +60,7 @@ export class DefaultLocationService implements LocationService { ): Promise { const currentEntity = unprocessed.pop(); if (!currentEntity) { - return []; + return entities; } const processed = await this.orchestrator.process({ entity: currentEntity.entity, From f9c0ad85d9741cb90b57d668577bb93a69a7f171 Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Mon, 22 Nov 2021 09:12:38 +0000 Subject: [PATCH 3/4] Addressing comments Signed-off-by: Nicolas Arnold --- .../src/service/DefaultLocationService.ts | 55 ++++++++++--------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.ts b/plugins/catalog-backend/src/service/DefaultLocationService.ts index 0e5f2380f8..d28112eda3 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.ts @@ -54,32 +54,36 @@ export class DefaultLocationService implements LocationService { return this.store.deleteLocation(id); } - private async getUnprocessedEntities( - unprocessed: DeferredEntity[], - entities: Entity[], + private async processEntities( + unprocessedEntities: DeferredEntity[], ): Promise { - const currentEntity = unprocessed.pop(); - if (!currentEntity) { - return entities; - } - const processed = await this.orchestrator.process({ - entity: currentEntity.entity, - state: {}, // we process without the existing cache - }); - - if (processed.ok) { - const { metadata, kind } = processed.completedEntity; - if ( - entities.some(e => e.metadata.name === metadata.name && e.kind === kind) - ) { - throw new Error(`Duplicate nested entity: ${metadata.name}`); + const entities: Entity[] = []; + while (unprocessedEntities.length) { + const currentEntity = unprocessedEntities.pop(); + if (!currentEntity) { + continue; + } + const processed = await this.orchestrator.process({ + entity: currentEntity.entity, + state: {}, // we process without the existing cache + }); + + if (processed.ok) { + const { metadata, kind } = processed.completedEntity; + if ( + entities.some( + e => e.metadata.name === metadata.name && e.kind === kind, + ) + ) { + throw new Error(`Duplicate nested entity: ${metadata.name}`); + } + unprocessedEntities.push(...processed.deferredEntities); + entities.push(processed.completedEntity); + } else { + throw Error(processed.errors.map(String).join(', ')); } - return await this.getUnprocessedEntities( - [...unprocessed, ...processed.deferredEntities], - [...entities, processed.completedEntity], - ); } - throw Error(processed.errors.map(String).join(', ')); + return entities; } private async dryRunCreateLocation( @@ -114,10 +118,7 @@ export class DefaultLocationService implements LocationService { const unprocessedEntities: DeferredEntity[] = [ { entity, locationKey: `${spec.type}:${spec.target}` }, ]; - const entities: Entity[] = await this.getUnprocessedEntities( - unprocessedEntities, - [], - ); + const entities: Entity[] = await this.processEntities(unprocessedEntities); return { exists: await existsPromise, From f658a341783ba4507b215c9dd77ffc3b75e90a02 Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Wed, 24 Nov 2021 09:31:12 +0000 Subject: [PATCH 4/4] Using stringEntityRef for comparision Signed-off-by: Nicolas Arnold --- .../src/service/DefaultLocationService.test.ts | 2 +- .../src/service/DefaultLocationService.ts | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts index e2b82ad2c0..562bae4867 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts @@ -196,7 +196,7 @@ describe('DefaultLocationServiceTest', () => { { type: 'url', target: 'https://backstage.io/catalog-info.yaml' }, true, ), - ).rejects.toThrowError('Duplicate nested entity: foo'); + ).rejects.toThrowError('Duplicate nested entity: location:default/foo'); }); it('should return exists false when the location does not exist beforehand', async () => { diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.ts b/plugins/catalog-backend/src/service/DefaultLocationService.ts index d28112eda3..8dd2274842 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.ts @@ -19,6 +19,7 @@ import { LocationSpec, LOCATION_ANNOTATION, ORIGIN_LOCATION_ANNOTATION, + stringifyEntityRef, } from '@backstage/catalog-model'; import { CatalogProcessingOrchestrator, @@ -69,13 +70,18 @@ export class DefaultLocationService implements LocationService { }); if (processed.ok) { - const { metadata, kind } = processed.completedEntity; if ( entities.some( - e => e.metadata.name === metadata.name && e.kind === kind, + e => + stringifyEntityRef(e) === + stringifyEntityRef(processed.completedEntity), ) ) { - throw new Error(`Duplicate nested entity: ${metadata.name}`); + throw new Error( + `Duplicate nested entity: ${stringifyEntityRef( + processed.completedEntity, + )}`, + ); } unprocessedEntities.push(...processed.deferredEntities); entities.push(processed.completedEntity);