From b416a5c4b11df57c5f6f4002673ae63f8435d9a9 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Mon, 8 Aug 2022 15:37:25 +0200 Subject: [PATCH] (core) Fix error when updating summary table formulas after rename Summary: Formulas in summary tables were being associated with the source table for automatic updating. When a table/column was renamed such that the formula needed to update to match, it would look for a column with the same colId but in the source table. Such a column might not exist which would lead to an error, or if it existed then the update would be wrong. This association was created while building formulas to display in the code view in a nested `_Summary` class, it didn't need to exist at all. So this diff simply prevents the association from being created. User report and discussion: https://grist.slack.com/archives/C0234CPPXPA/p1659717322297019 Test Plan: Extended `TestSummary.test_table_rename` Python test. Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D3568 --- sandbox/grist/codebuilder.py | 3 ++- sandbox/grist/gencode.py | 12 ++++++++++-- sandbox/grist/test_summary.py | 15 +++++++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/sandbox/grist/codebuilder.py b/sandbox/grist/codebuilder.py index 3466a278..ca5324d2 100644 --- a/sandbox/grist/codebuilder.py +++ b/sandbox/grist/codebuilder.py @@ -350,7 +350,8 @@ def parse_grist_names(builder): if not patch_source: return None in_text, in_value, in_patch = patch_source - return (in_value, in_patch.start, table_id, col_id) + if in_value: + return (in_value, in_patch.start, table_id, col_id) parsed_names = [] for node in asttokens.util.walk(atok.tree): diff --git a/sandbox/grist/gencode.py b/sandbox/grist/gencode.py index e922afd0..65f5ed7d 100644 --- a/sandbox/grist/gencode.py +++ b/sandbox/grist/gencode.py @@ -92,7 +92,10 @@ class GenCode(object): body = self._formula_cache.get(key) if body is None: default = get_type_default(col_info.type) - body = codebuilder.make_formula_body(col_info.formula, default, (table_id, col_info.colId)) + # If we have a table_id like `Table._Summary`, then we don't want to actually associate + # this field with any real table/column. + assoc_value = None if table_id.endswith("._Summary") else (table_id, col_info.colId) + body = codebuilder.make_formula_body(col_info.formula, default, assoc_value) self._new_formula_cache[key] = body decorator = '' @@ -147,7 +150,12 @@ class GenCode(object): for c in six.itervalues(s.columns) if c.isFormula) parts.append(indent(textbuilder.Text("\nclass _Summary:\n"))) for col_info in six.itervalues(formulas): - parts.append(indent(self._make_field(col_info, table_id), levels=2)) + # Associate this field with the fake table `table_id + "._Summary"`. + # We don't know which summary table each formula belongs to, there might be several, + # and we don't care here because this is just for display in the code view. + # The real formula will be associated with the real summary table elsewhere. + # Previously this field was accidentally associated with the source table, causing bugs. + parts.append(indent(self._make_field(col_info, table_id + "._Summary"), levels=2)) return textbuilder.Combiner(parts) diff --git a/sandbox/grist/test_summary.py b/sandbox/grist/test_summary.py index 2a0a58ed..d8d0601b 100644 --- a/sandbox/grist/test_summary.py +++ b/sandbox/grist/test_summary.py @@ -529,6 +529,13 @@ class Address: [2, "Address_summary_city_state", 1], ]) + # Add a column to the summary table with a name that doesn't exist in the source table, + # to test a specific bug fix. + self.add_column( + "Address_summary_city_state", "lookup", + formula="Address.lookupRecords(city=$city)", isFormula=True + ) + # Rename the table: this is what we are really testing in this test case. self.apply_user_action(["RenameTable", "Address", "Location"]) @@ -537,6 +544,14 @@ class Address: [2, "Location_summary_city_state", 1], ]) + # Check that the summary table column's formula was updated correctly. + self.assertTableData("_grist_Tables_column", cols="subset", rows="subset", data=[ + ["id", "colId", "formula"], + [19, "lookup", "Location.lookupRecords(city=$city)"], + ]) + # This column isn't expected in _do_test_updates(). + self.remove_column("Location_summary_city_state", "lookup") + # Verify that the bigger summary table respects all updates to the renamed source table. self._do_test_updates("Location", "Location_summary_city_state")