(core) Automatically remove empty summary table rows

Summary: When the `getSummarySourceGroup` function (used by the `$group` column) finds that the group is empty, raise a new special exception `EmptySummaryRow`. The engine catches this exception, avoids saving a value to the cell, and removes the record.

Test Plan: Updated several Python tests

Reviewers: georgegevoian

Reviewed By: georgegevoian

Subscribers: dsagal

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

sandbox/grist/docmodel.py

@@ -221,7 +221,8 @@ class DocModel(object):
     Marks a record for automatic removal. To use, create a formula in your table, e.g.
     'setAutoRemove', which calls `table.docmodel.setAutoRemove(boolean_value)`. Whenever it gets
     reevaluated and the boolean_value is true, the record will be automatically removed.
-    For now, it is only usable in metadata tables, although we could extend to user tables.
+    It's mostly used for metadata tables. It's also used for summary table rows with empty groups,
+    which requires a bit of extra care.
     """
     if yes_or_no:
       self._auto_remove_set.add(record)
@@ -235,7 +236,9 @@ class DocModel(object):
     # Sort to make sure removals are done in deterministic order.
     gone_records = sorted(self._auto_remove_set)
     self._auto_remove_set.clear()
-    self.remove(gone_records)
+    # setAutoRemove is called by formulas, notably summary tables, and shouldn't be blocked by ACL.
+    with self._engine.user_actions.indirect_actions():
+      self.remove(gone_records)
     return bool(gone_records)
 
   def remove(self, records):

sandbox/grist/engine.py

@@ -819,16 +819,21 @@ class Engine(object):
           # For common-case formulas, all cells in a column are likely to fail in the same way,
           # so don't bother trying more from this column until we've reordered.
           return
-
+        save_value = True
-        making_request = False
+        value = None
         try:
           # We figure out if we've hit a cycle here.  If so, we just let _recompute_on_cell
           # 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:
-          making_request = True
+          # The formula will be evaluated again soon when we have a response.
-          value = RequestingError
+          save_value = False
         except OrderError as e:
           if not required:
             # We're out of order, but for a cell we were evaluating opportunistically.
@@ -857,9 +862,7 @@ class Engine(object):
         if column.is_validation_column_name(col.col_id):
           value = (value in (True, None))
 
-        # When the formula raises a RequestingError, leave the existing value in the cell.
+        if save_value:
-        # The formula will be evaluated again soon when we have a response.
-        if not making_request:
           # Convert the value, and if needed, set, and include into the returned action.
           value = col.convert(value)
           previous = col.raw_get(row_id)
@@ -953,8 +956,9 @@ class Engine(object):
         raise self._cell_required_error  # pylint: disable=raising-bad-type
       self.formula_tracer(col, record)
       return result
-    except MemoryError:
+    except (MemoryError, table_module.EmptySummaryRow):
       # 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

@@ -536,9 +536,17 @@ class Table(object):
       # the column named self._summary_helper_col_id is a single reference
       # or a reference list.
       lookup_value = rec if self._summary_simple else functions.CONTAINS(rec)
-      return self._summary_source_table.lookup_records(**{
+      result = self._summary_source_table.lookup_records(**{
         self._summary_helper_col_id: lookup_value
       })
+
+      # 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
 
@@ -633,3 +641,10 @@ 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

@@ -158,14 +158,15 @@ class TestDerived(test_engine.EngineTestCase):
         actions.BulkUpdateRecord("Orders", [2, 6, 7], {"product": ["B", "B", "C"]}),
         actions.AddRecord("GristSummary_6_Orders", 7, {'year': 2013, 'product': 'B'}),
         actions.AddRecord("GristSummary_6_Orders", 8, {'year': 2015, 'product': 'C'}),
-        actions.BulkUpdateRecord("GristSummary_6_Orders", [2,3,4,5,7,8], {
+        actions.RemoveRecord("GristSummary_6_Orders", 4),
-          "amount": [15.0, 86.0, 0, 17.0, 15.0, 17.0]
+        actions.BulkUpdateRecord("GristSummary_6_Orders", [2,3,5,7,8], {
+          "amount": [15.0, 86.0, 17.0, 15.0, 17.0]
         }),
-        actions.BulkUpdateRecord("GristSummary_6_Orders", [2,3,4,5,7,8], {
+        actions.BulkUpdateRecord("GristSummary_6_Orders", [2,3,5,7,8], {
-          "count": [1, 3, 0, 1, 1, 1]
+          "count": [1, 3, 1, 1, 1]
         }),
-        actions.BulkUpdateRecord("GristSummary_6_Orders", [2,3,4,5,7,8], {
+        actions.BulkUpdateRecord("GristSummary_6_Orders", [2,3,5,7,8], {
-          "group": [[3], [4,5,6], [], [10], [2], [7]]
+          "group": [[3], [4,5,6], [10], [2], [7]]
         }),
       ],
     })
@@ -177,7 +178,6 @@ class TestDerived(test_engine.EngineTestCase):
       [1,   2012,   "A",  1,  15.0,   [1]],
       [2,   2013,   "A",  1,  15.0,   [3]],
       [3,   2014,   "B",  3,  86.0,   [4,5,6]],
-      [4,   2014,   "A",  0,  0.0,    []],
       [5,   2015,   "A",  1,  17.0,   [10]],
       [6,   2015,   "B",  2,  72.0,   [8,9]],
       [7,   2013,   "B",  1,  15.0,   [2]],
@@ -274,7 +274,6 @@ class TestDerived(test_engine.EngineTestCase):
     # Update a record so that a new line appears in the summary table.
     out_actions_update = self.update_record("Orders", 1, year=2007)
     self.assertPartialData("GristSummary_6_Orders", ["id", "year", "count", "amount", "group" ], [
-      [1,   2012,   0,  0.0,    []],
       [2,   2013,   2,  30.0,   [2,3]],
       [3,   2014,   3,  86.0,   [4,5,6]],
       [4,   2015,   4,  106.0,  [7,8,9,10]],
@@ -291,13 +290,18 @@ class TestDerived(test_engine.EngineTestCase):
     ])
     self.assertPartialOutActions(out_actions_undo, {
       "stored": [
-        actions.UpdateRecord("GristSummary_6_Orders", 1, {"group": [1]}),
+        actions.AddRecord("GristSummary_6_Orders", 1, {
-        actions.UpdateRecord("GristSummary_6_Orders", 1, {"count": 1}),
+          "amount": 15.0, "count": 1, "group": [1], "year": 2012
-        actions.UpdateRecord("GristSummary_6_Orders", 1, {"amount": 15.0}),
+        }),
         actions.RemoveRecord("GristSummary_6_Orders", 5),
         actions.UpdateRecord("Orders", 1, {"year": 2012}),
       ],
-      "calls": {"GristSummary_6_Orders": {"group": 1, "amount": 1, "count": 1},
+      "calls": {
-                "Orders": {"#lookup##summary#GristSummary_6_Orders": 1,
+        "GristSummary_6_Orders": {
-                           "#summary#GristSummary_6_Orders": 1}}
+          "#lookup#": 1, "#lookup#year": 1, "group": 1, "amount": 1, "count": 1
+        },
+        "Orders": {
+          "#lookup##summary#GristSummary_6_Orders": 1, "#summary#GristSummary_6_Orders": 1,
+        },
+      },
     })

sandbox/grist/test_display_cols.py

@@ -388,7 +388,7 @@ class TestUserActions(test_engine.EngineTestCase):
         ["RemoveRecord", "_grist_Tables_column", 28],
         ["RemoveColumn", "People", "gristHelper_Display2"],
       ],
-      "direct": [True, True, True, True, True, True],
+      "direct": [True, True, True, True, False, False],
       "undo": [
         ["BulkUpdateRecord", "People", [1, 2, 3], {"gristHelper_Display2": ["Netflix", "HBO", "NBC"]}],
         ["BulkUpdateRecord", "People", [1, 2, 3], {"gristHelper_Display": ["Narcos", "Game of Thrones", "Today"]}],

sandbox/grist/test_summary.py

@@ -459,9 +459,10 @@ class Address:
     self.assertPartialOutActions(out_actions, {
       "stored": [
         actions.UpdateRecord(source_tbl_name, 28, {'state': 'MA'}),
-        actions.BulkUpdateRecord(summary_tbl_name, [5,7], {'amount': [5.0 + 8.0, 0.0]}),
+        actions.RemoveRecord(summary_tbl_name, 7),
-        actions.BulkUpdateRecord(summary_tbl_name, [5,7], {'count': [2, 0]}),
+        actions.UpdateRecord(summary_tbl_name, 5, {'amount': 5.0 + 8.0}),
-        actions.BulkUpdateRecord(summary_tbl_name, [5,7], {'group': [[25, 28], []]}),
+        actions.UpdateRecord(summary_tbl_name, 5, {'count': 2}),
+        actions.UpdateRecord(summary_tbl_name, 5, {'group': [25, 28]}),
       ]
     })
 
@@ -522,7 +523,6 @@ class Address:
       [ 4,    "Chicago",  "IL"   , 1,       4.        ],
       [ 5,    "Bedford",  "MA"   , 1,       108.      ],
       [ 6,    "Buffalo",  "NY"   , 1,       7.        ],
-      [ 7,    "Bedford",  "NY"   , 0,       0.        ],
       [ 8,    "Boston",   "MA"   , 1,       9.        ],
       [ 9,    "Yonkers",  "NY"   , 1,       10.       ],
       [ 10,   "Salem",    "MA"   , 1,       5.0       ],

sandbox/grist/test_summary2.py

@@ -146,11 +146,11 @@ class TestSummary2(test_engine.EngineTestCase):
     self.assertPartialOutActions(out_actions, {
       "stored": [
         actions.UpdateRecord("Address", 28, {'state': 'MA'}),
-        actions.BulkUpdateRecord("GristSummary_7_Address", [5,7], {'amount': [5.0 + 8.0, 0.0]}),
+        actions.RemoveRecord("GristSummary_7_Address", 7),
-        actions.BulkUpdateRecord("GristSummary_7_Address", [5,7],
+        actions.UpdateRecord("GristSummary_7_Address", 5, {'amount': 5.0 + 8.0}),
-                                 {'average': [6.5, objtypes.RaisedException(ZeroDivisionError())]}),
+        actions.UpdateRecord("GristSummary_7_Address", 5, {'average': 6.5}),
-        actions.BulkUpdateRecord("GristSummary_7_Address", [5,7], {'count': [2, 0]}),
+        actions.UpdateRecord("GristSummary_7_Address", 5, {'count': 2}),
-        actions.BulkUpdateRecord("GristSummary_7_Address", [5,7], {'group': [[25, 28], []]}),
+        actions.UpdateRecord("GristSummary_7_Address", 5, {'group': [25, 28]}),
         actions.UpdateRecord("GristSummary_7_Address3", 5,  {'state': "MA"}),
         actions.BulkUpdateRecord("GristSummary_7_Address4", [1,4],
                                  {'amount': [1.+2+6+7+10+11, 5.+8+9]}),

sandbox/grist/test_summary_choicelist.py

@@ -168,19 +168,16 @@ class TestSummaryChoiceList(EngineTestCase):
       [21, ["a"], ["c", "d"], "foo"],
     ])
 
-    # Verify that the summary table rows containing 'b' are empty
+    # Verify that the summary table rows containing 'b' are removed
     self.assertTableData('GristSummary_6_Source', data=[
       ["id", "choices1", "group", "count"],
       [1, "a", [21], 1],
-      [2, "b", [], 0],
     ])
 
     self.assertTableData('GristSummary_6_Source2', data=[
       ["id", "choices1", "choices2", "group", "count"],
       [1, "a", "c", [21], 1],
       [2, "a", "d", [21], 1],
-      [3, "b", "c", [], 0],
-      [4, "b", "d", [], 0],
     ])
 
     # Add 'e' to choices2
@@ -190,7 +187,6 @@ class TestSummaryChoiceList(EngineTestCase):
     self.assertTableData('GristSummary_6_Source', data=[
       ["id", "choices1", "group", "count"],
       [1, "a", [21], 1],
-      [2, "b", [], 0],
     ])
 
     # New row added for 'e'
@@ -198,9 +194,7 @@ class TestSummaryChoiceList(EngineTestCase):
       ["id", "choices1", "choices2", "group", "count"],
       [1, "a", "c", [21], 1],
       [2, "a", "d", [21], 1],
-      [3, "b", "c", [], 0],
+      [3, "a", "e", [21], 1],
-      [4, "b", "d", [], 0],
-      [5, "a", "e", [21], 1],
     ])
 
     # Empty choices1
@@ -213,44 +207,26 @@ class TestSummaryChoiceList(EngineTestCase):
 
     self.assertTableData('GristSummary_6_Source', data=[
       ["id", "choices1", "group", "count"],
-      [1, "a", [], 0],
+      [2, "", [21], 1],
-      [2, "b", [], 0],
-      [3, "", [21], 1],
     ])
 
     self.assertTableData('GristSummary_6_Source2', data=[
       ["id", "choices1", "choices2", "group", "count"],
-      [1, "a", "c", [], 0],
+      [4, "", "c", [21], 1],
-      [2, "a", "d", [], 0],
+      [5, "", "d", [21], 1],
-      [3, "b", "c", [], 0],
+      [6, "", "e", [21], 1],
-      [4, "b", "d", [], 0],
-      [5, "a", "e", [], 0],
-      [6, "", "c", [21], 1],
-      [7, "", "d", [21], 1],
-      [8, "", "e", [21], 1],
     ])
 
     # Remove record from source
     self.remove_record("Source", 21)
 
-    # All summary rows are now empty
+    # All summary rows are now empty and thus removed
     self.assertTableData('GristSummary_6_Source', data=[
       ["id", "choices1", "group", "count"],
-      [1, "a", [], 0],
-      [2, "b", [], 0],
-      [3, "", [], 0],
     ])
 
     self.assertTableData('GristSummary_6_Source2', data=[
       ["id", "choices1", "choices2", "group", "count"],
-      [1, "a", "c", [], 0],
-      [2, "a", "d", [], 0],
-      [3, "b", "c", [], 0],
-      [4, "b", "d", [], 0],
-      [5, "a", "e", [], 0],
-      [6, "",  "c", [], 0],
-      [7, "",  "d", [], 0],
-      [8, "",  "e", [], 0],
     ])
 
     # Make rows with every combination of {a,b,ab} and {c,d,cd}
@@ -297,14 +273,10 @@ class TestSummaryChoiceList(EngineTestCase):
     summary_data = [
       ["id", "choices1", "choices2", "group", "count"],
       [1, "a", "c", [101, 103, 107, 109], 4],
-      [2, "a", "d", [104, 106, 107, 109], 4],
+      [2, "b", "c", [102, 103, 108, 109], 4],
-      [3, "b", "c", [102, 103, 108, 109], 4],
+      [3, "a", "d", [104, 106, 107, 109], 4],
       [4, "b", "d", [105, 106, 108, 109], 4],
-      [5, "a", "e", [], 0],
+      [5, "", "", [110], 1],
-      [6, "", "c", [], 0],
-      [7, "", "d", [], 0],
-      [8, "", "e", [], 0],
-      [9, "", "", [110], 1],
     ]
 
     self.assertTableData('GristSummary_6_Source2', data=summary_data)
@@ -429,18 +401,15 @@ class TestSummaryChoiceList(EngineTestCase):
        [5, 6, 7, 8],
        {'choices1': [u'aa', u'aa', u'bb', u'bb'],
         'choices2': [u'c', u'd', u'c', u'd']}],
+      ['BulkRemoveRecord', 'GristSummary_6_Source', [1, 2, 3, 4]],
       ['BulkUpdateRecord',
        'GristSummary_6_Source',
-       [1, 2, 3, 4, 5, 6, 7, 8],
+       [5, 6, 7, 8],
-       {'count': [0, 0, 0, 0, 1, 1, 1, 1]}],
+       {'count': [1, 1, 1, 1]}],
       ['BulkUpdateRecord',
        'GristSummary_6_Source',
-       [1, 2, 3, 4, 5, 6, 7, 8],
+       [5, 6, 7, 8],
-       {'group': [['L'],
+       {'group': [['L', 21],
-                  ['L'],
-                  ['L'],
-                  ['L'],
-                  ['L', 21],
                   ['L', 21],
                   ['L', 21],
                   ['L', 21]]}]
@@ -456,14 +425,6 @@ class TestSummaryChoiceList(EngineTestCase):
     # left over from each rename
     self.assertTableData('GristSummary_6_Source', data=[
       ["id", "choices1", "choices2", "group", "count"],
-      [1, "a", "c", [], 0],
-      [2, "a", "d", [], 0],
-      [3, "b", "c", [], 0],
-      [4, "b", "d", [], 0],
-      [5, "aa", "c", [], 0],
-      [6, "aa", "d", [], 0],
-      [7, "bb", "c", [], 0],
-      [8, "bb", "d", [], 0],
       [9, "aa", "cc", [21], 1],
       [10, "aa", "dd", [21], 1],
       [11, "bb", "cc", [21], 1],

sandbox/grist/useractions.py

@@ -997,7 +997,8 @@ class UserActions(object):
   def BulkRemoveRecord(self, table_id, row_ids):
     # table_rec will not be found for metadata tables, but they are not summary tables anyway.
     table_rec = self._docmodel.tables.lookupOne(tableId=table_id)
-    if table_rec and table_rec.summarySourceTable:
+    # docmodel.setAutoRemove is used for empty summary table rows, but does so 'indirectly'
+    if table_rec and table_rec.summarySourceTable and self._indirection_level == DIRECT_ACTION:
       raise ValueError("Cannot remove record from summary table")
 
     method = self._overrides.get(('BulkRemoveRecord', table_id), self.doBulkRemoveRecord)