From 0289e3ea1755c169283eebd18d75f93a72358e83 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Tue, 1 Dec 2020 16:53:33 -0500 Subject: [PATCH] (core) Fix issue with spurious changes produced by Calculate action. Summary: - Replace unicode strings with byte strings when decoding values in sandbox. - Columns that rely on float values should derive from NumericColumn, so that set() ensures that a float is stored even if loading an int. - Parse unmarshallable values (['U']) into an object that can be encoded back to the same value (rather than info a RaisedException). - Compare NaN's as equal for deciding whether a change is a no-op. Unrelated: - Removed a tiny bit of unhelpful logging Test Plan: Added a test case that reproduces several causes of Calculate discrepancies by loading various values into various types of formula columns. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D2676 --- app/server/lib/ActiveDoc.ts | 3 +-- sandbox/grist/column.py | 6 +++--- sandbox/grist/objtypes.py | 28 +++++++++++++++++++++++++--- sandbox/grist/test_types.py | 25 +++++++++++++------------ 4 files changed, 42 insertions(+), 20 deletions(-) diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 0881b6ec..4e902f08 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -564,7 +564,6 @@ export class ActiveDoc extends EventEmitter { */ public async fetchTable(docSession: OptDocSession, tableId: string, waitForFormulas: boolean = false): Promise { - this.logInfo(docSession, "fetchTable(%s, %s)", docSession, tableId); return this.fetchQuery(docSession, {tableId, filters: {}}, waitForFormulas); } @@ -593,7 +592,7 @@ export class ActiveDoc extends EventEmitter { const wantFull = waitForFormulas || query.tableId.startsWith('_grist_') || tableAccess.read === 'mixed'; const onDemand = this._onDemandActions.isOnDemand(query.tableId); - this.logInfo(docSession, "fetchQuery(%s, %s) %s", docSession, JSON.stringify(query), + this.logInfo(docSession, "fetchQuery %s %s", JSON.stringify(query), onDemand ? "(onDemand)" : "(regular)"); let data: TableDataAction; if (onDemand) { diff --git a/sandbox/grist/column.py b/sandbox/grist/column.py index d2a12b30..10a4dd51 100644 --- a/sandbox/grist/column.py +++ b/sandbox/grist/column.py @@ -239,7 +239,7 @@ class NumericColumn(BaseColumn): _sample_date = moment.ts_to_date(0) _sample_datetime = moment.ts_to_dt(0, None, moment.TZ_UTC) -class DateColumn(BaseColumn): +class DateColumn(NumericColumn): """ DateColumn contains numerical timestamps represented as seconds since epoch, in type float, to midnight of specific UTC dates. Accessing them yields date objects. @@ -250,7 +250,7 @@ class DateColumn(BaseColumn): def sample_value(self): return _sample_date -class DateTimeColumn(BaseColumn): +class DateTimeColumn(NumericColumn): """ DateTimeColumn contains numerical timestamps represented as seconds since epoch, in type float, and a timestamp associated with the column. Accessing them yields datetime objects. @@ -265,7 +265,7 @@ class DateTimeColumn(BaseColumn): def sample_value(self): return _sample_datetime -class PositionColumn(BaseColumn): +class PositionColumn(NumericColumn): def __init__(self, table, col_id, col_info): super(PositionColumn, self).__init__(table, col_id, col_info) # This is a list of row_ids, ordered by the position. diff --git a/sandbox/grist/objtypes.py b/sandbox/grist/objtypes.py index c64687a7..2a18dd2f 100644 --- a/sandbox/grist/objtypes.py +++ b/sandbox/grist/objtypes.py @@ -14,6 +14,7 @@ of the form ['U', repr(obj)]. import exceptions import traceback from datetime import date, datetime +from math import isnan import moment import records @@ -92,6 +93,14 @@ class AltText(object): raise InvalidTypedValue(self._typename, self._text) +class UnmarshallableValue(object): + """ + Represents an UnmarshallableValue. There is nothing we can do with it except encode it back. + """ + def __init__(self, value_repr): + self.value_repr = value_repr + + # Unique sentinel value representing a pending value. It's encoded as ['P'], and shown to the user # as "Loading..." text. With the switch to stored formulas, it's currently only used when a # document was just migrated. @@ -122,9 +131,11 @@ def strict_equal(a, b): return False def equal_encoding(a, b): - if isinstance(a, (str, unicode, float, bool, long, int)) or a is None: - # pylint: disable=unidiomatic-typecheck + # pylint: disable=unidiomatic-typecheck + if isinstance(a, (str, unicode, bool, long, int)) or a is None: return type(a) == type(b) and a == b + if isinstance(a, float): + return type(a) == type(b) and (a == b or (isnan(a) and isnan(b))) return encode_object(a) == encode_object(b) def encode_object(value): @@ -160,6 +171,8 @@ def encode_object(value): return ['O', {key: encode_object(val) for key, val in value.iteritems()}] elif value == _pending_sentinel: return ['P'] + elif isinstance(value, UnmarshallableValue): + return ['U', value.value_repr] except Exception as e: pass # We either don't know how to convert the value, or failed during the conversion. Instead we @@ -174,6 +187,13 @@ def decode_object(value): """ try: if not isinstance(value, (list, tuple)): + if isinstance(value, unicode): + # TODO For now, the sandbox uses binary strings throughout; see TODO in main.py for more + # on this. Strings that come from JS become Python binary strings, and we will not see + # unicode here. But we may see it if unmarshalling data that comes from DB, since + # DocStorage encodes/decodes values by marshaling JS strings as unicode. For consistency, + # convert those unicode strings to binary strings too. + return value.encode('utf8') return value code = value[0] args = value[1:] @@ -188,9 +208,11 @@ def decode_object(value): elif code == 'L': return [decode_object(item) for item in args] elif code == 'O': - return {key: decode_object(val) for key, val in args[0].iteritems()} + return {decode_object(key): decode_object(val) for key, val in args[0].iteritems()} elif code == 'P': return _pending_sentinel + elif code == 'U': + return UnmarshallableValue(args[0]) raise KeyError("Unknown object type code %r" % code) except Exception as e: return RaisedException(e) diff --git a/sandbox/grist/test_types.py b/sandbox/grist/test_types.py index afeb492c..650a52a7 100644 --- a/sandbox/grist/test_types.py +++ b/sandbox/grist/test_types.py @@ -169,12 +169,12 @@ class TestTypes(test_engine.EngineTestCase): "stored": [ ["ModifyColumn", "Types", "date", {"type": "Text"}], ["BulkUpdateRecord", "Types", [13, 14, 15, 16, 17, 18], - {"date": ["False", "True", "1509556595", "8.153", "0", "1"]}], + {"date": ["False", "True", "1509556595.0", "8.153", "0.0", "1.0"]}], ["UpdateRecord", "_grist_Tables_column", 25, {"type": "Text"}] ], "undo": [ ["BulkUpdateRecord", "Types", [13, 14, 15, 16, 17, 18], - {"date": [False, True, 1509556595, 8.153, 0, 1]}], + {"date": [False, True, 1509556595.0, 8.153, 0.0, 1.0]}], ["ModifyColumn", "Types", "date", {"type": "Date"}], ["UpdateRecord", "_grist_Tables_column", 25, {"type": "Date"}] ] @@ -187,10 +187,10 @@ class TestTypes(test_engine.EngineTestCase): [12, "Chîcágö", "Chîcágö", "Chîcágö", "Chîcágö", "Chîcágö"], [13, False, "False", "False", "False", "False"], [14, True, "True", "True", "True", "True"], - [15, 1509556595, "1509556595.0","1509556595","1509556595","1509556595"], + [15, 1509556595, "1509556595.0","1509556595","1509556595","1509556595.0"], [16, 8.153, "8.153", "8.153", "8.153", "8.153"], - [17, 0, "0.0", "0", "False", "0"], - [18, 1, "1.0", "1", "True", "1"], + [17, 0, "0.0", "0", "False", "0.0"], + [18, 1, "1.0", "1", "True", "1.0"], [19, "", "", "", "", ""], [20, None, None, None, None, None] ]) @@ -267,13 +267,13 @@ class TestTypes(test_engine.EngineTestCase): self.assertPartialOutActions(out_actions, { "stored": [ ["ModifyColumn", "Types", "date", {"type": "Numeric"}], - ["BulkUpdateRecord", "Types", [13, 14, 15, 17, 18, 19], - {"date": [0.0, 1.0, 1509556595.0, 0.0, 1.0, None]}], + ["BulkUpdateRecord", "Types", [13, 14, 19], + {"date": [0.0, 1.0, None]}], ["UpdateRecord", "_grist_Tables_column", 25, {"type": "Numeric"}] ], "undo": [ - ["BulkUpdateRecord", "Types", [13, 14, 15, 17, 18, 19], - {"date": [False, True, 1509556595, 0, 1, ""]}], + ["BulkUpdateRecord", "Types", [13, 14, 19], + {"date": [False, True, ""]}], ["ModifyColumn", "Types", "date", {"type": "Date"}], ["UpdateRecord", "_grist_Tables_column", 25, {"type": "Date"}] ] @@ -367,12 +367,13 @@ class TestTypes(test_engine.EngineTestCase): self.assertPartialOutActions(out_actions, { "stored": [ ["ModifyColumn", "Types", "date", {"type": "Int"}], - ["BulkUpdateRecord", "Types", [13, 14, 16, 19], {"date": [0, 1, 8, None]}], + ["BulkUpdateRecord", "Types", [13, 14, 15, 16, 17, 18, 19], + {"date": [0, 1, 1509556595, 8, 0, 1, None]}], ["UpdateRecord", "_grist_Tables_column", 25, {"type": "Int"}] ], "undo": [ - ["BulkUpdateRecord", "Types", [13, 14, 16, 19], - {"date": [False, True, 8.153, ""]}], + ["BulkUpdateRecord", "Types", [13, 14, 15, 16, 17, 18, 19], + {"date": [False, True, 1509556595.0, 8.153, 0.0, 1.0, ""]}], ["ModifyColumn", "Types", "date", {"type": "Date"}], ["UpdateRecord", "_grist_Tables_column", 25, {"type": "Date"}] ]