diff --git a/src/functions/helper_lookup.ts b/src/functions/helper_lookup.ts index 825d5e697e..26dfbc4d36 100644 --- a/src/functions/helper_lookup.ts +++ b/src/functions/helper_lookup.ts @@ -2,7 +2,7 @@ import { isZoneInside, positionToZone, zoneToXc } from "../helpers"; import { _t } from "../translation"; import { EvalContext, FunctionResultObject, Getters, Maybe, Range, UID } from "../types"; import { CircularDependencyError, EvaluationError, InvalidReferenceError } from "../types/errors"; -import { PivotCoreDefinition, PivotCoreMeasure } from "../types/pivot"; +import { PivotCoreMeasure } from "../types/pivot"; /** * Get the pivot ID from the formula pivot ID. @@ -37,11 +37,12 @@ export function assertDomainLength(domain: Maybe[]) { export function addPivotDependencies( evalContext: EvalContext, - coreDefinition: PivotCoreDefinition, + pivotId: UID, forMeasures: PivotCoreMeasure[] ) { //TODO This function can be very costly when used with PIVOT.VALUE and PIVOT.HEADER const dependencies: Range[] = []; + const coreDefinition = evalContext.getters.getPivotCoreDefinition(pivotId); if (coreDefinition.type === "SPREADSHEET" && coreDefinition.dataSet) { const { sheetId, zone } = coreDefinition.dataSet; @@ -62,8 +63,7 @@ export function addPivotDependencies( for (const measure of forMeasures) { if (measure.computedBy) { - const formula = evalContext.getters.getMeasureCompiledFormula(measure); - dependencies.push(...formula.dependencies.filter((range) => !range.invalidXc)); + dependencies.push(...evalContext.getters.getMeasureFullDependencies(pivotId, measure)); } } const originPosition = evalContext.__originCellPosition; diff --git a/src/functions/module_lookup.ts b/src/functions/module_lookup.ts index 6aab021d89..f2bedc777e 100644 --- a/src/functions/module_lookup.ts +++ b/src/functions/module_lookup.ts @@ -780,7 +780,7 @@ export const PIVOT_VALUE = { addPivotDependencies( this, - coreDefinition, + pivotId, coreDefinition.measures.filter((m) => m.id === _measure) ); pivot.init({ reload: pivot.needsReevaluation }); @@ -824,8 +824,7 @@ export const PIVOT_HEADER = { const _pivotId = getPivotId(_pivotFormulaId, this.getters); assertDomainLength(domainArgs); const pivot = this.getters.getPivot(_pivotId); - const coreDefinition = this.getters.getPivotCoreDefinition(_pivotId); - addPivotDependencies(this, coreDefinition, []); + addPivotDependencies(this, _pivotId, []); pivot.init({ reload: pivot.needsReevaluation }); const error = pivot.assertIsValid({ throwOnError: false }); if (error) { @@ -894,7 +893,7 @@ export const PIVOT = { const pivotId = getPivotId(_pivotFormulaId, this.getters); const pivot = this.getters.getPivot(pivotId); const coreDefinition = this.getters.getPivotCoreDefinition(pivotId); - addPivotDependencies(this, coreDefinition, coreDefinition.measures); + addPivotDependencies(this, pivotId, coreDefinition.measures); pivot.init({ reload: pivot.needsReevaluation }); const error = pivot.assertIsValid({ throwOnError: false }); if (error) { diff --git a/src/helpers/pivot/pivot_presentation.ts b/src/helpers/pivot/pivot_presentation.ts index 37a987d4c2..6fb4c492b4 100644 --- a/src/helpers/pivot/pivot_presentation.ts +++ b/src/helpers/pivot/pivot_presentation.ts @@ -14,6 +14,7 @@ import { isMatrix, } from "../../types"; import { CellErrorType, NotAvailableError } from "../../types/errors"; +import { UID } from "../../types/misc"; import { deepEquals, removeDuplicates, transpose2dPOJO } from "../misc"; import { NEXT_VALUE, @@ -50,6 +51,7 @@ type DomainGroups = { [colDomain: string]: { [rowDomain: string]: T } }; export default function (PivotClass: PivotUIConstructor) { class PivotPresentationLayer extends PivotClass { private getters: Getters; + private pivotId: UID; private cache: Record = {}; private rankAsc: CacheForMeasureAndField | undefined> = {}; private rankDesc: CacheForMeasureAndField | undefined> = {}; @@ -60,9 +62,10 @@ export default function (PivotClass: PivotUIConstructor) { DomainGroups | undefined > = {}; - constructor(custom: ModelConfig["custom"], params: PivotParams) { + constructor(pivotId, custom: ModelConfig["custom"], params: PivotParams) { super(custom, params); this.getters = params.getters; + this.pivotId = pivotId; } markAsDirtyForEvaluation(): void { @@ -117,7 +120,7 @@ export default function (PivotClass: PivotUIConstructor) { return handleError(error, measure.aggregator.toUpperCase()); } } - const formula = this.getters.getMeasureCompiledFormula(measure); + const formula = this.getters.getMeasureCompiledFormula(this.pivotId, measure); const getSymbolValue = (symbolName: string) => { const { columns, rows } = this.definition; if (columns.find((col) => col.nameWithGranularity === symbolName)) { diff --git a/src/helpers/pivot/pivot_registry.ts b/src/helpers/pivot/pivot_registry.ts index dae9b8a343..0adeb9918d 100644 --- a/src/helpers/pivot/pivot_registry.ts +++ b/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(); @@ -54,4 +59,40 @@ 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; + } + 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; + } +} diff --git a/src/plugins/core/pivot.ts b/src/plugins/core/pivot.ts index eee1a109eb..196212fe4b 100644 --- a/src/plugins/core/pivot.ts +++ b/src/plugins/core/pivot.ts @@ -1,6 +1,7 @@ import { compile } from "../../formulas"; -import { deepCopy, deepEquals } from "../../helpers"; +import { deepCopy, deepEquals, getCanonicalSymbolName } 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, @@ -21,11 +22,16 @@ interface Pivot { formulaId: string; } +interface MeasureState { + formula: RangeCompiledFormula; + fullDependencies: Range[]; +} + interface CoreState { nextFormulaId: number; pivots: Record; formulaIds: Record; - compiledMeasureFormulas: Record>; + compiledMeasureFormulas: Record>; } export class PivotCorePlugin extends CorePlugin implements CoreState { @@ -38,6 +44,7 @@ export class PivotCorePlugin extends CorePlugin implements CoreState "getMeasureCompiledFormula", "getPivotName", "isExistingPivot", + "getMeasureFullDependencies", ] as const; readonly nextFormulaId: number = 1; @@ -45,7 +52,7 @@ export class PivotCorePlugin extends CorePlugin implements CoreState [pivotId: UID]: Pivot | undefined; } = {}; public readonly formulaIds: { [formulaId: UID]: UID | undefined } = {}; - public readonly compiledMeasureFormulas: Record> = {}; + public readonly compiledMeasureFormulas: Record> = {}; allowDispatch(cmd: CoreCommand) { switch (cmd.type) { @@ -125,16 +132,33 @@ export class PivotCorePlugin extends CorePlugin implements CoreState } case "UPDATE_PIVOT": { this.history.update("pivots", cmd.pivotId, "definition", deepCopy(cmd.pivot)); - this.compileCalculatedMeasures(cmd.pivot.measures); + this.compileCalculatedMeasures(cmd.pivotId, cmd.pivot.measures); break; } } } adaptRanges(applyChange: ApplyRangeChange) { - for (const sheetId in this.compiledMeasureFormulas) { - for (const formulaString in this.compiledMeasureFormulas[sheetId]) { - const compiledFormula = this.compiledMeasureFormulas[sheetId][formulaString]; + 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 pivotId in this.compiledMeasureFormulas) { + for (const measureId in this.compiledMeasureFormulas[pivotId]) { + const measure = this.pivots[pivotId]?.definition.measures.find((m) => m.id === measureId); + if (!measure || !measure.computedBy) { + continue; + } + const sheetId = measure.computedBy.sheetId; + const compiledFormula = this.compiledMeasureFormulas[pivotId][measureId].formula; const newDependencies: Range[] = []; for (const range of compiledFormula.dependencies) { const change = applyChange(range); @@ -149,8 +173,9 @@ export class PivotCorePlugin extends CorePlugin implements CoreState compiledFormula.tokens, newDependencies ); - if (newFormulaString !== formulaString) { - this.replaceMeasureFormula(sheetId, formulaString, newFormulaString); + const oldFormulaString = measure.computedBy.formula; + if (newFormulaString !== oldFormulaString) { + this.replaceMeasureFormula(oldFormulaString, newFormulaString); } } } @@ -197,12 +222,18 @@ export class PivotCorePlugin extends CorePlugin implements CoreState return pivotId in this.pivots; } - getMeasureCompiledFormula(measure: PivotCoreMeasure): RangeCompiledFormula { + getMeasureCompiledFormula(pivotId: UID, measure: PivotCoreMeasure): RangeCompiledFormula { if (!measure.computedBy) { throw new Error(`Measure ${measure.fieldName} is not computed by formula`); } - const sheetId = measure.computedBy.sheetId; - return this.compiledMeasureFormulas[sheetId][measure.computedBy.formula]; + return this.compiledMeasureFormulas[pivotId][measure.id].formula; + } + + getMeasureFullDependencies(pivotId: UID, measure: PivotCoreMeasure): Range[] { + if (!measure.computedBy) { + throw new Error(`Measure ${measure.fieldName} is not computed by formula`); + } + return this.compiledMeasureFormulas[pivotId][measure.id].fullDependencies; } // ------------------------------------------------------------------------- @@ -215,27 +246,73 @@ export class PivotCorePlugin extends CorePlugin implements CoreState formulaId = this.nextFormulaId.toString() ) { this.history.update("pivots", pivotId, { definition: deepCopy(pivot), formulaId }); - this.compileCalculatedMeasures(pivot.measures); + this.compileCalculatedMeasures(pivotId, pivot.measures); this.history.update("formulaIds", formulaId, pivotId); this.history.update("nextFormulaId", this.nextFormulaId + 1); } - private compileCalculatedMeasures(measures: PivotCoreMeasure[]) { + private compileCalculatedMeasures(pivotId: UID, measures: PivotCoreMeasure[]) { for (const measure of measures) { if (measure.computedBy) { - const sheetId = measure.computedBy.sheetId; const compiledFormula = this.compileMeasureFormula( measure.computedBy.sheetId, measure.computedBy.formula ); this.history.update( "compiledMeasureFormulas", - sheetId, - measure.computedBy.formula, + pivotId, + measure.id, + "formula", compiledFormula ); } } + for (const measure of measures) { + if (measure.computedBy) { + const dependencies = this.computeMeasureFullDependencies(pivotId, measure); + this.history.update( + "compiledMeasureFormulas", + pivotId, + measure.id, + "fullDependencies", + dependencies + ); + } + } + } + + private computeMeasureFullDependencies( + pivotId: UID, + measure: PivotCoreMeasure, + exploredMeasures: Set = new Set() + ): Range[] { + const rangeList: Range[] = []; + const definition = this.getPivotCoreDefinition(pivotId); + const formula = this.getMeasureCompiledFormula(pivotId, measure); + for (const token of formula.tokens) { + if (token.type !== "SYMBOL") { + continue; + } + const existingMeasure = definition.measures.find( + (measureCandidate) => + getCanonicalSymbolName(measureCandidate.id) === token.value && + measure.id !== measureCandidate.id + ); + + if ( + !existingMeasure || + exploredMeasures.has(existingMeasure.id) || + !existingMeasure.computedBy + ) + continue; + + rangeList.push( + ...this.computeMeasureFullDependencies(pivotId, existingMeasure, exploredMeasures) + ); + } + rangeList.push(...formula.dependencies.filter((range) => !range.invalidXc)); + exploredMeasures.add(measure.id); + return rangeList; } private insertPivot(position: CellPosition, formulaId: UID, table: SpreadsheetPivotTable) { @@ -301,34 +378,38 @@ export class PivotCorePlugin extends CorePlugin implements CoreState }; } - private replaceMeasureFormula(sheetId: UID, formulaString: string, newFormulaString: string) { - this.history.update("compiledMeasureFormulas", sheetId, formulaString, undefined); - this.history.update( - "compiledMeasureFormulas", - sheetId, - newFormulaString, - this.compileMeasureFormula(sheetId, newFormulaString) - ); + private replaceMeasureFormula(formulaString: string, newFormulaString: string) { + // this.history.update("compiledMeasureFormulas", sheetId, formulaString, undefined); + // this.history.update( + // "compiledMeasureFormulas", + // sheetId, + // newFormulaString, + // this.compileMeasureFormula(sheetId, newFormulaString) + // ); for (const pivotId in this.pivots) { const pivot = this.pivots[pivotId]; if (!pivot) { continue; } - const def = deepCopy(pivot.definition); - - for (const measure of def.measures) { + let shouldUpdateMeasures = false; + 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 }); + shouldUpdateMeasures = true; + const measureIndex = pivot.definition.measures.indexOf(measure); + this.history.update( + "pivots", + pivotId, + "definition", + "measures", + measureIndex, + "computedBy", + { formula: newFormulaString, sheetId: measure.computedBy.sheetId } + ); } } + if (shouldUpdateMeasures) { + this.compileCalculatedMeasures(pivotId, pivot.definition.measures); + } } } diff --git a/src/plugins/core/range.ts b/src/plugins/core/range.ts index ceda12b806..5fdbc4d29f 100644 --- a/src/plugins/core/range.ts +++ b/src/plugins/core/range.ts @@ -42,6 +42,7 @@ interface RangeStringOptions { export class RangeAdapter implements CommandHandler { private getters: CoreGetters; private providers: Array = []; + private isAdaptingRanges: boolean = false; constructor(getters: CoreGetters) { this.getters = getters; } @@ -73,6 +74,9 @@ export class RangeAdapter implements CommandHandler { beforeHandle(command: Command) {} handle(cmd: Command) { + if (this.isAdaptingRanges) { + throw new Error("Plugins cannot dispatch commands during adaptRanges phase"); + } switch (cmd.type) { case "REMOVE_COLUMNS_ROWS": { let start: "left" | "top" = cmd.dimension === "COL" ? "left" : "top"; @@ -243,10 +247,12 @@ export class RangeAdapter implements CommandHandler { } private executeOnAllRanges(adaptRange: ApplyRangeChange, sheetId?: UID) { + this.isAdaptingRanges = true; const func = this.verifyRangeRemoved(adaptRange); for (const provider of this.providers) { provider(func, sheetId); } + this.isAdaptingRanges = false; } /** diff --git a/src/plugins/core/spreadsheet_pivot.ts b/src/plugins/core/spreadsheet_pivot.ts index 1e0973f228..daee552519 100644 --- a/src/plugins/core/spreadsheet_pivot.ts +++ b/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; diff --git a/src/plugins/ui_core_views/pivot_ui.ts b/src/plugins/ui_core_views/pivot_ui.ts index 8eb478a062..7638fff829 100644 --- a/src/plugins/ui_core_views/pivot_ui.ts +++ b/src/plugins/ui_core_views/pivot_ui.ts @@ -304,7 +304,7 @@ export class PivotUIPlugin extends UIPlugin { const definition = deepCopy(this.getters.getPivotCoreDefinition(pivotId)); if (!(pivotId in this.pivots)) { const Pivot = withPivotPresentationLayer(pivotRegistry.get(definition.type).ui); - this.pivots[pivotId] = new Pivot(this.custom, { definition, getters: this.getters }); + this.pivots[pivotId] = new Pivot(pivotId, this.custom, { definition, getters: this.getters }); } else if (recreate) { this.pivots[pivotId].onDefinitionChange(definition); } diff --git a/tests/pivots/pivot_calculated_measure.test.ts b/tests/pivots/pivot_calculated_measure.test.ts index 326e14b3e0..7552e0b86b 100644 --- a/tests/pivots/pivot_calculated_measure.test.ts +++ b/tests/pivots/pivot_calculated_measure.test.ts @@ -1188,3 +1188,34 @@ describe("Pivot calculated measure", () => { expect(getEvaluatedCell(model, "A6").value).toEqual(0); }); }); + +test("measure takes indirect dependency into account for recalculation", () => { + const grid = { + A1: "Customer", + A2: "Alice", + A3: "42", + A4: '=PIVOT.VALUE(1, "calculated")', + A5: '=PIVOT.VALUE(1, "basedOnCalculated")', + }; + const model = createModelFromGrid(grid); + const sheetId = model.getters.getActiveSheetId(); + addPivot(model, "A1:A2", { + measures: [ + { + id: "calculated", + fieldName: "calculated", + aggregator: "sum", + computedBy: { formula: "=A3", sheetId }, + }, + { + id: "basedOnCalculated", + fieldName: "basedOnCalculated", + aggregator: "sum", + computedBy: { formula: "=calculated", sheetId }, + }, + ], + }); + setCellContent(model, "A3", "43"); + expect(getEvaluatedCell(model, "A4").value).toEqual(43); + expect(getEvaluatedCell(model, "A5").value).toEqual(43); +}); diff --git a/tests/range_plugin.test.ts b/tests/range_plugin.test.ts index dd09fe4e96..c475dd4c15 100644 --- a/tests/range_plugin.test.ts +++ b/tests/range_plugin.test.ts @@ -745,3 +745,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" }); + } + } + addTestPlugin(corePluginRegistry, PluginDispatchInAdaptRanges); + + const model = new Model({}); + expect(() => { + addColumns(model, "before", "A", 1); + }).toThrowError("Plugins cannot dispatch commands during adaptRanges phase"); +});