diff --git a/.changeset/rude-pugs-deny.md b/.changeset/rude-pugs-deny.md new file mode 100644 index 0000000000..b877d96285 --- /dev/null +++ b/.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(); +``` diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index e5d464c29a..a46abda5d3 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -133,7 +133,7 @@ export class CatalogBuilder { ...permissionRules: Array< CatalogPermissionRule | Array > - ): void; + ): this; addProcessor( ...processors: Array> ): CatalogBuilder; @@ -164,6 +164,7 @@ export class CatalogBuilder { errors: Error[]; }) => Promise | void; }): void; + useLegacySingleProcessorValidation(): this; } // @alpha diff --git a/plugins/catalog-backend/src/integration.test.ts b/plugins/catalog-backend/src/integration.test.ts index dcfe9a1489..a63119bb67 100644 --- a/plugins/catalog-backend/src/integration.test.ts +++ b/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); diff --git a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts index cbb130d138..9731022582 100644 --- a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts +++ b/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 = { + validateEntityKind: validate, + }; + const processor2: Partial = { + 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( diff --git a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts index e95fede8b7..339469ff1d 100644 --- a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts +++ b/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`, ); diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index d5bc7057dd..8aab74c4ba 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/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(