From 0bdc82a17091abaf5df002f204501c9a269acb76 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Thu, 7 Jul 2022 22:43:12 +0200 Subject: [PATCH] (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 | 7 ++- sandbox/grist/engine.py | 20 ++++--- sandbox/grist/table.py | 17 +++++- sandbox/grist/test_derived.py | 32 ++++++----- sandbox/grist/test_display_cols.py | 2 +- sandbox/grist/test_summary.py | 8 +-- sandbox/grist/test_summary2.py | 10 ++-- sandbox/grist/test_summary_choicelist.py | 69 ++++++------------------ sandbox/grist/useractions.py | 3 +- 9 files changed, 78 insertions(+), 90 deletions(-) diff --git a/sandbox/grist/docmodel.py b/sandbox/grist/docmodel.py index faaf24d0..29265b17 100644 --- a/sandbox/grist/docmodel.py +++ b/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): diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index 84dac02a..01ab9963 100644 --- a/sandbox/grist/engine.py +++ b/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 - - making_request = False + save_value = True + 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 - value = RequestingError + # The formula will be evaluated again soon when we have a response. + 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. - # The formula will be evaluated again soon when we have a response. - if not making_request: + if save_value: # 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 diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index f5dc46bb..50802201 100644 --- a/sandbox/grist/table.py +++ b/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 diff --git a/sandbox/grist/test_derived.py b/sandbox/grist/test_derived.py index ca6a2fae..3ee33090 100644 --- a/sandbox/grist/test_derived.py +++ b/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], { - "amount": [15.0, 86.0, 0, 17.0, 15.0, 17.0] + actions.RemoveRecord("GristSummary_6_Orders", 4), + 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], { - "count": [1, 3, 0, 1, 1, 1] + actions.BulkUpdateRecord("GristSummary_6_Orders", [2,3,5,7,8], { + "count": [1, 3, 1, 1, 1] }), - actions.BulkUpdateRecord("GristSummary_6_Orders", [2,3,4,5,7,8], { - "group": [[3], [4,5,6], [], [10], [2], [7]] + actions.BulkUpdateRecord("GristSummary_6_Orders", [2,3,5,7,8], { + "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.UpdateRecord("GristSummary_6_Orders", 1, {"count": 1}), - actions.UpdateRecord("GristSummary_6_Orders", 1, {"amount": 15.0}), + actions.AddRecord("GristSummary_6_Orders", 1, { + "amount": 15.0, "count": 1, "group": [1], "year": 2012 + }), actions.RemoveRecord("GristSummary_6_Orders", 5), actions.UpdateRecord("Orders", 1, {"year": 2012}), ], - "calls": {"GristSummary_6_Orders": {"group": 1, "amount": 1, "count": 1}, - "Orders": {"#lookup##summary#GristSummary_6_Orders": 1, - "#summary#GristSummary_6_Orders": 1}} + "calls": { + "GristSummary_6_Orders": { + "#lookup#": 1, "#lookup#year": 1, "group": 1, "amount": 1, "count": 1 + }, + "Orders": { + "#lookup##summary#GristSummary_6_Orders": 1, "#summary#GristSummary_6_Orders": 1, + }, + }, }) diff --git a/sandbox/grist/test_display_cols.py b/sandbox/grist/test_display_cols.py index 8c571271..ff54dfa5 100644 --- a/sandbox/grist/test_display_cols.py +++ b/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"]}], diff --git a/sandbox/grist/test_summary.py b/sandbox/grist/test_summary.py index 337a4897..06122e57 100644 --- a/sandbox/grist/test_summary.py +++ b/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.BulkUpdateRecord(summary_tbl_name, [5,7], {'count': [2, 0]}), - actions.BulkUpdateRecord(summary_tbl_name, [5,7], {'group': [[25, 28], []]}), + actions.RemoveRecord(summary_tbl_name, 7), + actions.UpdateRecord(summary_tbl_name, 5, {'amount': 5.0 + 8.0}), + 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 ], diff --git a/sandbox/grist/test_summary2.py b/sandbox/grist/test_summary2.py index dd655bb2..c59767b3 100644 --- a/sandbox/grist/test_summary2.py +++ b/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.BulkUpdateRecord("GristSummary_7_Address", [5,7], - {'average': [6.5, objtypes.RaisedException(ZeroDivisionError())]}), - actions.BulkUpdateRecord("GristSummary_7_Address", [5,7], {'count': [2, 0]}), - actions.BulkUpdateRecord("GristSummary_7_Address", [5,7], {'group': [[25, 28], []]}), + actions.RemoveRecord("GristSummary_7_Address", 7), + actions.UpdateRecord("GristSummary_7_Address", 5, {'amount': 5.0 + 8.0}), + actions.UpdateRecord("GristSummary_7_Address", 5, {'average': 6.5}), + actions.UpdateRecord("GristSummary_7_Address", 5, {'count': 2}), + 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]}), diff --git a/sandbox/grist/test_summary_choicelist.py b/sandbox/grist/test_summary_choicelist.py index 24cac230..54cb2c41 100644 --- a/sandbox/grist/test_summary_choicelist.py +++ b/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], - [4, "b", "d", [], 0], - [5, "a", "e", [21], 1], + [3, "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, "b", [], 0], - [3, "", [21], 1], + [2, "", [21], 1], ]) 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", [21], 1], - [7, "", "d", [21], 1], - [8, "", "e", [21], 1], + [4, "", "c", [21], 1], + [5, "", "d", [21], 1], + [6, "", "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], - [3, "b", "c", [102, 103, 108, 109], 4], + [2, "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], - [6, "", "c", [], 0], - [7, "", "d", [], 0], - [8, "", "e", [], 0], - [9, "", "", [110], 1], + [5, "", "", [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], - {'count': [0, 0, 0, 0, 1, 1, 1, 1]}], + [5, 6, 7, 8], + {'count': [1, 1, 1, 1]}], ['BulkUpdateRecord', 'GristSummary_6_Source', - [1, 2, 3, 4, 5, 6, 7, 8], - {'group': [['L'], - ['L'], - ['L'], - ['L'], - ['L', 21], + [5, 6, 7, 8], + {'group': [['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], diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 79f7e6ad..6cd9316a 100644 --- a/sandbox/grist/useractions.py +++ b/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)