diff --git a/app/client/models/entities/ColumnRec.ts b/app/client/models/entities/ColumnRec.ts index 228694e2..fa453ca6 100644 --- a/app/client/models/entities/ColumnRec.ts +++ b/app/client/models/entities/ColumnRec.ts @@ -18,6 +18,9 @@ export interface ColumnRec extends IRowModel<"_grist_Tables_column"> { // Is a real formula column (not an empty column; i.e. contains a non-empty formula). isRealFormula: ko.Computed; + // Is a trigger formula column (not formula, but contains non-empty formula) + hasTriggerFormula: ko.Computed; + // Used for transforming a column. // Reference to the original column for a transform column, or to itself for a non-transforming column. origColRef: ko.Observable; @@ -56,6 +59,8 @@ export function createColumnRec(this: ColumnRec, docModel: DocModel): void { // Is this a real formula column (not an empty column; i.e. contains a non-empty formula). this.isRealFormula = ko.pureComputed(() => this.isFormula() && this.formula() !== ''); + // If this column has a trigger formula defined + this.hasTriggerFormula = ko.pureComputed(() => !this.isFormula() && this.formula() !== ''); // Used for transforming a column. // Reference to the original column for a transform column, or to itself for a non-transforming column. diff --git a/app/client/ui/FieldConfig.ts b/app/client/ui/FieldConfig.ts index b3292c83..9c221c1e 100644 --- a/app/client/ui/FieldConfig.ts +++ b/app/client/ui/FieldConfig.ts @@ -158,7 +158,8 @@ function createFormulaErrorObs(owner: MultiHolder, gristDoc: GristDoc, origColum function countErrors() { if (owner.isDisposed()) { return; } const tableData = gristDoc.docData.getTable(origColumn.table.peek().tableId.peek()); - if (tableData && origColumn.isRealFormula.peek()) { + const isFormula = origColumn.isRealFormula.peek() || origColumn.hasTriggerFormula.peek(); + if (tableData && isFormula) { const colId = origColumn.colId.peek(); const numCells = tableData.getColValues(colId)?.length || 0; const numErrors = tableData.countErrors(colId) || 0; diff --git a/app/client/widgets/FieldEditor.ts b/app/client/widgets/FieldEditor.ts index 81e65346..f54cb4b9 100644 --- a/app/client/widgets/FieldEditor.ts +++ b/app/client/widgets/FieldEditor.ts @@ -469,13 +469,16 @@ function getFormulaError( gristDoc: GristDoc, editRow: DataRowModel, column: ColumnRec ): Observable|undefined { const colId = column.colId.peek(); - let formulaError: Observable|undefined; const cellCurrentValue = editRow.cells[colId].peek(); - if (column.isFormula() && isRaisedException(cellCurrentValue)) { - const fv = formulaError = Observable.create(null, cellCurrentValue); + const isFormula = column.isFormula() || column.hasTriggerFormula(); + if (isFormula && isRaisedException(cellCurrentValue)) { + const formulaError = Observable.create(null, cellCurrentValue); gristDoc.docData.getFormulaError(column.table().tableId(), colId, editRow.getRowId()) - .then(value => { fv.set(value); }) + .then(value => { + formulaError.set(value); + }) .catch(reportError); + return formulaError; } - return formulaError; + return undefined; } diff --git a/app/client/widgets/FormulaEditor.ts b/app/client/widgets/FormulaEditor.ts index 8dd39398..119b567f 100644 --- a/app/client/widgets/FormulaEditor.ts +++ b/app/client/widgets/FormulaEditor.ts @@ -8,7 +8,9 @@ import {createMobileButtons, getButtonMargins} from 'app/client/widgets/EditorBu import {EditorPlacement, ISize} from 'app/client/widgets/EditorPlacement'; import {NewBaseEditor, Options} from 'app/client/widgets/NewBaseEditor'; import {undef} from 'app/common/gutil'; -import {dom, Observable, styled} from 'grainjs'; +import {Computed, dom, Observable, styled} from 'grainjs'; +import {isRaisedException} from "app/common/gristTypes"; +import {decodeObject, RaisedException} from "app/plugin/objtypes"; // How wide to expand the FormulaEditor when an error is shown in it. const minFormulaErrorWidth = 400; @@ -59,8 +61,28 @@ export class FormulaEditor extends NewBaseEditor { : options.commands; this._commandGroup = this.autoDispose(createGroup(allCommands, this, options.field.editingFormula)); - const hideErrDetails = Observable.create(null, true); - const formulaError = options.formulaError; + const hideErrDetails = Observable.create(this, true); + const raisedException = Computed.create(this, use => { + if (!options.formulaError) { + return null; + } + const error = isRaisedException(use(options.formulaError)) ? + decodeObject(use(options.formulaError)) as RaisedException: + new RaisedException(["Unknown error"]); + return error; + }); + const errorText = Computed.create(this, raisedException, (_, error) => { + if (!error) { + return ""; + } + return error.message ? `${error.name} : ${error.message}` : error.name; + }); + const errorDetails = Computed.create(this, raisedException, (_, error) => { + if (!error) { + return ""; + } + return error.details ?? ""; + }); this.autoDispose(this._formulaEditor); this._dom = dom('div.default_editor', @@ -103,21 +125,27 @@ export class FormulaEditor extends NewBaseEditor { aceObj.once("change", () => options.field.editingFormula(true)); }) ), - (formulaError ? + (options.formulaError ? dom('div.error_box', dom('div.error_msg', testId('formula-error-msg'), dom.on('click', () => { - hideErrDetails.set(!hideErrDetails.get()); - this._formulaEditor.resize(); + if (errorDetails.get()){ + hideErrDetails.set(!hideErrDetails.get()); + this._formulaEditor.resize(); + } }), - dom.domComputed(hideErrDetails, (hide) => cssCollapseIcon(hide ? 'Expand' : 'Collapse')), - dom.text((use) => { const f = use(formulaError) as string[]; return `${f[1]}: ${f[2]}`; }), - ), - dom('div.error_details', - dom.hide(hideErrDetails), - dom.text((use) => (use(formulaError) as string[])[3]), - testId('formula-error-details'), + dom.maybe(errorDetails, () => + dom.domComputed(hideErrDetails, (hide) => cssCollapseIcon(hide ? 'Expand' : 'Collapse')) + ), + dom.text(errorText), ), + dom.maybe(errorDetails, () => + dom('div.error_details', + dom.hide(hideErrDetails), + dom.text(errorDetails), + testId('formula-error-details'), + ) + ) ) : null ) ); diff --git a/app/plugin/objtypes.ts b/app/plugin/objtypes.ts index 611dc2c9..dd9c1710 100644 --- a/app/plugin/objtypes.ts +++ b/app/plugin/objtypes.ts @@ -67,7 +67,21 @@ export class ReferenceList { * optional details. */ export class RaisedException { - constructor(public name: string, public message?: string, public details?: string) {} + public name: string; + public details?: string; + public message?: string; + public user_input?: CellValue; + + constructor(list: any[]) { + if (!list.length) { + throw new Error("RaisedException requires a name as first element"); + } + list = [...list]; + this.name = list.shift(); + this.message = list.shift(); + this.details = list.shift(); + this.user_input = list.shift()?.u; + } /** * This is designed to look somewhat similar to Excel, e.g. #VALUE or #DIV/0!" @@ -195,7 +209,7 @@ export function decodeObject(value: CellValue): unknown { switch (code) { case 'D': return GristDateTime.fromGristValue(args[0], String(args[1])); case 'd': return GristDate.fromGristValue(args[0]); - case 'E': return new RaisedException(args[0], args[1], args[2]); + case 'E': return new RaisedException(args); case 'L': return (args as CellValue[]).map(decodeObject); case 'O': return mapValues(args[0] as {[key: string]: CellValue}, decodeObject, {sort: true}); case 'P': return new PendingValue(); diff --git a/sandbox/grist/column.py b/sandbox/grist/column.py index c3e477cd..7cf8f79d 100644 --- a/sandbox/grist/column.py +++ b/sandbox/grist/column.py @@ -138,7 +138,7 @@ class BaseColumn(object): """ self.set(row_id, self.getdefault()) - def get_cell_value(self, row_id): + def get_cell_value(self, row_id, restore=False): """ Returns the "rich" value for the given row_id, i.e. the value that would be seen by formulas. E.g. for ReferenceColumns it'll be the referred-to Record object. For cells containing @@ -146,8 +146,13 @@ class BaseColumn(object): error, this will re-raise that error. """ raw = self.raw_get(row_id) + if isinstance(raw, objtypes.RaisedException): - if isinstance(raw.error, depend.CircularRefError): + # For trigger formulas, we want to restore the previous value to recalculate + # the cell one more time. + if restore and raw.has_user_input(): + raw = raw.user_input + elif isinstance(raw.error, depend.CircularRefError): # Wrapping a CircularRefError in a CellError is often redundant, but # TODO a CellError is still useful if the calling cell is not involved in the cycle raise raw.error diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index df64ad02..63f41079 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -642,7 +642,16 @@ class Engine(object): col = table.get_column(col_id) checkpoint = self._get_undo_checkpoint() try: - return self._recompute_one_cell(None, table, col, row_id) + result = self._recompute_one_cell(None, table, col, row_id) + # If the error is gone for a trigger formula + if col.has_formula() and not col.is_formula(): + if not isinstance(result, objtypes.RaisedException): + # Get the error stored in the cell + # and change it to show to the user that no traceback is available + error_in_cell = objtypes.decode_object(col.raw_get(row_id)) + assert isinstance(error_in_cell, objtypes.RaisedException) + return error_in_cell.no_traceback() + return result finally: # It is possible for formula evaluation to have side-effects that produce DocActions (e.g. # lookupOrAddDerived() creates those). In case of get_formula_error(), these aren't fully @@ -775,7 +784,8 @@ class Engine(object): if is_first: self._is_node_exception_reported.add(node) log.info(value.details) - value = objtypes.RaisedException(value.error) # strip out details after logging + # strip out details after logging + value = objtypes.RaisedException(value.error, user_input=value.user_input) # TODO: validation columns should be wrapped to always return True/False (catching # exceptions), so that we don't need special handling here. @@ -831,11 +841,13 @@ class Engine(object): checkpoint = self._get_undo_checkpoint() record = table.Record(row_id, table._identity_relation) + value = None try: if cycle: raise depend.CircularRefError("Circular Reference") if not col.is_formula(): - result = col.method(record, table.user_table, col.get_cell_value(int(record)), self._user) + value = col.get_cell_value(int(record), restore=True) + result = col.method(record, table.user_table, value, self._user) else: result = col.method(record, table.user_table) if self._cell_required_error: @@ -866,7 +878,10 @@ class Engine(object): self.formula_tracer(col, record) include_details = (node not in self._is_node_exception_reported) if node else True - return objtypes.RaisedException(regular_error, include_details) + if not col.is_formula(): + return objtypes.RaisedException(regular_error, include_details, user_input=value) + else: + return objtypes.RaisedException(regular_error, include_details) def convert_action_values(self, action): """ diff --git a/sandbox/grist/objtypes.py b/sandbox/grist/objtypes.py index 729dce1c..70d36199 100644 --- a/sandbox/grist/objtypes.py +++ b/sandbox/grist/objtypes.py @@ -123,6 +123,10 @@ _max_js_int = 1<<31 def is_int_short(value): return -_max_js_int <= value < _max_js_int +def safe_shift(arg, default=None): + value = arg.pop(0) if arg else None + return default if value is None else value + def safe_repr(obj): """ Like repr(obj) but falls back to a simpler "" string when repr() itself fails. @@ -252,16 +256,39 @@ class RaisedException(object): RaisedException is registered under a special short name ("E") to save bytes since it's such a widely-used wrapper. To encode_args, it simply returns the entire encoded stored error, e.g. RaisedException(ValueError("foo")) is encoded as ["E", "ValueError", "foo"]. + + When user_input is passed, RaisedException(ValueError("foo"), user_input=2) is encoded as: + ["E", "ValueError", "foo", {u: 2}]. """ - def __init__(self, error, include_details=False, encoded_error=None): + + # Marker object that indicates that there was no user input. + NO_INPUT = object() + + def __init__(self, error, include_details=False, user_input=NO_INPUT): + self.user_input = user_input self.error = error - self.details = traceback.format_exc() if include_details else None - self._encoded_error = encoded_error or self._encode_error() + self.details = None + self._encoded_error = None + self._name = None + self._message = None + if error is not None: + self._fill_from_error(self.has_user_input(), include_details) def encode_args(self): - return self._encoded_error + if self._encoded_error is not None: + return self._encoded_error + if self.has_user_input(): + user_input = {"u": encode_object(self.user_input)} + else: + user_input = None + result = [self._name, self._message, self.details, user_input] + # Trim last values that are None + while len(result) > 1 and result[-1] is None: + result.pop() + self._encoded_error = result + return result - def _encode_error(self): + def _fill_from_error(self, include_message=False, include_details=False): # TODO: We should probably return all args, to communicate the error details to the browser # and to DB (for when we store formula results). There are two concerns: one is that it's # potentially quite verbose; the other is that it's makes the tests more annoying (again b/c @@ -272,18 +299,40 @@ class RaisedException(object): if not location: location = "\n(in referenced cell {error.location})".format(error=error) error = error.error - name = type(error).__name__ - if self.details: - return [name, str(error) + location, self.details] - if isinstance(error, InvalidTypedValue): - return [name, error.typename, error.value] - return [name] + self._name = type(error).__name__ + if include_details: + self.details = traceback.format_exc() + self._message = str(error) + location + elif isinstance(error, InvalidTypedValue): + self._message = error.typename + self.details = error.value + elif include_message: + self._message = str(error) + location + + def has_user_input(self): + return self.user_input is not RaisedException.NO_INPUT + + def no_traceback(self): + exc = RaisedException(None) + exc._name = self._name + exc.error = self.error + exc.user_input = self.user_input + exc.details = "This error is left over from before, and " + \ + "the formula hasn't been triggered since then." + exc._message = self._message + return exc @classmethod def decode_args(cls, *args): - # Decoding of a RaisedException is only enough to re-encode it. - return cls(None, encoded_error=list(args)) - + exc = cls(None) + args = list(args) + assert args + exc._name = safe_shift(args) + exc._message = safe_shift(args) + exc.details = safe_shift(args) + exc.user_input = safe_shift(args, {}) + exc.user_input = decode_object(exc.user_input.get("u", RaisedException.NO_INPUT)) + return exc class CellError(Exception): def __init__(self, table_id, col_id, row_id, error): diff --git a/sandbox/grist/test_engine.py b/sandbox/grist/test_engine.py index 510559af..40b675aa 100644 --- a/sandbox/grist/test_engine.py +++ b/sandbox/grist/test_engine.py @@ -12,6 +12,7 @@ import engine import logger import useractions import testutil +import objtypes log = logger.Logger(__name__, logger.DEBUG) @@ -262,6 +263,13 @@ class EngineTestCase(unittest.TestCase): for tbl in list_of_tables for col in tbl.columns)) + def assertFormulaError(self, exc, type_, message, tracebackRegexp=None): + self.assertIsInstance(exc, objtypes.RaisedException) + self.assertIsInstance(exc.error, type_) + self.assertEqual(str(exc.error), message) + if tracebackRegexp: + self.assertRegex(exc.details, tracebackRegexp) + def assertViews(self, list_of_views): """ Verifies that the given View test-records correspond to the metadata for views/sections/fields. diff --git a/sandbox/grist/test_formula_error.py b/sandbox/grist/test_formula_error.py index f97ef30b..11b7c7b7 100644 --- a/sandbox/grist/test_formula_error.py +++ b/sandbox/grist/test_formula_error.py @@ -46,13 +46,6 @@ else: } }) - def assertFormulaError(self, exc, type_, message, tracebackRegexp=None): - self.assertIsInstance(exc, objtypes.RaisedException) - self.assertIsInstance(exc.error, type_) - self.assertEqual(str(exc.error), message) - if tracebackRegexp: - self.assertRegex(exc.details, tracebackRegexp) - def test_formula_errors(self): self.load_sample(self.sample) diff --git a/sandbox/grist/test_trigger_formulas.py b/sandbox/grist/test_trigger_formulas.py index ff96d3b0..72ae2047 100644 --- a/sandbox/grist/test_trigger_formulas.py +++ b/sandbox/grist/test_trigger_formulas.py @@ -10,7 +10,12 @@ from schema import RecalcWhen log = logger.Logger(__name__, logger.INFO) -attr_error = objtypes.RaisedException(AttributeError()) +def column_error(table, column, user_input): + return objtypes.RaisedException( + AttributeError("Table '%s' has no column '%s'" % (table, column)), + user_input=user_input + ) +div_error = lambda value: objtypes.RaisedException(ZeroDivisionError("float division by zero"), user_input=value) class TestTriggerFormulas(test_engine.EngineTestCase): col = testutil.col_schema_row @@ -396,15 +401,17 @@ class TestTriggerFormulas(test_engine.EngineTestCase): out_actions = self.update_record("Creatures", 1, Name="Whale") self.assertTableData("Creatures", data=[ ["id","Name", "BossDef", "BossNvr", "BossUpd", "BossAll" ], - [1, "Whale", "Arthur", "Arthur", "Arthur", attr_error], + [1, "Whale", "Arthur", "Arthur", "Arthur", column_error("Creatures", "Ocean", "Arthur")], ]) # Add a record. BossUpd's formula still runs, though with an error. + no_column = column_error("Creatures", "Ocean", "") + no_column_value = column_error("Creatures", "Ocean", "Arthur") out_actions = self.add_record("Creatures", None, Name="Manatee") self.assertTableData("Creatures", data=[ ["id","Name", "BossDef", "BossNvr", "BossUpd", "BossAll" ], - [1, "Whale", "Arthur", "Arthur", "Arthur", attr_error], - [2, "Manatee", attr_error, "", attr_error, attr_error], + [1, "Whale", "Arthur", "Arthur", "Arthur", no_column_value], + [2, "Manatee", no_column, "", no_column, no_column], ]) @@ -596,3 +603,67 @@ class TestTriggerFormulas(test_engine.EngineTestCase): [1, "Whale", 3, "Arthur", "Arthur", "Neptune", "Neptune", "Indian", "Foo Bar "], [2, "Manatee", 2, "Poseidon", "", "Poseidon", "Poseidon", "ATLANTIC", "Foo Bar "], ]) + + sample_desc_math = { + "SCHEMA": [ + [1, "Math", [ + col(1, "A", "Numeric", False), + col(2, "B", "Numeric", False), + col(3, "C", "Numeric", False, "1/$A + 1/$B", recalcDeps=[1]), + ]], + ], + "DATA": { + } + } + sample_math = testutil.parse_test_sample(sample_desc_math) + + def test_triggers_on_error(self): + # In case of an error in a trigger formula can be reevaluated when new value is provided + self.load_sample(self.sample_math) + self.add_record("Math", A=0, B=1) + self.assertTableData("Math", data=[ + ["id", "A", "B", "C"], + [1, 0, 1, div_error(0)], + ]) + self.update_record("Math", 1, A=1) + self.assertTableData("Math", data=[ + ["id", "A", "B", "C"], + [1, 1, 1, 2], + ]) + # When the error is cased by external column, formula is not reevaluated + self.update_record("Math", 1, A=2, B=0) + self.update_record("Math", 1, A=1) + self.assertTableData("Math", data=[ + ["id", "A", "B", "C"], + [1, 1, 0, div_error(2)], + ]) + self.update_record("Math", 1, B=1) + self.assertTableData("Math", data=[ + ["id", "A", "B", "C"], + [1, 1, 1, div_error(2)], + ]) + + + def test_traceback_available_for_trigger_formula(self): + # In case of an error engine is able to retrieve a traceback. + self.load_sample(self.sample_math) + self.add_record("Math", A=0, B=0) + self.assertTableData("Math", data=[ + ["id", "A", "B", "C"], + [1, 0, 0, div_error(0)], + ]) + self.assertFormulaError(self.engine.get_formula_error('Math', 'C', 1), + ZeroDivisionError, 'float division by zero', + r"1/rec\.A \+ 1/rec\.B") + self.update_record("Math", 1, A=1) + + # Updating B should remove the traceback from an error, but the error should remain. + self.update_record("Math", 1, B=1) + self.assertTableData("Math", data=[ + ["id", "A", "B", "C"], + [1, 1, 1, div_error(0)], + ]) + error = self.engine.get_formula_error('Math', 'C', 1) + self.assertFormulaError(error, ZeroDivisionError, 'float division by zero') + self.assertEqual(error._message, 'float division by zero') + self.assertEqual(error.details, objtypes.RaisedException(ZeroDivisionError()).no_traceback().details)