(core) Fix undo error for automatically removed rows, especially in summary tables

Summary:
Fixes a bug noted here: https://grist.slack.com/archives/C069RUP71/p1662564341132349

This bug could happen quite easily as follows:

1. Have a formula in a summary table such as `$group.amount`. Typically there's also a `SUM` but that's not essential.
2. Find a group with nonzero values of `amount`.
3. Delete all rows in that group in the source table. Typically that just means one row in a lonely group.
4. The summary table row is automatically deleted.
5. Try to undo. This raises an error about trying to update a non-existent summary table row.

I tried to account for this undo problem in https://phab.getgrist.com/D3489 by not saving the updated value for `$group` when it was found to be empty. The reason this was insufficient is that `$group.amount` is immediately invalidated anyway when the source row(s) are deleted (I think because that's just how dependency relations involving references work) *and* the calculated value of `$group.amount` changes even if `$group` doesn't. For example, `$group.amount` may have previously been `[100, 200]`. After deleting the rows, `$group.amount` becomes `[0, 0]`. Keeping `$group` unchanged prevents `$group.amount` from just being `[]`, but deleting the source rows means that the amounts become the numeric default `0` which is still a change. This change in value is then noted which leads to saving an undo action to update the summary table record. All this happens in step 3 above, and the summary record is only deleted after that point.

This diff removes that special handling for `group` and instead adds a more general fix to `action_summary.py`. This inserts undo actions for deleted rows at the beginning of the undo list rather than at the end, which was already done for deleted tables and columns.

Test Plan: Python tests

Reviewers: dsagal

Reviewed By: dsagal

Subscribers: dsagal

Differential Revision: https://phab.getgrist.com/D3626

sandbox/grist/action_summary.py

@@ -85,27 +85,37 @@ class ActionSummary(object):
     table_id = root_name(table_id)
     col_id = root_name(col_id)
 
+    def update_action(filtered_row_ids, delta_index):
+      values = [column_delta[r][delta_index] for r in filtered_row_ids]
+      return actions.BulkUpdateRecord(table_id, filtered_row_ids, {col_id: values}).simplify()
+
     if not defunct:
-      row_ids = self.filter_out_gone_rows(table_id, full_row_ids)
+      row_ids_after = self.filter_out_gone_rows(table_id, full_row_ids)
-      if row_ids:
+      if row_ids_after:
-        values = [column_delta[r][1] for r in row_ids]
+        out_stored.append(update_action(row_ids_after, 1))
-        out_stored.append(actions.BulkUpdateRecord(table_id, row_ids, {col_id: values}).simplify())
 
     if self.is_created(table_id, col_id) and not defunct:
-      # A newly-create column, and not replacing a defunct one. Don't generate undo actions.
+      # A newly-created column, and not replacing a defunct one. Don't generate undo actions.
-      pass
+      return
-    else:
-      row_ids = self.filter_out_new_rows(table_id, full_row_ids)
-      if row_ids:
-        values = [column_delta[r][0] for r in row_ids]
-        undo_action = actions.BulkUpdateRecord(table_id, row_ids, {col_id: values}).simplify()
-        if defunct:
-          # If we deleted the column (or its containing table), then during undo, the updates for it
-          # should come after it's re-added. So we need to insert the undos *before*.
-          out_undo.insert(0, undo_action)
-        else:
-          out_undo.append(undo_action)
 
+    ## Maybe add one or two undo update actions for rows that existed before the change.
+    row_ids_before = self.filter_out_new_rows(table_id, full_row_ids)
+
+    if defunct:
+      preserved_row_ids = []
+    else:
+      preserved_row_ids = self.filter_out_gone_rows(table_id, row_ids_before)
+
+    preserved_row_ids_set = set(preserved_row_ids)
+    defunct_row_ids = [r for r in row_ids_before if r not in preserved_row_ids_set]
+
+    if preserved_row_ids:
+      out_undo.append(update_action(preserved_row_ids, 0))
+
+    if defunct_row_ids:
+      # Updates for deleted rows/columns/tables should come after they're re-added.
+      # So we need to insert the undos *before*.
+      out_undo.insert(0, update_action(defunct_row_ids, 0))
 
   def _forTable(self, table_id):
     return self._tables.get(table_id) or self._tables.setdefault(table_id, TableDelta())

