(core) Fix for ReferenceList conversion during table rename

Summary:
Renaming table after converting Ref column to RefList didn't work. During table rename, all Refs columns
are converted briefly to Int columns which treats values stored in RefList columns as errors, and stores its
`repr` strings. This could be recovered back if the value stored in RefList column was a plain list, but if we had there
a RecordList object, the RefList column didn't know how to parse that.

Test Plan: Added test

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D4233
This commit is contained in:
Jarosław Sadziński 2024-04-24 11:15:23 +02:00
parent bd07e9c026
commit 34c85757f1
4 changed files with 79 additions and 10 deletions

View File

@ -524,11 +524,24 @@ class ReferenceListColumn(BaseReferenceColumn):
Accessing them yields RecordSets.
"""
def set(self, row_id, value):
if isinstance(value, six.string_types) and value.startswith(u'['):
if isinstance(value, six.string_types):
# This is second part of a "hack" we have to do when we rename tables. During
# the rename, we briefly change all Ref columns to Int columns (to lose the table
# part), and then back to Ref columns. The values during this change are stored
# as serialized strings, which we expect to understand when the column is back to
# being a Ref column. We can either end up with a list of ints, or a RecordList
# serialized as a string.
# TODO: explain why we need to do this and why we have chosen the Int column
try:
value = json.loads(value)
# If it's a string that looks like JSON, try to parse it as such.
if value.startswith('['):
value = json.loads(value)
else:
# Else try to parse it as a RecordList
value = objtypes.RecordList.from_repr(value)
except Exception:
pass
super(ReferenceListColumn, self).set(row_id, value)
def _update_references(self, row_id, old_list, new_list):

View File

@ -361,6 +361,20 @@ class CellError(Exception):
class RecordList(list):
"""
A static method to recreate a RecordList from the output of __repr__.
It only restores the row_ids. The group_by and sort_by attributes are not restored.
"""
@staticmethod
def from_repr(repr_str):
if not repr_str.startswith('RecordList(['):
raise ValueError("Invalid RecordList representation")
# This is a string representation of a RecordList, which we can parse.
# > RecordList([1,2,3], group_by=%r, sort_by=%r)
# Match only rows, as group_by and sort_by are not used and can be stale.
numbers = repr_str.split('[')[1].split(']')[0].split(',')
return RecordList([int(v) for v in numbers])
"""
Just like list but allows setting custom attributes, which we use for remembering _group_by and
_sort_by attributes when storing RecordSet as usertypes.ReferenceList type.
@ -375,7 +389,6 @@ class RecordList(list):
list.__repr__(self), self._group_by, self._sort_by)
# We don't currently have a good way to convert an incoming marshalled record to a proper Record
# object for an appropriate table. We don't expect incoming marshalled records at all, but if such
# a thing happens, we'll construct this RecordStub.

View File

@ -431,12 +431,22 @@ class ReferenceList(BaseColumnType):
def do_convert(self, value):
if isinstance(value, six.string_types):
# If it's a string that looks like JSON, try to parse it as such.
if value.startswith('['):
try:
# This is second part of a "hack" we have to do when we rename tables. During
# the rename, we briefly change all Ref columns to Int columns (to lose the table
# part), and then back to Ref columns. The values during this change are stored
# as serialized strings, which we expect to understand when the column is back to
# being a Ref column. We can either end up with a list of ints, or a RecordList
# serialized as a string.
# TODO: explain why we need to do this and why we have chosen the Int column
try:
# If it's a string that looks like JSON, try to parse it as such.
if value.startswith('['):
value = json.loads(value)
except Exception:
pass
else:
# Else try to parse it as a RecordList
value = objtypes.RecordList.from_repr(value)
except Exception:
pass
if isinstance(value, RecordSet):
assert value._table.table_id == self.table_id

View File

@ -13,7 +13,40 @@ describe('ReferenceList', function() {
});
describe('other', function() {
it('allows to delete document with self reference', async function() {
it('fix: doesnt break when table is renamed', async function() {
// There was a bug in this scenario:
// 1. Create a Ref column that targets itself
// 2. Fill it up
// 3. Change it to RefList
// 4. Rename the table
// Previously, this would cause an error, cells were displaying Errors instead of values.
// Create a table with a Ref column that targets itself.
await session.tempNewDoc(cleanup);
await gu.sendActions([
['ModifyColumn', 'Table1', 'B', {type: 'Ref:Table1'}],
['AddRecord', 'Table1', null, {A: 'a', B: 1}],
['AddRecord', 'Table1', null, {A: 'b', B: 2}],
['AddRecord', 'Table1', null, {A: 'c', B: 3}],
]);
await gu.openColumnPanel();
await gu.getCell('B', 1).doClick();
await gu.setRefShowColumn('A');
// Now convert it to RefList.
await gu.setType('Reference List', {apply: true});
// Make sure we see the values.
assert.deepEqual(await gu.getVisibleGridCells('B', [1, 2, 3]), ['a', 'b', 'c']);
// Rename the table using widget in the section.
await gu.renameTable('Table1', 'Table2');
// Make sure we still see the values.
assert.deepEqual(await gu.getVisibleGridCells('B', [1, 2, 3]), ['a', 'b', 'c']);
});
it('fix: allows to delete document with self reference', async function() {
const docId = await session.tempNewDoc(cleanup);
await gu.sendActions([
['AddEmptyTable', 'Table2'],
@ -39,7 +72,7 @@ describe('ReferenceList', function() {
before(async function() {
await session.tempDoc(cleanup, 'Favorite_Films.grist');
await gu.toggleSidePanel('right');
await gu.toggleSidePanel('right', 'open');
await driver.find(".test-right-tab-pagewidget").click();
await driver.find('.test-config-data').click();
});