From b3fac9c107f59e2354fd6b8c57307113540d2579 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Tue, 13 Dec 2022 16:59:29 +0100 Subject: [PATCH 1/2] ignore processors emitting entities that are their own children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/smart-zoos-wash.md | 5 +++++ .../DefaultCatalogProcessingOrchestrator.test.ts | 13 +++++++++++-- .../src/processing/ProcessorOutputCollector.ts | 12 ++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 .changeset/smart-zoos-wash.md diff --git a/.changeset/smart-zoos-wash.md b/.changeset/smart-zoos-wash.md new file mode 100644 index 0000000000..ffb5ca0f01 --- /dev/null +++ b/.changeset/smart-zoos-wash.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Ignore attempts at emitting the current entity as a child of itself. diff --git a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts index 9731022582..ceb6992d7a 100644 --- a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts +++ b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts @@ -258,13 +258,22 @@ describe('DefaultCatalogProcessingOrchestrator', () => { }, }; + const child: Entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'Test2', + namespace: 'test1', + }, + }; + it('enforces catalog rules', async () => { const integrations = ScmIntegrations.fromConfig(new ConfigReader({})); const processor: jest.Mocked = { getProcessorName: jest.fn(), validateEntityKind: jest.fn(async () => true), readLocation: jest.fn(async (_l, _o, emit) => { - emit(processingResult.entity({ type: 't', target: 't' }, entity)); + emit(processingResult.entity({ type: 't', target: 't' }, child)); return true; }), }; @@ -300,7 +309,7 @@ describe('DefaultCatalogProcessingOrchestrator', () => { getProcessorName: jest.fn(), validateEntityKind: jest.fn(async () => true), readLocation: jest.fn(async (_l, _o, emit) => { - emit(processingResult.entity({ type: 't', target: 't' }, entity)); + emit(processingResult.entity({ type: 't', target: 't' }, child)); return true; }), }; diff --git a/plugins/catalog-backend/src/processing/ProcessorOutputCollector.ts b/plugins/catalog-backend/src/processing/ProcessorOutputCollector.ts index 19f3b33df4..55282baa38 100644 --- a/plugins/catalog-backend/src/processing/ProcessorOutputCollector.ts +++ b/plugins/catalog-backend/src/processing/ProcessorOutputCollector.ts @@ -19,6 +19,7 @@ import { ANNOTATION_LOCATION, ANNOTATION_ORIGIN_LOCATION, stringifyLocationRef, + stringifyEntityRef, } from '@backstage/catalog-model'; import { assertError } from '@backstage/errors'; import { Logger } from 'winston'; @@ -89,6 +90,17 @@ export class ProcessorOutputCollector { return; } + // The processor contract says you should return the "trunk" (current) + // entity, not emit it. But it happens that this is misunderstood or + // accidentally forgotten. This can lead to circular references which at + // best is wasteful, so we try to be helpful by ignoring such emitted + // entities. + if ( + stringifyEntityRef(entity) === stringifyEntityRef(this.parentEntity) + ) { + return; + } + // Note that at this point, we have only validated the envelope part of // the entity data. Annotations are not part of that, so we have to be // defensive. If the annotations were malformed (e.g. were not a valid From 648a20fa0ffd87f5da34abb30b90bc2390d3d74c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Tue, 13 Dec 2022 22:16:36 +0100 Subject: [PATCH 2/2] log error too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .../DefaultCatalogProcessingOrchestrator.ts | 8 +++--- .../processing/ProcessorOutputCollector.ts | 28 +++++++++++++------ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts index 339469ff1d..d177a7d4a8 100644 --- a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts +++ b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts @@ -187,7 +187,7 @@ export class DefaultCatalogProcessingOrchestrator res = await processor.preProcessEntity( res, context.location, - context.collector.onEmit, + context.collector.forProcessor(processor), context.originLocation, context.cache.forProcessor(processor), ); @@ -300,7 +300,7 @@ export class DefaultCatalogProcessingOrchestrator for (const maybeRelativeTarget of targets) { if (type === 'file' && maybeRelativeTarget.endsWith(path.sep)) { - context.collector.onEmit( + context.collector.generic()( processingResult.inputError( context.location, `LocationEntityProcessor cannot handle ${type} type location with target ${context.location.target} that ends with a path separator`, @@ -326,7 +326,7 @@ export class DefaultCatalogProcessingOrchestrator presence, }, presence === 'optional', - context.collector.onEmit, + context.collector.forProcessor(processor), this.options.parser, context.cache.forProcessor(processor, target), ); @@ -365,7 +365,7 @@ export class DefaultCatalogProcessingOrchestrator res = await processor.postProcessEntity( res, context.location, - context.collector.onEmit, + context.collector.forProcessor(processor), context.cache.forProcessor(processor), ); } catch (e) { diff --git a/plugins/catalog-backend/src/processing/ProcessorOutputCollector.ts b/plugins/catalog-backend/src/processing/ProcessorOutputCollector.ts index 55282baa38..0ee6191284 100644 --- a/plugins/catalog-backend/src/processing/ProcessorOutputCollector.ts +++ b/plugins/catalog-backend/src/processing/ProcessorOutputCollector.ts @@ -24,6 +24,7 @@ import { import { assertError } from '@backstage/errors'; import { Logger } from 'winston'; import { + CatalogProcessor, CatalogProcessorResult, DeferredEntity, EntityRelationSpec, @@ -51,8 +52,17 @@ export class ProcessorOutputCollector { private readonly parentEntity: Entity, ) {} - get onEmit(): (i: CatalogProcessorResult) => void { - return i => this.receive(i); + generic(): (i: CatalogProcessorResult) => void { + return i => this.receive(this.logger, i); + } + + forProcessor( + processor: CatalogProcessor, + ): (i: CatalogProcessorResult) => void { + const logger = this.logger.child({ + processor: processor.getProcessorName(), + }); + return i => this.receive(logger, i); } results() { @@ -65,9 +75,9 @@ export class ProcessorOutputCollector { }; } - private receive(i: CatalogProcessorResult) { + private receive(logger: Logger, i: CatalogProcessorResult) { if (this.done) { - this.logger.warn( + logger.warn( `Item of type "${ i.type }" was emitted after processing had completed. Stack trace: ${ @@ -85,7 +95,7 @@ export class ProcessorOutputCollector { entity = validateEntityEnvelope(i.entity); } catch (e) { assertError(e); - this.logger.debug(`Envelope validation failed at ${location}, ${e}`); + logger.debug(`Envelope validation failed at ${location}, ${e}`); this.errors.push(e); return; } @@ -95,9 +105,11 @@ export class ProcessorOutputCollector { // accidentally forgotten. This can lead to circular references which at // best is wasteful, so we try to be helpful by ignoring such emitted // entities. - if ( - stringifyEntityRef(entity) === stringifyEntityRef(this.parentEntity) - ) { + const entityRef = stringifyEntityRef(entity); + if (entityRef === stringifyEntityRef(this.parentEntity)) { + logger.warn( + `Ignored emitted entity ${entityRef} whose ref was identical to the one being processed. This commonly indicates mistakenly emitting the input entity instead of returning it.`, + ); return; }