(core) AddOrUpdateRecord user action

Summary:
New user action as described in https://grist.quip.com/fZSrAnJKgO5j/Add-or-Update-Records-API, with options to allow most of the mentioned possible behaviours.

The Python code is due to Alex (as should be obvious from the u in behaviours).

Test Plan: Added a unit test.

Reviewers: alexmojaki

Reviewed By: alexmojaki

Differential Revision: https://phab.getgrist.com/D3239

app/server/lib/GranularAccess.ts

@@ -34,7 +34,8 @@ import get = require('lodash/get');
 
 // tslint:disable:no-bitwise
 
-// Actions that add/update/remove/replace rows.
+// Actions that add/update/remove/replace rows (DocActions only - UserActions
+// may also result in row changes but are not in this list).
 const ACTION_WITH_TABLE_ID = new Set(['AddRecord', 'BulkAddRecord', 'UpdateRecord', 'BulkUpdateRecord',
                                       'RemoveRecord', 'BulkRemoveRecord',
                                       'ReplaceTableData', 'TableData',
@@ -379,27 +380,25 @@ export class GranularAccess implements GranularAccessForBundle {
 
   /**
    * Check if user may be able to apply a list of actions.  Throws if
-   * user cannot apply an action.  Returns true if a user can apply an
-   * action, or false if we need to defer making that determination
+   * user cannot apply an action.  Returns true if a user can perhaps apply an
+   * action, or false if we know we need to defer making that determination
    * until the data engine translates the user actions to doc actions.
    */
   public async assertCanMaybeApplyUserActions(docSession: OptDocSession, actions: UserAction[]): Promise<boolean> {
-    let canCertainlyApply =  true;
+    if (this._hasExceptionalFullAccess(docSession)) { return true; }
+
+    let canMaybeApply = true;
     for (const action of actions) {
       if (!await this.assertCanMaybeApplyUserAction(docSession, action)) {
-        canCertainlyApply = false;
+        canMaybeApply = false;
         break;
       }
     }
-    // If changes could include Python formulas, then user must have
-    // +S before we even consider passing these to the data engine.
-    // Since we don't track rule or schema changes at this stage, we
-    // approximate with the user's access rights at beginning of
-    // bundle.
-    if (!canCertainlyApply && scanActionsRecursively(actions, (a) => this.needEarlySchemaPermission(a))) {
-      await this._assertSchemaAccess(docSession);
-    }
-    return canCertainlyApply;
+
+    await this._checkPossiblePythonFormulaModification(docSession, actions);
+    await this._checkAddOrUpdateAccess(docSession, actions);
+
+    return canMaybeApply;
   }
 
   /**
@@ -493,6 +492,7 @@ export class GranularAccess implements GranularAccessForBundle {
    */
   public async assertCanMaybeApplyUserAction(docSession: OptDocSession, a: UserAction|DocAction): Promise<boolean> {
     const name = a[0] as string;
+    if (this._hasExceptionalFullAccess(docSession)) { return true; }
     if (OK_ACTIONS.has(name)) { return true; }
     if (SPECIAL_ACTIONS.has(name)) {
       if (await this.hasNuancedAccess(docSession)) {
@@ -510,6 +510,10 @@ export class GranularAccess implements GranularAccessForBundle {
       return this.assertCanMaybeApplyUserActions(docSession, a[1] as UserAction[]);
     } else if (name === 'ApplyDocActions') {
       return this.assertCanMaybeApplyUserActions(docSession, a[1] as UserAction[]);
+    } else if (name === 'AddOrUpdateRecord') {
+      // This case is a bit tricky.
+      // Access is checked separately in _checkAddOrUpdateAccess.
+      return true;
     } else if (isDataAction(a)) {
       const tableId = getTableId(a);
       if (tableId.startsWith('_grist_')) {
@@ -763,6 +767,61 @@ export class GranularAccess implements GranularAccessForBundle {
     return result;
   }
 
+  // AddOrUpdateRecord requires broad read access to a table.
+  // But tables can be renamed, and access can be granted and removed
+  // within a bundle.
+  //
+  // For now, we forbid the combination of AddOrUpdateRecord and
+  // with actions other than other AddOrUpdateRecords, or simple data
+  // changes.
+  //
+  // Access rules and user attributes might change during the bundle.
+  // We deny based on access rights at the beginning of the bundle,
+  // as for _checkPossiblePythonFormulaModification. This is on the
+  // theory that someone who can change access rights can do anything.
+  //
+  // There might be uses for applying AddOrUpdateRecord in a nuanced
+  // way within the scope of what a user can read, but there's no easy
+  // way to do that within the data engine as currently
+  // formulated. Could perhaps be done for on-demand tables though.
+  private async _checkAddOrUpdateAccess(docSession: OptDocSession, actions: UserAction[]) {
+    if (!scanActionsRecursively(actions, (a) => a[0] === 'AddOrUpdateRecord')) {
+      // Don't need to apply this particular check.
+      return;
+    }
+    // Fail if being combined with anything fancy.
+    if (scanActionsRecursively(actions, (a) => {
+      const name = a[0];
+      return !['ApplyUndoActions', 'ApplyDocActions', 'AddOrUpdateRecord'].includes(String(name)) &&
+        !(isDataAction(a) && !getTableId(a).startsWith('_grist_'));
+    })) {
+      throw new Error('Can only combine AddOrUpdate with simple data changes');
+    }
+    // Check for read access, and that we're not touching metadata.
+    await applyToActionsRecursively(actions, async (a) => {
+      if (a[0] !== 'AddOrUpdateRecord') { return; }
+      const tableId = validTableIdString(a[1]);
+      if (tableId.startsWith('_grist_')) {
+        throw new Error(`AddOrUpdate cannot yet be used on metadata tables`);
+      }
+      const tableAccess = await this.getTableAccess(docSession, tableId);
+      accessChecks.fatal.read.throwIfNotFullyAllowed(tableAccess);
+      accessChecks.fatal.update.throwIfDenied(tableAccess);
+      accessChecks.fatal.create.throwIfDenied(tableAccess);
+    });
+  }
+
+  private async _checkPossiblePythonFormulaModification(docSession: OptDocSession, actions: UserAction[]) {
+    // If changes could include Python formulas, then user must have
+    // +S before we even consider passing these to the data engine.
+    // Since we don't track rule or schema changes at this stage, we
+    // approximate with the user's access rights at beginning of
+    // bundle.
+    if (scanActionsRecursively(actions, (a) => this.needEarlySchemaPermission(a))) {
+      await this._assertSchemaAccess(docSession);
+    }
+  }
+
   /**
    * Get the role the session user has for this document.  User may be overridden,
    * in which case the role of the override is returned.
@@ -1952,6 +2011,7 @@ class UserAttributes {
 interface IAccessCheck {
   get(ps: PermissionSetWithContext): string;
   throwIfDenied(ps: PermissionSetWithContext): void;
+  throwIfNotFullyAllowed(ps: PermissionSetWithContext): void;
 }
 
 class AccessCheck implements IAccessCheck {
@@ -1969,6 +2029,16 @@ class AccessCheck implements IAccessCheck {
   public throwIfDenied(ps: PermissionSetWithContext): void {
     const result = ps.perms[this.access];
     if (result !== 'deny') { return; }
+    this._throwError(ps);
+  }
+
+  public throwIfNotFullyAllowed(ps: PermissionSetWithContext): void {
+    const result = ps.perms[this.access];
+    if (result === 'allow') { return; }
+    this._throwError(ps);
+  }
+
+  private _throwError(ps: PermissionSetWithContext): void {
     const memos = ps.getMemos()[this.access];
     const label =
       this.access === 'schemaEdit' ? 'structure' :
@@ -1989,7 +2059,8 @@ export const accessChecks = {
 // This AccessCheck allows everything.
 const dummyAccessCheck: IAccessCheck = {
   get() { return 'allow'; },
-  throwIfDenied() {}
+  throwIfDenied() {},
+  throwIfNotFullyAllowed() {}
 };
 
 
@@ -2130,9 +2201,7 @@ function getCensorMethod(tableId: string): (rec: RecordEditor) => void {
 function scanActionsRecursively(actions: (DocAction|UserAction)[],
                                 check: (action: DocAction|UserAction) => boolean): boolean {
   for (const a of actions) {
-    if (a[0] === 'ApplyUndoActions') {
-      return scanActionsRecursively(a[1] as UserAction[], check);
-    } else if (a[0] === 'ApplyDocActions') {
+    if (a[0] === 'ApplyUndoActions' || a[0] === 'ApplyDocActions') {
       return scanActionsRecursively(a[1] as UserAction[], check);
     }
     if (check(a)) { return true; }
@@ -2140,6 +2209,16 @@ function scanActionsRecursively(actions: (DocAction|UserAction)[],
   return false;
 }
 
+async function applyToActionsRecursively(actions: (DocAction|UserAction)[],
+                                         op: (action: DocAction|UserAction) => Promise<void>): Promise<void> {
+  for (const a of actions) {
+    if (a[0] === 'ApplyUndoActions' || a[0] === 'ApplyDocActions') {
+      await applyToActionsRecursively(a[1] as UserAction[], op);
+    }
+    await op(a);
+  }
+}
+
 /**
  * Takes an action, and removes certain cells from it.  The action
  * passed in is modified in place, and also returned as part of a list
@@ -2266,3 +2345,8 @@ export class User implements UserInfo {
     return results;
   }
 }
+
+export function validTableIdString(tableId: any): string {
+  if (typeof tableId !== 'string') { throw new Error(`Expected tableId to be a string`); }
+  return tableId;
+}

sandbox/grist/acl_formula.py

@@ -51,7 +51,7 @@ def parse_acl_formula_json(acl_formula):
 
 
 # Entities encountered in ACL formulas, which may get renamed.
-#   type: 'recCol'|'userAttr'|'userAttrCol',
+#   type : 'recCol'|'userAttr'|'userAttrCol',
 #   start_pos: number,        # start position of the token in the code.
 #   name: string,             # the name that may be updated by a rename.
 #   extra: string|None,       # name of userAttr in case of userAttrCol; otherwise None.

sandbox/grist/actions.py

@@ -49,7 +49,7 @@ ReplaceTableData = namedtuple_eq('ReplaceTableData', BulkAddRecord._fields)
 #     table_id: string name of the table.
 #     col_id:   string name of column
 #     col_info: dictionary with particular keys
-#         type:       string type of the column
+#         type :      string type of the column
 #         isFormula:  bool, whether it is a formula column
 #         formula:    string text of the formula, or empty string
 #     Other keys may be set in col_info (e.g. widgetOptions, label) but are not currently used in

sandbox/grist/test_useractions.py

@@ -896,6 +896,145 @@ class TestUserActions(test_engine.EngineTestCase):
       [2, 2, json.dumps({"included": ["b", "c"]}), None, 1]
     ])
 
+  def test_add_or_update(self):
+    sample = testutil.parse_test_sample({
+      "SCHEMA": [
+        [1, "Table1", [
+          [1, "first_name", "Text", False, "",     "first_name", ""],
+          [2, "last_name",  "Text", False, "",     "last_name",  ""],
+          [3, "pet",        "Text", False, "",     "pet",        ""],
+          [4, "color",      "Text", False, "",     "color",      ""],
+          [5, "formula",    "Text", True,  "''",   "formula",    ""],
+        ]],
+      ],
+      "DATA": {
+        "Table1": [
+          ["id", "first_name", "last_name"],
+          [1, "John", "Doe"],
+          [2, "John", "Smith"],
+        ],
+      }
+    })
+    self.load_sample(sample)
+
+    def check(where, values, options, stored):
+      self.assertPartialOutActions(
+        self.apply_user_action(["AddOrUpdateRecord", "Table1", where, values, options]),
+        {"stored": stored},
+      )
+
+    # Exactly one match, so on_many=none has no effect
+    check(
+      {"first_name": "John", "last_name": "Smith"},
+      {"pet": "dog", "color": "red"},
+      {"on_many": "none"},
+      [["UpdateRecord", "Table1", 2, {"color": "red", "pet": "dog"}]],
+    )
+
+    # Look for a record with pet=dog and change it to pet=cat
+    check(
+      {"first_name": "John", "pet": "dog"},
+      {"pet": "cat"},
+      {},
+      [["UpdateRecord", "Table1", 2, {"pet": "cat"}]],
+    )
+
+    # Two records match first_name=John, by default we only update the first
+    check(
+      {"first_name": "John"},
+      {"color": "blue"},
+      {},
+      [["UpdateRecord", "Table1", 1, {"color": "blue"}]],
+    )
+
+    # Update all matching records
+    check(
+      {"first_name": "John"},
+      {"color": "green"},
+      {"on_many": "all"},
+      [
+        ["UpdateRecord", "Table1", 1, {"color": "green"}],
+        ["UpdateRecord", "Table1", 2, {"color": "green"}],
+      ],
+    )
+
+    # Don't update any records when there's several matches
+    check(
+      {"first_name": "John"},
+      {"color": "yellow"},
+      {"on_many": "none"},
+      [],
+    )
+
+    # Invalid value of on_many
+    with self.assertRaises(ValueError):
+      check(
+        {"first_name": "John"},
+        {"color": "yellow"},
+        {"on_many": "other"},
+        [],
+      )
+
+    # Since there's at least one matching record and update=False, do nothing
+    check(
+      {"first_name": "John"},
+      {"color": "yellow"},
+      {"update": False},
+      [],
+    )
+
+    # Since there's no matching records and add=False, do nothing
+    check(
+      {"first_name": "John", "last_name": "Johnson"},
+      {"first_name": "Jack", "color": "yellow"},
+      {"add": False},
+      [],
+    )
+
+    # No matching record, make a new one.
+    # first_name=Jack in `values` overrides first_name=John in `where`
+    check(
+      {"first_name": "John", "last_name": "Johnson"},
+      {"first_name": "Jack", "color": "yellow"},
+      {},
+      [
+        ["AddRecord", "Table1", 3,
+        {"color": "yellow", "first_name": "Jack", "last_name": "Johnson"}]
+      ],
+    )
+
+    # Specifying a row ID in `where` is allowed
+    check(
+      {"first_name": "Bob", "id": 100},
+      {"pet": "fish"},
+      {},
+      [["AddRecord", "Table1", 100, {"first_name": "Bob", "pet": "fish"}]],
+    )
+
+    # Now the row already exists
+    check(
+      {"first_name": "Bob", "id": 100},
+      {"pet": "fish"},
+      {},
+      [],
+    )
+
+    # Nothing matches this `where`, but the row ID already exists
+    with self.assertRaises(AssertionError):
+      check(
+        {"first_name": "Alice", "id": 100},
+        {"pet": "fish"},
+        {},
+        [],
+      )
+
+    # Formula columns in `where` can't be used as values when creating records
+    check(
+      {"formula": "anything"},
+      {"first_name": "Alice"},
+      {},
+      [["AddRecord", "Table1", 101, {"first_name": "Alice"}]],
+    )
 
   def test_reference_lookup(self):
     sample = testutil.parse_test_sample({

sandbox/grist/useractions.py

@@ -792,6 +792,33 @@ class UserActions(object):
       # Otherwise, this is an error. We can't save individual values to formula columns.
       raise ValueError("Can't save value to formula column %s" % col_id)
 
+  @useraction
+  def AddOrUpdateRecord(self, table_id, where, col_values, options):
+    table = self._engine.tables[table_id]
+    records = list(table.lookup_records(**where))
+
+    if records and options.get("update", True):
+      if len(records) > 1:
+        on_many = options.get("on_many", "first")
+        if on_many == "first":
+          records = records[:1]
+        elif on_many == "none":
+          return
+        elif on_many != "all":
+          raise ValueError("on_many should be 'first', 'none', or 'all', not %r" % on_many)
+
+      for record in records:
+        self.UpdateRecord(table_id, record.id, col_values)
+
+    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()
+      }
+      values.update(col_values)
+      self.AddRecord(table_id, values.pop("id", None), values)
+
   #----------------------------------------
   # RemoveRecords & co.
   #----------------------------------------