From 5258fa649d21e3a0301fd48fbc8c126a687ba09e Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Mon, 30 Aug 2021 21:55:14 +0200 Subject: [PATCH] (core) Fix self-updating trigger formulas used in lookups Summary: The problem: For a data-cleaning column (one that depends on itself) `doBulkUpdateRecord` calls `prevent_recalc(should_prevent=False)`` which is supposed to cause it to get calculated eventually. But before that it calls `_do_doc_action` -> `apply_doc_action` -> `_bring_lookups_up_to_date` which recalculates a lookup column which eventually calls `_recompute_step(allow_evaluation=False)` on the data-cleaning column which shouldn't really do anything significant but it both modifies the set `self.recompute_map[node]` and then removes it from the map after it's empty. The solution: when `allow_evaluation=False` and `self._prevent_recompute_map[node]` is nonempty, ensure `self.recompute_map[node]` is not modified, and check the map directly (instead of `dirty_rows` which can now be separate) to see if the set is empty before cleanup. Test Plan: Added a lookup column to test_self_trigger, ensured that this caused the test to fail without the other two changes in engine.py. Reviewers: paulfitz Reviewed By: paulfitz Subscribers: dsagal Differential Revision: https://phab.getgrist.com/D3006 --- sandbox/grist/engine.py | 15 ++++++++++--- sandbox/grist/test_trigger_formulas.py | 29 +++++++++++++++++++------- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index 6dd68cad..df64ad02 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -707,7 +707,13 @@ class Engine(object): exempt = self._prevent_recompute_map.get(node, None) if exempt: - dirty_rows.difference_update(exempt) + # If allow_evaluation=False we're not supposed to actually compute dirty_rows. + # But we may need to compute them later, + # so ensure self.recompute_map[node] isn't mutated by separating it from dirty_rows. + # Therefore dirty_rows is assigned a new value. Note that -= would be a mutation. + dirty_rows = dirty_rows - exempt + if allow_evaluation: + self.recompute_map[node] = dirty_rows require_rows = sorted(require_rows or []) @@ -797,9 +803,12 @@ class Engine(object): finally: for row_id in cleaned: - # this modifies self.recompute_map[node], to which dirty_rows is a reference + # Usually dirty_rows refers to self.recompute_map[node], so this modifies both dirty_rows.discard(row_id) - if not dirty_rows: + # However it's possible for them to be different + # (see above where `exempt` is nonempty and allow_evaluation=True) + # so here we check self.recompute_map[node] directly + if not self.recompute_map[node]: self.recompute_map.pop(node) def _recompute_one_cell(self, frame, table, col, row_id, cycle=False, node=None): diff --git a/sandbox/grist/test_trigger_formulas.py b/sandbox/grist/test_trigger_formulas.py index 89cb8567..ff96d3b0 100644 --- a/sandbox/grist/test_trigger_formulas.py +++ b/sandbox/grist/test_trigger_formulas.py @@ -457,29 +457,44 @@ class TestTriggerFormulas(test_engine.EngineTestCase): def test_self_trigger(self): # A trigger formula may be triggered by changes to the column itself. # Check that it gets recalculated. - self.load_sample(self.sample) + sample_desc = copy.deepcopy(self.sample_desc) + creatures_table = sample_desc["SCHEMA"][0] + creatures_columns = creatures_table[-1] + # Set BossUpd column to depend on Ocean and itself. # Append something to ensure we are testing a case without a fixed point, to ensure # that doesn't cause an infinite update loop. - self.update_record('_grist_Tables_column', 6, recalcDeps=['L', 2, 6], - formula="UPPER(value or $Ocean.Head) + '+'") - self.assertTableData("Creatures", data=[ + self.assertEqual(creatures_columns[5][1], "BossUpd") + creatures_columns[5] = testutil.col_schema_row( + 6, "BossUpd", "Text", False, "UPPER(value or $Ocean.Head) + '+'", recalcDeps=[2, 6] + ) + + # Previously there was a bug that meant that columns involved in lookups + # did not recalculate their trigger formulas after changes to themselves + creatures_columns.append(testutil.col_schema_row( + 21, "Lookup", "Any", True, "Creatures.lookupRecords(BossUpd='')" + )) + + sample = testutil.parse_test_sample(sample_desc) + self.load_sample(sample) + + self.assertTableData("Creatures", cols="subset", data=[ ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"], [1, "Dolphin", 2, "Arthur", "Arthur", "Arthur", "Arthur", "Atlantic" ], ]) self.update_record('Creatures', 1, Ocean=3) - self.assertTableData("Creatures", data=[ + self.assertTableData("Creatures", cols="subset", data=[ ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"], [1, "Dolphin", 3, "Arthur", "Arthur", "ARTHUR+", "Neptune", "Indian" ], ]) self.update_record('Creatures', 1, BossUpd="None") - self.assertTableData("Creatures", data=[ + self.assertTableData("Creatures", cols="subset", data=[ ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"], [1, "Dolphin", 3, "Arthur", "Arthur", "NONE+", "Neptune", "Indian" ], ]) self.update_record('Creatures', 1, BossUpd="") - self.assertTableData("Creatures", data=[ + self.assertTableData("Creatures", cols="subset", data=[ ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"], [1, "Dolphin", 3, "Arthur", "Arthur", "NEPTUNE+", "Neptune", "Indian" ], ])