From 48ca124f23eff287bb8af9b4414fe360198ab785 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Thu, 13 Aug 2020 16:56:36 -0400 Subject: [PATCH] (core) Render unmarshallable values as non-errors, using their repr() strings. Summary: - Instead of sending an "UnmarshallableError" as an exception, introduce an "Unmarshallable" type of value, represented as ['U', repr(value)] - Unmarshallable values are rendered using a bluish text color, no longer a pink background. - Factor out ErrorDom to be simpler and cleaner. - Add GristObjCode enum, and simplify related helpers. - Use safe_repr() for when repr() itself fails - Handle conversion errors using safe_repr() when str() fails Test Plan: Added a test case based on a fixture covering a bunch of cases. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D2584 --- app/common/ValueFormatter.ts | 1 + app/common/gristTypes.ts | 36 ++++++++++++------- sandbox/grist/objtypes.py | 68 ++++++++++++++---------------------- sandbox/grist/usertypes.py | 13 ++++--- 4 files changed, 60 insertions(+), 58 deletions(-) diff --git a/app/common/ValueFormatter.ts b/app/common/ValueFormatter.ts index e0bddda5..b3c500ee 100644 --- a/app/common/ValueFormatter.ts +++ b/app/common/ValueFormatter.ts @@ -22,6 +22,7 @@ export function formatObject(args: [string, ...any[]]): string { case 'd': return moment.tz(objArgs[0] * 1000, 'UTC').format("YYYY-MM-DD"); case 'R': return `${objArgs[0]}[${objArgs[1]}]`; case 'E': return gristTypes.formatError(args); + case 'U': return String(args[1]); case 'P': return PENDING_DATA_PLACEHOLDER; } return objType + "(" + JSON.stringify(objArgs).slice(1, -1) + ")"; diff --git a/app/common/gristTypes.ts b/app/common/gristTypes.ts index 9653b977..201ae0e3 100644 --- a/app/common/gristTypes.ts +++ b/app/common/gristTypes.ts @@ -7,7 +7,15 @@ export type GristType = 'Any' | 'Attachments' | 'Blob' | 'Bool' | 'Choice' | 'Da 'Id' | 'Int' | 'ManualSortPos' | 'Numeric' | 'PositionNumber' | 'Ref' | 'RefList' | 'Text'; // Letter codes for CellValue types encoded as [code, args...] tuples. -export type GristObjType = 'L' | 'D' | 'd' | 'R' | 'E' | 'P'; +export const enum GristObjCode { + List = 'L', + DateTime = 'D', + Date = 'd', + Reference = 'R', + Exception = 'E', + Pending = 'P', + Unmarshallable = 'U', +} export const MANUALSORT = 'manualSort'; @@ -50,11 +58,19 @@ export function isObject(value: CellValue): value is [string, any?] { return Array.isArray(value); } +/** + * Returns GristObjCode of the value if the value is an object, or null otherwise. + * The return type includes any string, since we should not assume we can only get valid codes. + */ +export function getObjCode(value: CellValue): GristObjCode|string|null { + return Array.isArray(value) ? value[0] : null; +} + /** * Returns whether a value (as received in a DocAction) represents a raised exception. */ export function isRaisedException(value: CellValue): boolean { - return Array.isArray(value) && value[0] === 'E'; + return getObjCode(value) === GristObjCode.Exception; } /** @@ -62,21 +78,14 @@ export function isRaisedException(value: CellValue): boolean { * which is a valid value for list types in grist. */ export function isListOrNull(value: CellValue): boolean { - return value === null || (Array.isArray(value) && value[0] === 'L'); + return value === null || (Array.isArray(value) && value[0] === GristObjCode.List); } /** * Returns whether a value (as received in a DocAction) represents an empty list. */ export function isEmptyList(value: CellValue): boolean { - return Array.isArray(value) && value.length === 1 && value[0] === 'L'; -} - -/** - * Returns whether a value (as received in a DocAction) represents a "Pending" value. - */ -export function isPending(value: CellValue): boolean { - return Array.isArray(value) && value[0] === 'P'; + return Array.isArray(value) && value.length === 1 && value[0] === GristObjCode.List; } /** @@ -97,8 +106,11 @@ function isNumber(v: CellValue) { return typeof v === 'number' || typeof v === ' function isNumberOrNull(v: CellValue) { return isNumber(v) || v === null; } function isBoolean(v: CellValue) { return typeof v === 'boolean' || v === 1 || v === 0; } +// These values are not regular cell values, even in a column of type Any. +const abnormalValueTypes: string[] = [GristObjCode.Exception, GristObjCode.Pending, GristObjCode.Unmarshallable]; + function isNormalValue(value: CellValue) { - return !(Array.isArray(value) && (value[0] === 'E' || value[0] === 'P')); + return !abnormalValueTypes.includes(getObjCode(value)!); } /** diff --git a/sandbox/grist/objtypes.py b/sandbox/grist/objtypes.py index c6ae971a..053e94b9 100644 --- a/sandbox/grist/objtypes.py +++ b/sandbox/grist/objtypes.py @@ -7,11 +7,11 @@ Non-primitive values are represented in actions as [type_name, args...]. objtypes.encode_object(obj) - returns a marshallable list representation. objtypes.decode_object(val) - returns an object represented by the [name, args...] argument. -If an object cannot be encoded or decoded, a RaisedError exception is encoded or returned instead. -In a formula, this would cause an exception to be raised. +If an object cannot be encoded or decoded, an "UnmarshallableValue" is returned instead +of the form ['U', repr(obj)]. """ -import marshal import exceptions +import marshal import traceback from datetime import date, datetime @@ -123,29 +123,35 @@ def deregister_converter(name): if prev: del _registered_converters_by_type[prev[0]] +def safe_repr(obj): + """ + Like repr(obj) but falls back to a simpler "" string when repr() itself fails. + """ + try: + return repr(obj) + except Exception: + return '<' + type(obj).__name__ + '>' + def encode_object(obj): """ Given an object, returns [typename, args...] array of marshallable values, which should be sufficient to reconstruct `obj`. Given a primitive object, returns it unchanged. - If obj failed to encode, yields an encoding for RaisedException(UnmarshallableError, message). - I.e. on reading this back, and using the value, we'll get UnmarshallableError exception. + If obj failed to encode, yields an encoding for an UnmarshallableValue object, containing + the repr(obj) string. """ try: t = type(obj) - try: - converter = ( - _registered_converters_by_type.get(t) or - _registered_converters_by_type[getattr(t, '_objtypes_converter_type', t)]) - except KeyError: - raise UnmarshallableError("No converter for type %s" % type(obj)) + converter = ( + _registered_converters_by_type.get(t) or + _registered_converters_by_type[getattr(t, '_objtypes_converter_type', t)]) return converter(obj) except Exception as e: - # Don't risk calling encode_object recursively; instead encode a RaisedException error - # manually with arguments that ought not fail. - return ["E", "UnmarshallableError", str(e), repr(obj)] + # We either don't know how to convert the value, or failed during the conversion. Instead we + # return an "UnmarshallableValue" object, with repr() of the value to show to the user. + return ['U', safe_repr(obj)] def decode_object(value): @@ -189,10 +195,7 @@ class SelfConverter(object): def _encode_obj_impl(converter, name): def inner(obj): - try: - args = converter.encode_args(obj) - except Exception: - raise UnmarshallableError("Encoding of %s failed" % name) + args = converter.encode_args(obj) for arg in args: check_marshallable(arg) @@ -245,7 +248,11 @@ class RaisedException(object): @classmethod def decode_args(cls, *args): - return cls(decode_object(args)) + # Decoding of a RaisedException is currently only used in tests. + name = args[0] + exc_type = getattr(exceptions, name) + assert isinstance(exc_type, type) and issubclass(exc_type, BaseException) + return cls(exc_type(*args[1:])) def __eq__(self, other): return isinstance(other, type(self)) and self.encode_args() == other.encode_args() @@ -254,29 +261,6 @@ class RaisedException(object): return not self.__eq__(other) -class ExceptionConverter(object): - """ - Converter for any type derived from BaseException. On encoding it returns the exception object's - .args attribute, and uses them on decoding as constructor arguments to instantiate the error. - """ - @classmethod - def encode_args(cls, obj): - return list(getattr(obj, 'args', ())) - - @classmethod - def decode_args(cls, type_, args): - return type_(*args) - - -# Register all Exceptions as valid types that can be handled by Grist. -for _, my_type in exceptions.__dict__.iteritems(): - if isinstance(my_type, type) and issubclass(my_type, BaseException): - register_converter(ExceptionConverter, my_type) - -# Register the special exceptions we defined. -register_converter(ExceptionConverter, UnmarshallableError) -register_converter(ExceptionConverter, ConversionError) - # Register the special wrapper class for raised exceptions with a custom short name. register_converter(SelfConverter, RaisedException, "E") diff --git a/sandbox/grist/usertypes.py b/sandbox/grist/usertypes.py index 58e86007..a0943a5d 100644 --- a/sandbox/grist/usertypes.py +++ b/sandbox/grist/usertypes.py @@ -171,10 +171,15 @@ class BaseColumnType(object): return self.do_convert(value_to_convert) except Exception as e: # If conversion failed, return a string to serve as alttext. - if isinstance(value_to_convert, six.text_type): - # str() will fail for a non-ascii unicode object, which needs an explicit encoding. - return value_to_convert.encode('utf8') - return str(value_to_convert) + try: + if isinstance(value_to_convert, six.text_type): + # str() will fail for a non-ascii unicode object, which needs an explicit encoding. + return value_to_convert.encode('utf8') + return str(value_to_convert) + except Exception: + # If converting to string failed, we should still produce something. + return objtypes.safe_repr(value_to_convert) + # This is a user-facing method, hence the camel-case naming, as for `lookupRecords` and such. @classmethod