[FIX] CorePlugins: Prevent dispatch during adaptRange

The method `adaptRange` was originally designed for each plugin to
update their own ranges, whithout having to manually react to the
different commands that alter the sheets structure.

We have abused the system when we introduced the spreadsheet pivots to
allow the modification of a pivot dataset (which is stored in
PivotCorePlugin) from another entry point `SpreadsheetPivotCorePlugin`)
by dispatching an UPDATE_PIVOT command but there are some side effects
that we did not account for, the dispatched command is forwarded to
EVERY other plugin. Meaning that during an adaptRange, which should only
concern a specific plugin, we end up notifying a change to the whole
core/coreview stack.

This revision removes the access to 'dispatch' during the handling of
`adaptRange` so we ensure that the changes are kept locally.

closes #7633

Task: 5380747
X-original-commit: 53c92a2
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
Signed-off-by: Rémi Rahir (rar) <[email protected]>

Author: rrahir

packages/o-spreadsheet-engine/src/helpers/pivot/pivot_registry.ts

@@ -1,10 +1,11 @@
 import { Registry } from "../../registry";
 import { CoreGetters } from "../../types/core_getters";
 import { Getters } from "../../types/getters";
-import { UID } from "../../types/misc";
+import { ApplyRangeChange, UID } from "../../types/misc";
 import { ModelConfig } from "../../types/model";
 import { PivotCoreDefinition, PivotField, PivotFields } from "../../types/pivot";
 import { Pivot } from "../../types/pivot_runtime";
+import { Range } from "../../types/range";
 import { PivotRuntimeDefinition } from "./pivot_runtime_definition";
 import { SpreadsheetPivotRuntimeDefinition } from "./spreadsheet_pivot/runtime_definition_spreadsheet_pivot";
 import { SpreadsheetPivot } from "./spreadsheet_pivot/spreadsheet_pivot";
@@ -36,6 +37,11 @@ export interface PivotRegistryItem {
   isGroupable: (field: PivotField) => boolean;
   canHaveCustomGroup: (field: PivotField) => boolean;
   isPivotUnused: (getters: Getters, pivotId: UID) => boolean;
+  adaptRanges?: (
+    getters: CoreGetters,
+    definition: PivotCoreDefinition,
+    applyChange: ApplyRangeChange
+  ) => PivotCoreDefinition;
 }
 
 export const pivotRegistry = new Registry<PivotRegistryItem>();
@@ -60,4 +66,40 @@ pivotRegistry.add("SPREADSHEET", {
   isGroupable: () => true,
   canHaveCustomGroup: (field: PivotField) => field.type === "char" && !field.isCustomField,
   isPivotUnused: () => true,
+  adaptRanges: (getters, definition, applyChange) => {
+    if (definition.type !== "SPREADSHEET" || !definition.dataSet) {
+      return definition;
+    }
+    const { sheetId, zone } = definition.dataSet;
+    const range = getters.getRangeFromZone(sheetId, zone);
+    const adaptedRange = adaptPivotRange(range, applyChange);
+
+    if (adaptedRange === range) {
+      return definition;
+    }
+
+    const dataSet = adaptedRange && {
+      sheetId: adaptedRange.sheetId,
+      zone: adaptedRange.zone,
+    };
+    return { ...definition, dataSet };
+  },
 });
+
+function adaptPivotRange(
+  range: Range | undefined,
+  applyChange: ApplyRangeChange
+): Range | undefined {
+  if (!range) {
+    return undefined;
+  }
+  const change = applyChange(range);
+  switch (change.changeType) {
+    case "NONE":
+      return range;
+    case "REMOVE":
+      return undefined;
+    default:
+      return change.range;
+  }
+}

packages/o-spreadsheet-engine/src/plugins/core/pivot.ts

@@ -1,6 +1,7 @@
 import { compile } from "../../formulas/compiler";
 import { deepCopy, deepEquals } from "../../helpers/misc";
 import { createPivotFormula, getMaxObjectId } from "../../helpers/pivot/pivot_helpers";
+import { pivotRegistry } from "../../helpers/pivot/pivot_registry";
 import { SpreadsheetPivotTable } from "../../helpers/pivot/table_spreadsheet_pivot";
 import { CommandResult, CoreCommand } from "../../types/commands";
 import { CellPosition, UID } from "../../types/misc";
