Skip to content

Commit

Permalink
[FIX] evaluation: evaluateFormula no longer throws
Browse files Browse the repository at this point in the history 
The getter `evaluateFormula` required to be wrapped in a try/catch
statement to handle the evaluation errors but as this is quite error
prone, we decided to change it to no longer throw. The getter will
instead the value of the error that was previously thrown.

closes #3371

Task: 3576149
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
  • Loading branch information
@rrahir
rrahir committed Jan 4, 2024
1 parent 7d54be7 commit ba682a9
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 70 deletions.
24 changes: 16 additions & 8 deletions src/plugins/ui/evaluation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
EvalContext,
Format,
FormattedValue,
FunctionReturnValue,
Getters,
invalidateEvaluationCommands,
MatrixArg,
Expand Down Expand Up @@ -83,15 +84,22 @@ export class EvaluationPlugin extends UIPlugin {
// Getters
// ---------------------------------------------------------------------------

evaluateFormula(formulaString: string, sheetId: UID = this.getters.getActiveSheetId()): any {
const compiledFormula = compile(formulaString);
const params = this.getCompilationParameters(() => {});

const ranges: Range[] = [];
for (let xc of compiledFormula.dependencies) {
ranges.push(this.getters.getRangeFromSheetXC(sheetId, xc));
evaluateFormula(
formulaString: string,
sheetId: UID = this.getters.getActiveSheetId()
): FunctionReturnValue | null {
try {
const compiledFormula = compile(formulaString);
const params = this.getCompilationParameters(() => {});

const ranges: Range[] = [];
for (let xc of compiledFormula.dependencies) {
ranges.push(this.getters.getRangeFromSheetXC(sheetId, xc));
}
return compiledFormula.execute(ranges, ...params).value;
} catch (error) {
return error instanceof EvaluationError ? error.errorType : CellErrorType.GenericError;
}
return compiledFormula.execute(ranges, ...params).value;
}

/**
Expand Down
62 changes: 29 additions & 33 deletions src/plugins/ui/evaluation_conditional_format.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,41 +125,37 @@ export class EvaluationConditionalFormatPlugin extends UIPlugin {
this.computedIcons[activeSheetId] = {};
const computedStyle = this.computedStyles[activeSheetId];
for (let cf of this.getters.getConditionalFormats(activeSheetId).reverse()) {
try {
switch (cf.rule.type) {
case "ColorScaleRule":
for (let range of cf.ranges) {
this.applyColorScale(range, cf.rule);
}
break;
case "IconSetRule":
for (let range of cf.ranges) {
this.applyIcon(range, cf.rule);
}
break;
default:
for (let ref of cf.ranges) {
const zone: Zone = this.getters.getRangeFromSheetXC(activeSheetId, ref).zone;
for (let row = zone.top; row <= zone.bottom; row++) {
for (let col = zone.left; col <= zone.right; col++) {
const pr: (cell: Cell | undefined, rule: CellIsRule) => boolean =
this.rulePredicate[cf.rule.type];
let cell = this.getters.getCell(activeSheetId, col, row);
if (pr && pr(cell, cf.rule)) {
if (!computedStyle[col]) computedStyle[col] = [];
// we must combine all the properties of all the CF rules applied to the given cell
computedStyle[col][row] = Object.assign(
computedStyle[col]?.[row] || {},
cf.rule.style
);
}
switch (cf.rule.type) {
case "ColorScaleRule":
for (let range of cf.ranges) {
this.applyColorScale(range, cf.rule);
}
break;
case "IconSetRule":
for (let range of cf.ranges) {
this.applyIcon(range, cf.rule);
}
break;
default:
for (let ref of cf.ranges) {
const zone: Zone = this.getters.getRangeFromSheetXC(activeSheetId, ref).zone;
for (let row = zone.top; row <= zone.bottom; row++) {
for (let col = zone.left; col <= zone.right; col++) {
const pr: (cell: Cell | undefined, rule: CellIsRule) => boolean =
this.rulePredicate[cf.rule.type];
let cell = this.getters.getCell(activeSheetId, col, row);
if (pr && pr(cell, cf.rule)) {
if (!computedStyle[col]) computedStyle[col] = [];
// we must combine all the properties of all the CF rules applied to the given cell
computedStyle[col][row] = Object.assign(
computedStyle[col]?.[row] || {},
cf.rule.style
);
}
}
}
break;
}
} catch (_) {
// we don't care about the errors within the evaluation of a rule
}
break;
}
}
}
Expand Down Expand Up @@ -188,7 +184,7 @@ export class EvaluationConditionalFormatPlugin extends UIPlugin {
return percentile(rangeValues, Number(threshold.value) / 100, true);
case "formula":
const value = threshold.value && this.getters.evaluateFormula(threshold.value);
return !(value instanceof Promise) ? value : null;
return typeof value === "number" ? value : null;
default:
return null;
}
Expand Down
45 changes: 30 additions & 15 deletions tests/formulas/compiler.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { compile } from "../../src/formulas/index";
import { functionRegistry } from "../../src/functions";
import { createRange } from "../../src/helpers";
import { ArgType, CompiledFormula } from "../../src/types";
import { getCellError, setCellContent } from "../test_helpers";
import { evaluateCell, evaluateCellFormat, restoreDefaultFunctions } from "../test_helpers/helpers";

function compiledBaseFunction(formula: string): CompiledFormula {
Expand Down Expand Up @@ -283,47 +284,61 @@ describe("compile functions", () => {

const m = new Model();

expect(() => m.getters.evaluateFormula("=?ANYEXPECTED(A1:A2)")).toThrowError(
setCellContent(m, "B1", "=?ANYEXPECTED(A1:A2)");
setCellContent(m, "B2", "=BOOLEANEXPECTED(A1:A2)");
setCellContent(m, "B3", "=DATEEXPECTED(A1:A2)");
setCellContent(m, "B4", "=NUMBEREXPECTED(A1:A2)");
setCellContent(m, "B5", "=STRINGEXPECTED(A1:A2)");
setCellContent(m, "B6", "=ANYEXPECTED(A1:A$2)");
setCellContent(m, "B7", "=ANYEXPECTED(sheet2!A1:A$2)");
setCellContent(m, "B8", "=A2:A3");
setCellContent(m, "B9", "=+A2:A3");
setCellContent(m, "B10", "=A1+A2:A3");
setCellContent(m, "B11", "=-A2:A3");
setCellContent(m, "B12", "=A1-A2:A3");
setCellContent(m, "B13", "=A1+A4*A5:A6-A2");
setCellContent(m, "B14", "=ANYEXPECTED(A1:A1)");

expect(getCellError(m, "B1")).toBe(
"Function ANYEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
);
expect(() => m.getters.evaluateFormula("=BOOLEANEXPECTED(A1:A2)")).toThrowError(
expect(getCellError(m, "B2")).toBe(
"Function BOOLEANEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
);
expect(() => m.getters.evaluateFormula("=DATEEXPECTED(A1:A2)")).toThrowError(
expect(getCellError(m, "B3")).toBe(
"Function DATEEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
);
expect(() => m.getters.evaluateFormula("=NUMBEREXPECTED(A1:A2)")).toThrowError(
expect(getCellError(m, "B4")).toBe(
"Function NUMBEREXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
);
expect(() => m.getters.evaluateFormula("=STRINGEXPECTED(A1:A2)")).toThrowError(
expect(getCellError(m, "B5")).toBe(
"Function STRINGEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
);

expect(() => m.getters.evaluateFormula("=ANYEXPECTED(A1:A$2)")).toThrowError(
expect(getCellError(m, "B6")).toBe(
"Function ANYEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
);
expect(() => m.getters.evaluateFormula("=ANYEXPECTED(sheet2!A1:A$2)")).toThrowError(
expect(getCellError(m, "B7")).toBe(
"Function ANYEXPECTED expects the parameter 1 to be a single value or a single cell reference, not a range."
);
expect(() => m.getters.evaluateFormula("=A2:A3")).toThrowError(
expect(getCellError(m, "B8")).toBe(
"Function EQ expects its parameters to be single values or single cell references, not ranges."
);
expect(() => m.getters.evaluateFormula("=+A2:A3")).toThrowError(
expect(getCellError(m, "B9")).toBe(
"Function UPLUS expects its parameters to be single values or single cell references, not ranges."
);
expect(() => m.getters.evaluateFormula("=A1+A2:A3")).toThrowError(
expect(getCellError(m, "B10")).toBe(
"Function ADD expects its parameters to be single values or single cell references, not ranges."
);
expect(() => m.getters.evaluateFormula("=-A2:A3")).toThrowError(
expect(getCellError(m, "B11")).toBe(
"Function UMINUS expects its parameters to be single values or single cell references, not ranges."
);
expect(() => m.getters.evaluateFormula("=A1-A2:A3")).toThrowError(
expect(getCellError(m, "B12")).toBe(
"Function MINUS expects its parameters to be single values or single cell references, not ranges."
);
expect(() => m.getters.evaluateFormula("=A1+A4*A5:A6-A2")).toThrowError(
expect(getCellError(m, "B13")).toBe(
"Function MULTIPLY expects its parameters to be single values or single cell references, not ranges."
);
expect(() => m.getters.evaluateFormula("=ANYEXPECTED(A1:A1)")).not.toThrow();
expect(getCellError(m, "B14")).toBeUndefined();
});
});

Expand Down
8 changes: 4 additions & 4 deletions tests/functions/module_lookup.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ describe("COLUMN formula", () => {
test("functional test without grid context", () => {
const model = new Model();
setCellContent(model, "A1", "kikoulol");
expect(() => model.getters.evaluateFormula("=COLUMN()")).toThrow();
expect(() => model.getters.evaluateFormula("=COLUMN(A1)")).not.toThrow();
expect(model.getters.evaluateFormula("=COLUMN()")).toBe("#ERROR");
expect(model.getters.evaluateFormula("=COLUMN(A1)")).toBe(1);
});

test("functional tests on cell arguments", () => {
Expand Down Expand Up @@ -330,8 +330,8 @@ describe("ROW formula", () => {
test("functional test without grid context", () => {
const model = new Model();
setCellContent(model, "A1", "kikoulol");
expect(() => model.getters.evaluateFormula("=ROW()")).toThrow();
expect(() => model.getters.evaluateFormula("=ROW(A1)")).not.toThrow();
expect(model.getters.evaluateFormula("=ROW()")).toBe("#ERROR");
expect(model.getters.evaluateFormula("=ROW(A1)")).toBe(1);
});

test("functional tests on cell arguments", () => {
Expand Down
13 changes: 5 additions & 8 deletions tests/plugins/evaluation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,11 +360,9 @@ describe("evaluateCells", () => {
expect(evaluateCell("A1", { A1: "=IF(A2<>0,1+1,sum(A2,A3))", A2: "0", A3: "10" })).toBe(10);
});

test("evaluate formula throws when we pass an invalid formula", () => {
test("evaluate formula returns the cell error value when we pass an invalid formula", () => {
let model = new Model();
expect(() => {
model.getters.evaluateFormula("=min(abc)");
}).toThrow();
expect(model.getters.evaluateFormula("=min(abc)")).toBe("#ERROR");
});

test("various expressions with boolean", () => {
Expand Down Expand Up @@ -1075,19 +1073,18 @@ describe("evaluate formula getter", () => {
expect(model.getters.evaluateFormula("=Sheet2!A1")).toBe(11);
});

// i think these formulas should throw
test("in a not existing sheet", () => {
expect(() => model.getters.evaluateFormula("=Sheet99!A1")).toThrow();
expect(model.getters.evaluateFormula("=Sheet99!A1")).toBe("#ERROR");
});

test("evaluate a cell in error", () => {
setCellContent(model, "A1", "=mqsdlkjfqsdf(((--");
expect(() => model.getters.evaluateFormula("=A1")).toThrow();
expect(model.getters.evaluateFormula("=A1")).toBe("#NAME?");
});

test("evaluate an invalid formula", () => {
setCellContent(model, "A1", "=min(abc)");
expect(() => model.getters.evaluateFormula("=A1")).toThrow();
expect(model.getters.evaluateFormula("=A1")).toBe("#BAD_EXPR");
});

test("EVALUATE_CELLS with no argument re-evaluate all the cells", () => {
Expand Down
4 changes: 2 additions & 2 deletions tests/xlsx_export.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -465,12 +465,12 @@ describe("Test XLSX export", () => {
{
id: "14",
ranges: ["A1:A5"],
rule: { type: "CellIsRule", operator: "IsEmpty", style },
rule: { type: "CellIsRule", operator: "IsEmpty", values: [], style },
},
{
id: "15",
ranges: ["A1:A5"],
rule: { type: "CellIsRule", operator: "IsNotEmpty", style },
rule: { type: "CellIsRule", operator: "IsNotEmpty", values: [], style },
},
{
id: "16",
Expand Down

0 comments on commit ba682a9

Please sign in to comment.