Merge pull request #15212 from backstage/freben/circ-emit

ignore processors emitting entities that are their own children

.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.

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<CatalogProcessor> = {
         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;
         }),
       };

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) {

plugins/catalog-backend/src/processing/ProcessorOutputCollector.ts

@@ -19,10 +19,12 @@ import {
   ANNOTATION_LOCATION,
   ANNOTATION_ORIGIN_LOCATION,
   stringifyLocationRef,
+  stringifyEntityRef,
 } from '@backstage/catalog-model';
 import { assertError } from '@backstage/errors';
 import { Logger } from 'winston';
 import {
+  CatalogProcessor,
   CatalogProcessorResult,
   DeferredEntity,
   EntityRelationSpec,
@@ -50,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() {
@@ -64,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: ${
@@ -84,11 +95,24 @@ 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;
       }
 
+      // 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.
+      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;
+      }
+
       // 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