From 693db2bdff7627a0a6953f784e7567c6ceb5a236 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= <rar@odoo.com>
Date: Thu, 1 Aug 2024 13:13:01 +0200
Subject: [PATCH] [FIX] pivot: Prevent faulty pivot domain to crash
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The getter `getPivotCellFromPosition` parses pivot cells domain args
but this step assumes that the domain is valid (e.g. refers to a valid
groupby,measure combination). Unfortunately, if the domain is invalid,
the parsing will crash.

Similarly to the decision made for the getter `evaluateFormula`[^1]
we wrap the parsing in a try/catch statement and in case of faulty evaluation,
return the same result as for invalid/non-existing pivots, that is an
empty Pivotcell.

[^1]: See https://github.com/odoo/o-spreadsheet/pull/3371

closes odoo/o-spreadsheet#4741

Task: 4088765
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
---
 src/plugins/ui_core_views/pivot_ui.ts | 46 +++++++++++++++------------
 tests/pivots/pivot_plugin.test.ts     | 22 ++++++++++++-
 2 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/src/plugins/ui_core_views/pivot_ui.ts b/src/plugins/ui_core_views/pivot_ui.ts
index 47de1ab44f..6d513ce703 100644
--- a/src/plugins/ui_core_views/pivot_ui.ts
+++ b/src/plugins/ui_core_views/pivot_ui.ts
@@ -207,33 +207,37 @@ export class PivotUIPlugin extends UIPlugin {
       const pivotRow = position.row - mainPosition.row;
       return pivotCells[pivotCol][pivotRow];
     }
-    if (functionName === "PIVOT.HEADER" && args.at(-2) === "measure") {
-      const domain = pivot.parseArgsToPivotDomain(
-        args.slice(1, -2).map((value) => ({ value } as FunctionResultObject))
-      );
-      return {
-        type: "MEASURE_HEADER",
-        domain,
-        measure: args.at(-1)?.toString() || "",
-      };
-    } else if (functionName === "PIVOT.HEADER") {
+    try {
+      if (functionName === "PIVOT.HEADER" && args.at(-2) === "measure") {
+        const domain = pivot.parseArgsToPivotDomain(
+          args.slice(1, -2).map((value) => ({ value } as FunctionResultObject))
+        );
+        return {
+          type: "MEASURE_HEADER",
+          domain,
+          measure: args.at(-1)?.toString() || "",
+        };
+      } else if (functionName === "PIVOT.HEADER") {
+        const domain = pivot.parseArgsToPivotDomain(
+          args.slice(1).map((value) => ({ value } as FunctionResultObject))
+        );
+        return {
+          type: "HEADER",
+          domain,
+        };
+      }
+      const [measure, ...domainArgs] = args.slice(1);
       const domain = pivot.parseArgsToPivotDomain(
-        args.slice(1).map((value) => ({ value } as FunctionResultObject))
+        domainArgs.map((value) => ({ value } as FunctionResultObject))
       );
       return {
-        type: "HEADER",
+        type: "VALUE",
         domain,
+        measure: measure?.toString() || "",
       };
+    } catch (_) {
+      return EMPTY_PIVOT_CELL;
     }
-    const [measure, ...domainArgs] = args.slice(1);
-    const domain = pivot.parseArgsToPivotDomain(
-      domainArgs.map((value) => ({ value } as FunctionResultObject))
-    );
-    return {
-      type: "VALUE",
-      domain,
-      measure: measure?.toString() || "",
-    };
   }
 
   getPivot(pivotId: UID) {
diff --git a/tests/pivots/pivot_plugin.test.ts b/tests/pivots/pivot_plugin.test.ts
index e689a4f270..d28d541531 100644
--- a/tests/pivots/pivot_plugin.test.ts
+++ b/tests/pivots/pivot_plugin.test.ts
@@ -1,4 +1,5 @@
-import { setCellContent } from "../test_helpers/commands_helpers";
+import { EMPTY_PIVOT_CELL } from "../../src/helpers/pivot/table_spreadsheet_pivot";
+import { selectCell, setCellContent } from "../test_helpers/commands_helpers";
 import { createModelFromGrid, toCellPosition } from "../test_helpers/helpers";
 import { addPivot } from "../test_helpers/pivot_helpers";
 
@@ -28,4 +29,23 @@ describe("Pivot plugin", () => {
     setCellContent(model, "G1", "=PIVOT.HEADER(1)");
     expect(isSpillPivotFormula("G1")).toBe(false);
   });
+
+  test("getPivotCellFromPosition doesn't throw with invalid pivot domain", () => {
+    // prettier-ignore
+    const grid = {
+      A1: "Customer", B1: "Price", C1: '=PIVOT.VALUE(1,"Price","5","Bob")',
+      A2: "Alice",    B2: "10",
+      A3: "Bob",      B3: "30",
+    };
+    const model = createModelFromGrid(grid);
+    addPivot(model, "A1:B3", {
+      columns: [],
+      rows: [{ name: "Customer" }],
+      measures: [{ name: "Price", aggregator: "sum" }],
+    });
+    selectCell(model, "C1");
+    expect(model.getters.getPivotCellFromPosition(model.getters.getActivePosition())).toMatchObject(
+      EMPTY_PIVOT_CELL
+    );
+  });
 });