Archives
/
gristlabs_grist-core
mirror of https://github.com/gristlabs/grist-core
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

(core) Keep referencing columns when their table is deleted, convert to appropriate type

Browse Source 
Summary:
Previously, when a table was deleted, ref/reflist columns pointing at that table were deleted as well. This diff changes that, converting the columns to a suitable type instead.

See here for discussion and decisions: https://grist.slack.com/archives/C069RUP71/p1686060034848139

Test Plan: Added Python tests for most cases, and a DocApi test for where Python has to call JS code to convert the values.

Reviewers: paulfitz

Reviewed By: paulfitz

Differential Revision: https://phab.getgrist.com/D4011
alex/skip-fstrings-3.9
Alex Hall 8 months ago
parent a0fc11c8d1
commit 8644f346af
4 changed files with 331 additions and 31 deletions
Show Stats Download Patch File Download Diff File

184
sandbox/grist/test_table_actions.py
Unescape View File

@ -283,11 +283,12 @@ class TestTableActions(test_engine.EngineTestCase):
[3, 3],
])
# Check that reference columns to this table get removed, with associated fields.
# Check that reference columns to this table get converted
self.assertTables([
Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[
Column(5, "manualSort", "ManualSortPos", False, "", 0),
Column(6, "name", "Text", False, "", 0),
Column(7, "address", "Int", False, "", 0),
Column(8, "city", "Any", True, "$address.city", 0),
]),
# Note that the summary table is also gone.
@ -296,13 +297,194 @@ class TestTableActions(test_engine.EngineTestCase):
View(2, sections=[
Section(3, parentKey="record", tableRef=2, fields=[
Field(7, colRef=6),
Field(8, colRef=7),
Field(9, colRef=8),
]),
]),
View(3, sections=[
Section(8, parentKey="record", tableRef=2, fields=[
Field(22, colRef=6),
Field(23, colRef=7),
Field(24, colRef=8),
]),
]),
])
@test_engine.test_undo
def test_remove_table_referenced_by_formula(self):
self.init_sample_data()
# Add a simple formula column of reference type.
# Its type will be changed to Any.
self.add_column(
"People", "address2", type="Ref:Address", isFormula=True, formula="$address"
)
# Add a similar reflist column, but change it to a data column.
# The formula is just an easy way to populate values for the test.
# A data column of type reflist should be changed to text.
self.add_column(
"People", "addresses", type="RefList:Address", isFormula=True, formula="[$address, $address]"
)
self.modify_column("People", "addresses", isFormula=False)
# Now remove the referenced table and see what happens to the ref columns.
self.apply_user_action(["RemoveTable", "Address"])
self.assertTables([
Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[
Column(5, "manualSort", "ManualSortPos", False, "", 0),
Column(6, "name", "Text", False, "", 0),
# Original data column of type Ref:Address is changed to Int.
Column(7, "address", "Int", False, "", 0),
Column(8, "city", "Any", True, "$address.city", 0),
# Formula column of type Ref:Address is changed to Any.
Column(13, "address2", "Any", True, "$address", 0),
# Data column of type RefList:Address is changed to Text.
Column(14, "addresses", "Text", False, "[$address, $address]", 0),
]),
])
self.assertTableData('People', cols="subset", data=[
["id", "name", "address", "address2", "addresses"],
[ 1, "Alice", 22, 22, "22,22"],
[ 2, "Bob", 25, 25, "25,25"],
[ 3, "Carol", 27, 27, "27,27"],
])
@test_engine.test_undo
def test_remove_table_referenced_by_summary_groupby_col_without_visible_col(self):
self.init_sample_data()
# Create a summary table of People grouped by address (a reference column).
self.apply_user_action(["CreateViewSection", 2, 0, 'record', [7], None])
self.assertTables([
Table(1, "Address", primaryViewId=1, summarySourceTable=0, columns=[
Column(1, "manualSort", "ManualSortPos", False, "", 0),
Column(2, "city", "Text", False, "", 0),
Column(3, "state", "Text", False, "", 0),
Column(4, "amount", "Numeric", False, "", 0),
]),
Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[
Column(5, "manualSort", "ManualSortPos", False, "", 0),
Column(6, "name", "Text", False, "", 0),
Column(7, "address", "Ref:Address", False, "", 0),
Column(8, "city", "Any", True, "$address.city", 0),
]),
Table(3, "Address_summary_state", 0, 1, columns=[
Column(9, "state", "Text", False, "", summarySourceCol=3),
Column(10, "group", "RefList:Address", True, summarySourceCol=0,
formula="table.getSummarySourceGroup(rec)"),
Column(11, "count", "Int", True, summarySourceCol=0, formula="len($group)"),
Column(12, "amount", "Numeric", True, summarySourceCol=0, formula="SUM($group.amount)"),
]),
Table(4, "People_summary_address", 0, 2, columns=[
Column(13, "address", "Ref:Address", False, "", summarySourceCol=7),
Column(14, "group", "RefList:People", True, summarySourceCol=0,
formula="table.getSummarySourceGroup(rec)"),
Column(15, "count", "Int", True, summarySourceCol=0, formula="len($group)"),
]),
])
self.assertTableData('People_summary_address', data=[
["id", "address", "count", "group"],
[1, 22, 1, [1]],
[2, 25, 1, [2]],
[3, 27, 1, [3]],
])
# Now remove the referenced table.
self.apply_user_action(["RemoveTable", "Address"])
# In both the People and summary tables, the 'address' reference column
# is converted to Int, because it didn't have a visible/display column.
self.assertTables([
Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[
Column(5, "manualSort", "ManualSortPos", False, "", 0),
Column(6, "name", "Text", False, "", 0),
Column(7, "address", "Int", False, "", 0),
Column(8, "city", "Any", True, "$address.city", 0),
]),
Table(4, "People_summary_address", 0, 2, columns=[
Column(13, "address", "Int", False, "", summarySourceCol=7),
Column(14, "group", "RefList:People", True, summarySourceCol=0,
formula="table.getSummarySourceGroup(rec)"),
Column(15, "count", "Int", True, summarySourceCol=0, formula="len($group)"),
]),
])
self.assertTableData('People', cols="subset", data=self.people_table_data)
self.assertTableData('People_summary_address', data=[
["id", "address", "count", "group"],
[1, 22, 1, [1]],
[2, 25, 1, [2]],
[3, 27, 1, [3]],
])
@test_engine.test_undo
def test_remove_table_referenced_by_summary_groupby_col_with_visible_col(self):
# Similar to the test above, but now the reference column has a visible column.
self.init_sample_data()
self.modify_column("People", "address", visibleCol=2)
self.apply_user_action(["SetDisplayFormula", "People", 0, 7, "$address.city"])
self.apply_user_action(["CreateViewSection", 2, 0, 'record', [7], None])
self.assertTables([
Table(1, "Address", primaryViewId=1, summarySourceTable=0, columns=[
Column(1, "manualSort", "ManualSortPos", False, "", 0),
Column(2, "city", "Text", False, "", 0),
Column(3, "state", "Text", False, "", 0),
Column(4, "amount", "Numeric", False, "", 0),
]),
Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[
Column(5, "manualSort", "ManualSortPos", False, "", 0),
Column(6, "name", "Text", False, "", 0),
Column(7, "address", "Ref:Address", False, "", 0),
Column(8, "city", "Any", True, "$address.city", 0),
Column(13, "gristHelper_Display", "Any", True, "$address.city", 0),
]),
Table(3, "Address_summary_state", 0, 1, columns=[
Column(9, "state", "Text", False, "", summarySourceCol=3),
Column(10, "group", "RefList:Address", True, summarySourceCol=0,
formula="table.getSummarySourceGroup(rec)"),
Column(11, "count", "Int", True, summarySourceCol=0, formula="len($group)"),
Column(12, "amount", "Numeric", True, summarySourceCol=0, formula="SUM($group.amount)"),
]),
Table(4, "People_summary_address", 0, 2, columns=[
Column(14, "address", "Ref:Address", False, "", summarySourceCol=7),
Column(15, "group", "RefList:People", True, summarySourceCol=0,
formula="table.getSummarySourceGroup(rec)"),
Column(16, "count", "Int", True, summarySourceCol=0, formula="len($group)"),
Column(17, "gristHelper_Display", "Any", True, "$address.city", 0),
]),
])
self.assertTableData('People_summary_address', data=[
["id", "address", "count", "group", "gristHelper_Display"],
[1, 22, 1, [1], "Albany"],
[2, 25, 1, [2], "Bedford"],
[3, 27, 1, [3], "Buffalo"],
])
self.apply_user_action(["RemoveTable", "Address"])
self.assertTables([
Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[
Column(5, "manualSort", "ManualSortPos", False, "", 0),
Column(6, "name", "Text", False, "", 0),
# Reference column is converted to the visible column type, i.e. Text.
Column(7, "address", "Text", False, "", 0),
Column(8, "city", "Any", True, "$address.city", 0),
]),
Table(4, "People_summary_address", 0, 2, columns=[
# Reference column is converted to the visible column type, i.e. Text.
Column(14, "address", "Text", False, "", summarySourceCol=7),
Column(15, "group", "RefList:People", True, summarySourceCol=0,
formula="table.getSummarySourceGroup(rec)"),
Column(16, "count", "Int", True, summarySourceCol=0, formula="len($group)"),
]),
])
self.assertTableData('People', cols="subset", data=[
["id", "name", "address" ],
[ 1, "Alice", "Albany"],
[ 2, "Bob", "Bedford"],
[ 3, "Carol", "Buffalo"],
])
self.assertTableData('People_summary_address', data=[
["id", "address", "count", "group"],
[ 4, "Albany", 1, [1]],
[ 5, "Bedford", 1, [2]],
[ 6, "Buffalo", 1, [3]],
])

