(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

@@ -357,21 +357,35 @@ function parseColValues<T extends ColValues | BulkColValues>(
 }
 
 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;
 }

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;
     }

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<void>;
 
 // 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<string>(_.flatMap(records, r => Object.keys({...r.fields, ...r.require})));
+    }
+
     function convertToBulkColValues(records: Array<Types.Record | Types.NewRecord>): 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<string>(_.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<T>(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);

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;

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
+}

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');

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;
 }
 

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):

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)

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]],
+      ])

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": [

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)