sandbox/grist/engine.py

@@ -826,11 +826,6 @@ class Engine(object):
           # know, so it can set the cell value appropriately and do some other bookkeeping.
           cycle = required and (node, row_id) in self._locked_cells
           value = self._recompute_one_cell(table, col, row_id, cycle=cycle, node=node)
-        except table_module.EmptySummaryRow:
-          # This record is going to be deleted after the update loop completes.
-          # Don't save a value for it because that will lead to broken undo actions
-          # trying to update a record that doesn't exist.
-          save_value = False
         except RequestingError:
           # The formula will be evaluated again soon when we have a response.
           save_value = False
@@ -956,9 +951,8 @@ class Engine(object):
         raise self._cell_required_error  # pylint: disable=raising-bad-type
       self.formula_tracer(col, record)
       return result
-    except (MemoryError, table_module.EmptySummaryRow):
+    except MemoryError:
       # Don't try to wrap memory errors.
-      # EmptySummaryRow should be handled in _recompute_step
       raise
     except:  # pylint: disable=bare-except
       # Since col.method runs untrusted user code, we use a bare except to catch all

sandbox/grist/table.py

@@ -563,10 +563,6 @@ class Table(object):
 
       # Remove rows with empty groups
       self._engine.docmodel.setAutoRemove(rec, not result)
-      if not result:
-        # The group is empty, tell the engine that this record will be deleted
-        raise EmptySummaryRow()
-
       return result
     else:
       return None
@@ -662,10 +658,3 @@ class Table(object):
     # creates a dependency and brings formula columns up-to-date.
     self._engine._use_node(col.node, relation, row_ids)
     return [col.get_cell_value(row_id) for row_id in row_ids]
-
-
-class EmptySummaryRow(Exception):
-  """
-  Special exception indicating that the summary group is empty and the row should be removed.
-  """
-  pass

sandbox/grist/test_derived.py

@@ -280,6 +280,24 @@ class TestDerived(test_engine.EngineTestCase):
       [5,   2007,   1,  15.0,   [1]],
     ])
 
