[IMP] data: match data versions to release versions

This commit fixes two issues at once.

Issue 1
-------
Currently, the migration process will ONLY run if the version of the original
data (in json) is smaller than the last version set in the source code
(`CURRENT_VERSION`). This means that any additional step set outside of the
library is not run without incrementing CURRENT_VERSION in o-spreadsheet.

Example:
- we bump the latest version to 25 in o-spreadsheet;
- we push that value in Odoo
- users start creating spreadsheets in that version so their spreadsheets are
  set to version 25
- later on, we need to add a migration step for whatever reason in Odoo, we
  assign it to 25.1

=> the migration code will check against the latest version in the source (25)
vs the one in the spreadsheet data (25 as well) and will simply skip the
migration.
So 25.1 will never run...

Issue 2
-------
The current scheme of the incremented integer is impractical when it comes
to figure out which odoo version it matches(*).

Knowing the release version is usefull for bug fixes to know which version of
the code generated the json data.

With this commit, data format version are now aligned with odoo versions
Current master is 18.3. Next, it'll be 18.4, 19.0, 19.1, etc.

(*) o-spreadsheet releases follow the same version numbers as odoo releases.

closes #5973

Task: 4659206
Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com>

Author: LucasLefevre

src/helpers/ui/paste_interactive.ts

