From ec023a3ba64632440bd5a6d5cd8703dc8660b745 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Thu, 28 Jan 2021 01:54:27 -0500 Subject: [PATCH] (core) Fix another cause of inconsistency that can be triggered by bad DocActions. Summary: An incorrect DocAction (as possible from an Undo of a non-last action) could cause RemoveRecord on an already missing record. This used to create an invalid undo, and wreak havoc when a series of DocActions later fails and needs to be reverted. To fix, consider RemoveRecord of a missing record to be a no-op. Test Plan: Includes a new test case that triggers the problem. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D2717 --- sandbox/grist/docactions.py | 5 +++++ sandbox/grist/engine.py | 3 ++- sandbox/grist/test_undo.py | 29 +++++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/sandbox/grist/docactions.py b/sandbox/grist/docactions.py index cf72d8cd..1185a605 100644 --- a/sandbox/grist/docactions.py +++ b/sandbox/grist/docactions.py @@ -34,6 +34,11 @@ class DocActions(object): def BulkRemoveRecord(self, table_id, row_ids): table = self._engine.tables[table_id] + # Ignore records that don't exist in the table. + row_ids = [r for r in row_ids if r in table.row_ids] + if not row_ids: + return + # Collect the undo values, and unset all values in the column (i.e. set to defaults), just to # make sure we don't have stale values hanging around. undo_values = {} diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index 50b780ef..9b104e3f 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -434,7 +434,8 @@ class Engine(object): 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)) + + "\n".join(' #%s %s' % + (c.id, 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): diff --git a/sandbox/grist/test_undo.py b/sandbox/grist/test_undo.py index da2023f9..12414b86 100644 --- a/sandbox/grist/test_undo.py +++ b/sandbox/grist/test_undo.py @@ -30,6 +30,9 @@ class TestUndo(test_engine.EngineTestCase): re.compile(r"Internal schema inconsistent.*'NewCol'", re.S)): self.apply_undo_actions(out_actions1.undo) + # Check that schema and metadata look OK. + self.engine.assert_schema_consistent() + # Doc state should be unchanged. # A little cheating here: assertPartialData() below checks the same thing, but the private @@ -51,3 +54,29 @@ class TestUndo(test_engine.EngineTestCase): [3, "Address", [21]], [4, "Table1", [22,23,24,25,26]], ]) + + def test_import_undo(self): + # Here we reproduce another bad situation. A more complex example with the same essence arose + # during undo of imports when the undo could omit part of the action bundle. + self.load_sample(testsamples.sample_students) + + out_actions1 = self.apply_user_action(['AddEmptyTable']) + out_actions2 = self.add_column('Table1', 'D', type='Text') + out_actions3 = self.remove_column('Table1', 'D') + out_actions4 = self.apply_user_action(['RemoveTable', 'Table1']) + out_actions5 = self.apply_user_action(['AddTable', 'Table1', [{'id': 'X'}]]) + + undo_actions = [da for out in [out_actions1, out_actions2, out_actions4, out_actions5] + for da in out.undo] + with self.assertRaises(AssertionError): + self.apply_undo_actions(undo_actions) + + # The undo failed, and data should look as before the undo. + self.engine.assert_schema_consistent() + 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]], + ])