From 0de0cb0f4a667031bf8934fdbdee887297159d53 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Fri, 11 Feb 2022 15:10:53 +0200 Subject: [PATCH] (core) Add PUT /records DocApi endpoint to AddOrUpdate records Summary: As designed in https://grist.quip.com/fZSrAnJKgO5j/Add-or-Update-Records-API Current `POST /records` adds records, and `PATCH /records` updates them by row ID. This adds `PUT /records` to 'upsert' records, applying the AddOrUpdate user action. PUT was chosen because it's idempotent. Using a separate method (instead of inferring based on the request body) also cleanly separates validation, documentation, etc. The name `require` for the new property was suggested by Paul because `where` isn't very clear when adding records. Test Plan: New DocApi tests Reviewers: jarek Reviewed By: jarek Differential Revision: https://phab.getgrist.com/D3251 --- app/common/ValueParser.ts | 38 ++++++++++++------ app/gen-server/lib/DocApiForwarder.ts | 2 +- app/server/lib/DocApi.ts | 37 ++++++++++++++++-- app/server/lib/DocApiTypes-ti.ts | 17 ++++++++ app/server/lib/DocApiTypes.ts | 15 +++++++ app/server/lib/FlexServer.ts | 2 +- app/server/lib/requestUtils.ts | 8 +++- sandbox/grist/column.py | 19 +-------- sandbox/grist/table.py | 4 ++ sandbox/grist/test_lookups.py | 28 ++++++++++++++ sandbox/grist/test_useractions.py | 56 ++++++++++++++++++++++++--- sandbox/grist/useractions.py | 42 +++++++++++++++++--- 12 files changed, 220 insertions(+), 48 deletions(-) diff --git a/app/common/ValueParser.ts b/app/common/ValueParser.ts index df473142..a679c961 100644 --- a/app/common/ValueParser.ts +++ b/app/common/ValueParser.ts @@ -357,21 +357,35 @@ function parseColValues( } export function parseUserAction(ua: UserAction, docData: DocData): UserAction { - const actionType = ua[0] as string; - let parseBulk: boolean; - - if (['AddRecord', 'UpdateRecord'].includes(actionType)) { - parseBulk = false; - } else if (['BulkAddRecord', 'BulkUpdateRecord', 'ReplaceTableData'].includes(actionType)) { - parseBulk = true; - } else { - return ua; + switch (ua[0]) { + case 'AddRecord': + case 'UpdateRecord': + return _parseUserActionColValues(ua, docData, false); + case 'BulkAddRecord': + case 'BulkUpdateRecord': + case 'ReplaceTableData': + return _parseUserActionColValues(ua, docData, true); + case 'AddOrUpdateRecord': + // Parse `require` (2) and `col_values` (3). The action looks like: + // ['AddOrUpdateRecord', table_id, require, col_values, options] + // (`col_values` is called `fields` in the API) + ua = _parseUserActionColValues(ua, docData, false, 2); + ua = _parseUserActionColValues(ua, docData, false, 3); + return ua; + default: + return ua; } +} +// Returns a copy of the user action with one element parsed, by default the last one +function _parseUserActionColValues(ua: UserAction, docData: DocData, parseBulk: boolean, index?: number +): UserAction { ua = ua.slice(); const tableId = ua[1] as string; - const lastIndex = ua.length - 1; - const colValues = ua[lastIndex] as ColValues | BulkColValues; - ua[lastIndex] = parseColValues(tableId, colValues, docData, parseBulk); + if (index === undefined) { + index = ua.length - 1; + } + const colValues = ua[index] as ColValues | BulkColValues; + ua[index] = parseColValues(tableId, colValues, docData, parseBulk); return ua; } diff --git a/app/gen-server/lib/DocApiForwarder.ts b/app/gen-server/lib/DocApiForwarder.ts index 6e7415c7..adb632b6 100644 --- a/app/gen-server/lib/DocApiForwarder.ts +++ b/app/gen-server/lib/DocApiForwarder.ts @@ -93,7 +93,7 @@ export class DocApiForwarder { method: req.method, headers, }; - if (['POST', 'PATCH'].includes(req.method)) { + if (['POST', 'PATCH', 'PUT'].includes(req.method)) { // uses `req` as a stream options.body = req; } diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index 150ba910..8e8f0c3f 100644 --- a/app/server/lib/DocApi.ts +++ b/app/server/lib/DocApi.ts @@ -56,9 +56,10 @@ const MAX_PARALLEL_REQUESTS_PER_DOC = 10; type WithDocHandler = (activeDoc: ActiveDoc, req: RequestWithLogin, resp: Response) => Promise; // Schema validators for api endpoints that creates or updates records. -const {RecordsPatch, RecordsPost} = t.createCheckers(DocApiTypesTI, GristDataTI); +const {RecordsPatch, RecordsPost, RecordsPut} = t.createCheckers(DocApiTypesTI, GristDataTI); RecordsPatch.setReportedPath("body"); RecordsPost.setReportedPath("body"); +RecordsPut.setReportedPath("body"); /** * Middleware for validating request's body with a Checker instance. @@ -265,13 +266,16 @@ export class DocWorkerApi { return allSame; } + function fieldNames(records: any[]) { + return new Set(_.flatMap(records, r => Object.keys({...r.fields, ...r.require}))); + } + function convertToBulkColValues(records: Array): BulkColValues { // User might want to create empty records, without providing a field name, for example for requests: // { records: [{}] }; { records: [{fields:{}}] } // Retrieve all field names from fields property. - const fieldNames = new Set(_.flatMap(records, r => Object.keys(r.fields ?? {}))); const result: BulkColValues = {}; - for (const fieldName of fieldNames) { + for (const fieldName of fieldNames(records)) { result[fieldName] = records.map(record => record.fields?.[fieldName] ?? null); } return result; @@ -414,6 +418,31 @@ export class DocWorkerApi { }) ); + // Add or update records given in records format + this._app.put('/api/docs/:docId/tables/:tableId/records', canEdit, validate(RecordsPut), + withDoc(async (activeDoc, req, res) => { + const {records} = req.body as Types.RecordsPut; + const {tableId} = req.params; + const {noadd, noupdate, noparse, allow_empty_require} = req.query; + const onmany = stringParam(req.query.onmany || "first", "onmany", ["first", "none", "all"]); + const options = { + add: !isAffirmative(noadd), + update: !isAffirmative(noupdate), + on_many: onmany, + allow_empty_require: isAffirmative(allow_empty_require), + }; + const actions = records.map(rec => + ["AddOrUpdateRecord", tableId, rec.require, rec.fields || {}, options] + ); + await handleSandboxError(tableId, [...fieldNames(records)], activeDoc.applyUserActions( + docSessionFromRequest(req), + actions, + {parseStrings: !isAffirmative(noparse)}, + )); + res.json(null); + }) + ); + // Add a new webhook and trigger this._app.post('/api/docs/:docId/tables/:tableId/_subscribe', isOwner, withDoc(async (activeDoc, req, res) => { @@ -937,7 +966,7 @@ async function handleSandboxError(tableId: string, colNames: string[], p: Pro if (match) { throw new ApiError(`Invalid row id ${match[1]}`, 400); } - match = e.message.match(/\[Sandbox\] KeyError u?'(.*?)'/); + match = e.message.match(/\[Sandbox] KeyError u?'(?:Table \w+ has no column )?(\w+)'/); if (match) { if (match[1] === tableId) { throw new ApiError(`Table not found "${tableId}"`, 404); diff --git a/app/server/lib/DocApiTypes-ti.ts b/app/server/lib/DocApiTypes-ti.ts index 6306e2fb..77e1ee6f 100644 --- a/app/server/lib/DocApiTypes-ti.ts +++ b/app/server/lib/DocApiTypes-ti.ts @@ -17,6 +17,17 @@ export const Record = t.iface([], { }), }); +export const AddOrUpdateRecord = t.iface([], { + "require": t.intersection(t.iface([], { + [t.indexKey]: "CellValue", + }), t.iface([], { + "id": t.opt("number"), + })), + "fields": t.opt(t.iface([], { + [t.indexKey]: "CellValue", + })), +}); + export const RecordsPatch = t.iface([], { "records": t.tuple("Record", t.rest(t.array("Record"))), }); @@ -25,10 +36,16 @@ export const RecordsPost = t.iface([], { "records": t.tuple("NewRecord", t.rest(t.array("NewRecord"))), }); +export const RecordsPut = t.iface([], { + "records": t.tuple("AddOrUpdateRecord", t.rest(t.array("AddOrUpdateRecord"))), +}); + const exportedTypeSuite: t.ITypeSuite = { NewRecord, Record, + AddOrUpdateRecord, RecordsPatch, RecordsPost, + RecordsPut, }; export default exportedTypeSuite; diff --git a/app/server/lib/DocApiTypes.ts b/app/server/lib/DocApiTypes.ts index e515048f..6e3b01b7 100644 --- a/app/server/lib/DocApiTypes.ts +++ b/app/server/lib/DocApiTypes.ts @@ -15,6 +15,14 @@ export interface Record { fields: { [coldId: string]: CellValue }; } +/** + * JSON schema for api /record endpoint. Used in PUT method for adding or updating records. + */ +export interface AddOrUpdateRecord { + require: { [coldId: string]: CellValue } & { id?: number }; + fields?: { [coldId: string]: CellValue }; +} + /** * JSON schema for the body of api /record PATCH endpoint */ @@ -28,3 +36,10 @@ export interface RecordsPatch { export interface RecordsPost { records: [NewRecord, ...NewRecord[]]; // at least one record is required } + +/** + * JSON schema for the body of api /record PUT endpoint + */ +export interface RecordsPut { + records: [AddOrUpdateRecord, ...AddOrUpdateRecord[]]; // at least one record is required +} diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 48735a1e..844b1d58 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -1671,7 +1671,7 @@ function allowTestLogin() { function trustOriginHandler(req: express.Request, res: express.Response, next: express.NextFunction) { if (trustOrigin(req, res)) { res.header("Access-Control-Allow-Credentials", "true"); - res.header("Access-Control-Allow-Methods", "GET, PATCH, POST, DELETE, OPTIONS"); + res.header("Access-Control-Allow-Methods", "GET, PATCH, PUT, POST, DELETE, OPTIONS"); res.header("Access-Control-Allow-Headers", "Authorization, Content-Type, X-Requested-With"); } else { throw new Error('Unrecognized origin'); diff --git a/app/server/lib/requestUtils.ts b/app/server/lib/requestUtils.ts index 4f9d4018..3353784a 100644 --- a/app/server/lib/requestUtils.ts +++ b/app/server/lib/requestUtils.ts @@ -218,8 +218,12 @@ export function optStringParam(p: any): string|undefined { } export function stringParam(p: any, name: string, allowed?: string[]): string { - if (typeof p !== 'string') { throw new Error(`${name} parameter should be a string: ${p}`); } - if (allowed && !allowed.includes(p)) { throw new Error(`${name} parameter ${p} should be one of ${allowed}`); } + if (typeof p !== 'string') { + throw new ApiError(`${name} parameter should be a string: ${p}`, 400); + } + if (allowed && !allowed.includes(p)) { + throw new ApiError(`${name} parameter ${p} should be one of ${allowed}`, 400); + } return p; } diff --git a/sandbox/grist/column.py b/sandbox/grist/column.py index 273bb52d..92d44c7d 100644 --- a/sandbox/grist/column.py +++ b/sandbox/grist/column.py @@ -473,23 +473,8 @@ class BaseReferenceColumn(BaseColumn): .get_column_rec(self.table_id, self.col_id).visibleCol.colId or "id" ) - column = self._target_table.get_column(col_id) - # `value` is an object encoded for transmission from JS to Python, - # which is decoded to `decoded_value`. - # `raw_value` is the kind of value that would be stored in `column`. - # `rich_value` is the type of value used in formulas, especially with `lookupRecords`. - # For example, for a Date column, `raw_value` is a numerical timestamp - # and `rich_value` is a `datetime.date` object, - # assuming `value` isn't of an invalid type. - # However `value` could either be just a number - # (in which case `decoded_value` would be a number as well) - # or an encoded date (or even datetime) object like ['d', number] - # (in which case `decoded_value` would be a `datetime.date` object, - # which would get converted back to a number and then back to a date object again!) - decoded_value = objtypes.decode_object(value) - raw_value = column.convert(decoded_value) - rich_value = column._convert_raw_value(raw_value) - return self._target_table.lookup_one_record(**{col_id: rich_value}) + value = objtypes.decode_object(value) + return self._target_table.lookup_one_record(**{col_id: value}) class ReferenceColumn(BaseReferenceColumn): diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index 47e4ae04..0e49a72a 100644 --- a/sandbox/grist/table.py +++ b/sandbox/grist/table.py @@ -443,6 +443,10 @@ class Table(object): # the marker is moved to col_id so that the LookupMapColumn knows how to # update its index correctly for that column. col_id = lookup._Contains(col_id) + else: + col = self.get_column(col_id) + # Convert `value` to the correct type of rich value for that column + value = col._convert_raw_value(col.convert(value)) key.append(value) col_ids.append(col_id) col_ids = tuple(col_ids) diff --git a/sandbox/grist/test_lookups.py b/sandbox/grist/test_lookups.py index 1b48332a..335215fe 100644 --- a/sandbox/grist/test_lookups.py +++ b/sandbox/grist/test_lookups.py @@ -773,3 +773,31 @@ return ",".join(str(r.id) for r in Students.lookupRecords(firstName=fn, lastName ["id", "lookup"], [1, [None, 0, 1, 2, 3, 'foo']], ]) + + def test_conversion(self): + # Test that values are converted to the type of the column when looking up + # i.e. '123' is converted to 123 + # and 'foo' is converted to AltText('foo') + self.load_sample(testutil.parse_test_sample({ + "SCHEMA": [ + [1, "Table1", [ + [1, "num", "Numeric", False, "", "", ""], + [2, "lookup1", "RefList:Table1", True, "Table1.lookupRecords(num='123')", "", ""], + [3, "lookup2", "RefList:Table1", True, "Table1.lookupRecords(num='foo')", "", ""], + ]] + ], + "DATA": { + "Table1": [ + ["id", "num"], + [1, 123], + [2, 'foo'], + ] + } + })) + + self.assertTableData( + "Table1", data=[ + ["id", "num", "lookup1", "lookup2"], + [1, 123, [1], [2]], + [2, 'foo', [1], [2]], + ]) diff --git a/sandbox/grist/test_useractions.py b/sandbox/grist/test_useractions.py index 3281aa34..deb92e59 100644 --- a/sandbox/grist/test_useractions.py +++ b/sandbox/grist/test_useractions.py @@ -905,6 +905,7 @@ class TestUserActions(test_engine.EngineTestCase): [3, "pet", "Text", False, "", "pet", ""], [4, "color", "Text", False, "", "color", ""], [5, "formula", "Text", True, "''", "formula", ""], + [6, "date", "Date", False, None, "date", ""], ]], ], "DATA": { @@ -917,9 +918,9 @@ class TestUserActions(test_engine.EngineTestCase): }) self.load_sample(sample) - def check(where, values, options, stored): + def check(require, values, options, stored): self.assertPartialOutActions( - self.apply_user_action(["AddOrUpdateRecord", "Table1", where, values, options]), + self.apply_user_action(["AddOrUpdateRecord", "Table1", require, values, options]), {"stored": stored}, ) @@ -958,6 +959,26 @@ class TestUserActions(test_engine.EngineTestCase): ], ) + # Update all records with empty require and allow_empty_require + check( + {}, + {"color": "greener"}, + {"on_many": "all", "allow_empty_require": True}, + [ + ["UpdateRecord", "Table1", 1, {"color": "greener"}], + ["UpdateRecord", "Table1", 2, {"color": "greener"}], + ], + ) + + # Missing allow_empty_require + with self.assertRaises(ValueError): + check( + {}, + {"color": "greenest"}, + {}, + [], + ) + # Don't update any records when there's several matches check( {"first_name": "John"}, @@ -992,7 +1013,7 @@ class TestUserActions(test_engine.EngineTestCase): ) # No matching record, make a new one. - # first_name=Jack in `values` overrides first_name=John in `where` + # first_name=Jack in `values` overrides first_name=John in `require` check( {"first_name": "John", "last_name": "Johnson"}, {"first_name": "Jack", "color": "yellow"}, @@ -1003,7 +1024,7 @@ class TestUserActions(test_engine.EngineTestCase): ], ) - # Specifying a row ID in `where` is allowed + # Specifying a row ID in `require` is allowed check( {"first_name": "Bob", "id": 100}, {"pet": "fish"}, @@ -1019,7 +1040,7 @@ class TestUserActions(test_engine.EngineTestCase): [], ) - # Nothing matches this `where`, but the row ID already exists + # Nothing matches this `require`, but the row ID already exists with self.assertRaises(AssertionError): check( {"first_name": "Alice", "id": 100}, @@ -1028,7 +1049,7 @@ class TestUserActions(test_engine.EngineTestCase): [], ) - # Formula columns in `where` can't be used as values when creating records + # Formula columns in `require` can't be used as values when creating records check( {"formula": "anything"}, {"first_name": "Alice"}, @@ -1036,6 +1057,29 @@ class TestUserActions(test_engine.EngineTestCase): [["AddRecord", "Table1", 101, {"first_name": "Alice"}]], ) + with self.assertRaises(ValueError): + # Row ID too high + check( + {"first_name": "Alice", "id": 2000000}, + {"pet": "fish"}, + {}, + [], + ) + + # Check that encoded objects are decoded correctly + check( + {"date": ['d', 950400]}, + {}, + {}, + [["AddRecord", "Table1", 102, {"date": 950400}]], + ) + check( + {"date": ['d', 950400]}, + {"date": ['d', 1900800]}, + {}, + [["UpdateRecord", "Table1", 102, {"date": 1900800}]], + ) + def test_reference_lookup(self): sample = testutil.parse_test_sample({ "SCHEMA": [ diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index f015e7a2..dcdad556 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -13,7 +13,7 @@ import actions import column import sort_specs import identifiers -from objtypes import strict_equal, encode_object +from objtypes import strict_equal, encode_object, decode_object import schema from schema import RecalcWhen import summary @@ -319,6 +319,8 @@ class UserActions(object): for i, row_id in enumerate(filled_row_ids): if row_id is None or row_id < 0: filled_row_ids[i] = row_id = next_row_id + elif row_id > 1000000: + raise ValueError("Row ID too high") next_row_id = max(next_row_id, row_id) + 1 # Whenever we add new rows, remember the mapping from any negative row_ids to their final @@ -793,9 +795,35 @@ class UserActions(object): raise ValueError("Can't save value to formula column %s" % col_id) @useraction - def AddOrUpdateRecord(self, table_id, where, col_values, options): + def AddOrUpdateRecord(self, table_id, require, col_values, options): + """ + Add or Update ('upsert') a single record depending on `options` + and on whether a record matching `require` already exists. + + `require` and `col_values` are dictionaries mapping column IDs to single cell values. + + By default, if `table.lookupRecords(**require)` returns any records, + update the first one with the values in `col_values`. + Otherwise create a new record with values `{**require, **col_values}`. + + `options` is a dictionary with optional settings to choose other behaviours: + - Set "on_many" to "all" or "none" to change which records are updated when several match. + - Set "update" or "add" to False to disable updating or adding records respectively, + i.e. if you only want to add records that don't already exist + or if you only want to update records that do already exist. + - Set "allow_empty_require" to True to allow `require` to be an empty dictionary, + which would mean that every record in the table is matched. + Otherwise this will raise an error to prevent mistakes like updating an entire column. + """ table = self._engine.tables[table_id] - records = list(table.lookup_records(**where)) + + if not require and not options.get("allow_empty_require", False): + raise ValueError("require is empty but allow_empty_require isn't set") + + # Decode `require` before looking up, but let AddRecord/UpdateRecord decode the final + # values when adding/updating + decoded_require = {k: decode_object(v) for k, v in six.iteritems(require)} + records = list(table.lookup_records(**decoded_require)) if records and options.get("update", True): if len(records) > 1: @@ -813,8 +841,12 @@ class UserActions(object): if not records and options.get("add", True): values = { key: value - for key, value in six.iteritems(where) - if not table.get_column(key).is_formula() + for key, value in six.iteritems(require) + if not ( + table.get_column(key).is_formula() and + # Check that there actually is a formula and this isn't just an empty column + self._engine.docmodel.get_column_rec(table_id, key).formula + ) } values.update(col_values) self.AddRecord(table_id, values.pop("id", None), values)