(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
This commit is contained in:
Alex Hall 2021-08-30 21:55:14 +02:00
parent 29dd33a45c
commit 5258fa649d
2 changed files with 34 additions and 10 deletions

View File

@ -707,7 +707,13 @@ class Engine(object):
exempt = self._prevent_recompute_map.get(node, None) exempt = self._prevent_recompute_map.get(node, None)
if exempt: 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 []) require_rows = sorted(require_rows or [])
@ -797,9 +803,12 @@ class Engine(object):
finally: finally:
for row_id in cleaned: 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) 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) self.recompute_map.pop(node)
def _recompute_one_cell(self, frame, table, col, row_id, cycle=False, node=None): def _recompute_one_cell(self, frame, table, col, row_id, cycle=False, node=None):

View File

@ -457,29 +457,44 @@ class TestTriggerFormulas(test_engine.EngineTestCase):
def test_self_trigger(self): def test_self_trigger(self):
# A trigger formula may be triggered by changes to the column itself. # A trigger formula may be triggered by changes to the column itself.
# Check that it gets recalculated. # 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. # Set BossUpd column to depend on Ocean and itself.
# Append something to ensure we are testing a case without a fixed point, to ensure # Append something to ensure we are testing a case without a fixed point, to ensure
# that doesn't cause an infinite update loop. # that doesn't cause an infinite update loop.
self.update_record('_grist_Tables_column', 6, recalcDeps=['L', 2, 6], self.assertEqual(creatures_columns[5][1], "BossUpd")
formula="UPPER(value or $Ocean.Head) + '+'") creatures_columns[5] = testutil.col_schema_row(
self.assertTableData("Creatures", data=[ 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"], ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"],
[1, "Dolphin", 2, "Arthur", "Arthur", "Arthur", "Arthur", "Atlantic" ], [1, "Dolphin", 2, "Arthur", "Arthur", "Arthur", "Arthur", "Atlantic" ],
]) ])
self.update_record('Creatures', 1, Ocean=3) 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"], ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"],
[1, "Dolphin", 3, "Arthur", "Arthur", "ARTHUR+", "Neptune", "Indian" ], [1, "Dolphin", 3, "Arthur", "Arthur", "ARTHUR+", "Neptune", "Indian" ],
]) ])
self.update_record('Creatures', 1, BossUpd="None") 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"], ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"],
[1, "Dolphin", 3, "Arthur", "Arthur", "NONE+", "Neptune", "Indian" ], [1, "Dolphin", 3, "Arthur", "Arthur", "NONE+", "Neptune", "Indian" ],
]) ])
self.update_record('Creatures', 1, BossUpd="") self.update_record('Creatures', 1, BossUpd="")
self.assertTableData("Creatures", data=[ self.assertTableData("Creatures", cols="subset", data=[
["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"], ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"],
[1, "Dolphin", 3, "Arthur", "Arthur", "NEPTUNE+", "Neptune", "Indian" ], [1, "Dolphin", 3, "Arthur", "Arthur", "NEPTUNE+", "Neptune", "Indian" ],
]) ])