Merge pull request #14898 from backstage/freben/validate-some-more

enable multi-processor validation of entity kinds

.changeset/rude-pugs-deny.md

@@ -0,0 +1,27 @@
+---
+'@backstage/plugin-catalog-backend': minor
+---
+
+The catalog no longer stops after the first processor `validateEntityKind`
+method returns `true` when validating entity kind shapes. Instead, it continues
+through all registered processors that have this method, and requires that _at
+least one_ of them returned true.
+
+The old behavior of stopping early made it harder to extend existing core kinds
+with additional fields, since the `BuiltinKindsEntityProcessor` is always
+present at the top of the processing chain and ensures that your additional
+validation code would never be run.
+
+This is technically a breaking change, although it should not affect anybody
+under normal circumstances, except if you had problematic validation code that
+you were unaware that it was not being run. That code may now start to exhibit
+those problems.
+
+If you need to disable this new behavior, `CatalogBuilder` as used in your
+`packages/backend/src/plugins/catalog.ts` file now has a
+`useLegacySingleProcessorValidation()` method to go back to the old behavior.
+
+```diff
+ const builder = await CatalogBuilder.create(env);
++builder.useLegacySingleProcessorValidation();
+```

plugins/catalog-backend/api-report.md

@@ -133,7 +133,7 @@ export class CatalogBuilder {
     ...permissionRules: Array<
       CatalogPermissionRule | Array<CatalogPermissionRule>
     >
-  ): void;
+  ): this;
   addProcessor(
     ...processors: Array<CatalogProcessor | Array<CatalogProcessor>>
   ): CatalogBuilder;
@@ -164,6 +164,7 @@ export class CatalogBuilder {
       errors: Error[];
     }) => Promise<void> | void;
   }): void;
+  useLegacySingleProcessorValidation(): this;
 }
 
 // @alpha

plugins/catalog-backend/src/integration.test.ts

@@ -245,6 +245,7 @@ class TestHarness {
       logger,
       parser: defaultEntityDataParser,
       policy: EntityPolicies.allOf([]),
+      legacySingleProcessorValidation: false,
     });
     const stitcher = new Stitcher(db, logger);
     const catalog = new DefaultEntitiesCatalog(db, stitcher);

plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts

@@ -97,6 +97,7 @@ describe('DefaultCatalogProcessingOrchestrator', () => {
       parser: defaultEntityDataParser,
       policy: EntityPolicies.allOf([]),
       rulesEnforcer: { isAllowed: () => true },
+      legacySingleProcessorValidation: false,
     });
 
     it('runs a minimal processing', async () => {
@@ -190,6 +191,54 @@ describe('DefaultCatalogProcessingOrchestrator', () => {
         ok: true,
       });
     });
