[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.

Task: 5380747

Author: rrahir

src/helpers/pivot/pivot_registry.ts

@@ -1,6 +1,6 @@
 import { ModelConfig } from "../../model";
 import { Registry } from "../../registries/registry";
-import { CoreGetters, Getters } from "../../types";
+import { ApplyRangeChange, CoreGetters, Getters, Range } from "../../types";
 import { PivotCoreDefinition, PivotField, PivotFields } from "../../types/pivot";
 import { Pivot } from "../../types/pivot_runtime";
 import { PivotRuntimeDefinition } from "./pivot_runtime_definition";
@@ -32,6 +32,11 @@ export interface PivotRegistryItem {
   datetimeGranularities: string[];
   isMeasureCandidate: (field: PivotField) => boolean;
   isGroupable: (field: PivotField) => boolean;
+  adaptRanges?: (
+    getters: CoreGetters,
+    definition: PivotCoreDefinition,
+    applyChange: ApplyRangeChange
+  ) => PivotCoreDefinition;
 }
 
 export const pivotRegistry = new Registry<PivotRegistryItem>();
@@ -54,4 +59,43 @@ pivotRegistry.add("SPREADSHEET", {
   datetimeGranularities: [...dateGranularities, "hour_number", "minute_number", "second_number"],
   isMeasureCandidate: (field: PivotField) => !["datetime", "boolean"].includes(field.type),
   isGroupable: () => true,
+  adaptRanges: (getters, definition, applyChange) => {
+    if (definition.type !== "SPREADSHEET" || !definition.dataSet) {
+      return definition;
+    }
+    // if (definition.dataSet) {
+    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 };
+    // this.dispatch("UPDATE_PIVOT", { pivotId, pivot: { ...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;
+  }
+}

src/plugins/core/pivot.ts

@@ -1,6 +1,7 @@
 import { compile } from "../../formulas";
 import { deepCopy, deepEquals } from "../../helpers";
 import { createPivotFormula, getMaxObjectId } from "../../helpers/pivot/pivot_helpers";
+import { pivotRegistry } from "../../helpers/pivot/pivot_registry";
 import { SpreadsheetPivotTable } from "../../helpers/pivot/table_spreadsheet_pivot";
 import {
   ApplyRangeChange,
@@ -132,6 +133,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];
@@ -314,21 +327,37 @@ 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 }
+          );
         }
       }
+
+      // const def = deepCopy(pivot.definition);
+
+      // for (const measure of def.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 });
+      //   }
+      // }
     }
   }
 

src/plugins/core/spreadsheet_pivot.ts

@@ -1,31 +1,7 @@
 import { isZoneValid } from "../../helpers";
-import {
-  ApplyRangeChange,
-  CommandResult,
-  CoreCommand,
-  PivotCoreDefinition,
-  Range,
-} from "../../types";
+import { CommandResult, CoreCommand, PivotCoreDefinition } from "../../types";
 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) {
@@ -37,30 +13,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;

src/plugins/core_plugin.ts

@@ -51,7 +51,14 @@ export class CorePlugin<State = any>
     uuidGenerator,
   }: CorePluginConfig) {
     super(stateObserver, dispatch, canDispatch);
-    range.addRangeProvider(this.adaptRanges.bind(this));
+
+    // Bind adaptRanges to a version of `this` where `dispatch` always throws
+    const thisWithThrowingDispatch = Object.create(this);
+    thisWithThrowingDispatch.dispatch = () => {
+      throw new Error("dispatch is not allowed in adaptRanges context");
+    };
+    range.addRangeProvider(this.adaptRanges.bind(thisWithThrowingDispatch));
+
     this.getters = getters;
     this.uuidGenerator = uuidGenerator;
   }