From 679f7c5e95be93fc639b3aa9cd3677a29de6a7bb Mon Sep 17 00:00:00 2001
From: "TANGUY Antoine (SIB)" <tanguyantoine@users.noreply.github.com>
Date: Tue, 16 Aug 2022 16:51:29 +0200
Subject: [PATCH] fix(catalog-backend): add entity ref within errors

Signed-off-by: TANGUY Antoine (SIB) <tanguyantoine@users.noreply.github.com>
---
 .changeset/small-lemons-brake.md              |  5 ++
 ...faultCatalogProcessingOrchestrator.test.ts | 76 +++++++++++++++----
 .../DefaultCatalogProcessingOrchestrator.ts   | 11 ++-
 3 files changed, 75 insertions(+), 17 deletions(-)
 create mode 100644 .changeset/small-lemons-brake.md

diff --git a/.changeset/small-lemons-brake.md b/.changeset/small-lemons-brake.md
new file mode 100644
index 0000000000..ac7906fc28
--- /dev/null
+++ b/.changeset/small-lemons-brake.md
@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend': patch
+---
+
+Include entity ref into error message when catalog policies fail
diff --git a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts
index 30c668bb38..cbb130d138 100644
--- a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts
+++ b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.test.ts
@@ -20,6 +20,7 @@ import {
   ANNOTATION_ORIGIN_LOCATION,
   Entity,
   EntityPolicies,
+  EntityPolicy,
   LocationEntity,
 } from '@backstage/catalog-model';
 import { ScmIntegrations } from '@backstage/integration';
@@ -35,6 +36,7 @@ import { CatalogRulesEnforcer } from '../ingestion/CatalogRules';
 import { DefaultCatalogProcessingOrchestrator } from './DefaultCatalogProcessingOrchestrator';
 import { defaultEntityDataParser } from '../modules/util/parse';
 import { ConfigReader } from '@backstage/config';
+import { InputError } from '@backstage/errors';
 
 class FooBarProcessor implements CatalogProcessor {
   getProcessorName = () => 'foo-bar';
@@ -191,23 +193,23 @@ describe('DefaultCatalogProcessingOrchestrator', () => {
   });
 
   describe('rules', () => {
-    it('enforces catalog rules', async () => {
-      const entity: LocationEntity = {
-        apiVersion: 'backstage.io/v1beta1',
-        kind: 'Location',
-        metadata: {
-          name: 'l',
-          annotations: {
-            [ANNOTATION_ORIGIN_LOCATION]: 'url:https://example.com/origin.yaml',
-            [ANNOTATION_LOCATION]: 'url:https://example.com/origin.yaml',
-          },
+    const entity: LocationEntity = {
+      apiVersion: 'backstage.io/v1beta1',
+      kind: 'Location',
+      metadata: {
+        name: 'l',
+        annotations: {
+          [ANNOTATION_ORIGIN_LOCATION]: 'url:https://example.com/origin.yaml',
+          [ANNOTATION_LOCATION]: 'url:https://example.com/origin.yaml',
         },
-        spec: {
-          type: 'url',
-          target: 'http://example.com/entity.yaml',
-        },
-      };
+      },
+      spec: {
+        type: 'url',
+        target: 'http://example.com/entity.yaml',
+      },
+    };
 
+    it('enforces catalog rules', async () => {
       const integrations = ScmIntegrations.fromConfig(new ConfigReader({}));
       const processor: jest.Mocked<CatalogProcessor> = {
         getProcessorName: jest.fn(),
@@ -241,5 +243,49 @@ describe('DefaultCatalogProcessingOrchestrator', () => {
         orchestrator.process({ entity, state: {} }),
       ).resolves.toEqual(expect.objectContaining({ ok: false }));
     });
+
+    it('includes entity ref within error', 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));
+          return true;
+        }),
+      };
+      const parser: CatalogProcessorParser = jest.fn();
+      const rulesEnforcer: jest.Mocked<CatalogRulesEnforcer> = {
+        isAllowed: jest.fn(),
+      };
+
+      class FailingEntityPolicy implements EntityPolicy {
+        async enforce(_entity: Entity): Promise<Entity> {
+          // eslint-disable-next-line no-throw-literal
+          throw 'boom';
+        }
+      }
+      const orchestrator = new DefaultCatalogProcessingOrchestrator({
+        processors: [processor],
+        integrations,
+        logger: getVoidLogger(),
+        parser,
+        policy: EntityPolicies.allOf([new FailingEntityPolicy()]),
+        rulesEnforcer,
+      });
+
+      await expect(
+        orchestrator.process({ entity, state: {} }),
+      ).resolves.toEqual(
+        expect.objectContaining({
+          ok: false,
+          errors: [
+            new InputError(
+              "Policy check failed for location:default/l; caused by unknown error 'boom'",
+            ),
+          ],
+        }),
+      );
+    });
   });
 });
diff --git a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts
index 9effaeb3d9..79876f70d8 100644
--- a/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts
+++ b/plugins/catalog-backend/src/processing/DefaultCatalogProcessingOrchestrator.ts
@@ -211,11 +211,18 @@ export class DefaultCatalogProcessingOrchestrator
     try {
       policyEnforcedEntity = await this.options.policy.enforce(entity);
     } catch (e) {
-      throw new InputError('Policy check failed', e);
+      throw new InputError(
+        `Policy check failed for ${stringifyEntityRef(entity)}`,
+        e,
+      );
     }
 
     if (!policyEnforcedEntity) {
-      throw new Error('Policy unexpectedly returned no data');
+      throw new Error(
+        `Policy unexpectedly returned no data for ${stringifyEntityRef(
+          entity,
+        )}`,
+      );
     }
 
     return policyEnforcedEntity;