(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
This commit is contained in:
Cyprien P 2023-03-14 17:11:26 +01:00
parent b3590c8a6f
commit 2b2e19c5b5
2 changed files with 65 additions and 1 deletions

View File

@ -167,7 +167,7 @@ class SummaryActions(object):
prior = {c.colId: c for c in table.columns} prior = {c.colId: c for c in table.columns}
for ci in all_colinfo: for ci in all_colinfo:
col = prior.get(ci.colId) 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 yield col
else: else:
result = self.useractions.doAddColumn(table.tableId, ci.colId, result = self.useractions.doAddColumn(table.tableId, ci.colId,

View File

@ -1247,3 +1247,67 @@ class TestSummary2(test_engine.EngineTestCase):
[ 3, "IL", [4], 1, 4. ], [ 3, "IL", [4], 1, 4. ],
[ 4, "MA", [5,8], 2, 5.+9 ], [ 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)"),
])
])