(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

@@ -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 = {}

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):

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]],
+    ])