(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

@@ -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<boolean>;
 
+  // Is a trigger formula column (not formula, but contains non-empty formula)
+  hasTriggerFormula: ko.Computed<boolean>;
+
   // 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<number>;
@@ -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.

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;

app/client/widgets/FieldEditor.ts

@@ -469,13 +469,16 @@ function getFormulaError(
   gristDoc: GristDoc, editRow: DataRowModel, column: ColumnRec
 ): Observable<CellValue>|undefined {
   const colId = column.colId.peek();
-  let formulaError: Observable<CellValue>|undefined;
   const cellCurrentValue = editRow.cells[colId].peek();
-  if (column.isFormula() && isRaisedException(cellCurrentValue)) {
+  const isFormula = column.isFormula() || column.hasTriggerFormula();
-    const fv = formulaError = Observable.create(null, cellCurrentValue);
+  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 undefined;
+}

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 hideErrDetails = Observable.create(this, true);
-    const formulaError = options.formulaError;
+    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', () => {
+              if (errorDetails.get()){
                 hideErrDetails.set(!hideErrDetails.get());
                 this._formulaEditor.resize();
+              }
             }),
-            dom.domComputed(hideErrDetails, (hide) => cssCollapseIcon(hide ? 'Expand' : 'Collapse')),
+            dom.maybe(errorDetails, () =>
-            dom.text((use) => { const f = use(formulaError) as string[]; return `${f[1]}: ${f[2]}`; }),
+              dom.domComputed(hideErrDetails, (hide) => cssCollapseIcon(hide ? 'Expand' : 'Collapse'))
             ),
+            dom.text(errorText),
+          ),
+          dom.maybe(errorDetails, () =>
             dom('div.error_details',
               dom.hide(hideErrDetails),
-            dom.text((use) => (use(formulaError) as string[])[3]),
+              dom.text(errorDetails),
               testId('formula-error-details'),
-          ),
+            )
+          )
         ) : null
       )
     );

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();

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

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,6 +878,9 @@ class Engine(object):
       self.formula_tracer(col, record)
 
       include_details = (node not in self._is_node_exception_reported) if node else True
+      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):

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 "<type-name>" 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.details = None
-    self._encoded_error = encoded_error or self._encode_error()
+    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):
+    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__
+    self._name = type(error).__name__
-    if self.details:
+    if include_details:
-      return [name, str(error) + location, self.details]
+      self.details = traceback.format_exc()
-    if isinstance(error, InvalidTypedValue):
+      self._message = str(error) + location
-      return [name, error.typename, error.value]
+    elif isinstance(error, InvalidTypedValue):
-    return [name]
+      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.
+    exc = cls(None)
-    return cls(None, encoded_error=list(args))
+    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):

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.

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)
 

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],
+      [1,   "Whale",   "Arthur",   "Arthur",  "Arthur",   no_column_value],
-      [2,   "Manatee", attr_error, "",        attr_error, attr_error],
+      [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 <foo.bar@getgrist.com>"],
       [2,   "Manatee", 2,       "Poseidon",  "",        "Poseidon", "Poseidon", "ATLANTIC",  "Foo Bar <foo.bar@getgrist.com>"],
     ])
+
+  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)