From 0675bc5aa2b03cc1b8c68181d5886882aaa58869 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= Date: Thu, 2 Mar 2023 12:37:31 +0000 Subject: [PATCH] [FIX] figure,chart: Enforce unicity of figure ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 odoo/o-spreadsheet#2166 X-original-commit: f56c6eb8c7f88599ae49e17f137079ca3b925aa4 Signed-off-by: Pierre Rousseau (pro) Signed-off-by: Rémi Rahir (rar) --- src/migrations/data.ts | 66 +++- src/plugins/core/chart.ts | 51 ++- src/plugins/core/figures.ts | 8 +- src/types/commands.ts | 2 + src/types/workbook_data.ts | 1 + tests/__snapshots__/xlsx_export.test.ts.snap | 374 ++++++++++++++++++- tests/data.test.ts | 5 + tests/plugins/chart/basic_chart.test.ts | 58 +++ tests/plugins/figures.test.ts | 8 +- tests/plugins/import_export.test.ts | 52 +++ tests/xlsx_export.test.ts | 4 +- 11 files changed, 584 insertions(+), 45 deletions(-) diff --git a/src/migrations/data.ts b/src/migrations/data.ts index 4a34af5292..6d8e9ea598 100644 --- a/src/migrations/data.ts +++ b/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): Partial { + data = forceUnicityOfFigure(data); + data = setDefaults(data); + return data; +} + +/** + * Force the unicity of figure ids accross sheets + */ +function forceUnicityOfFigure(data: Partial): Partial { + 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): Partial { + 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; } diff --git a/src/plugins/core/chart.ts b/src/plugins/core/chart.ts index 6f6eb6c3cc..2ba8988527 100644 --- a/src/plugins/core/chart.ts +++ b/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 implements ChartState { readonly charts: Record = {}; 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 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 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 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[]; - for (let figure of figures) { + const figures: FigureData[] = []; + for (let sheetFigure of sheetFigures) { + const figure = sheetFigure as FigureData; 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 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; } } diff --git a/src/plugins/core/figures.ts b/src/plugins/core/figures.ts index a9d808b2e5..e601042125 100644 --- a/src/plugins/core/figures.ts +++ b/src/plugins/core/figures.ts @@ -14,7 +14,7 @@ interface FigureState { } export class FigurePlugin extends CorePlugin implements FigureState { - static getters = ["getFigures", "getFigure"] as const; + static getters = ["getFigures", "getFigure", "getFigureSheetId"] as const; readonly figures: { [sheet: string]: Record | undefined; } = {}; @@ -116,6 +116,12 @@ export class FigurePlugin extends CorePlugin 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 // --------------------------------------------------------------------------- diff --git a/src/types/commands.ts b/src/types/commands.ts index 38f3216da5..e00215dfdb 100644 --- a/src/types/commands.ts +++ b/src/types/commands.ts @@ -1140,6 +1140,8 @@ export const enum CommandResult { NonContinuousTargets, DuplicatedFigureId, InvalidSelectionStep, + DuplicatedChartId, + ChartDoesNotExist, } export interface CommandHandler { diff --git a/src/types/workbook_data.ts b/src/types/workbook_data.ts index 867eaf47a8..7a5c072113 100644 --- a/src/types/workbook_data.ts +++ b/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 { diff --git a/tests/__snapshots__/xlsx_export.test.ts.snap b/tests/__snapshots__/xlsx_export.test.ts.snap index 9ccb074d91..4554875427 100644 --- a/tests/__snapshots__/xlsx_export.test.ts.snap +++ b/tests/__snapshots__/xlsx_export.test.ts.snap @@ -3473,16 +3473,18 @@ Object { - - - - - + + + + + + + @@ -3491,13 +3493,11 @@ Object { - - - - + + @@ -3521,6 +3521,11 @@ Object { + + + + + @@ -3529,13 +3534,11 @@ Object { - - - - + + @@ -3558,7 +3561,7 @@ Object { - + @@ -4163,7 +4166,7 @@ Object { ", "contentType": "chart", - "path": "xl/charts/chart1.xml", + "path": "xl/charts/chart2.xml", }, Object { "content": " @@ -4198,7 +4201,7 @@ Object { - + @@ -4368,7 +4371,7 @@ Object { }, Object { "content": " - + ", "contentType": undefined, "path": "xl/drawings/_rels/drawing1.xml.rels", @@ -4388,7 +4391,7 @@ Object { - + @@ -4435,6 +4438,291 @@ Object { + + + + + + + + + + + + + + + + + + + + test + + + + + + + + + + + + + + + + + + + + + + + + + + + Sheet1!B1 + + + + + + + + + + + + + + + Sheet1!A2:A4 + + + + + + + + + Sheet1!B2:B4 + + + + + + + + + + + + + + + + + Sheet1!C1 + + + + + + + + + + + + + + + Sheet1!A2:A4 + + + + + + + + + Sheet1!C2:C4 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +", + "contentType": "chart", + "path": "xl/charts/chart1.xml", + }, + Object { + "content": " + + + + + + + + + + + + @@ -4701,7 +4989,7 @@ Object { ", "contentType": "chart", - "path": "xl/charts/chart1.xml", + "path": "xl/charts/chart2.xml", }, Object { "content": " @@ -4751,6 +5039,52 @@ Object { + + + + 0 + + + 9525 + + + 0 + + + 9525 + + + + + 5 + + + 542925 + + + 14 + + + 133350 + + + + + + + + + + + + + + + + + + + ", "contentType": "drawing", "path": "xl/drawings/drawing0.xml", @@ -4971,6 +5305,7 @@ Object { Object { "content": " + ", "contentType": undefined, "path": "xl/drawings/_rels/drawing0.xml.rels", @@ -4988,6 +5323,7 @@ Object { + diff --git a/tests/data.test.ts b/tests/data.test.ts index c31ee5a9e4..b6c0e0ddf3 100644 --- a/tests/data.test.ts +++ b/tests/data.test.ts @@ -27,6 +27,7 @@ describe("load data", () => { isVisible: true, }, ], + uniqueFigureIds: true, }); expect(load({})).toEqual({ @@ -52,6 +53,7 @@ describe("load data", () => { isVisible: true, }, ], + uniqueFigureIds: true, }); }); @@ -83,6 +85,7 @@ describe("load data", () => { isVisible: true, }, ], + uniqueFigureIds: true, }); }); @@ -114,6 +117,7 @@ describe("load data", () => { isVisible: true, }, ], + uniqueFigureIds: true, }); }); @@ -186,6 +190,7 @@ describe("load data", () => { isVisible: true, }, ], + uniqueFigureIds: true, }); }); }); diff --git a/tests/plugins/chart/basic_chart.test.ts b/tests/plugins/chart/basic_chart.test.ts index ea14221d47..d767af292f 100644 --- a/tests/plugins/chart/basic_chart.test.ts +++ b/tests/plugins/chart/basic_chart.test.ts @@ -577,6 +577,64 @@ describe("datasource tests", function () { expect(result).toBeCancelledBecause(CommandResult.InvalidDataSet); }); + test("cannot duplicate chart ids", () => { + const model = new Model(); + const cmd1 = createChart( + model, + { + dataSets: ["Sheet1!B1:B4"], + labelRange: "Sheet1!A2:A4", + type: "line", + }, + "1" + ); + expect(cmd1).toBeSuccessfullyDispatched(); + + const cmd2 = createChart( + model, + { + dataSets: ["Sheet1!C1:C4"], + labelRange: "Sheet1!A2:A4", + type: "bar", + }, + "1" + ); + expect(cmd2).toBeCancelledBecause(CommandResult.DuplicatedChartId); + createSheet(model, { sheetId: "42" }); + const cmd3 = createChart( + model, + { + dataSets: ["Sheet1!C1:C4"], + labelRange: "Sheet1!A2:A4", + type: "bar", + }, + "1", + "42" + ); + expect(cmd3).toBeCancelledBecause(CommandResult.DuplicatedChartId); + }); + + test("reject updates that target a cinexisting chart", () => { + createChart( + model, + { + dataSets: ["Sheet1!B1:B4"], + labelRange: "Sheet1!A2:A4", + type: "line", + }, + "1" + ); + createSheet(model, { sheetId: "42" }); + const result = model.dispatch("UPDATE_CHART", { + definition: model.getters.getChartDefinition("1"), + sheetId: model.getters.getActiveSheetId(), + id: "2", + }); + + updateChart(model, "1", { legendPosition: "left" }); + expect(result).toBeCancelledBecause(CommandResult.ChartDoesNotExist); + }); + test("chart is not selected after creation and update", () => { const chartId = "1234"; createChart( diff --git a/tests/plugins/figures.test.ts b/tests/plugins/figures.test.ts index c00ba7b468..38f20d9b91 100644 --- a/tests/plugins/figures.test.ts +++ b/tests/plugins/figures.test.ts @@ -391,9 +391,15 @@ describe("figure plugin", () => { createSheet(model, { sheetId: "42" }); const cmd2 = model.dispatch("CREATE_FIGURE", { - sheetId: "42", + sheetId: model.getters.getActiveSheetId(), figure, }); expect(cmd2).toBeCancelledBecause(CommandResult.DuplicatedFigureId); + + const cmd3 = model.dispatch("CREATE_FIGURE", { + sheetId: "42", + figure, + }); + expect(cmd3).toBeCancelledBecause(CommandResult.DuplicatedFigureId); }); }); diff --git a/tests/plugins/import_export.test.ts b/tests/plugins/import_export.test.ts index 0a556f7f96..8acea00be4 100644 --- a/tests/plugins/import_export.test.ts +++ b/tests/plugins/import_export.test.ts @@ -532,6 +532,56 @@ describe("Export", () => { const exp = model.exportData(); expect(exp.sheets![0].cells!.A1!).toEqual({ style: 1 }); }); + + test("chart figures without a definition are not exported", () => { + const model = new Model({ + sheets: [ + { + id: "someuuid", + figures: [ + { + id: "otheruuid", + x: 100, + y: 100, + width: 100, + height: 100, + tag: "chart", + data: { + type: "line", + title: "demo chart", + labelRange: "A1:A4", + dataSets: ["B1:B4", "C1:C4"], + }, + }, + { + id: "id2", + x: 100, + y: 100, + width: 100, + height: 100, + }, + ], + }, + ], + }); + model.dispatch("DELETE_FIGURE", { id: "otheruuid", sheetId: "someuuid" }); + expect(model.exportData()).toMatchObject({ + sheets: [ + { + id: "someuuid", + figures: [ + { + id: "id2", + x: 100, + y: 100, + width: 100, + height: 100, + }, + ], + }, + ], + }); + }); }); test("complete import, then export", () => { @@ -599,6 +649,7 @@ test("complete import, then export", () => { top: ["thin", "#000"] as BorderDescr, }, }, + uniqueFigureIds: true, }; const model = new Model(modelData); expect(model).toExport(modelData); @@ -669,6 +720,7 @@ test("import then export (figures)", () => { styles: {}, formats: {}, borders: {}, + uniqueFigureIds: true, }; const model = new Model(modelData); expect(model).toExport(modelData); diff --git a/tests/xlsx_export.test.ts b/tests/xlsx_export.test.ts index fb153dd8a5..5dbe55557f 100644 --- a/tests/xlsx_export.test.ts +++ b/tests/xlsx_export.test.ts @@ -755,7 +755,7 @@ describe("Test XLSX export", () => { labelRange: "Sheet1!A2:A4", type: "bar", }, - "1" + "2" ); expect(await exportPrettifiedXlsx(model)).toMatchSnapshot(); }); @@ -850,7 +850,7 @@ describe("Test XLSX export", () => { labelRange: "Sheet1!A2:A4", type: "bar", }, - "1", + "2", "42" ); expect(await exportPrettifiedXlsx(model)).toMatchSnapshot();