From 08b91c4cb7628ec9d2777aef67042efc1965c22a Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Tue, 3 Sep 2024 01:58:48 -0400 Subject: [PATCH] (core) Fix serialization of values derived from primitive types, like IntEnum. Summary: There is a range of types that -- when returned from formulas -- used to cause bad errors (that looked like a data engine crash and were reported as "Memory Error") because they looked like primitive types but were not marshallable. For example, IntEnum. We now encode such values as the primitive type they are based on. Test Plan: - Added a unittest that encode_object() now handles problematic values. - Added a browser test case that problematic values are no longer causing errors. Reviewers: jarek Reviewed By: jarek Differential Revision: https://phab.getgrist.com/D4336 --- sandbox/grist/objtypes.py | 17 +- sandbox/grist/test_objtypes.py | 72 ++++++ test/nbrowser/Formulas.ts | 396 +++++++++++++++++++++++++++++++++ 3 files changed, 482 insertions(+), 3 deletions(-) create mode 100644 sandbox/grist/test_objtypes.py create mode 100644 test/nbrowser/Formulas.ts diff --git a/sandbox/grist/objtypes.py b/sandbox/grist/objtypes.py index a77b83b4..ea5335f4 100644 --- a/sandbox/grist/objtypes.py +++ b/sandbox/grist/objtypes.py @@ -167,15 +167,26 @@ def encode_object(value): Produces a Grist-encoded version of the value, e.g. turning a Date into ['d', timestamp]. Returns ['U', repr(value)] if it fails to encode otherwise. """ + # pylint: disable=unidiomatic-typecheck try: - if isinstance(value, (six.text_type, float, bool)) or value is None: + # A primitive type can be returned directly. + if type(value) in (six.text_type, float, bool) or value is None: return value + # Other instances of these types must be derived; cast these to the primitive type to ensure + # they are marshallable. + elif isinstance(value, six.text_type): + return six.text_type(value) + elif isinstance(value, float): + return float(value) + elif isinstance(value, bool): + return bool(value) elif isinstance(value, six.binary_type): return value.decode('utf8') elif isinstance(value, six.integer_types): if not is_int_short(value): - raise UnmarshallableError("Integer too large") - return value + return ['U', str(value)] + # Cast to a primitive type to ensure it's marshallable (e.g. enum.IntEnum would not be). + return int(value) elif isinstance(value, AltText): return six.text_type(value) elif isinstance(value, records.Record): diff --git a/sandbox/grist/test_objtypes.py b/sandbox/grist/test_objtypes.py new file mode 100644 index 00000000..6aed76f9 --- /dev/null +++ b/sandbox/grist/test_objtypes.py @@ -0,0 +1,72 @@ +import datetime +import enum +import marshal +import unittest + +import objtypes + +class TestObjTypes(unittest.TestCase): + class Int(int): + pass + class Float(float): + pass + class Text(str): + pass + class MyEnum(enum.IntEnum): + ONE = 1 + class FussyFloat(float): + def __float__(self): + raise TypeError("Cannot cast FussyFloat to float") + + + # (value, expected encoded value, expected decoded value) + values = [ + (17, 17), + (-17, -17), + (0, 0), + # The following is an unmarshallable value. + (12345678901234567890, ['U', '12345678901234567890']), + (0.0, 0.0), + (1e-20, 1e-20), + (1e20, 1e20), + (1e40, 1e40), + (float('infinity'), float('infinity')), + (True, True), + (Int(5), 5), + (MyEnum.ONE, 1), + (Float(3.3), 3.3), + (Text("Hello"), u"Hello"), + (datetime.date(2024, 9, 2), ['d', 1725235200.0]), + (datetime.datetime(2024, 9, 2, 3, 8, 21), ['D', 1725246501, 'UTC']), + # This is also unmarshallable. + (FussyFloat(17.0), ['U', '17.0']), + # Various other values are unmarshallable too. + (len, ['U', '']), + # List, and list with an unmarshallable value. + ([Float(6), "", MyEnum.ONE], ['L', 6, "", 1]), + ([Text("foo"), FussyFloat(-0.5)], ['L', "foo", ['U', '-0.5']]), + ] + + def test_encode_object(self): + for (value, expected_encoded) in self.values: + encoded = objtypes.encode_object(value) + + # Check that encoding is as expected. + self.assertStrictEqual(encoded, expected_encoded, 'encoding of %r' % value) + + # Check it can be round-tripped through marshalling. + marshaled = marshal.dumps(encoded) + self.assertStrictEqual(marshal.loads(marshaled), encoded, 'de-marshalling of %r' % value) + + # Check that the decoded value, though it may not be identical, encodes identically. + decoded = objtypes.decode_object(encoded) + re_encoded = objtypes.encode_object(decoded) + self.assertStrictEqual(re_encoded, encoded, 're-encoding of %r' % value) + + def assertStrictEqual(self, a, b, msg=None): + self.assertEqual(a, b, '%s: %r != %r' % (msg, a, b)) + self.assertEqual(type(a), type(b), '%s: %r != %r' % (msg, type(a), type(b))) + + +if __name__ == "__main__": + unittest.main() diff --git a/test/nbrowser/Formulas.ts b/test/nbrowser/Formulas.ts new file mode 100644 index 00000000..0c2859b5 --- /dev/null +++ b/test/nbrowser/Formulas.ts @@ -0,0 +1,396 @@ +import {assert, driver, Key, WebElement} from 'mocha-webdriver'; +import * as gu from 'test/nbrowser/gristUtils'; +import {setupTestSuite} from 'test/nbrowser/testUtils'; + +async function checkHasLinkStyle(elem: WebElement, yesNo: boolean) { + assert.equal(await elem.getCssValue('text-decoration-line'), yesNo ? 'underline' : 'none'); +} + + +describe('Formulas', function() { + this.timeout(20000); + const cleanup = setupTestSuite(); + + let session: gu.Session; + let docId: string; + + after(async function() { + // In case of an error, close any open autocomplete and cell editor, to ensure we don't + // interfere with subsequent tests by unsaved value triggering an alert on unload. + await driver.sendKeys(Key.ESCAPE); + await driver.sendKeys(Key.ESCAPE); + }); + + before(async function() { + session = await gu.session().login(); + docId = (await session.tempDoc(cleanup, 'Favorite_Films.grist')).id; + }); + + it('should highlight column in full edit mode', async function() { + await gu.addColumn('A'); + await gu.addColumn('B'); + await gu.addColumn('C'); + // Make sure we are not in edit mode, finding column C is enough. + await gu.getColumnHeader({ col : 'C'}); + await driver.sendKeys('='); + await gu.waitAppFocus(false); + if (await driver.find('.test-editor-tooltip-convert').isPresent()) { + await driver.find('.test-editor-tooltip-convert').click(); + } + await driver.sendKeys(" "); + // Make sure we are now in edit mode. + await gu.getColumnHeader({ col : '$C'}); + // Move mouse over other column, and make sure it is highlighted + const hoverOver = async (col: string) => + await driver.withActions((actions) => ( + actions + .move({origin: gu.getCell(col, 1)}) + .move({origin: gu.getCell(col, 2)}) + )); + const tooltipId = '.test-column-formula-tooltip'; + // Helper to test if hover is on right column. + const isHoverOn = async (col: string) => { + // Make sure we have only 1 tooltip. + assert.equal(1, (await driver.findAll(tooltipId)).length); + // Make sure column has hover class. + assert.isTrue(await gu.getColumnHeader({ col }).matches(".hover-column")); + // Make sure first row has hover class. + assert.isTrue(await gu.getCell(col, 1).matches(".hover-column")); + // Make sure tooltip shows correct text. + assert.equal(`Click to insert ${col}`, await driver.find(tooltipId).getText()); + }; + // Helper to test that no column is in hover state. + const noHoverAtAll = async () => { + // No tooltip is present. + assert.equal(0, (await driver.findAll(tooltipId)).length); + // Make sure no column has hover class. + assert.equal(0, (await driver.findAll(".hover-column")).length); + }; + // Helper to test that column is not in hover state + const noHoverOn = async (col: string) => { + // Header doesn't have hover class + assert.isFalse(await gu.getColumnHeader({ col }).matches(".hover-column")); + // Fields don't have hover class + assert.isFalse(await gu.getCell(col, 1).matches(".hover-column")); + // If there is a tooltip, it doesn't have text with this column + if ((await driver.findAll(tooltipId)).length) { + assert.notEqual(`Click to insert ${col}`, await driver.find(tooltipId).getText()); + } + }; + await hoverOver('$A'); + await isHoverOn('$A'); + await noHoverOn('$B'); + // Make sure tooltip is closed and opened on another column. + await hoverOver('$B'); + await isHoverOn('$B'); + await noHoverOn('$A'); + + // Make sure it is closed when leaving rows from the corners: + // - First moving on the row number + await hoverOver('$A'); + await isHoverOn('$A'); + await driver.withActions((actions) => actions.move({origin: driver.find('.gridview_data_row_num')})); + await noHoverAtAll(); + // - Moving over add button + await hoverOver('$A'); + await isHoverOn('$A'); + await driver.withActions((actions) => actions.move({origin: driver.find('.mod-add-column')})); + await noHoverAtAll(); + // - Moving below last row + await hoverOver('$A'); + await isHoverOn('$A'); + await driver.withActions((actions) => + actions + .move({origin: gu.getCell('$A', 7)}) + .move({origin: gu.getCell('$A', 7), y : 22 + 1}) + ); + await noHoverAtAll(); + // - Moving right after last column + await hoverOver('$A'); + await isHoverOn('$A'); + // First move to the last cell, + await driver.withActions((actions) => + actions + .move({origin: gu.getCell("$C", 7)}) // move add row on last column + ); + await isHoverOn('$C'); + await noHoverOn('$A'); + // and then a little bit to the right (100px is width of the field) + await driver.withActions((actions) => + actions + .move({origin: gu.getCell("$C", 7), x : 100 + 1}) + ); + await noHoverAtAll(); + + // - Moving mouse on top of the grid. + await hoverOver('$A'); + await isHoverOn('$A'); + // move on the A header, + await driver.withActions((actions) => actions.move({origin: gu.getColumnHeader({ col : '$A' })})); + // still hover should be on A column, + await isHoverOn('$A'); + // and now jump out of the grid (22 is height of the row) + await driver.withActions((actions) => actions.move({origin: gu.getColumnHeader({ col : '$A' }), y : -22 - 3})); + await noHoverAtAll(); + // undo adding 3 columns + await driver.sendKeys(Key.ESCAPE); + await gu.undo(3); + await gu.checkForErrors(); + }); + + it('should evaluate formulas requiring lazy-evaluation', async function() { + await gu.renameColumn({col: 'Budget (millions)'}, 'Budget'); + + await gu.addColumn('A'); + await gu.enterFormula('IFERROR($Invalid if $Budget > 50 else $Budget, "X")'); + assert.deepEqual(await gu.getVisibleGridCells('A', [1, 2, 3]), ['30', 'X', '10']); + + await gu.addColumn('B'); + // This formula triggers an error for one cell, AltText for another. + await gu.enterFormula('($Budget - 30) / ($Budget - 10) or "hello"'); + await gu.setType(/Numeric/); + assert.deepEqual(await gu.getVisibleGridCells('B', [1, 2, 3]), ['hello', '0.5555555556', '#DIV/0!']); + + // ISERROR considers exceptions and AltText values. + await gu.addColumn('C'); + await gu.enterFormula('ISERROR($B)'); + assert.deepEqual(await gu.getVisibleGridCells('C', [1, 2, 3]), ['true', 'false', 'true']); + + // ISERR considers exceptions but not AltText values. + await gu.addColumn('D'); + await gu.enterFormula('(ISERR($B)'); + assert.deepEqual(await gu.getVisibleGridCells('D', [1, 2, 3]), ['false', 'false', 'true']); + }); + + it('should support formulas returning unmarshallable or weird values', async function() { + // Formulas can return strange values, and Grist should do a reasonable job displaying them. + // In particular, this verifies a fix to a bug where some values could cause an error that + // looked like a crash of the data engine. + await gu.getCell({rowNum: 1, col: 'A'}).click(); + + // Our goal is to test output of formulas, so skip the slow and flaky typing in of a long + // multi-line formula, use API to set it instead. + const api = session.createHomeApi(); + await api.applyUserActions(docId, [['ModifyColumn', 'Films', 'A', { + isFormula: true, + formula: `\ +import enum +class Int(int): + pass +class Float(float): + pass +class Text(str): + pass +class MyEnum(enum.IntEnum): + ONE = 1 +class FussyFloat(float): + def __float__(self): + raise TypeError("Cannot cast FussyFloat to float") + +if $id > 1: + return None +return [ + -17, 0.0, 12345678901234567890, 1e-20, True, + Int(5), MyEnum.ONE, Float(3.3), Text('Hello'), + datetime.date(2024, 9, 2), datetime.datetime(2024, 9, 2, 3, 8, 21), + FussyFloat(17.0), [Float(6), '', MyEnum.ONE] +] +` + }]]); + + // Wait for the row we expect to become empty, to ensure the formula got processed. + await gu.waitToPass(async () => assert.equal(await gu.getCell({rowNum: 2, col: 'A'}).getText(), "")); + // Check the result of the formula: normal return, values correspond to what we asked. + const expected = `\ +[-17, 0, 12345678901234567890, 1e-20, true, \ +5, 1, 3.3, "Hello", \ +2024-09-02, 2024-09-02T03:08:21.000Z, \ +17.0, [6, "", 1]]`; + assert.deepEqual(await gu.getVisibleGridCells('A', [1, 2, 3]), [expected, '', '']); + }); + + it('should strip out leading equal-sign users might think is needed', async function() { + await gu.getCell({rowNum: 1, col: 'A'}).click(); + await gu.enterFormula('$Budget*10'); + assert.deepEqual(await gu.getVisibleGridCells('A', [1, 2, 3]), ['300', '550', '100']); + await gu.enterFormula('= $Budget*100'); + assert.deepEqual(await gu.getVisibleGridCells('A', [1, 2, 3]), ['3000', '5500', '1000']); + + await gu.sendKeys(Key.ENTER); + assert.equal(await gu.getFormulaText(), ' $Budget*100'); + await gu.sendKeys(Key.ESCAPE); + await gu.undo(2); + }); + + it('should not fail when formulas have valid indent or leading whitespace', async function() { + await gu.getCell({rowNum: 1, col: 'A'}).click(); + + await gu.enterFormula(" $Budget * 10"); + assert.deepEqual(await gu.getVisibleGridCells('A', [1, 2, 3]), ['300', '550', '100']); + + await driver.sendKeys('='); + await gu.waitAppFocus(false); + // A single long string often works, but sometimes fails, so break up into multiple. + await gu.sendKeys(` if $Budget > 50:${Key.chord(Key.SHIFT, Key.ENTER)}`); + await driver.sleep(50); + // The next line should get auto-indented. + await gu.sendKeys(`return 'Big'${Key.chord(Key.SHIFT, Key.ENTER)}`); + await driver.sleep(50); + // In the next line, we want to remove one level of indent. + await gu.sendKeys(`${Key.BACK_SPACE}return 'Small'`); + await gu.sendKeys(Key.ENTER); + await gu.waitForServer(); + + await gu.sendKeys(Key.ENTER); + assert.equal(await gu.getFormulaText(), " if $Budget > 50:\n return 'Big'\n return 'Small'"); + await gu.sendKeys(Key.ESCAPE); + + assert.deepEqual(await gu.getVisibleGridCells('A', [1, 2, 3]), ['Small', 'Big', 'Small']); + + await gu.undo(2); + }); + + it('should support autocompletion from lowercase values', async function() { + await gu.toggleSidePanel('right', 'close'); + await gu.getCell({rowNum: 1, col: 'A'}).click(); + await driver.sendKeys('='); + await gu.waitAppFocus(false); + + // Type in "me", and expect uppercase completions like "MEDIAN". + await driver.sendKeys('me'); + await gu.waitToPass(async () => + assert.includeMembers(await driver.findAll('.ace_autocomplete .ace_line', el => el.getText()), [ + "ME\nDIAN\n(value, *more_values)\n ", + "me\nmoryview(\n ", + ]) + ); + + // Using a completion of a function with signature should only insert an appropriate snippet. + await driver.sendKeys(Key.DOWN); + await driver.sendKeys(Key.ENTER); + await driver.findContentWait('.ace_content', /^MEDIAN\($/, 1000); + await driver.sendKeys(Key.ESCAPE); + await gu.waitAppFocus(true); + + // Check that this works also for table names ("fri" finds "Friends") + await driver.sendKeys('='); + await gu.waitAppFocus(false); + await driver.sendKeys('fri'); + await gu.waitToPass(async () => { + const completions = await driver.findAll('.ace_autocomplete .ace_line', el => el.getText()); + assert.isTrue(completions[0].startsWith("Fri\nends")); + assert.isTrue(completions[1].startsWith("Fri\nends.\nlookupOne\n(colName=, ...)")); + assert.isTrue(completions[2].startsWith("Fri\nends.\nlookupRecords\n(colName=, ...)")); + assert.isTrue(completions[3].startsWith("Fri\nends.lookupRecords(Favorite_Film=$id)")); + }); + await driver.sendKeys(Key.DOWN, Key.ENTER); + + // Check that completing a table's method suggests lookup methods with signatures. + await driver.sendKeys('.'); + await gu.waitToPass(async () => { + const completions = await driver.findAll('.ace_autocomplete .ace_line', el => el.getText()); + assert.isTrue(completions[0].startsWith("Friends.\nall")); + assert.isTrue(completions[1].startsWith("Friends.\nlookupOne\n(colName=, ...)")); + assert.isTrue(completions[2].startsWith("Friends.\nlookupRecords\n(colName=, ...)")); + assert.isTrue(completions[3].startsWith("Friends.\nlookupRecords(Favorite_Film=$id)")); + assert.isTrue(completions[4].startsWith("Friends.\nRecord")); + assert.isTrue(completions[5].startsWith("Friends.\nRecordSet")); + }); + + // Check that selecting a table method inserts an appropriate snippet. + await driver.sendKeys(Key.DOWN, Key.DOWN, Key.ENTER); + await driver.findContentWait('.ace_content', /^Friends\.lookupOne\($/, 1000); + await driver.sendKeys(Key.ESCAPE, Key.ESCAPE); + await gu.waitAppFocus(true); + + // Check that some built-in values are recognized in lowercase. + async function testBuiltin(typedText: string, expectedCompletion: string) { + await driver.sendKeys('='); + await gu.waitAppFocus(false); + await driver.sendKeys(typedText); + await gu.waitToPass(async () => + assert.include(await driver.findAll('.ace_autocomplete .ace_line', el => el.getText()), expectedCompletion)); + await driver.sendKeys(Key.ESCAPE, Key.ESCAPE); + await gu.waitAppFocus(true); + } + await testBuiltin('tr', 'Tr\nue\n '); + await testBuiltin('fa', 'Fa\nlse\n '); + await testBuiltin('no', 'No\nne\n '); + }); + + it('should link some suggested functions to their documentation', async function() { + await gu.getCell({rowNum: 1, col: 'A'}).click(); + await driver.sendKeys('='); + await gu.waitAppFocus(false); + await driver.sendKeys('me'); + + await gu.waitToPass(async () => { + const completions = await driver.findAll('.ace_autocomplete .ace_line'); + assert.include(await completions[0].getText(), 'ME\nDIAN\n(value, *more_values)\n '); + assert.include(await completions[1].getText(), 'me\nmoryview(\n '); + + // Check that the link is rendered with an underline. + await checkHasLinkStyle(completions[0].findContent('span', /ME/), true); + await checkHasLinkStyle(completions[0].findContent('span', /DIAN/), true); + await checkHasLinkStyle(completions[0].findContent('span', /value/), false); + }); + + // Click the link part: it should open a new tab to a documentation URL. + await driver.findContent('.ace_autocomplete .ace_line span', /DIAN/).click(); + // Switch to the new tab, and wait for the page to load. + let handles = await driver.getAllWindowHandles(); + await driver.switchTo().window(handles[1]); + await gu.waitForUrl('support.getgrist.com'); + assert.equal(await driver.getCurrentUrl(), 'https://support.getgrist.com/functions/#median'); + await driver.close(); + await driver.switchTo().window(handles[0]); + + // Click now a part of the completion that's not the link. It should insert the suggestion. + await driver.findContent('.ace_autocomplete .ace_line span', /value/).click(); + await driver.findContentWait('.ace_content', /^MEDIAN\($/, 1000); + await driver.sendKeys(Key.ESCAPE); + await gu.waitAppFocus(true); + + // Check that this works also for table names ("fri" finds "Friends") + await driver.sendKeys('='); + await gu.waitAppFocus(false); + await driver.sendKeys('Friends.'); + // Formula autocompletions in Ace editor are flaky (particularly when on a busy machine where + // setTimeout of 0 may take longer than expected). If the completion didn't work the first + // time, re-type the last character to trigger it again. This seems reliable. + if (!await driver.findContentWait('.ace_autocomplete .ace_line', 'Friends.\nRecord\n ', 500).catch(() => false)) { + await driver.sendKeys(Key.BACK_SPACE, '.'); + } + + await gu.waitToPass(async () => { + const completions = await driver.findAll('.ace_autocomplete .ace_line'); + assert.include(await completions[0].getText(), 'Friends.\nall\n '); + assert.include(await completions[1].getText(), 'Friends.\nlookupOne\n(colName=, ...)\n'); + assert.include(await completions[2].getText(), 'Friends.\nlookupRecords\n(colName=, ...)\n'); + assert.include(await completions[3].getText(), 'Friends.\nlookupRecords(Favorite_Film=$id)\n '); + assert.include(await completions[4].getText(), 'Friends.\nRecord\n '); + assert.include(await completions[5].getText(), 'Friends.\nRecordSet\n '); + + await checkHasLinkStyle(completions[1].findContent('span', /Friends/), false); + await checkHasLinkStyle(completions[1].findContent('span', /lookupOne/), true); + await checkHasLinkStyle(completions[1].findContent('span', '('), false); + await checkHasLinkStyle(completions[2].findContent('span', /Friends/), false); + await checkHasLinkStyle(completions[2].findContent('span', /lookupRecords/), true); + }, 4000); + + // Again, click the link part. + await driver.findContent('.ace_autocomplete .ace_line span', /lookupRecords/).click(); + handles = await driver.getAllWindowHandles(); + await driver.switchTo().window(handles[1]); + await gu.waitForUrl('support.getgrist.com'); + assert.equal(await driver.getCurrentUrl(), 'https://support.getgrist.com/functions/#lookuprecords'); + await driver.close(); + await driver.switchTo().window(handles[0]); + + // Now click the non-link part. + await driver.findContent('.ace_autocomplete .ace_line', /lookupRecords/).findContent('span', /Friends/).click(); + await driver.findContentWait('.ace_content', /^Friends\.lookupRecords\($/, 1000); + await driver.sendKeys(Key.ESCAPE, Key.ESCAPE); + await gu.waitAppFocus(true); + }); +});