+
+    it('runs all processor validations when asked to', async () => {
+      const validate = jest.fn(async () => true);
+      const processor1: Partial<CatalogProcessor> = {
+        validateEntityKind: validate,
+      };
+      const processor2: Partial<CatalogProcessor> = {
+        validateEntityKind: validate,
+      };
+
+      const legacy = new DefaultCatalogProcessingOrchestrator({
+        processors: [
+          processor1 as CatalogProcessor,
+          processor2 as CatalogProcessor,
+        ],
+        integrations: ScmIntegrations.fromConfig(new ConfigReader({})),
+        logger: getVoidLogger(),
+        parser: defaultEntityDataParser,
+        policy: EntityPolicies.allOf([]),
+        rulesEnforcer: { isAllowed: () => true },
+        legacySingleProcessorValidation: true,
+      });
+
+      const modern = new DefaultCatalogProcessingOrchestrator({
+        processors: [
+          processor1 as CatalogProcessor,
+          processor2 as CatalogProcessor,
+        ],
+        integrations: ScmIntegrations.fromConfig(new ConfigReader({})),
+        logger: getVoidLogger(),
+        parser: defaultEntityDataParser,
+        policy: EntityPolicies.allOf([]),
+        rulesEnforcer: { isAllowed: () => true },
+        legacySingleProcessorValidation: false,
+      });
+
+      await expect(legacy.process({ entity })).resolves.toMatchObject({
+        ok: true,
+      });
+      expect(validate).toHaveBeenCalledTimes(1);
+
+      validate.mockClear();
+
+      await expect(modern.process({ entity })).resolves.toMatchObject({
+        ok: true,
+      });
+      expect(validate).toHaveBeenCalledTimes(2);
+    });
   });
 
   describe('rules', () => {
@@ -231,6 +280,7 @@ describe('DefaultCatalogProcessingOrchestrator', () => {
         parser,
         policy: EntityPolicies.allOf([]),
         rulesEnforcer,
+        legacySingleProcessorValidation: false,
       });
 
       rulesEnforcer.isAllowed.mockReturnValueOnce(true);
@@ -272,6 +322,7 @@ describe('DefaultCatalogProcessingOrchestrator', () => {
         parser,
         policy: EntityPolicies.allOf([new FailingEntityPolicy()]),
         rulesEnforcer,
+        legacySingleProcessorValidation: false,
       });
 
       await expect(

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

@@ -76,6 +76,7 @@ export class DefaultCatalogProcessingOrchestrator
       parser: CatalogProcessorParser;
       policy: EntityPolicy;
       rulesEnforcer: CatalogRulesEnforcer;
+      legacySingleProcessorValidation: boolean;
     },
   ) {}
 
@@ -253,19 +254,17 @@ export class DefaultCatalogProcessingOrchestrator
       );
     }
 
-    let foundKind = false;
+    let valid = false;
 
     for (const processor of this.options.processors) {
       if (processor.validateEntityKind) {
         try {
-          foundKind = await processor.validateEntityKind(entity);
-          if (foundKind) {
-            // TODO(freben): It would make sense to keep running, so that
-            // multiple processors could have a go at making checks. For
-            // example, an org may want to add additional rules on top of the
-            // provided ones. But that would be a breaking change, so we'll
-            // postpone that to a future processors rewrite.
-            break;
+          const thisValid = await processor.validateEntityKind(entity);
+          if (thisValid) {
+            valid = true;
+            if (this.options.legacySingleProcessorValidation) {
+              break;
+            }
           }
         } catch (e) {
           throw new InputError(
@@ -276,7 +275,7 @@ export class DefaultCatalogProcessingOrchestrator
       }
     }
 
-    if (!foundKind) {
+    if (!valid) {
       throw new InputError(
         `No processor recognized the entity ${context.entityRef} as valid, possibly caused by a foreign kind or apiVersion`,
       );

plugins/catalog-backend/src/service/CatalogBuilder.ts

@@ -154,6 +154,7 @@ export class CatalogBuilder {
   private locationAnalyzer: LocationAnalyzer | undefined = undefined;
   private readonly permissionRules: CatalogPermissionRule[];
   private allowedLocationType: string[];
+  private legacySingleProcessorValidation = false;
 
   /**
    * Creates a catalog builder.
@@ -384,6 +385,7 @@ export class CatalogBuilder {
     >
   ) {
     this.permissionRules.push(...permissionRules.flat());
+    return this;
   }
 
   /**
@@ -396,6 +398,15 @@ export class CatalogBuilder {
     return this;
   }
 
+  /**
+   * Enables the legacy behaviour of canceling validation early whenever only a
+   * single processor declares an entity kind to be valid.
+   */
+  useLegacySingleProcessorValidation(): this {
+    this.legacySingleProcessorValidation = true;
+    return this;
+  }
+
   /**
    * Wires up and returns all of the component parts of the catalog
    */
@@ -429,6 +440,7 @@ export class CatalogBuilder {
       logger,
       parser,
       policy,
+      legacySingleProcessorValidation: this.legacySingleProcessorValidation,
     });
     const stitcher = new Stitcher(dbClient, logger);
     const unauthorizedEntitiesCatalog = new DefaultEntitiesCatalog(