mirror of
https://github.com/gristlabs/grist-core.git
synced 2024-10-27 20:44:07 +00:00
(core) When checking for metadata consistency, check for stray column records too
Summary: Currently, an undo of a non-last action can leave the doc in an inconsistent state. For example, it may remove a table, but fail to remove all columns of it from metadata. We normally check that schema corresponds to metadata, but stray columns were not visible to this check, and instead caused later table additions to fail. This diff fixes the check to fail the action that causes stray columns, and to restore the doc to a consistent state. Note that this only handles schema-metadata inconsistencies, but an undo of a non-last action can easily create other surprises. Test Plan: Added a test case that triggered inconsistency before, and now triggers a failed undo. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D2715
This commit is contained in:
parent
b5c1fc0c1a
commit
14cdd47675
@ -199,8 +199,8 @@ export class GristDoc extends DisposableWithEvents {
|
|||||||
|
|
||||||
/* Command binding */
|
/* Command binding */
|
||||||
this.autoDispose(commands.createGroup({
|
this.autoDispose(commands.createGroup({
|
||||||
undo() { this._undoStack.sendUndoAction(); },
|
undo() { this._undoStack.sendUndoAction().catch(reportError); },
|
||||||
redo() { this._undoStack.sendRedoAction(); },
|
redo() { this._undoStack.sendRedoAction().catch(reportError); },
|
||||||
reloadPlugins() { this.docComm.reloadPlugins().then(() => G.window.location.reload(false)); },
|
reloadPlugins() { this.docComm.reloadPlugins().then(() => G.window.location.reload(false)); },
|
||||||
}, this, true));
|
}, this, true));
|
||||||
|
|
||||||
|
@ -422,6 +422,21 @@ class Engine(object):
|
|||||||
raise AssertionError("Internal schema different from that in metadata:\n" +
|
raise AssertionError("Internal schema different from that in metadata:\n" +
|
||||||
"".join(difflib.unified_diff(a, b, fromfile="internal", tofile="metadata")))
|
"".join(difflib.unified_diff(a, b, fromfile="internal", tofile="metadata")))
|
||||||
|
|
||||||
|
# Check there are no stray column records (they aren't picked up by schema diffs, but will
|
||||||
|
# cause inconsistencies with future tables).
|
||||||
|
# TODO: This inconsistency can be triggered by undo of an AddTable action if the table
|
||||||
|
# acquired more columns in subsequent actions. We may want to check for similar situations
|
||||||
|
# with other metadata, e.g. ViewSection fields, where they'd cause different symptoms.
|
||||||
|
# (Or better ensure consistency by design by applying undo correctly, probably via rebase).
|
||||||
|
valid_table_refs = set(meta_tables.row_ids)
|
||||||
|
col_parent_ids = set(meta_columns.columns['parentId'])
|
||||||
|
if col_parent_ids > valid_table_refs:
|
||||||
|
collist = sorted(actions.transpose_bulk_action(meta_columns),
|
||||||
|
key=lambda c: (c.parentId, c.parentPos))
|
||||||
|
raise AssertionError("Internal schema inconsistent; extra columns in metadata:\n"
|
||||||
|
+ "\n".join(' ' + str(schema.SchemaColumn(c.colId, c.type, bool(c.isFormula), c.formula))
|
||||||
|
for c in collist if c.parentId not in valid_table_refs))
|
||||||
|
|
||||||
def dump_state(self):
|
def dump_state(self):
|
||||||
self.dep_graph.dump_graph()
|
self.dep_graph.dump_graph()
|
||||||
self.dump_recompute_map()
|
self.dump_recompute_map()
|
||||||
|
@ -332,7 +332,7 @@ def build_schema(meta_tables, meta_columns, include_builtin=True):
|
|||||||
coldict = {t: list(cols) for t, cols in itertools.groupby(collist, lambda r: r.parentId)}
|
coldict = {t: list(cols) for t, cols in itertools.groupby(collist, lambda r: r.parentId)}
|
||||||
|
|
||||||
for t in actions.transpose_bulk_action(meta_tables):
|
for t in actions.transpose_bulk_action(meta_tables):
|
||||||
columns = OrderedDict((c.colId, SchemaColumn(c.colId, c.type, c.isFormula, c.formula))
|
columns = OrderedDict((c.colId, SchemaColumn(c.colId, c.type, bool(c.isFormula), c.formula))
|
||||||
for c in coldict[t.id])
|
for c in coldict[t.id])
|
||||||
schema[t.tableId] = SchemaTable(t.tableId, columns)
|
schema[t.tableId] = SchemaTable(t.tableId, columns)
|
||||||
return schema
|
return schema
|
||||||
|
53
sandbox/grist/test_undo.py
Normal file
53
sandbox/grist/test_undo.py
Normal file
@ -0,0 +1,53 @@
|
|||||||
|
import re
|
||||||
|
import test_engine
|
||||||
|
import testsamples
|
||||||
|
|
||||||
|
class TestUndo(test_engine.EngineTestCase):
|
||||||
|
def test_bad_undo(self):
|
||||||
|
# Sometimes undo can make metadata inconsistent with schema. Check that we disallow it.
|
||||||
|
self.load_sample(testsamples.sample_students)
|
||||||
|
out_actions1 = self.apply_user_action(['AddEmptyTable'])
|
||||||
|
self.assertPartialData("_grist_Tables", ["id", "tableId", "columns"], [
|
||||||
|
[1, "Students", [1,2,4,5,6]],
|
||||||
|
[2, "Schools", [10,12]],
|
||||||
|
[3, "Address", [21]],
|
||||||
|
[4, "Table1", [22,23,24,25]],
|
||||||
|
])
|
||||||
|
|
||||||
|
# Add a column, and check that it's present in the metadata.
|
||||||
|
self.add_column('Table1', 'NewCol', type='Text')
|
||||||
|
self.assertPartialData("_grist_Tables", ["id", "tableId", "columns"], [
|
||||||
|
[1, "Students", [1,2,4,5,6]],
|
||||||
|
[2, "Schools", [10,12]],
|
||||||
|
[3, "Address", [21]],
|
||||||
|
[4, "Table1", [22,23,24,25,26]],
|
||||||
|
])
|
||||||
|
|
||||||
|
# Now undo just the first action. The list of undo DocActions for it does not mention the
|
||||||
|
# newly added column, and fails to clean it up. This would leave the doc in an inconsistent
|
||||||
|
# state, and we should not allow it.
|
||||||
|
with self.assertRaisesRegexp(AssertionError,
|
||||||
|
re.compile(r"Internal schema inconsistent.*'NewCol'", re.S)):
|
||||||
|
self.apply_undo_actions(out_actions1.undo)
|
||||||
|
|
||||||
|
# Doc state should be unchanged.
|
||||||
|
|
||||||
|
# A little cheating here: assertPartialData() below checks the same thing, but the private
|
||||||
|
# calculated field "columns" in _grist_Tables metadata is left out of date by the failed undo.
|
||||||
|
# In practice it's harmless: properly calculated fields get restored correct, and the private
|
||||||
|
# metadata fields get brought up-to-date when used via Record interface, which is what we do
|
||||||
|
# using this assertEqual().
|
||||||
|
self.assertEqual([[r.id, r.tableId, map(int, r.columns)]
|
||||||
|
for r in self.engine.docmodel.tables.table.filter_records()], [
|
||||||
|
[1, "Students", [1,2,4,5,6]],
|
||||||
|
[2, "Schools", [10,12]],
|
||||||
|
[3, "Address", [21]],
|
||||||
|
[4, "Table1", [22,23,24,25,26]],
|
||||||
|
])
|
||||||
|
|
||||||
|
self.assertPartialData("_grist_Tables", ["id", "tableId", "columns"], [
|
||||||
|
[1, "Students", [1,2,4,5,6]],
|
||||||
|
[2, "Schools", [10,12]],
|
||||||
|
[3, "Address", [21]],
|
||||||
|
[4, "Table1", [22,23,24,25,26]],
|
||||||
|
])
|
Loading…
Reference in New Issue
Block a user