From 2b2e19c5b581382f4b9b1bdd376da4454b0fc3b3 Mon Sep 17 00:00:00 2001 From: Cyprien P Date: Tue, 14 Mar 2023 17:11:26 +0100 Subject: [PATCH] (core) Fix issue with summary update. Summary: The problem is that the implementation for a summary update was relying on type consistency to get columns (ie: matches agains colId and type). Type consistency is an attempt at maintaining consistent type across same-named column for summaries of same table. But the problem is that the consistency of types is NOT a strict guarantee or an invariant, more of a best-effort attempt (there are too many possible sequences of operations possible with renaming/adding/removing in summary tables and the underlying table). With current implementation and with a document violating the type consistency, a summary table could end up with fields referencing columns to the former summary table (more detail below(1)). Which is a bad state (yields js errors on the client). This diff fixes this issue by relaxing the type comparison when search for same-named column. (1) __Below is a description of how a violation of type consistency could end-up in bad state document (example taken from the reported bug):__ > In this document, let's assume two summary tables `Table1 [by A]` and `Table1 [Totals]`. Let's also assume Table1 and `Table1 [Totals]` both have an `Amount(Numeric)` column, and that `Table1 [by A]` has one `Amount(Any)` column (violating the type consistency principle). Now when users wanted to change the `Table1 [Totals]` section to group by 'A', grist found that there is already a summary table with same grouping. But it couldn't find a matching column for `Amount(Numeric)` so it created a new one. Except that because there was still an `Amount(Any)` the new column was named `Amount2` which caused following code to ignore it and in particular forgetting to update it's corresponding section's field which was then pointing toward the column of a different table (which is bad). Test Plan: Added python test. Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D3809 --- sandbox/grist/summary.py | 2 +- sandbox/grist/test_summary2.py | 64 ++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/sandbox/grist/summary.py b/sandbox/grist/summary.py index b0db847d..d46d97d6 100644 --- a/sandbox/grist/summary.py +++ b/sandbox/grist/summary.py @@ -167,7 +167,7 @@ class SummaryActions(object): prior = {c.colId: c for c in table.columns} for ci in all_colinfo: col = prior.get(ci.colId) - if col and col.type == ci.type and col.formula == ci.formula: + if col and col.formula == ci.formula: yield col else: result = self.useractions.doAddColumn(table.tableId, ci.colId, diff --git a/sandbox/grist/test_summary2.py b/sandbox/grist/test_summary2.py index ada27b91..7f21c397 100644 --- a/sandbox/grist/test_summary2.py +++ b/sandbox/grist/test_summary2.py @@ -1247,3 +1247,67 @@ class TestSummary2(test_engine.EngineTestCase): [ 3, "IL", [4], 1, 4. ], [ 4, "MA", [5,8], 2, 5.+9 ], ]) + + #---------------------------------------------------------------------- + @test_engine.test_undo + def test_update_summary_with_suffixed_colId(self): + # Verifies that summary update correctly when one of the formula + # columns has a suffixed colId + + self.load_sample(self.sample) + + # Let's create two summary table, one with totals (no grouped by columns) and one grouped by + # "city". + self.apply_user_action(["CreateViewSection", 1, 0, "record", [], None]) + self.apply_user_action(["CreateViewSection", 1, 0, "record", [11], None]) + + # Change type of Amount columns to "Any" only for table Address_summary_city. User actions keep + # types consistent across same-named columns for all summary tables with the same source table, + # but here we want to test for the case where types are inconsistent. Hence we bypass user + # actions and directly use doc actions. + self.engine.apply_doc_action(actions.UpdateRecord("_grist_Tables_column", 20, {'type': 'Any'})) + self.engine.apply_doc_action(actions.ModifyColumn("Address_summary_city", "amount", {'type': + 'Any'})) + self.engine.assert_schema_consistent() + + self.assertTables([ + self.starting_table, + Table(2, "Address_summary", primaryViewId=0, summarySourceTable=1, columns=[ + Column(14, "group", "RefList:Address", isFormula=True, summarySourceCol=0, + formula="table.getSummarySourceGroup(rec)"), + Column(15, "count", "Int", isFormula=True, summarySourceCol=0, + formula="len($group)"), + # This column has type Numeric + Column(16, "amount", "Numeric", isFormula=True, summarySourceCol=0, + formula="SUM($group.amount)"), + ]), + Table(3, "Address_summary_city", primaryViewId=0, summarySourceTable=1, columns=[ + Column(17, "city", "Text", isFormula=False, summarySourceCol=11, + formula=""), + Column(18, "group", "RefList:Address", isFormula=True, summarySourceCol=0, + formula="table.getSummarySourceGroup(rec)"), + Column(19, "count", "Int", isFormula=True, summarySourceCol=0, + formula="len($group)"), + # This column has type Any + Column(20, "amount", "Any", isFormula=True, summarySourceCol=0, + formula="SUM($group.amount)"), + ]), + ]) + + # Now let's add "city" to the summary table with no grouped by column + self.apply_user_action(["UpdateSummaryViewSection", 2, [11]]) + + # Check that summary table now has one column Amount of type Any. + self.assertTables([ + self.starting_table, + Table(3, "Address_summary_city", primaryViewId=0, summarySourceTable=1, columns=[ + Column(17, "city", "Text", isFormula=False, summarySourceCol=11, + formula=""), + Column(18, "group", "RefList:Address", isFormula=True, summarySourceCol=0, + formula="table.getSummarySourceGroup(rec)"), + Column(19, "count", "Int", isFormula=True, summarySourceCol=0, + formula="len($group)"), + Column(20, "amount", "Any", isFormula=True, summarySourceCol=0, + formula="SUM($group.amount)"), + ]) + ])