From ee7c379ba2e741aee142b0a142cfb5dafd725976 Mon Sep 17 00:00:00 2001
From: Dominique Pfister <dpfister@adobe.com>
Date: Wed, 25 Sep 2024 09:06:12 +0200
Subject: [PATCH] fix: IndexConfig should not allow broken config

BREAKING CHANGE: some indexing configurations will no longer be accepted
---
 packages/helix-shared-config/src/IndexConfig.js | 13 +++++++++++++
 ...indexconfigs.test.js => IndexConfig.test.js} | 14 +++++++-------
 .../test/specs/queryconfigs/broken.json         | 17 -----------------
 .../test/specs/queryconfigs/duplicate.yaml      | 12 ++++++++++++
 4 files changed, 32 insertions(+), 24 deletions(-)
 rename packages/helix-shared-config/test/{indexconfigs.test.js => IndexConfig.test.js} (96%)
 delete mode 100644 packages/helix-shared-config/test/specs/queryconfigs/broken.json
 create mode 100644 packages/helix-shared-config/test/specs/queryconfigs/duplicate.yaml

diff --git a/packages/helix-shared-config/src/IndexConfig.js b/packages/helix-shared-config/src/IndexConfig.js
index 5078b9f6..b8b6c604 100644
--- a/packages/helix-shared-config/src/IndexConfig.js
+++ b/packages/helix-shared-config/src/IndexConfig.js
@@ -193,4 +193,17 @@ export class IndexConfig extends SchemaDerivedConfig {
     this._version = this._cfg.version;
     return this;
   }
+
+  /**
+   * Validates the loaded configuration and coerces types and sets defaulst
+   */
+  async validate() {
+    await super.validate();
+
+    if (this._document?.errors?.length) {
+      const detail = this._document.errors.map(({ message }) => (message)).join('\n');
+      throw new Error(`Invalid index configuration:
+        ${detail}`);
+    }
+  }
 }
diff --git a/packages/helix-shared-config/test/indexconfigs.test.js b/packages/helix-shared-config/test/IndexConfig.test.js
similarity index 96%
rename from packages/helix-shared-config/test/indexconfigs.test.js
rename to packages/helix-shared-config/test/IndexConfig.test.js
index b8dba6df..bd695496 100644
--- a/packages/helix-shared-config/test/indexconfigs.test.js
+++ b/packages/helix-shared-config/test/IndexConfig.test.js
@@ -72,18 +72,18 @@ describe('Index Config Loading', () => {
     assert.deepEqual(actual, expected);
   });
 
-  it('Does not trip over broken config', async () => {
+  it('Trips over broken config', async () => {
     const cfg = new IndexConfig()
       .withConfigPath(path.resolve(SPEC_ROOT, 'broken.yaml'));
 
-    await cfg.init();
-
-    assert.strictEqual(cfg.getQuery('foo', 'bar'), undefined);
+    await assert.rejects(async () => cfg.init(), /Implicit map keys need to be followed by map value/);
+  });
 
-    const actual = cfg.toJSON();
-    const expected = JSON.parse(await fs.readFile(path.resolve(SPEC_ROOT, 'broken.json'), 'utf-8'));
+  it('Trips over config with duplicate property', async () => {
+    const cfg = new IndexConfig()
+      .withConfigPath(path.resolve(SPEC_ROOT, 'duplicate.yaml'));
 
-    assert.deepEqual(actual, expected);
+    await assert.rejects(async () => cfg.init(), /Map keys must be unique at line 10, column 7/);
   });
 
   it('Does not trip over non-existing config', async () => {
diff --git a/packages/helix-shared-config/test/specs/queryconfigs/broken.json b/packages/helix-shared-config/test/specs/queryconfigs/broken.json
deleted file mode 100644
index 786cd597..00000000
--- a/packages/helix-shared-config/test/specs/queryconfigs/broken.json
+++ /dev/null
@@ -1,17 +0,0 @@
-{
-  "indices": {
-    "blog-posts": {
-      "fetch": "https://${repo}-${owner}.project-helix.page/${path}",
-      "properties": {
-        "author": {
-          "faceted": true,
-          "select": "main > div:nth-of-type(3) > p:nth-of-type(1)",
-          "value": "${match('by (.*)')}\n"
-        }
-      },
-      "source": "html"
-    }
-  },
-  "version": "1",
-  "​": null
-}
diff --git a/packages/helix-shared-config/test/specs/queryconfigs/duplicate.yaml b/packages/helix-shared-config/test/specs/queryconfigs/duplicate.yaml
new file mode 100644
index 00000000..6abbf67e
--- /dev/null
+++ b/packages/helix-shared-config/test/specs/queryconfigs/duplicate.yaml
@@ -0,0 +1,12 @@
+version: 1
+
+indices:
+  blog-posts:
+    properties:
+      author:
+        select: main > div:nth-of-type(3) > p:nth-of-type(1)
+        value: |
+          ${match('by (.*)')}
+      author:
+        select: other
+        value: ${match('by (.*)')}