From 23c0338fce0cbc3ed6dc9128ce102a42f4faafc7 Mon Sep 17 00:00:00 2001 From: Krzysztof Budnik <571316+budnix@users.noreply.github.com> Date: Tue, 8 Jun 2021 15:39:18 +0200 Subject: [PATCH] Fix validator double call issue (#8139) The commit fixes an issue where the validator function is called twice on cell value change. Moreover, the PR fixes a bug where the validity states of the linked workbooks are not updated and fixes an issue where the wrong cells are validated when some index transformation on the table was applied. Issue: #8138 --- .changelogs/8138.json | 7 + .../formulas/__tests__/validation.spec.js | 213 +++++++++++++++--- src/plugins/formulas/engine/register.js | 7 +- src/plugins/formulas/formulas.js | 93 ++++++-- test/helpers/common.js | 7 +- 5 files changed, 265 insertions(+), 62 deletions(-) create mode 100644 .changelogs/8138.json diff --git a/.changelogs/8138.json b/.changelogs/8138.json new file mode 100644 index 00000000000..8d2fc7a56fe --- /dev/null +++ b/.changelogs/8138.json @@ -0,0 +1,7 @@ +{ + "title": "Fixed an issue where the validator function is called twice when the Formulas plugin is enabled.", + "type": "fixed", + "issue": 8138, + "breaking": false, + "framework": "none" +} diff --git a/src/plugins/formulas/__tests__/validation.spec.js b/src/plugins/formulas/__tests__/validation.spec.js index 65a174ef3a9..fb670b8a84a 100644 --- a/src/plugins/formulas/__tests__/validation.spec.js +++ b/src/plugins/formulas/__tests__/validation.spec.js @@ -6,6 +6,7 @@ describe('Formulas general', () => { beforeEach(function() { this.$container = $(`
`).appendTo('body'); + this.$container2 = $(`
`).appendTo('body'); }); afterEach(function() { @@ -14,9 +15,35 @@ describe('Formulas general', () => { } if (this.$container) { - destroy(); + try { + if (this.$container.handsontable('getInstance')) { + destroy(); + } + } catch (e) { + // In some of the test cases we're manually destroying the Handsontable instances, so 'getInstance' may + // throw a post-mortem error. + if (!e.message.includes('instance has been destroyed')) { + throw e; + } + } + this.$container.remove(); } + + if (this.$container2) { + try { + if (this.$container2.handsontable('getInstance')) { + this.$container2.handsontable('getInstance').destroy(); + } + } catch (e) { + // In some of the test cases we're manually destroying the Handsontable instances, so 'getInstance' may + // throw a post-mortem error. + if (!e.message.includes('instance has been destroyed')) { + throw e; + } + } + this.$container2.remove(); + } }); describe('cooperation with validation', () => { @@ -53,100 +80,138 @@ describe('Formulas general', () => { }); it('should validate result of formula for dependant cells properly (formula returns text)', async() => { - const hot = handsontable({ + const beforeValidate = jasmine.createSpy('beforeValidate'); + + handsontable({ data: [ [0, '=A1', '=B1', '=C1'] ], formulas: { engine: HyperFormula }, - type: 'numeric' + type: 'numeric', + beforeValidate, }); - hot.setDataAtCell(0, 0, 'text'); + setDataAtCell(0, 0, 'text'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 1)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); + expect(beforeValidate).toHaveBeenCalledWith('text', 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('text', 0, 1, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('text', 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('text', 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 1)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(true); }); it('should validate result of formula and dependant cells properly (formula returns error)', async() => { - const hot = handsontable({ + const beforeValidate = jasmine.createSpy('beforeValidate'); + + handsontable({ data: [ ['=B1+5', 2, '=A1', '=C1'] ], formulas: { engine: HyperFormula }, - type: 'numeric' + type: 'numeric', + beforeValidate, }); - hot.setDataAtCell(0, 0, '=B1+5a'); + setDataAtCell(0, 0, '=B1+5a'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); + expect(beforeValidate).toHaveBeenCalledWith('#ERROR!', 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#ERROR!', 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#ERROR!', 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(true); - hot.setDataAtCell(0, 0, '=B1+5'); + setDataAtCell(0, 0, '=B1+5'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(false); }); it('should validate result of formula and dependant cells properly (formula create circular dependency)', async() => { - const hot = handsontable({ + const beforeValidate = jasmine.createSpy('beforeValidate'); + + handsontable({ data: [ ['=B1+5', 2, '=A1', '=C1'] ], formulas: { engine: HyperFormula }, - type: 'numeric' + type: 'numeric', + beforeValidate, }); - hot.setDataAtCell(0, 0, '=C1'); + setDataAtCell(0, 0, '=C1'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); + expect(beforeValidate).toHaveBeenCalledWith('#CYCLE!', 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#CYCLE!', 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#CYCLE!', 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(true); - hot.setDataAtCell(0, 0, '=B1+5'); + setDataAtCell(0, 0, '=B1+5'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(false); }); it('should validate result of formula and dependant cells properly (formula create #REF! error)', async() => { - const hot = handsontable({ + const beforeValidate = jasmine.createSpy('beforeValidate'); + + handsontable({ data: [ ['=E1', 2, '=A1', '=C1', 22] ], formulas: { engine: HyperFormula }, - type: 'numeric' + type: 'numeric', + beforeValidate, }); - hot.alter('remove_col', 4); + alter('remove_col', 4); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); + expect(beforeValidate).not.toHaveBeenCalled(); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(false); + + await new Promise(resolve => validateCells(resolve)); + + expect(beforeValidate).toHaveBeenCalledWith('#REF!', 0, 0, 'validateCells', void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#REF!', 0, 2, 'validateCells', void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#REF!', 0, 3, 'validateCells', void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(true); }); it('should not automatically validate changes when the engine is modified from the outside code', async() => { @@ -168,8 +233,7 @@ describe('Formulas general', () => { expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - hot.validateCells(); - await sleep(100); // Validator is asynchronous. + await new Promise(resolve => hot.validateCells(resolve)); expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); @@ -201,5 +265,82 @@ describe('Formulas general', () => { expect(afterValidate).toHaveBeenCalledWith(false, '=A1', 0, 6, 'validateCells', void 0); expect(afterValidate).toHaveBeenCalledWith(false, '12/1/2016', 0, 7, 'validateCells', void 0); }); + + it('should only call the validator once for modified cells', async() => { + const validator1 = jasmine.createSpy('validator1').and.callFake((value, callback) => callback(true)); + const validator2 = jasmine.createSpy('validator2').and.callFake((value, callback) => callback(true)); + + handsontable({ + data: [ + ['=B1+5', 2, '=D1', 'text', 'foo'] + ], + formulas: { + engine: HyperFormula, + sheetName: 'Sheet1' + }, + validator: validator1, + }); + spec().$container2.handsontable({ + data: [ + ['=Sheet1!A1', 2, '=D1', 'D', '=A1+1'] + ], + formulas: { + engine: getPlugin('formulas').engine, + sheetName: 'Sheet2' + }, + validator: validator2, + }); + + setDataAtCell(0, 1, 6); + + await sleep(100); // Validator is asynchronous. + + expect(validator1).toHaveBeenCalledTimes(2); + expect(validator1).toHaveBeenCalledWith(6, jasmine.any(Function)); + expect(validator1).toHaveBeenCalledWith(11, jasmine.any(Function)); + expect(validator2).toHaveBeenCalledTimes(2); + expect(validator2).toHaveBeenCalledWith(11, jasmine.any(Function)); + expect(validator2).toHaveBeenCalledWith(12, jasmine.any(Function)); + + setDataAtCell(0, 4, 'bar'); + + await sleep(100); // Validator is asynchronous. + + expect(validator1).toHaveBeenCalledTimes(3); + expect(validator1).toHaveBeenCalledWith('bar', jasmine.any(Function)); + expect(validator2).toHaveBeenCalledTimes(2); + }); + + it('should validate correct visual cells', async() => { + const beforeValidate = jasmine.createSpy('beforeValidate'); + const hot = handsontable({ + data: [ + ['1', 2, '=D1', 'text1', 'foo1'], + ['2', 2, '=D2', 'text2', 'foo2'], + ['3', 2, '=D3', 'text3', 'foo3'], + ['4', 2, '=D4', 'text4', 'foo4'], + ['5', 2, '=D5', 'text5', '=A1+3'], + ], + formulas: { + engine: HyperFormula + }, + validator(value, callback) { + callback(false); + }, + beforeValidate, + }); + + hot.columnIndexMapper.indexesSequence.setValues([0, 2, 3, 4, 1]); + hot.rowIndexMapper.indexesSequence.setValues([0, 2, 3, 4, 1]); + + render(); + setDataAtCell(0, 0, 6); + + await sleep(100); // Validator is asynchronous. + + expect(beforeValidate).toHaveBeenCalledTimes(2); + expect(beforeValidate).toHaveBeenCalledWith(6, 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(9, 3, 4, void 0, void 0, void 0); + }); }); }); diff --git a/src/plugins/formulas/engine/register.js b/src/plugins/formulas/engine/register.js index d15745fe76e..93cf1ccc6c1 100644 --- a/src/plugins/formulas/engine/register.js +++ b/src/plugins/formulas/engine/register.js @@ -130,13 +130,16 @@ export function registerEngine(engineClass, hotSettings, hotInstance) { } /** + * Returns the list of the Handsontable instances linked to the specific engine instance. + * * @param {object} engine The engine instance. - * @returns {Handsontable[]} Returns an array with Handsontable instances. + * @returns {Map} Returns Map with Handsontable instances. */ export function getRegisteredHotInstances(engine) { const engineRegistry = getEngineRelationshipRegistry(); + const hotInstances = engineRegistry.size === 0 ? [] : Array.from(engineRegistry.get(engine) ?? []); - return engineRegistry.size === 0 ? [] : Array.from(engineRegistry.get(engine) ?? []); + return new Map(hotInstances.map(hot => [hot.getPlugin('formulas').sheetId, hot])); } /** diff --git a/src/plugins/formulas/formulas.js b/src/plugins/formulas/formulas.js index e673b32dbf1..0e413d40da1 100644 --- a/src/plugins/formulas/formulas.js +++ b/src/plugins/formulas/formulas.js @@ -146,10 +146,12 @@ export class Formulas extends BasePlugin { this.addHook('afterLoadData', (...args) => this.onAfterLoadData(...args)); this.addHook('modifyData', (...args) => this.onModifyData(...args)); this.addHook('modifySourceData', (...args) => this.onModifySourceData(...args)); - this.addHook('afterSetSourceDataAtCell', (...args) => this.onAfterSetSourceDataAtCell(...args)); - this.addHook('beforeChange', (...args) => this.onBeforeChange(...args)); this.addHook('beforeValidate', (...args) => this.onBeforeValidate(...args)); + this.addHook('afterSetSourceDataAtCell', (...args) => this.onAfterSetSourceDataAtCell(...args)); + this.addHook('afterSetDataAtCell', (...args) => this.onAfterSetDataAtCell(...args)); + this.addHook('afterSetDataAtRowProp', (...args) => this.onAfterSetDataAtCell(...args)); + this.addHook('beforeCreateRow', (...args) => this.onBeforeCreateRow(...args)); this.addHook('beforeCreateCol', (...args) => this.onBeforeCreateCol(...args)); @@ -386,13 +388,13 @@ export class Formulas extends BasePlugin { * recalculated dependent cells. * * @private - * @param {object[]} changedCells The values and location of applied changes within HF engine. + * @param {object[]} dependentCells The values and location of applied changes within HF engine. * @param {boolean} [renderSelf] `true` if it's supposed to render itself, `false` otherwise. */ - renderDependentSheets(changedCells, renderSelf = false) { + renderDependentSheets(dependentCells, renderSelf = false) { const affectedSheetIds = new Set(); - changedCells.forEach((change) => { + dependentCells.forEach((change) => { // For the Named expression the address is empty, hence the `sheetId` is undefined. const sheetId = change?.address?.sheet; @@ -400,22 +402,10 @@ export class Formulas extends BasePlugin { if (!affectedSheetIds.has(sheetId)) { affectedSheetIds.add(sheetId); } - - if (!this.#internalOperationPending && sheetId === this.sheetId) { - const { row, col } = change.address; - - // It will just re-render certain cell when necessary. - this.hot.validateCell(this.hot.getDataAtCell(row, col), this.hot.getCellMeta(row, col), () => {}); - } } }); - const hotInstances = new Map( - getRegisteredHotInstances(this.engine) - .map(hot => [hot.getPlugin('formulas').sheetId, hot]) - ); - - hotInstances.forEach((relatedHot, sheetId) => { + getRegisteredHotInstances(this.engine).forEach((relatedHot, sheetId) => { if ( (renderSelf || (sheetId !== this.sheetId)) && affectedSheetIds.has(sheetId) @@ -426,6 +416,48 @@ export class Formulas extends BasePlugin { }); } + /** + * Validates dependent cells based on the cells that are modified by the change. + * + * @private + * @param {object[]} dependentCells The values and location of applied changes within HF engine. + * @param {object[]} [changedCells] The values and location of applied changes by developer (through API or UI). + */ + validateDependentCells(dependentCells, changedCells = []) { + const stringifyAddress = (change) => { + const { + row, + col, + sheet + } = change?.address ?? {}; + + return isDefined(sheet) ? `${sheet}:${row}x${col}` : ''; + }; + const changedCellsSet = new Set(changedCells.map(change => stringifyAddress(change))); + + dependentCells.forEach((change) => { + // For the Named expression the address is empty, hence the `sheetId` is undefined. + const sheetId = change?.address?.sheet; + const addressId = stringifyAddress(change); + + // Validate the cells that depend on the calculated formulas. Skip that cells + // where the user directly changes the values - the Core triggers those validators. + if (sheetId !== void 0 && !changedCellsSet.has(addressId)) { + const { row, col } = change.address; + const visualRow = this.hot.toVisualRow(row); + const visualColumn = this.hot.toVisualColumn(col); + const hot = getRegisteredHotInstances(this.engine).get(sheetId); + + // It will just re-render certain cell when necessary. + hot.validateCell( + hot.getDataAtCell(visualRow, visualColumn), + hot.getCellMeta(visualRow, visualColumn), + () => {} + ); + } + }); + } + /** * Sync a change from the change-related hooks with the engine. * @@ -471,7 +503,10 @@ export class Formulas extends BasePlugin { sheet: this.sheetId, }; - return this.engine.getCellValue(address); + const cellValue = this.engine.getCellValue(address); + + // If `cellValue` is an object it is expected to be an error + return (typeof cellValue === 'object' && cellValue !== null) ? cellValue.value : cellValue; } return value; @@ -623,24 +658,34 @@ export class Formulas extends BasePlugin { } /** - * `onBeforeChange` hook callback. + * `onAfterSetDataAtCell` hook callback. * * @private * @param {Array[]} changes An array of changes in format [[row, prop, oldValue, value], ...]. */ - onBeforeChange(changes) { + onAfterSetDataAtCell(changes) { const dependentCells = []; const outOfBoundsChanges = []; + const changedCells = []; changes.forEach(([row, prop, , newValue]) => { const column = this.hot.propToCol(prop); + const physicalRow = this.hot.toPhysicalRow(row); + const physicalColumn = this.hot.toPhysicalColumn(column); + const address = { + row: physicalRow, + col: physicalColumn, + sheet: this.sheetId, + }; - if (this.hot.toPhysicalRow(row) !== null && this.hot.toPhysicalColumn(column) !== null) { + if (physicalRow !== null && physicalColumn !== null) { dependentCells.push(...this.syncChangeWithEngine(row, column, newValue)); } else { outOfBoundsChanges.push([row, column, newValue]); } + + changedCells.push({ address }); }); if (outOfBoundsChanges.length) { @@ -658,6 +703,7 @@ export class Formulas extends BasePlugin { } this.renderDependentSheets(dependentCells); + this.validateDependentCells(dependentCells, changedCells); } /** @@ -668,6 +714,7 @@ export class Formulas extends BasePlugin { */ onAfterSetSourceDataAtCell(changes) { const dependentCells = []; + const changedCells = []; changes.forEach(([row, column, , newValue]) => { const address = { @@ -682,10 +729,12 @@ export class Formulas extends BasePlugin { return; } + changedCells.push({ address }); dependentCells.push(...this.engine.setCellContents(address, newValue)); }); this.renderDependentSheets(dependentCells); + this.validateDependentCells(dependentCells, changedCells); } /** diff --git a/test/helpers/common.js b/test/helpers/common.js index c22ca03a9d2..b117509565e 100644 --- a/test/helpers/common.js +++ b/test/helpers/common.js @@ -92,6 +92,7 @@ export const getSelected = handsontableMethodFactory('getSelected'); export const getSelectedLast = handsontableMethodFactory('getSelectedLast'); export const getSelectedRange = handsontableMethodFactory('getSelectedRange'); export const getSelectedRangeLast = handsontableMethodFactory('getSelectedRangeLast'); +export const getSettings = handsontableMethodFactory('getSettings'); export const getSourceData = handsontableMethodFactory('getSourceData'); export const getSourceDataArray = handsontableMethodFactory('getSourceDataArray'); export const getSourceDataAtCell = handsontableMethodFactory('getSourceDataAtCell'); @@ -114,14 +115,16 @@ export const selectColumns = handsontableMethodFactory('selectColumns'); export const selectRows = handsontableMethodFactory('selectRows'); export const setCellMeta = handsontableMethodFactory('setCellMeta'); export const setDataAtCell = handsontableMethodFactory('setDataAtCell'); -export const setSourceDataAtCell = handsontableMethodFactory('setSourceDataAtCell'); export const setDataAtRowProp = handsontableMethodFactory('setDataAtRowProp'); +export const setSourceDataAtCell = handsontableMethodFactory('setSourceDataAtCell'); export const spliceCellsMeta = handsontableMethodFactory('spliceCellsMeta'); export const spliceCol = handsontableMethodFactory('spliceCol'); export const spliceRow = handsontableMethodFactory('spliceRow'); export const toVisualRow = handsontableMethodFactory('toVisualRow'); -export const updateSettings = handsontableMethodFactory('updateSettings'); export const undo = handsontableMethodFactory('undo'); +export const updateSettings = handsontableMethodFactory('updateSettings'); +export const validateCell = handsontableMethodFactory('validateCell'); +export const validateCells = handsontableMethodFactory('validateCells'); const specContext = {};