diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index 50220f2f..aca122ed 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -199,8 +199,8 @@ export class GristDoc extends DisposableWithEvents { /* Command binding */ this.autoDispose(commands.createGroup({ - undo() { this._undoStack.sendUndoAction(); }, - redo() { this._undoStack.sendRedoAction(); }, + undo() { this._undoStack.sendUndoAction().catch(reportError); }, + redo() { this._undoStack.sendRedoAction().catch(reportError); }, reloadPlugins() { this.docComm.reloadPlugins().then(() => G.window.location.reload(false)); }, }, this, true)); diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index 47511f7c..50b780ef 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -422,6 +422,21 @@ class Engine(object): raise AssertionError("Internal schema different from that in metadata:\n" + "".join(difflib.unified_diff(a, b, fromfile="internal", tofile="metadata"))) + # Check there are no stray column records (they aren't picked up by schema diffs, but will + # cause inconsistencies with future tables). + # TODO: This inconsistency can be triggered by undo of an AddTable action if the table + # acquired more columns in subsequent actions. We may want to check for similar situations + # with other metadata, e.g. ViewSection fields, where they'd cause different symptoms. + # (Or better ensure consistency by design by applying undo correctly, probably via rebase). + valid_table_refs = set(meta_tables.row_ids) + col_parent_ids = set(meta_columns.columns['parentId']) + if col_parent_ids > valid_table_refs: + collist = sorted(actions.transpose_bulk_action(meta_columns), + key=lambda c: (c.parentId, c.parentPos)) + raise AssertionError("Internal schema inconsistent; extra columns in metadata:\n" + + "\n".join(' ' + str(schema.SchemaColumn(c.colId, c.type, bool(c.isFormula), c.formula)) + for c in collist if c.parentId not in valid_table_refs)) + def dump_state(self): self.dep_graph.dump_graph() self.dump_recompute_map() diff --git a/sandbox/grist/schema.py b/sandbox/grist/schema.py index ef9a58e3..df1c5da0 100644 --- a/sandbox/grist/schema.py +++ b/sandbox/grist/schema.py @@ -332,7 +332,7 @@ def build_schema(meta_tables, meta_columns, include_builtin=True): coldict = {t: list(cols) for t, cols in itertools.groupby(collist, lambda r: r.parentId)} for t in actions.transpose_bulk_action(meta_tables): - columns = OrderedDict((c.colId, SchemaColumn(c.colId, c.type, c.isFormula, c.formula)) + columns = OrderedDict((c.colId, SchemaColumn(c.colId, c.type, bool(c.isFormula), c.formula)) for c in coldict[t.id]) schema[t.tableId] = SchemaTable(t.tableId, columns) return schema diff --git a/sandbox/grist/test_undo.py b/sandbox/grist/test_undo.py new file mode 100644 index 00000000..da2023f9 --- /dev/null +++ b/sandbox/grist/test_undo.py @@ -0,0 +1,53 @@ +import re +import test_engine +import testsamples + +class TestUndo(test_engine.EngineTestCase): + def test_bad_undo(self): + # Sometimes undo can make metadata inconsistent with schema. Check that we disallow it. + self.load_sample(testsamples.sample_students) + out_actions1 = self.apply_user_action(['AddEmptyTable']) + self.assertPartialData("_grist_Tables", ["id", "tableId", "columns"], [ + [1, "Students", [1,2,4,5,6]], + [2, "Schools", [10,12]], + [3, "Address", [21]], + [4, "Table1", [22,23,24,25]], + ]) + + # Add a column, and check that it's present in the metadata. + self.add_column('Table1', 'NewCol', type='Text') + self.assertPartialData("_grist_Tables", ["id", "tableId", "columns"], [ + [1, "Students", [1,2,4,5,6]], + [2, "Schools", [10,12]], + [3, "Address", [21]], + [4, "Table1", [22,23,24,25,26]], + ]) + + # Now undo just the first action. The list of undo DocActions for it does not mention the + # newly added column, and fails to clean it up. This would leave the doc in an inconsistent + # state, and we should not allow it. + with self.assertRaisesRegexp(AssertionError, + re.compile(r"Internal schema inconsistent.*'NewCol'", re.S)): + self.apply_undo_actions(out_actions1.undo) + + # Doc state should be unchanged. + + # A little cheating here: assertPartialData() below checks the same thing, but the private + # calculated field "columns" in _grist_Tables metadata is left out of date by the failed undo. + # In practice it's harmless: properly calculated fields get restored correct, and the private + # metadata fields get brought up-to-date when used via Record interface, which is what we do + # using this assertEqual(). + self.assertEqual([[r.id, r.tableId, map(int, r.columns)] + for r in self.engine.docmodel.tables.table.filter_records()], [ + [1, "Students", [1,2,4,5,6]], + [2, "Schools", [10,12]], + [3, "Address", [21]], + [4, "Table1", [22,23,24,25,26]], + ]) + + self.assertPartialData("_grist_Tables", ["id", "tableId", "columns"], [ + [1, "Students", [1,2,4,5,6]], + [2, "Schools", [10,12]], + [3, "Address", [21]], + [4, "Table1", [22,23,24,25,26]], + ])