+    self.assertPartialOutActions(out_actions_update, {
+      'stored': [
+        ['UpdateRecord', 'Orders', 1, {'year': 2007}],
+        ['AddRecord', 'Orders_summary_year', 5, {'year': 2007}],
+        ['RemoveRecord', 'Orders_summary_year', 1],
+        ['UpdateRecord', 'Orders_summary_year', 5, {'amount': 15.0}],
+        ['UpdateRecord', 'Orders_summary_year', 5, {'count': 1}],
+        ['UpdateRecord', 'Orders_summary_year', 5, {'group': ['L', 1]}],
+      ],
+      'undo': [
+        ['UpdateRecord', 'Orders_summary_year', 1, {'group': ['L', 1]}],
+        ['UpdateRecord', 'Orders_summary_year', 1, {'count': 1}],
+        ['UpdateRecord', 'Orders_summary_year', 1, {'amount': 15.0}],
+        ['UpdateRecord', 'Orders', 1, {'year': 2012}],
+        ['RemoveRecord', 'Orders_summary_year', 5],
+        ['AddRecord', 'Orders_summary_year', 1, {'group': ['L'], 'year': 2012}],
+      ]})
+
     # Undo and ensure that the new line is gone from the summary table.
     out_actions_undo = self.apply_undo_actions(out_actions_update.undo)
     self.assertPartialData("Orders_summary_year", ["id", "year", "count", "amount", "group" ], [
@@ -289,13 +307,7 @@ class TestDerived(test_engine.EngineTestCase):
       [4,   2015,   4,  106.0,  [7,8,9,10]],
     ])
     self.assertPartialOutActions(out_actions_undo, {
-      "stored": [
+      "stored": out_actions_update.undo[::-1],
-        actions.AddRecord("Orders_summary_year", 1, {
-          "amount": 15.0, "count": 1, "group": [1], "year": 2012
-        }),
-        actions.RemoveRecord("Orders_summary_year", 5),
-        actions.UpdateRecord("Orders", 1, {"year": 2012}),
-      ],
       "calls": {
         "Orders_summary_year": {
           "#lookup#": 1, "#lookup#year": 1, "group": 1, "amount": 1, "count": 1

sandbox/grist/test_engine.py

@@ -388,6 +388,8 @@ def test_undo(test_method):
       self.assertEqualDocData(self.getFullEngineData(), expected_engine_data)
   return wrapped
 
+test_undo.__test__ = False  # tells pytest that this isn't a test
+
 
 class TestEngine(EngineTestCase):
   samples = {}

sandbox/grist/test_summary.py

@@ -503,6 +503,28 @@ class Address:
       ]
     })
 
+    # Add a new source record which creates a new summary row,
+    # then delete the source row which implicitly deletes the summary row.
+    # Overall this is a no-op, but it tests a specific undo-related bugfix.
+    self.add_record(source_tbl_name, city="Nowhere", state="??", amount=666)
+    out_actions = self.remove_record(source_tbl_name, 34)
+    self.assertOutActions(out_actions, {
+      'calc': [],
+      'direct': [True, False],
+      'stored': [
+        ['RemoveRecord', source_tbl_name, 34],
+        ['RemoveRecord', summary_tbl_name, 12],
+      ],
+      'undo': [
+        ['UpdateRecord', summary_tbl_name, 12, {'group': ['L', 34]}],
+        ['UpdateRecord', summary_tbl_name, 12, {'count': 1}],
+        ['UpdateRecord', summary_tbl_name, 12, {'amount': 666.0}],
+        ['AddRecord', source_tbl_name, 34, {'amount': 666.0, 'city': 'Nowhere', 'state': '??'}],
+        ['AddRecord', summary_tbl_name, 12,
+         {'city': 'Nowhere', 'group': ['L'], 'state': '??'}],
+      ],
+    })
+
     # Verify the resulting data after all the updates.
     self.assertTableData(summary_tbl_name, cols="subset", data=[
       [ "id", "city",     "state", "count", "amount"  ],

sandbox/grist/test_undo.py

@@ -1,6 +1,8 @@
 import re
 import test_engine
 import testsamples
+import testutil
+
 
 class TestUndo(test_engine.EngineTestCase):
   def test_bad_undo(self):
@@ -80,3 +82,43 @@ class TestUndo(test_engine.EngineTestCase):
       [3,   "Address", [21]],
       [4,   "Table1", [22, 23]],
     ])
+
+  @test_engine.test_undo
+  def test_auto_remove_undo(self):
+    """
+    Test that a formula using docmodel.setAutoRemove doesn't break when undoing.
+    We don't actually recommend using docmodel.setAutoRemove in formulas,
+    but it'd be nice, and this is really testing that a bugfix about summary tables
+    also helps outside of summary tables.
+    """
+    self.load_sample(testutil.parse_test_sample({
+      "SCHEMA": [
+        [1, "Table", [
+          [11, "amount", "Numeric", False, "", "", ""],
+          [12, "amount2", "Numeric", True, "$amount", "", ""],
+          [13, "remove", "Any", True,
+           "table.table._engine.docmodel.setAutoRemove(rec, not $amount2)", "", ""],
+        ]]
+      ],
+      "DATA": {
+        "Table": [
+          ["id", "amount", "amount2"],
+          [21, 1, 1],
+          [22, 2, 2],
+        ]
+      }
+    }))
+    out_actions = self.update_record('Table', 21, amount=0)
+    self.assertOutActions(out_actions, {
+      'calc': [],
+      'direct': [True, False],
+      'stored': [['UpdateRecord', 'Table', 21, {'amount': 0.0}],
+                 ['RemoveRecord', 'Table', 21]],
+      'undo': [['UpdateRecord', 'Table', 21, {'amount2': 1.0}],
+               ['UpdateRecord', 'Table', 21, {'amount': 1.0}],
+               ['AddRecord', 'Table', 21, {}]],
+    })
+    self.assertTableData('Table', cols="subset", data=[
+      ["id", "amount", "amount2"],
+      [22, 2, 2],
+    ])