[FIX] figure,chart: Enforce unicity of figure ids

The issue of duplicated chart ids was first addressed in PR #2102 by
trying to defined chart ids per sheet (just like figures)
Unfortunately, the fix was not appropriate for several reasons:
1. Some commands in Odoo were not dispatching the sheetId along with the
   chartId, making the mapping sheetId, chartId hazardous
2. There was absolutely 0 verification that the commands targeting a
   chartId were also providing a sheet Id that matched. So the said
   commands cannot be trusted either

This commit is exploring the other solution that is forcing the unicity
of a figure id. The data are adapted so that figures with a duplicated
id well have the latter updated to ensure unicity.

This commit also tries to solve the wrong `sheetId` parameter in
`UPDATE_CHART` by simply ignoring it in the commands. It's not necesarry
since we now have the unicity of figure ids.

closes #2166

X-original-commit: f56c6eb
Signed-off-by: Pierre Rousseau (pro) <[email protected]>
Signed-off-by: Rémi Rahir (rar) <[email protected]>

src/migrations/data.ts

@@ -4,7 +4,7 @@ import {
   FORBIDDEN_IN_EXCEL_REGEX,
   FORMULA_REF_IDENTIFIER,
 } from "../constants";
-import { getItemId, toXC, toZone } from "../helpers/index";
+import { getItemId, toXC, toZone, UuidGenerator } from "../helpers/index";
 import { StateUpdateMessage } from "../types/collaborative/transport_service";
 import {
   CoreCommand,
@@ -54,17 +54,7 @@ export function load(data?: any, verboseImport?: boolean): WorkbookData {
       data = migrate(data);
     }
   }
-
-  // sanity check: try to fix missing fields/corrupted state by providing
-  // sensible default values
-  data = Object.assign(createEmptyWorkbookData(), data, { version: CURRENT_VERSION });
-  data.sheets = data.sheets.map((s, i) =>
-    Object.assign(createEmptySheet(`Sheet${i + 1}`, `Sheet${i + 1}`), s)
-  );
-
-  if (data.sheets.length === 0) {
-    data.sheets.push(createEmptySheet(INITIAL_SHEET_ID, "Sheet1"));
-  }
+  data = repairData(data);
   return data;
 }
 
@@ -84,6 +74,7 @@ function migrate(data: any): WorkbookData {
   for (let i = index; i < MIGRATIONS.length; i++) {
     data = MIGRATIONS[i].applyMigration(data);
   }
+
   return data;
 }
 
@@ -316,6 +307,56 @@ const MIGRATIONS: Migration[] = [
   },
 ];
 
