From 8684c9e930a443bf0d33dae2d623336fd077f4f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Sat, 25 Sep 2021 21:14:19 +0200 Subject: [PATCH] (core) Adding traceback to trigger formulas Summary: Traceback is available on the Creator Panel in the formula editor. It is evaluated the same way as for normal formulas. In case when the traceback is not available, only the error name is displayed with information that traceback is not available. Cell with an error, when edited, shows the previous valid value that was used before the error happened (or None for new rows). Value is stored inside the RaisedException object that is stored in a cell. Test Plan: Created tests Reviewers: alexmojaki Reviewed By: alexmojaki Subscribers: alexmojaki, dsagal Differential Revision: https://phab.getgrist.com/D3033 --- app/client/models/entities/ColumnRec.ts | 5 ++ app/client/ui/FieldConfig.ts | 3 +- app/client/widgets/FieldEditor.ts | 13 ++-- app/client/widgets/FormulaEditor.ts | 54 +++++++++++++---- app/plugin/objtypes.ts | 18 +++++- sandbox/grist/column.py | 9 ++- sandbox/grist/engine.py | 23 +++++-- sandbox/grist/objtypes.py | 77 +++++++++++++++++++----- sandbox/grist/test_engine.py | 8 +++ sandbox/grist/test_formula_error.py | 7 --- sandbox/grist/test_trigger_formulas.py | 79 +++++++++++++++++++++++-- 11 files changed, 244 insertions(+), 52 deletions(-) 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)