From c23637e6d80ebb7ab7e926666ecc25d02ce0f2dd Mon Sep 17 00:00:00 2001
From: Samuel Degueldre <sad@odoo.com>
Date: Mon, 22 Nov 2021 11:01:17 +0100
Subject: [PATCH] [FIX] components: throw on duplicate t-key instead of hanging
 the app

---
 src/compiler/code_generator.ts                | 11 ++++
 .../__snapshots__/t_foreach.test.ts.snap      | 55 +++++++++++++++++++
 tests/components/t_foreach.test.ts            | 34 +++++++++++-
 3 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/src/compiler/code_generator.ts b/src/compiler/code_generator.ts
index 11251e0b..536c506c 100644
--- a/src/compiler/code_generator.ts
+++ b/src/compiler/code_generator.ts
@@ -734,6 +734,10 @@ export class CodeGenerator {
     this.addLine(
       `const [${keys}, ${vals}, ${l}, ${c}] = prepareList(${compileExpr(ast.collection)});`
     );
+    // Throw errors on duplicate keys in dev mode
+    if (this.dev) {
+      this.addLine(`const keys${block.id} = new Set();`);
+    }
     this.addLine(`for (let ${loopVar} = 0; ${loopVar} < ${l}; ${loopVar}++) {`);
     this.target.indentLevel++;
     this.addLine(`ctx[\`${ast.elem}\`] = ${vals}[${loopVar}];`);
@@ -750,6 +754,13 @@ export class CodeGenerator {
       this.addLine(`ctx[\`${ast.elem}_value\`] = ${keys}[${loopVar}];`);
     }
     this.addLine(`let key${this.target.loopLevel} = ${ast.key ? compileExpr(ast.key) : loopVar};`);
+    if (this.dev) {
+      // Throw error on duplicate keys in dev mode
+      this.addLine(
+        `if (keys${block.id}.has(key${this.target.loopLevel})) { throw new Error(\`Got duplicate key in t-foreach: \${key${this.target.loopLevel}}\`)}`
+      );
+      this.addLine(`keys${block.id}.add(key${this.target.loopLevel});`);
+    }
     let id: string;
     if (ast.memo) {
       this.target.hasCache = true;
diff --git a/tests/components/__snapshots__/t_foreach.test.ts.snap b/tests/components/__snapshots__/t_foreach.test.ts.snap
index 4d119f66..3791e5f1 100644
--- a/tests/components/__snapshots__/t_foreach.test.ts.snap
+++ b/tests/components/__snapshots__/t_foreach.test.ts.snap
@@ -39,6 +39,61 @@ exports[`list of components components in a node in a t-foreach  2`] = `
 }"
 `;
 
+exports[`list of components crash on duplicate key in dev mode 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  return function template(ctx, node, key = \\"\\") {
+    ctx = Object.create(ctx);
+    const [k_block1, v_block1, l_block1, c_block1] = prepareList([1,2]);
+    const keys1 = new Set();
+    for (let i1 = 0; i1 < l_block1; i1++) {
+      ctx[\`item\`] = v_block1[i1];
+      let key1 = 'child';
+      if (keys1.has(key1)) { throw new Error(\`Got duplicate key in t-foreach: \${key1}\`)}
+      keys1.add(key1);
+      const props2 = {}
+      helpers.validateProps(\`Child\`, props2, ctx)
+      c_block1[i1] = withKey(component(\`Child\`, props2, key + \`__1__\${key1}\`, node, ctx), key1);
+    }
+    return list(c_block1);
+  }
+}"
+`;
+
+exports[`list of components crash on duplicate key in dev mode 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`\`);
+  }
+}"
+`;
+
+exports[`list of components crash on duplicate key in dev mode 3`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  return function template(ctx, node, key = \\"\\") {
+    ctx = Object.create(ctx);
+    const [k_block1, v_block1, l_block1, c_block1] = prepareList([1,2]);
+    for (let i1 = 0; i1 < l_block1; i1++) {
+      ctx[\`item\`] = v_block1[i1];
+      let key1 = 'child';
+      c_block1[i1] = withKey(component(\`Child\`, {}, key + \`__1__\${key1}\`, node, ctx), key1);
+    }
+    return list(c_block1);
+  }
+}"
+`;
+
 exports[`list of components list of sub components inside other nodes 1`] = `
 "function anonymous(bdom, helpers
 ) {
diff --git a/tests/components/t_foreach.test.ts b/tests/components/t_foreach.test.ts
index 01626e5f..7f3d010e 100644
--- a/tests/components/t_foreach.test.ts
+++ b/tests/components/t_foreach.test.ts
@@ -1,5 +1,11 @@
-import { Component, mount, onMounted, useState, xml } from "../../src/index";
-import { makeTestFixture, nextTick, snapshotEverything, useLogLifecycle } from "../helpers";
+import { App, Component, mount, onMounted, useState, xml } from "../../src/index";
+import {
+  makeTestFixture,
+  nextTick,
+  snapshotApp,
+  snapshotEverything,
+  useLogLifecycle,
+} from "../helpers";
 
 snapshotEverything();
 
@@ -291,4 +297,28 @@ describe("list of components", () => {
     expect((parent.el as HTMLElement).innerHTML).toBe("<div>2</div><div>1</div>");
     expect(childInstances.length).toBe(2);
   });
+
+  test("crash on duplicate key in dev mode", async () => {
+    const consoleInfo = console.info;
+    console.info = jest.fn();
+    class Child extends Component {
+      static template = xml``;
+    }
+
+    class Parent extends Component {
+      static template = xml`
+        <t t-foreach="[1, 2]" t-as="item" t-key="'child'">
+          <Child/>
+        </t>
+      `;
+      static components = { Child };
+    }
+    const app = new App(Parent);
+    app.configure({ dev: true });
+    await expect(async () => {
+      await app.mount(fixture);
+    }).rejects.toThrowError("Got duplicate key in t-foreach: child");
+    snapshotApp(app);
+    console.info = consoleInfo;
+  });
 });