@@ -141,6 +142,18 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
   }
 
   adaptRanges(applyChange: ApplyRangeChange) {
+    for (const pivotId in this.pivots) {
+      const definition = deepCopy(this.pivots[pivotId]?.definition);
+      if (!definition) {
+        continue;
+      }
+      const newDefinition = pivotRegistry
+        .get(definition.type)
+        ?.adaptRanges?.(this.getters, definition, applyChange);
+      if (newDefinition && !deepEquals(definition, newDefinition)) {
+        this.history.update("pivots", pivotId, "definition", newDefinition);
+      }
+    }
     for (const sheetId in this.compiledMeasureFormulas) {
       for (const formulaString in this.compiledMeasureFormulas[sheetId]) {
         const compiledFormula = this.compiledMeasureFormulas[sheetId][formulaString];
@@ -325,19 +338,19 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
       if (!pivot) {
         continue;
       }
-      const def = deepCopy(pivot.definition);
 
-      for (const measure of def.measures) {
+      for (const measure of pivot.definition.measures) {
         if (measure.computedBy?.formula === formulaString) {
-          const measureIndex = def.measures.indexOf(measure);
-          if (measureIndex !== -1) {
-            def.measures[measureIndex].computedBy = {
-              formula: newFormulaString,
-              sheetId,
-            };
-          }
-
-          this.dispatch("UPDATE_PIVOT", { pivotId, pivot: def });
+          const measureIndex = pivot.definition.measures.indexOf(measure);
+          this.history.update(
+            "pivots",
+            pivotId,
+            "definition",
+            "measures",
+            measureIndex,
+            "computedBy",
+            { formula: newFormulaString, sheetId }
+          );
         }
       }
     }

packages/o-spreadsheet-engine/src/plugins/core/range.ts

@@ -32,6 +32,7 @@ import { Range, RangeData, RangeStringOptions } from "../../types/range";
 export class RangeAdapter implements CommandHandler<CoreCommand> {
   private getters: CoreGetters;
   private providers: Array<RangeProvider["adaptRanges"]> = [];
+  private isAdaptingRanges: boolean = false;
   constructor(getters: CoreGetters) {
     this.getters = getters;
   }
@@ -66,6 +67,9 @@ export class RangeAdapter implements CommandHandler<CoreCommand> {
   beforeHandle(command: Command) {}
 
   handle(cmd: CoreCommand) {
+    if (this.isAdaptingRanges) {
+      throw new Error("Plugins cannot dispatch commands during adaptRanges phase");
+    }
     const rangeAdapter = getRangeAdapter(cmd);
     if (rangeAdapter?.applyChange) {
       this.executeOnAllRanges(
@@ -99,10 +103,12 @@ export class RangeAdapter implements CommandHandler<CoreCommand> {
     sheetId: UID,
     sheetName: AdaptSheetName
   ) {
+    this.isAdaptingRanges = true;
     const func = this.verifyRangeRemoved(adaptRange);
     for (const provider of this.providers) {
       provider(func, sheetId, sheetName);
     }
+    this.isAdaptingRanges = false;
   }
 
   /**

packages/o-spreadsheet-engine/src/plugins/core/spreadsheet_pivot.ts

@@ -1,28 +1,8 @@
 import { isZoneValid } from "../../helpers/zones";
 import { CommandResult, CoreCommand } from "../../types/commands";
-import { ApplyRangeChange } from "../../types/misc";
 import { PivotCoreDefinition } from "../../types/pivot";
-import { Range } from "../../types/range";
 import { CorePlugin } from "../core_plugin";
 
-function adaptPivotRange(
-  range: Range | undefined,
-  applyChange: ApplyRangeChange
-): Range | undefined {
-  if (!range) {
-    return undefined;
-  }
-  const change = applyChange(range);
-  switch (change.changeType) {
-    case "NONE":
-      return range;
-    case "REMOVE":
-      return undefined;
-    default:
-      return change.range;
-  }
-}
-
 export class SpreadsheetPivotCorePlugin extends CorePlugin {
   allowDispatch(cmd: CoreCommand) {
     switch (cmd.type) {
@@ -34,30 +14,6 @@ export class SpreadsheetPivotCorePlugin extends CorePlugin {
     return CommandResult.Success;
   }
 
-  adaptRanges(applyChange: ApplyRangeChange) {
-    for (const pivotId of this.getters.getPivotIds()) {
-      const definition = this.getters.getPivotCoreDefinition(pivotId);
-      if (definition.type !== "SPREADSHEET") {
-        continue;
-      }
-      if (definition.dataSet) {
-        const { sheetId, zone } = definition.dataSet;
-        const range = this.getters.getRangeFromZone(sheetId, zone);
-        const adaptedRange = adaptPivotRange(range, applyChange);
-
-        if (adaptedRange === range) {
-          return;
-        }
-
-        const dataSet = adaptedRange && {
-          sheetId: adaptedRange.sheetId,
-          zone: adaptedRange.zone,
-        };
-        this.dispatch("UPDATE_PIVOT", { pivotId, pivot: { ...definition, dataSet } });
-      }
-    }
-  }
-
   private checkDataSetValidity(definition: PivotCoreDefinition) {
     if (definition.type === "SPREADSHEET" && definition.dataSet) {
       const { zone, sheetId } = definition.dataSet;

tests/range_plugin.test.ts

@@ -744,3 +744,17 @@ test.each([
   const adaptedRange = model.getters.createAdaptedRanges([range], 1, 1, sheetId);
   expect(model.getters.getRangeString(adaptedRange[0], sheetId)).toBe(expected);
 });
+
+test("Plugins cannot dispatch a command during adaptRanges", () => {
+  class PluginDispatchInAdaptRanges extends CorePlugin {
+    adaptRanges() {
+      this.dispatch("DELETE_SHEET", { sheetId: "s1", sheetName: "coucou" });
+    }
+  }
+  addTestPlugin(corePluginRegistry, PluginDispatchInAdaptRanges);
+
+  const model = new Model({});
+  expect(() => {
+    addColumns(model, "before", "A", 1);
+  }).toThrowError("Plugins cannot dispatch commands during adaptRanges phase");
+});