From 0e4daaa753fb481a9ff10e6bbc565365aa9809dd Mon Sep 17 00:00:00 2001
From: Patrik Oldsberg <poldsberg@gmail.com>
Date: Thu, 2 Dec 2021 20:54:55 +0100
Subject: [PATCH] catalog-backend: disallow duplicate entity search keys

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
---
 .changeset/hot-dragons-kneel.md               |  5 ++
 .../catalog-backend/src/stitching/Stitcher.ts |  6 ++-
 .../src/stitching/buildEntitySearch.test.ts   | 46 +++++++++++++++++++
 .../src/stitching/buildEntitySearch.ts        | 19 ++++++++
 4 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 .changeset/hot-dragons-kneel.md

diff --git a/.changeset/hot-dragons-kneel.md b/.changeset/hot-dragons-kneel.md
new file mode 100644
index 0000000000..a2b6241b69
--- /dev/null
+++ b/.changeset/hot-dragons-kneel.md
@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend': patch
+---
+
+Reject catalog entities that have duplicate fields that vary only in casing.
diff --git a/plugins/catalog-backend/src/stitching/Stitcher.ts b/plugins/catalog-backend/src/stitching/Stitcher.ts
index 507f3aa53e..b4a693a4fa 100644
--- a/plugins/catalog-backend/src/stitching/Stitcher.ts
+++ b/plugins/catalog-backend/src/stitching/Stitcher.ts
@@ -209,6 +209,11 @@ export class Stitcher {
       entity.metadata.etag = hash;
     }
 
+    // This may throw if the entity is invalid, so we call it before
+    // the final_entites write, even though we may end up not needing
+    // to write the search index.
+    const searchEntries = buildEntitySearch(entityId, entity);
+
     const rowsChanged = await this.database<DbFinalEntitiesRow>(
       'final_entities',
     )
@@ -235,7 +240,6 @@ export class Stitcher {
     // B writes the entity ->
     // B writes search ->
     // A writes search
-    const searchEntries = buildEntitySearch(entityId, entity);
     await this.database<DbSearchRow>('search')
       .where({ entity_id: entityId })
       .delete();
diff --git a/plugins/catalog-backend/src/stitching/buildEntitySearch.test.ts b/plugins/catalog-backend/src/stitching/buildEntitySearch.test.ts
index 9043dab9ba..58551a461f 100644
--- a/plugins/catalog-backend/src/stitching/buildEntitySearch.test.ts
+++ b/plugins/catalog-backend/src/stitching/buildEntitySearch.test.ts
@@ -163,5 +163,51 @@ describe('buildEntitySearch', () => {
         { entity_id: 'eid', key: 'relations.t2', value: 'k:ns/b' },
       ]);
     });
+
+    it('rejects duplicate keys', () => {
+      expect(() =>
+        buildEntitySearch('eid', {
+          relations: [],
+          apiVersion: 'a',
+          kind: 'b',
+          metadata: {
+            name: 'n',
+            Namespace: 'ns',
+          },
+        }),
+      ).toThrow(
+        `Entity has duplicate keys that vary only in casing, 'metadata.namespace'`,
+      );
+
+      expect(() =>
+        buildEntitySearch('eid', {
+          relations: [
+            { type: 'dup', target: { kind: 'k', namespace: 'ns', name: 'a' } },
+            { type: 'DUP', target: { kind: 'k', namespace: 'ns', name: 'b' } },
+          ],
+          apiVersion: 'a',
+          kind: 'b',
+          metadata: { name: 'n' },
+        }),
+      ).toThrow(
+        `Entity has duplicate keys that vary only in casing, 'relations.dup'`,
+      );
+
+      expect(() =>
+        buildEntitySearch('eid', {
+          apiVersion: 'a',
+          kind: 'b',
+          metadata: { name: 'n' },
+          spec: {
+            owner: 'o',
+            OWNER: 'o',
+            lifecycle: 'production',
+            lifeCycle: 'production',
+          },
+        }),
+      ).toThrow(
+        `Entity has duplicate keys that vary only in casing, 'spec.owner', 'spec.lifecycle'`,
+      );
+    });
   });
 });
diff --git a/plugins/catalog-backend/src/stitching/buildEntitySearch.ts b/plugins/catalog-backend/src/stitching/buildEntitySearch.ts
index ed84dfa018..b6c3eead2a 100644
--- a/plugins/catalog-backend/src/stitching/buildEntitySearch.ts
+++ b/plugins/catalog-backend/src/stitching/buildEntitySearch.ts
@@ -19,6 +19,7 @@ import {
   ENTITY_DEFAULT_NAMESPACE,
   stringifyEntityRef,
 } from '@backstage/catalog-model';
+import { InputError } from '@backstage/errors';
 import { DbSearchRow } from '../database/tables';
 
 // These are excluded in the generic loop, either because they do not make sense
@@ -184,5 +185,23 @@ export function buildEntitySearch(
     });
   }
 
+  // This validates that there are no keys that vary only in casing, such
+  // as `spec.foo` and `spec.Foo`.
+  const keys = new Set(raw.map(r => r.key));
+  const lowerKeys = new Set(raw.map(r => r.key.toLocaleLowerCase('en-US')));
+  if (keys.size !== lowerKeys.size) {
+    const difference = [];
+    for (const key of keys) {
+      const lower = key.toLocaleLowerCase('en-US');
+      if (!lowerKeys.delete(lower)) {
+        difference.push(lower);
+      }
+    }
+    const badKeys = `'${difference.join("', '")}'`;
+    throw new InputError(
+      `Entity has duplicate keys that vary only in casing, ${badKeys}`,
+    );
+  }
+
   return mapToRows(raw, entityId);
 }