+/**
+ * This function is used to repair faulty data independently of the migration.
+ */
+export function repairData(data: Partial<WorkbookData>): Partial<WorkbookData> {
+  data = forceUnicityOfFigure(data);
+  data = setDefaults(data);
+  return data;
+}
+
+/**
+ * Force the unicity of figure ids accross sheets
+ */
+function forceUnicityOfFigure(data: Partial<WorkbookData>): Partial<WorkbookData> {
+  if (data.uniqueFigureIds) {
+    return data;
+  }
+  const figureIds = new Set();
+  const uuidGenerator = new UuidGenerator();
+  for (const sheet of data.sheets || []) {
+    for (const figure of sheet.figures || []) {
+      if (figureIds.has(figure.id)) {
+        figure.id += uuidGenerator.uuidv4();
+      }
+      figureIds.add(figure.id);
+    }
+  }
+
+  data.uniqueFigureIds = true;
+  return data;
+}
+
+/**
+ * sanity check: try to fix missing fields/corrupted state by providing
+ * sensible default values
+ */
+function setDefaults(data: Partial<WorkbookData>): Partial<WorkbookData> {
+  data = Object.assign(createEmptyWorkbookData(), data, { version: CURRENT_VERSION });
+  data.sheets = data.sheets
+    ? data.sheets.map((s, i) =>
+        Object.assign(createEmptySheet(`Sheet${i + 1}`, `Sheet${i + 1}`), s)
+      )
+    : [];
+
+  if (data.sheets.length === 0) {
+    data.sheets.push(createEmptySheet(INITIAL_SHEET_ID, "Sheet1"));
+  }
+
+  return data;
+}
+
 /**
  * The goal of this function is to repair corrupted/wrong initial messages caused by
  * a bug.
@@ -460,6 +501,7 @@ export function createEmptyWorkbookData(sheetName = "Sheet1"): WorkbookData {
     formats: {},
     borders: {},
     revisionId: DEFAULT_REVISION_ID,
+    uniqueFigureIds: true,
   };
   return data;
 }

src/plugins/core/chart.ts

@@ -12,11 +12,13 @@ import {
   Command,
   CommandResult,
   CoreCommand,
+  CreateChartCommand,
   ExcelWorkbookData,
   Figure,
   FigureData,
   Pixel,
   UID,
+  UpdateChartCommand,
   WorkbookData,
 } from "../../types/index";
 import { CorePlugin } from "../core_plugin";
@@ -44,8 +46,8 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
   readonly charts: Record<UID, AbstractChart | undefined> = {};
 
   private createChart = chartFactory(this.getters);
-  private validateChartDefinition = (definition: ChartDefinition) =>
-    validateChartDefinition(this, definition);
+  private validateChartDefinition = (cmd: CreateChartCommand | UpdateChartCommand) =>
+    validateChartDefinition(this, cmd.definition);
 
   adaptRanges(applyChange: ApplyRangeChange) {
     for (const [chartId, chart] of Object.entries(this.charts)) {
@@ -60,8 +62,15 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
   allowDispatch(cmd: Command) {
     switch (cmd.type) {
       case "CREATE_CHART":
+        return this.checkValidations(
+          cmd,
+          this.chainValidations(this.validateChartDefinition, this.checkChartDuplicate)
+        );
       case "UPDATE_CHART":
-        return this.validateChartDefinition(cmd.definition);
+        return this.checkValidations(
+          cmd,
+          this.chainValidations(this.validateChartDefinition, this.checkChartExists)
+        );
       default:
         return CommandResult.Success;
     }
@@ -71,10 +80,10 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
     switch (cmd.type) {
       case "CREATE_CHART":
         this.addFigure(cmd.id, cmd.sheetId, cmd.position, cmd.size);
-        this.addChart(cmd.id, cmd.sheetId, cmd.definition);
+        this.addChart(cmd.id, cmd.definition);
         break;
       case "UPDATE_CHART": {
-        this.addChart(cmd.id, cmd.sheetId, cmd.definition);
+        this.addChart(cmd.id, cmd.definition);
         break;
       }
       case "DUPLICATE_SHEET": {
@@ -170,10 +179,17 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
       for (let sheet of data.sheets) {
         // TODO This code is false, if two plugins want ot insert figures on the sheet, it will crash !
         const sheetFigures = this.getters.getFigures(sheet.id);
-        const figures = sheetFigures as FigureData<any>[];
-        for (let figure of figures) {
+        const figures: FigureData<any>[] = [];
+        for (let sheetFigure of sheetFigures) {
+          const figure = sheetFigure as FigureData<any>;
           if (figure && figure.tag === "chart") {
-            figure.data = this.getChartDefinition(figure.id);
+            const data = this.charts[figure.id]?.getDefinition();
+            if (data) {
+              figure.data = data;
+              figures.push(figure);
+            }
+          } else {
+            figures.push(figure);
           }
         }
         sheet.figures = figures;
@@ -234,7 +250,22 @@ export class ChartPlugin extends CorePlugin<ChartState> implements ChartState {
    * Add a chart in the local state. If a chart already exists, this chart is
    * replaced
    */
-  private addChart(id: UID, sheetId: UID, definition: ChartDefinition) {
-    this.history.update("charts", id, this.createChart(id, definition, sheetId));
+  private addChart(id: UID, definition: ChartDefinition) {
+    const sheetId = this.getters.getFigureSheetId(id);
+    if (sheetId) {
+      this.history.update("charts", id, this.createChart(id, definition, sheetId));
+    }
+  }
+
+  private checkChartDuplicate(cmd: CreateChartCommand): CommandResult {
+    return this.getters.getFigureSheetId(cmd.id)
+      ? CommandResult.DuplicatedChartId
+      : CommandResult.Success;
+  }
+
+  private checkChartExists(cmd: UpdateChartCommand): CommandResult {
+    return this.getters.getFigureSheetId(cmd.id)
+      ? CommandResult.Success
+      : CommandResult.ChartDoesNotExist;
   }
 }

src/plugins/core/figures.ts

@@ -14,7 +14,7 @@ interface FigureState {
 }
 
 export class FigurePlugin extends CorePlugin<FigureState> implements FigureState {
-  static getters = ["getFigures", "getFigure"] as const;
+  static getters = ["getFigures", "getFigure", "getFigureSheetId"] as const;
   readonly figures: {
     [sheet: string]: Record<UID, Figure | undefined> | undefined;
   } = {};
@@ -116,6 +116,12 @@ export class FigurePlugin extends CorePlugin<FigureState> implements FigureState
     return this.figures[sheetId]?.[figureId];
   }
 
+  getFigureSheetId(figureId: string): UID | undefined {
+    return Object.keys(this.figures).find(
+      (sheetId) => this.figures[sheetId]?.[figureId] !== undefined
+    );
+  }
+
   // ---------------------------------------------------------------------------
   // Import/Export
   // ---------------------------------------------------------------------------

src/types/commands.ts

@@ -1140,6 +1140,8 @@ export const enum CommandResult {
   NonContinuousTargets,
   DuplicatedFigureId,
   InvalidSelectionStep,
+  DuplicatedChartId,
+  ChartDoesNotExist,
 }
 
 export interface CommandHandler<T> {

src/types/workbook_data.ts

@@ -56,6 +56,7 @@ export interface WorkbookData {
   borders: { [key: number]: Border };
   entities: { [key: string]: { [key: string]: any } };
   revisionId: UID;
+  uniqueFigureIds: boolean;
 }
 
 export interface ExcelWorkbookData extends WorkbookData {