@@ -1,4 +1,4 @@
-import { CURRENT_VERSION } from "../../migrations/data";
+import { getCurrentVersion } from "../../migrations/data";
 import { _t } from "../../translation";
 import {
   ClipboardPasteOptions,
@@ -75,7 +75,7 @@ export async function interactivePasteFromOS(
   } catch (error) {
     const parsedSpreadsheetContent = parsedClipboardContent.data;
 
-    if (parsedSpreadsheetContent?.version !== CURRENT_VERSION) {
+    if (parsedSpreadsheetContent?.version !== getCurrentVersion()) {
       env.raiseError(
         _t(
           "An unexpected error occurred while pasting content.\

src/migrations/data.ts

@@ -15,11 +15,18 @@ import { XlsxReader } from "../xlsx/xlsx_reader";
 import { migrationStepRegistry } from "./migration_steps";
 
 /**
- * This is the current state version number. It should be incremented each time
- * a breaking change is made in the way the state is handled, and an upgrade
- * function should be defined
+ * Represents the current version of the exported JSON data.
+ * A new version must be created whenever a breaking change is introduced in the export format.
+ * To define a new version, add an upgrade function to `migrationStepRegistry`.
  */
-export const CURRENT_VERSION = 25;
+export function getCurrentVersion() {
+  return getSortedVersions().at(-1)!;
+}
+
+function getSortedVersions() {
+  return migrationStepRegistry.getKeys().sort(compareVersions);
+}
+
 const INITIAL_SHEET_ID = "Sheet1";
 
 /**
@@ -47,7 +54,10 @@ export function load(data?: any, verboseImport?: boolean): WorkbookData {
 
   // apply migrations, if needed
   if ("version" in data) {
-    if (data.version < CURRENT_VERSION) {
+    if (isLegacyVersioning(data)) {
+      data.version = LEGACY_VERSION_MAPPING[data.version];
+    }
+    if (data.version !== getCurrentVersion()) {
       console.debug("Migrating data from version", data.version);
       data = migrate(data);
     }
@@ -58,6 +68,44 @@ export function load(data?: any, verboseImport?: boolean): WorkbookData {
   return data;
 }
 
+const LEGACY_VERSION_MAPPING = {
+  25: "18.2",
+  24: "18.1.1",
+  23: "18.1",
+  22: "18.0.4",
+  21: "18.0.3",
+  20: "18.0.2",
+  19: "18.0.1",
+  18: "18.0",
+  17: "17.4",
+  16: "17.3",
+  15: "17.2",
+  14: "16.4",
+  13: "16.3",
+  12: "15.4",
+
+  // not accurate starting at this point
+  11: "0.10",
+  10: "0.9",
+  9: "0.8",
+  8: "0.7",
+  7: "0.6",
+  6: "0.5",
+  5: "0.4",
+  4: "0.3",
+  3: "0.2",
+  2: "0.1",
+  1: "0",
+};
+
+/**
+ * Versions used to be an incremented integer.
+ * This was later changed to match release versions (matching Odoo release names).
+ */
+function isLegacyVersioning(data: { version: number | string }): boolean {
+  return typeof data.version === "number";
+}
+
 // -----------------------------------------------------------------------------
 // Migrations
 // -----------------------------------------------------------------------------
@@ -83,12 +131,11 @@ function compareVersions(v1: string, v2: string): number {
 
 function migrate(data: any): WorkbookData {
   const start = performance.now();
-  const steps = migrationStepRegistry
-    .getAll()
-    .sort((a, b) => compareVersions(a.versionFrom, b.versionFrom));
-  const index = steps.findIndex((step) => step.versionFrom === data.version.toString());
-  for (let i = index; i < steps.length; i++) {
-    data = steps[i].migrate(data);
+  const versions = getSortedVersions();
+  const index = versions.findIndex((v) => v === data.version);
+  for (let i = index + 1; i < versions.length; i++) {
+    const nextVersion = versions[i];
+    data = migrationStepRegistry.get(nextVersion).migrate(data);
   }
   console.debug("Data migrated in", performance.now() - start, "ms");
   return data;
@@ -131,7 +178,7 @@ function forceUnicityOfFigure(data: Partial<WorkbookData>): Partial<WorkbookData
  */
 function setDefaults(partialData: Partial<WorkbookData>): Partial<WorkbookData> {
   const data: WorkbookData = Object.assign(createEmptyWorkbookData(), partialData, {
-    version: CURRENT_VERSION,
+    version: getCurrentVersion(),
   });
   data.sheets = data.sheets
     ? data.sheets.map((s, i) =>
@@ -291,7 +338,7 @@ export function createEmptySheet(sheetId: UID, name: string): SheetData {
 
 export function createEmptyWorkbookData(sheetName = "Sheet1"): WorkbookData {
   return {
-    version: CURRENT_VERSION,
+    version: getCurrentVersion(),
     sheets: [createEmptySheet(INITIAL_SHEET_ID, sheetName)],
     styles: {},
     formats: {},

src/migrations/migration_steps.ts

@@ -10,26 +10,23 @@ import { normalizeV9 } from "./legacy_tools";
 import { WEEK_START } from "./locale";
 
 export interface MigrationStep {
-  versionFrom: string;
   migrate: (data: any) => any;
 }
 
 export const migrationStepRegistry = new Registry<MigrationStep>();
 
 migrationStepRegistry
-  .add("migration_1", {
+  .add("0.1", {
     // add the `activeSheet` field on data
-    versionFrom: "1",
     migrate(data: any): any {
       if (data.sheets && data.sheets[0]) {
         data.activeSheet = data.sheets[0].name;
       }
       return data;
     },
   })
-  .add("migration_2", {
+  .add("0.2", {
     // add an id field in each sheet
-    versionFrom: "2",
     migrate(data: any): any {
       if (data.sheets && data.sheets.length) {
         for (let sheet of data.sheets) {
@@ -39,9 +36,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_3", {
+  .add("0.3", {
     // activeSheet is now an id, not the name of a sheet
-    versionFrom: "3",
     migrate(data: any): any {
       if (data.sheets && data.activeSheet) {
         const activeSheet = data.sheets.find((s) => s.name === data.activeSheet);
@@ -50,19 +46,17 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_4", {
+  .add("0.4", {
     // add figures object in each sheets
-    versionFrom: "4",
     migrate(data: any): any {
       for (let sheet of data.sheets || []) {
         sheet.figures = sheet.figures || [];
       }
       return data;
     },
   })
-  .add("migration_5", {
+  .add("0.5", {
     // normalize the content of the cell if it is a formula to avoid parsing all the formula that vary only by the cells they use
-    versionFrom: "5",
     migrate(data: any): any {
       for (let sheet of data.sheets || []) {
         for (let xc in sheet.cells || []) {
@@ -75,9 +69,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_6", {
+  .add("0.6", {
     // transform chart data structure
-    versionFrom: "6",
     migrate(data: any): any {
       for (let sheet of data.sheets || []) {
         for (let f in sheet.figures || []) {
@@ -100,9 +93,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_7", {
+  .add("0.7", {
     // remove single quotes in sheet names
-    versionFrom: "7",
     migrate(data: any): any {
       const namesTaken: string[] = [];
       for (let sheet of data.sheets || []) {
@@ -165,9 +157,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_8", {
+  .add("0.8", {
     // transform chart data structure with design attributes
-    versionFrom: "8",
     migrate(data: any): any {
       for (const sheet of data.sheets || []) {
         for (const chart of sheet.figures || []) {
@@ -180,9 +171,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_9", {
+  .add("0.9", {
     // de-normalize formula to reduce exported json size (~30%)
-    versionFrom: "9",
     migrate(data: any): any {
       for (let sheet of data.sheets || []) {
         for (let xc in sheet.cells || []) {
@@ -201,9 +191,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_10", {
+  .add("0.10", {
     // normalize the formats of the cells
-    versionFrom: "10",
     migrate(data: any): any {
       const formats: { [formatId: number]: Format } = {};
       for (let sheet of data.sheets || []) {
@@ -218,26 +207,23 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_11", {
+  .add("15.4", {
     // Add isVisible to sheets
-    versionFrom: "11",
     migrate(data: any): any {
       for (let sheet of data.sheets || []) {
         sheet.isVisible = true;
       }
       return data;
     },
   })
-  .add("migration_12", {
+  .add("15.4.1", {
     // Fix data filter duplication
-    versionFrom: "12",
     migrate(data: any): any {
       return fixOverlappingFilters(data);
     },
   })
-  .add("migration_12_5", {
+  .add("16.3", {
     // Change Border description structure
-    versionFrom: "12.5",
     migrate(data: any): any {
       for (const borderId in data.borders) {
         const border = data.borders[borderId];
@@ -253,9 +239,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_13", {
+  .add("16.4", {
     // Add locale to spreadsheet settings
-    versionFrom: "13",
     migrate(data: any): any {
       if (!data.settings) {
         data.settings = {};
@@ -266,16 +251,14 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_14", {
+  .add("16.4.1", {
     // Fix datafilter duplication (post saas-17.1)
-    versionFrom: "14",
     migrate(data: any): any {
       return fixOverlappingFilters(data);
     },
   })
-  .add("migration_14_5", {
+  .add("17.2", {
     // Rename filterTable to tables
-    versionFrom: "14.5",
     migrate(data: any): any {
       for (const sheetData of data.sheets || []) {
         sheetData.tables = sheetData.tables || sheetData.filterTables || [];
@@ -284,9 +267,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_15", {
+  .add("17.3", {
     // Add pivots
-    versionFrom: "15",
     migrate(data: any): any {
       if (!data.pivots) {
         data.pivots = {};
@@ -297,9 +279,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_16", {
+  .add("17.4", {
     // transform chart data structure (2)
-    versionFrom: "16",
     migrate(data: any): any {
       for (const sheet of data.sheets || []) {
         for (const f in sheet.figures || []) {
@@ -320,18 +301,16 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_17", {
+  .add("18.0", {
     // Empty migration to allow external modules to add their own migration steps
     // before this version
-    versionFrom: "17",
     migrate(data: any): any {
       return data;
     },
   })
-  .add("migration_18", {
+  .add("18.0.1", {
     // Change measures and dimensions `name` to `fieldName`
     // Add id to measures
-    versionFrom: "18",
     migrate(data: any): any {
       interface PivotCoreMeasureV17 {
         name: string;
@@ -362,9 +341,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_19", {
+  .add("18.0.2", {
     // "Add weekStart to locale",
-    versionFrom: "19",
     migrate(data: any): any {
       const locale = data.settings?.locale;
       if (locale) {
@@ -374,9 +352,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_20", {
+  .add("18.0.3", {
     // group style and format into zones,
-    versionFrom: "20",
     migrate(data: any): any {
       for (const sheet of data.sheets || []) {
         sheet.styles = {};
@@ -394,9 +371,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_21", {
+  .add("18.0.4", {
     // "Add operator in gauge inflection points",
-    versionFrom: "21",
     migrate(data: WorkbookData): any {
       for (const sheet of data.sheets || []) {
         for (const figure of sheet.figures || []) {
@@ -415,9 +391,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_22", {
+  .add("18.1", {
     // "tables are no longer inserted with filters by default",
-    versionFrom: "22",
     migrate(data: WorkbookData): any {
       for (const sheet of data.sheets || []) {
         for (const table of sheet.tables || []) {
@@ -429,9 +404,8 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_23", {
+  .add("18.1.1", {
     // Flatten cell content: { content: "value" } -> "value"
-    versionFrom: "23",
     migrate(data: WorkbookData): any {
       for (const sheet of data.sheets || []) {
         for (const xc in sheet.cells) {
@@ -444,12 +418,16 @@ migrationStepRegistry
       return data;
     },
   })
-  .add("migration_24", {
+  .add("18.2", {
     // Empty migration to allow odoo migrate pivot custom sorting.
-    versionFrom: "24",
     migrate(data: WorkbookData): any {
       return data;
     },
+  })
+  .add("18.3", {
+    migrate(data) {
+      return data;
+    },
   });
 
 function fixOverlappingFilters(data: any): any {

src/plugins/ui_stateful/clipboard.ts

@@ -6,7 +6,7 @@ import { SELECTION_BORDER_COLOR } from "../../constants";
 import { getClipboardDataPositions } from "../../helpers/clipboard/clipboard_helpers";
 import { getMaxFigureSize } from "../../helpers/figures/figure/figure";
 import { UuidGenerator, isZoneValid, union } from "../../helpers/index";
-import { CURRENT_VERSION } from "../../migrations/data";
+import { getCurrentVersion } from "../../migrations/data";
 import { _t } from "../../translation";
 import {
   ClipboardData,
@@ -47,7 +47,7 @@ type MinimalClipboardData = {
 };
 
 export interface SpreadsheetClipboardData extends MinimalClipboardData {
-  version?: number;
+  version?: string;
   clipboardId?: string;
 }
 /**
@@ -528,7 +528,7 @@ export class ClipboardPlugin extends UIPlugin {
 
   private getSheetData(): SpreadsheetClipboardData {
     const data = {
-      version: CURRENT_VERSION,
+      version: getCurrentVersion(),
       clipboardId: this.clipboardId,
     };
     if (this.copiedData && "figureId" in this.copiedData) {

src/types/workbook_data.ts

@@ -57,7 +57,7 @@ interface WorkbookSettings {
 type PivotData = { formulaId: string } & PivotCoreDefinition;
 
 export interface WorkbookData {
-  version: number;
+  version: string;
   sheets: SheetData[];
   styles: { [key: number]: Style };
   formats: { [key: number]: Format };

src/xlsx/xlsx_reader.ts

@@ -26,7 +26,7 @@ import { getXLSXFilesOfType } from "./helpers/xlsx_helper";
 import { XLSXImportWarningManager } from "./helpers/xlsx_parser_error_manager";
 import { escapeTagNamespaces, parseXML } from "./helpers/xml_helpers";
 
-const EXCEL_IMPORT_VERSION = 24;
+const EXCEL_IMPORT_VERSION = "18.3";
 
 export class XlsxReader {
   warningManager: XLSXImportWarningManager;

tests/model/data.test.ts

@@ -1,12 +1,12 @@
 import { DEFAULT_REVISION_ID } from "../../src/constants";
-import { CURRENT_VERSION, load } from "../../src/migrations/data";
+import { getCurrentVersion, load } from "../../src/migrations/data";
 import { DEFAULT_LOCALE } from "../../src/types";
 
 describe("load data", () => {
   test("create empty workbookdata when loading nothing", () => {
     const emptyWorkbook = load({});
     expect(emptyWorkbook).toMatchObject({
-      version: CURRENT_VERSION,
+      version: getCurrentVersion(),
       borders: {},
       styles: {},
       formats: {},
@@ -41,7 +41,7 @@ describe("load data", () => {
         sheets: [{ id: "asdf", merges: ["A1:B2"] }],
       })
     ).toMatchObject({
-      version: CURRENT_VERSION,
+      version: getCurrentVersion(),
       borders: {},
       styles: {},
       formats: {},
@@ -73,7 +73,7 @@ describe("load data", () => {
         sheets: [{ name: "Sheet1", merges: ["A1:B2"] }],
       })
     ).toMatchObject({
-      version: CURRENT_VERSION,
+      version: getCurrentVersion(),
       borders: {},
       styles: {},
       formats: {},
@@ -154,7 +154,7 @@ describe("load data", () => {
         sheets: [{ merges: ["A1:B2"] }],
       })
     ).toMatchObject({
-      version: CURRENT_VERSION,
+      version: getCurrentVersion(),
       borders: {},
       styles: {},
       formats: {},
@@ -205,7 +205,7 @@ describe("load data", () => {
         ],
       })
     ).toMatchObject({
-      version: CURRENT_VERSION,
+      version: getCurrentVersion(),
       sheets: [
         {
           figures: [

tests/model/model_import_export.test.ts

@@ -8,7 +8,7 @@ import {
 } from "../../src/constants";
 import { toCartesian, toZone } from "../../src/helpers";
 import { DEFAULT_TABLE_CONFIG } from "../../src/helpers/table_presets";
-import { CURRENT_VERSION } from "../../src/migrations/data";
+import { getCurrentVersion } from "../../src/migrations/data";
 import {
   BorderDescr,
   ColorScaleRule,
@@ -59,7 +59,7 @@ describe("Migrations", () => {
       ],
     });
     const data = model.exportData();
-    expect(data.version).toBe(CURRENT_VERSION);
+    expect(data.version).toBe(getCurrentVersion());
     expect(data.sheets[0].id).toBeDefined();
     expect(data.sheets[0].figures).toBeDefined();
     expect(data.sheets[0].cells.A1).toBe("=A1");
@@ -82,7 +82,7 @@ describe("Migrations", () => {
     });
     const data = model.exportData();
     const cells = data.sheets[0].cells;
-    expect(data.version).toBe(CURRENT_VERSION);
+    expect(data.version).toBe(getCurrentVersion());
     // formulas are de-normalized with version 9
     expect(cells.A1).toBe("=A1");
     expect(cells.A2).toBe("=1");
@@ -450,8 +450,8 @@ describe("Migrations", () => {
       ],
     });
     const data = model.exportData();
-    expect(data.version).toEqual(CURRENT_VERSION);
-    expect(CURRENT_VERSION).toBeGreaterThanOrEqual(14.5);
+    expect(data.version).toEqual(getCurrentVersion());
+    expect(Number(getCurrentVersion())).toBeGreaterThanOrEqual(14.5);
     expect(data.sheets[0].tables).toEqual([
       {
         range: "A1:C2",
@@ -473,7 +473,7 @@ describe("Migrations", () => {
     });
     expect(model.getters.getTables("1")).toMatchObject([{ range: { zone: toZone("A1:B2") } }]);
     let data = model.exportData();
-    expect(data.version).toBe(CURRENT_VERSION);
+    expect(data.version).toBe(getCurrentVersion());
     expect(data.sheets[0].tables).toEqual([
       {
         range: "A1:B2",
@@ -504,7 +504,7 @@ describe("Migrations", () => {
     expect(getCell(model, "A1")?.style).toEqual(style);
     expect(getBorder(model, "A1")).toEqual(border);
     const data = model.exportData();
-    expect(data.version).toBe(CURRENT_VERSION);
+    expect(data.version).toBe(getCurrentVersion());
     expect(data.sheets[0].cells).toEqual({ A1: "hi" });
     expect(data.sheets[0].formats).toEqual({ A1: 1 });
     expect(data.sheets[0].styles).toEqual({ A1: 1 });
@@ -565,7 +565,7 @@ describe("Migrations", () => {
     });
     expect(model.getters.getTables("1")).toMatchObject([{ range: { zone: toZone("A1:B2") } }]);
     const data = model.exportData();
-    expect(data.version).toBe(CURRENT_VERSION);
+    expect(data.version).toBe(getCurrentVersion());
     expect(data.sheets[0].tables).toEqual([
       {
         range: "A1:B2",
@@ -715,7 +715,7 @@ describe("Export", () => {
 
 test("complete import, then export", () => {
   const modelData = {
-    version: CURRENT_VERSION,
+    version: getCurrentVersion(),
     revisionId: DEFAULT_REVISION_ID,
     sheets: [
       {
@@ -810,7 +810,7 @@ test("complete import, then export", () => {
 test("can import cells outside sheet size", () => {
   const sheetId = "someuuid";
   const modelData = {
-    version: CURRENT_VERSION,
+    version: getCurrentVersion(),
     sheets: [
       {
         id: sheetId,
@@ -846,7 +846,7 @@ test("Data of a duplicate sheet are correctly duplicated", () => {
 
 test("import then export (figures)", () => {
   const modelData = {
-    version: CURRENT_VERSION,
+    version: getCurrentVersion(),
     revisionId: DEFAULT_REVISION_ID,
     sheets: [
       {