From 8866b62f3ddb2a9e1cd842feea7002b30b5276bb Mon Sep 17 00:00:00 2001
From: Nicolas Arnold <nic@roadie.io>
Date: Thu, 18 Nov 2021 17:42:17 +0000
Subject: [PATCH] Fail when adding duplicate locations using the dry run flag

This change here adds in logic to the DefaultLocationService that will prematurely terminate the dry run addition of locations if there is a duplicate entity in the unprocessed entities.
Adding this logic here will avoid an infinite loop if a monorepo arch is used (for example if a target references itself). It will also enforce some best practices.
I have also added tests.

You can test this out using this catalog-info.yaml: https://github.com/RoadieHQ/monorepo-infinte-loop/blob/main/catalog-info.yaml

Signed-off-by: Nicolas Arnold <nic@roadie.io>
---
 .changeset/thirty-fireants-occur.md           |  5 ++
 .../service/DefaultLocationService.test.ts    | 51 +++++++++++++++++++
 .../src/service/DefaultLocationService.ts     | 50 +++++++++++-------
 3 files changed, 88 insertions(+), 18 deletions(-)
 create mode 100644 .changeset/thirty-fireants-occur.md

diff --git a/.changeset/thirty-fireants-occur.md b/.changeset/thirty-fireants-occur.md
new file mode 100644
index 0000000000..dc9348ac6e
--- /dev/null
+++ b/.changeset/thirty-fireants-occur.md
@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend': patch
+---
+
+Detect a duplicate entities when adding locations through dry run
diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts
index a089989d20..e2b82ad2c0 100644
--- a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts
+++ b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts
@@ -148,6 +148,57 @@ describe('DefaultLocationServiceTest', () => {
       expect(result.exists).toBe(true);
     });
 
+    it('should fail when there are duplicate entities using dry run', async () => {
+      store.listLocations.mockResolvedValueOnce([]);
+      orchestrator.process.mockResolvedValueOnce({
+        ok: true,
+        state: {},
+        completedEntity: {
+          apiVersion: 'backstage.io/v1alpha1',
+          kind: 'Location',
+          metadata: {
+            name: 'foo',
+          },
+        },
+        deferredEntities: [
+          {
+            entity: {
+              apiVersion: 'backstage.io/v1alpha1',
+              kind: 'Location',
+              metadata: {
+                name: 'foo',
+              },
+            },
+            locationKey: 'file:///tmp/mock.yaml',
+          },
+        ],
+        relations: [],
+        errors: [],
+      });
+
+      orchestrator.process.mockResolvedValueOnce({
+        ok: true,
+        state: {},
+        completedEntity: {
+          apiVersion: 'backstage.io/v1alpha1',
+          kind: 'Location',
+          metadata: {
+            name: 'foo',
+          },
+        },
+        deferredEntities: [],
+        relations: [],
+        errors: [],
+      });
+
+      await expect(
+        locationService.createLocation(
+          { type: 'url', target: 'https://backstage.io/catalog-info.yaml' },
+          true,
+        ),
+      ).rejects.toThrowError('Duplicate nested entity: foo');
+    });
+
     it('should return exists false when the location does not exist beforehand', async () => {
       orchestrator.process.mockResolvedValueOnce({
         ok: true,
diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.ts b/plugins/catalog-backend/src/service/DefaultLocationService.ts
index 2166341935..d339064d33 100644
--- a/plugins/catalog-backend/src/service/DefaultLocationService.ts
+++ b/plugins/catalog-backend/src/service/DefaultLocationService.ts
@@ -54,6 +54,34 @@ export class DefaultLocationService implements LocationService {
     return this.store.deleteLocation(id);
   }
 
+  private async getUnprocessedEntities(
+    unprocessed: DeferredEntity[],
+    entities: Entity[],
+  ): Promise<Entity[]> {
+    const currentEntity = unprocessed.pop();
+    if (!currentEntity) {
+      return [];
+    }
+    const processed = await this.orchestrator.process({
+      entity: currentEntity.entity,
+      state: {}, // we process without the existing cache
+    });
+
+    if (processed.ok) {
+      const { metadata, kind } = processed.completedEntity;
+      if (
+        entities.some(e => e.metadata.name === metadata.name && e.kind === kind)
+      ) {
+        throw new Error(`Duplicate nested entity: ${metadata.name}`);
+      }
+      return await this.getUnprocessedEntities(
+        [...unprocessed, ...processed.deferredEntities],
+        [...entities, processed.completedEntity],
+      );
+    }
+    throw Error(processed.errors.map(String).join(', '));
+  }
+
   private async dryRunCreateLocation(
     spec: LocationSpec,
   ): Promise<{ location: Location; entities: Entity[]; exists?: boolean }> {
@@ -86,24 +114,10 @@ export class DefaultLocationService implements LocationService {
     const unprocessedEntities: DeferredEntity[] = [
       { entity, locationKey: `${spec.type}:${spec.target}` },
     ];
-    const entities: Entity[] = [];
-    while (unprocessedEntities.length) {
-      const currentEntity = unprocessedEntities.pop();
-      if (!currentEntity) {
-        continue;
-      }
-      const processed = await this.orchestrator.process({
-        entity: currentEntity.entity,
-        state: {}, // we process without the existing cache
-      });
-
-      if (processed.ok) {
-        unprocessedEntities.push(...processed.deferredEntities);
-        entities.push(processed.completedEntity);
-      } else {
-        throw Error(processed.errors.map(String).join(', '));
-      }
-    }
+    const entities: Entity[] = await this.getUnprocessedEntities(
+      unprocessedEntities,
+      [],
+    );
 
     return {
       exists: await existsPromise,