52
sandbox/grist/testscript.json
Unescape View File

@ -2398,13 +2398,13 @@
"USER_ACTION": ["RemoveTable", "Schools"],
"ACTIONS": {
"stored": [
// Assert that calc actions include a RemoveColumn for Students.school
["RemoveRecord", "_grist_Tables_column", 5],
["RemoveColumn", "Students", "school"],
// Students.school is converted from Ref:Schools to Int
["ModifyColumn", "Students", "school", {"type": "Int"}],
["UpdateRecord", "_grist_Tables_column", 5, {"type": "Int"}],
["BulkRemoveRecord", "_grist_Tables_column", [10, 12]],
["RemoveRecord", "_grist_Tables", 2],
["RemoveTable", "Schools"],
// Assert that calc actions include a RemoveColumn for Students.school
["BulkUpdateRecord", "Students", [1, 2, 3, 4, 5, 6, 8],
{"schoolRegion": [["E","AttributeError"], ["E","AttributeError"],
["E","AttributeError"], ["E","AttributeError"], ["E","AttributeError"],
@ -2418,12 +2418,8 @@
],
"direct": [true, true, true, true, true, false, false],
"undo": [
["AddRecord", "_grist_Tables_column", 5,
{"parentPos": 5.0, "parentId": 1,
"colId": "school", "type": "Ref:Schools", "label": "school"}],
["BulkUpdateRecord", "Students", [1, 2, 3, 4, 5, 6, 8],
{"school": [2, 8, 6, 8, 1, 5, 7]}],
["AddColumn", "Students", "school", {"isFormula": false, "formula": "", "type": "Ref:Schools"}],
["ModifyColumn", "Students", "school", {"type": "Ref:Schools"}],
["UpdateRecord", "_grist_Tables_column", 5, {"type": "Ref:Schools"}],
["BulkAddRecord", "_grist_Tables_column", [10, 12], {
"colId": ["name", "address"],
"label": ["Name", "Address"], "parentId": [2, 2], "parentPos": [1.0, 2.0],
@ -2449,14 +2445,14 @@
}],
["CHECK_OUTPUT", {
"Students": [
["id","firstName","lastName","@fullName","@fullNameLen","@schoolShort","@schoolRegion"],
[1, "Barack", "Obama", "Barack Obama", 12, ["E","AttributeError"], ["E","AttributeError"]],
[2, "George W", "Bush", "George W Bush",13, ["E","AttributeError"], ["E","AttributeError"]],
[3, "Bill", "Clinton", "Bill Clinton", 12, ["E","AttributeError"], ["E","AttributeError"]],
[4, "George H", "Bush", "George H Bush",13, ["E","AttributeError"], ["E","AttributeError"]],
[5, "Ronald", "Reagan", "Ronald Reagan",13, ["E","AttributeError"], ["E","AttributeError"]],
[6, "Jimmy", "Carter", "Jimmy Carter", 12, ["E","AttributeError"], ["E","AttributeError"]],
[8, "Gerald", "Ford", "Gerald Ford", 11, ["E","AttributeError"], ["E","AttributeError"]]],
["id","firstName","lastName","@fullName","@fullNameLen","school","@schoolShort","@schoolRegion"],
[1, "Barack", "Obama", "Barack Obama", 12, 2, ["E","AttributeError"], ["E","AttributeError"]],
[2, "George W", "Bush", "George W Bush",13, 8, ["E","AttributeError"], ["E","AttributeError"]],
[3, "Bill", "Clinton", "Bill Clinton", 12, 6, ["E","AttributeError"], ["E","AttributeError"]],
[4, "George H", "Bush", "George H Bush",13, 8, ["E","AttributeError"], ["E","AttributeError"]],
[5, "Ronald", "Reagan", "Ronald Reagan",13, 1, ["E","AttributeError"], ["E","AttributeError"]],
[6, "Jimmy", "Carter", "Jimmy Carter", 12, 5, ["E","AttributeError"], ["E","AttributeError"]],
[8, "Gerald", "Ford", "Gerald Ford", 11, 7, ["E","AttributeError"], ["E","AttributeError"]]],
"Address": [
["id", "city", "state", "country", "@region"],
[2, "Eureka", "IL", "US", "North America"],
@ -2470,23 +2466,23 @@
"USER_ACTION": ["RemoveTable", "Students"],
"ACTIONS": {
"stored": [
["BulkRemoveRecord", "_grist_Tables_column", [1, 2, 3, 4, 6, 9]],
["BulkRemoveRecord", "_grist_Tables_column", [1, 2, 3, 4, 5, 6, 9]],
["RemoveRecord", "_grist_Tables", 1],
["RemoveTable", "Students"]
],
"direct": [true, true, true],
"undo": [
["BulkAddRecord", "_grist_Tables_column", [1, 2, 3, 4, 6, 9], {
"colId": ["firstName", "lastName", "fullName", "fullNameLen", "schoolShort",
["BulkAddRecord", "_grist_Tables_column", [1, 2, 3, 4, 5, 6, 9], {
"colId": ["firstName", "lastName", "fullName", "fullNameLen", "school", "schoolShort",
"schoolRegion"],
"formula": ["", "", "rec.firstName + ' ' + rec.lastName", "len(rec.fullName)",
"formula": ["", "", "rec.firstName + ' ' + rec.lastName", "len(rec.fullName)", "",
"rec.school.name.split(' ')[0]",
"addr = $school.address\naddr.state if addr.country == 'US' else addr.region"],
"isFormula": [false, false, true, true, true, true],
"label": ["First Name", "Last Name", "Full Name", "Full Name Length", "School Short",
"isFormula": [false, false, true, true, false, true, true],
"label": ["First Name", "Last Name", "Full Name", "Full Name Length", "school","School Short",
"School Region"],
"parentId": [1, 1, 1, 1, 1, 1], "parentPos": [1, 2, 3, 4, 6, 7],
"type": ["Text", "Text", "Any", "Any", "Any", "Any"]
"parentId": [1, 1, 1, 1, 1, 1, 1], "parentPos": [1, 2, 3, 4, 5, 6, 7],
"type": ["Text", "Text", "Any", "Any", "Int", "Any", "Any"]
}],
["AddRecord", "_grist_Tables", 1, {"tableId": "Students"}],
["BulkAddRecord", "Students", [1, 2, 3, 4, 5, 6, 8], {
@ -2494,6 +2490,7 @@
"fullName": ["Barack Obama", "George W Bush", "Bill Clinton", "George H Bush", "Ronald Reagan", "Jimmy Carter", "Gerald Ford"],
"fullNameLen": [12, 13, 12, 13, 13, 12, 11],
"lastName": ["Obama", "Bush", "Clinton", "Bush", "Reagan", "Carter", "Ford"],
"school": [2, 8, 6, 8, 1, 5, 7],
"schoolRegion": [["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"],
["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"]],
"schoolShort": [["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"],
@ -2510,7 +2507,8 @@
"formula": "rec.school.name.split(' ')[0]", "type": "Any",
"id": "schoolShort"},
{ "isFormula": true, "formula": "addr = $school.address\naddr.state if addr.country == 'US' else addr.region", "type": "Any" ,
"id": "schoolRegion"}
"id": "schoolRegion"},
{"formula": "", "id": "school", "isFormula": false, "type": "Int"}
]]
]
}

60
sandbox/grist/useractions.py
Unescape View File

@ -858,6 +858,11 @@ class UserActions(object):
# If there are group-by columns based on this, change their properties to match (including
# colId, for renaming), except formula/isFormula.
changes = select_keys(col_values, _inherited_groupby_col_fields)
for field in ['displayCol', 'visibleCol']:
if field in col_values and not col_values[field]:
# If displayCol or visibleCol is being cleared in this col, it should be cleared
# in the groupby columns based on this.
changes[field] = col_values[field]
if 'type' in col_values:
changes['type'] = summary.summary_groupby_col_type(col_values['type'])
if col_values['type'] != changes['type']:
@ -1095,8 +1100,13 @@ class UserActions(object):
# If there are summary tables based on this table, remove those too.
remove_table_recs.extend(st for t in remove_table_recs for st in t.summaryTables)
# If other tables have columns referring to this table, remove them.
self.doRemoveColumns(self._collect_back_references(remove_table_recs))
# Handle columns in other tables referring to this table
for ref in self._collect_back_references(remove_table_recs):
if ref.summarySourceCol:
# Skip summary groupby columns, as updating their values is forbidden.
# They will be handled automatically when the source column is updated.
continue
self._convert_reference_col_for_deleted_table(ref)
# Remove all view sections and fields for all tables being removed.
# Bypass the check for raw data view sections.
@ -1125,6 +1135,50 @@ class UserActions(object):
for table_id in remove_table_ids:
self._do_doc_action(actions.RemoveTable(table_id))
def _convert_reference_col_for_deleted_table(self, col):
# col is a column of type Ref:{table_id} or RefList:{table_id}
# where table_id is a table that is being deleted.
# That type will become invalid. We need to convert it to a type that is valid.
table_id = col.parentId.tableId
col_id = col.colId
visible_col = col.visibleCol
display_col = col.displayCol
if col.isFormula:
# Formula columns are easy, as they allow the Any type.
# The contents will probably become some errors which the user should handle.
self.ModifyColumn(table_id, col_id, dict(type="Any"))
return
# For data columns, we may also need to update the values.
if not (visible_col and display_col):
# If there's no visible/display column, we just keep row IDs.
if col.type.startswith("Ref:"):
self.ModifyColumn(table_id, col_id, dict(type="Int"))
else:
# Data columns can't be of type Any, and there's no type that can
# hold a list of numbers. So we convert the lists of row IDs
# to strings containing comma-separated row IDs.
# We need to get the values before changing the column type.
table = self._engine.tables[table_id]
new_values = [",".join(map(str, row)) for row in self._get_column_values(col)]
self.ModifyColumn(table_id, col_id, dict(type="Text"))
self.BulkUpdateRecord(table_id, list(table.row_ids), {col_id: new_values})
return
if col.type.startswith("Ref:") and visible_col.type != "Any":
# This case is easy: we copy the values from the display column directly into
# the converted ex-reference column. No need for any complicated conversion.
self.CopyFromColumn(table_id, display_col.colId, col_id, visible_col.widgetOptions)
self.ModifyColumn(table_id, col_id, dict(type=visible_col.type))
else:
# Otherwise, we need to do a 'full' type conversion.
# Note that this involves `call_external`, i.e. calling JS code.
# This is impossible in the Python tests, so this case is tested in DocApi.ts.
self.ConvertFromColumn(
# widgetOptions and visibleColRef are generally used for parsing values into the new type.
# They're not need here since we're just formatting as Text.
table_id, col_id, col_id, typ="Text", widgetOptions="", visibleColRef=0
)
@override_action('BulkRemoveRecord', '_grist_Tables_column')
def _removeColumnRecords(self, table_id, row_ids):
@ -1602,7 +1656,7 @@ class UserActions(object):
if src_column.is_formula():
self._engine.bring_col_up_to_date(src_column)
# NOTE: This action is invoked only in a single place (during type/colum/data)
# NOTE: This action is invoked only in a single place in the client (during type/column/data
# transformation - where user has a chance to adjust some widgetOptions (though
# the UI is limited). Those widget options were already cleared (in js) and are either
# nullish (default ones) or are truly adjusted. As Grist doesn't know if the widgetOptions

66
test/server/lib/DocApi.ts
Unescape View File

@ -1506,6 +1506,72 @@ function testDocApi() {
}
}
// This is mostly tested in Python, but this case requires the data engine to call
// 'external' (i.e. JS) code to do the type conversion.
it("converts reference columns when the target table is deleted", async () => {
// Create a test document.
const ws1 = (await userApi.getOrgWorkspaces('current'))[0].id;
const docId = await userApi.newDoc({name: 'testdoc'}, ws1);
const docUrl = `${serverUrl}/api/docs/${docId}`;
// Make a new table with a reference column pointing at Table1, displaying column A.
let resp = await axios.post(`${docUrl}/apply`, [
['AddTable', 'Table2', [{id: 'R', type: 'RefList:Table1'}]],
['ModifyColumn', 'Table2', 'R', {visibleCol: 2}],
['SetDisplayFormula', 'Table2', 0, 6, "$R.A"],
['BulkAddRecord', 'Table1', [1, 2], {A: ["Alice", "Bob"]}],
['BulkAddRecord', 'Table2', [1], {R: [["L", 1, 2]]}],
], chimpy);
assert.equal(resp.status, 200);
// Now delete the referenced table.
// This action has to be separate for the test to pass.
resp = await axios.post(`${docUrl}/apply`, [
['RemoveTable', 'Table1'],
], chimpy);
assert.equal(resp.status, 200);
resp = await axios.get(`${docUrl}/tables/Table2/columns`, chimpy);
assert.deepEqual(resp.data, {
"columns": [
{
"id": "R",
"fields": {
"colRef": 6,
"parentId": 2,
"parentPos": 6,
// Type changed from RefList to Text
"type": "Text",
"widgetOptions": "",
"isFormula": false,
"formula": "",
"label": "R",
"description": "",
"untieColIdFromLabel": false,
"summarySourceCol": 0,
// Display and visible columns cleared
"displayCol": 0,
"visibleCol": 0,
"rules": null,
"recalcWhen": 0,
"recalcDeps": null
}
}
]
}
);
resp = await axios.get(`${docUrl}/tables/Table2/records`, chimpy);
assert.deepEqual(resp.data, {
records:
[
// Reflist converted to comma separated display values.
{id: 1, fields: {R: "Alice, Bob"}},
]
}
);
});
it("parses strings in user actions", async () => {
// Create a test document.
const ws1 = (await userApi.getOrgWorkspaces('current'))[0].id;

Write Preview
Loading…
Cancel
Save