(core) Adds endpoint to update webhook

Summary:
Adds a new endpoint to update webhook.

Perform some refactoring to allow code reuse from endpoint allowing to _subscribe and _unsubscribe webhooks.

One aspect of webhook is that url are stored in the home db while the rest of the fields (tableRef, isReadyColRef, ...) are stored in sqlite. So care must be taken when updating fields, to properly rollback if anything should fail.

Follow up diff will bring UI to edit webhook list

Test Plan: Updated doc api server tests

Reviewers: jarek

Reviewed By: jarek

Subscribers: paulfitz

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

app/common/Triggers-ti.ts

@@ -0,0 +1,41 @@
+/**
+ * This module was automatically generated by `ts-interface-builder`
+ */
+import * as t from "ts-interface-checker";
+// tslint:disable:object-literal-key-quotes
+
+export const WebhookFields = t.iface([], {
+  "url": "string",
+  "eventTypes": t.array(t.union(t.lit("add"), t.lit("update"))),
+  "tableId": "string",
+  "enabled": t.opt("boolean"),
+  "isReadyColumn": t.opt(t.union("string", "null")),
+});
+
+export const WebhookSubscribe = t.iface([], {
+  "url": "string",
+  "eventTypes": t.array(t.union(t.lit("add"), t.lit("update"))),
+  "enabled": t.opt("boolean"),
+  "isReadyColumn": t.opt(t.union("string", "null")),
+});
+
+export const WebhookUpdate = t.iface([], {
+  "id": "string",
+  "fields": "WebhookPatch",
+});
+
+export const WebhookPatch = t.iface([], {
+  "url": t.opt("string"),
+  "eventTypes": t.opt(t.array(t.union(t.lit("add"), t.lit("update")))),
+  "tableId": t.opt("string"),
+  "enabled": t.opt("boolean"),
+  "isReadyColumn": t.opt(t.union("string", "null")),
+});
+
+const exportedTypeSuite: t.ITypeSuite = {
+  WebhookFields,
+  WebhookSubscribe,
+  WebhookUpdate,
+  WebhookPatch,
+};
+export default exportedTypeSuite;

app/common/Triggers.ts

@@ -0,0 +1,32 @@
+export interface WebhookFields {
+  url: string;
+  eventTypes: Array<"add"|"update">;
+  tableId: string;
+  enabled?: boolean;
+  isReadyColumn?: string|null;
+}
+
+// WebhookSubscribe should be `Omit<WebhookFields, 'tableId'>` (because subscribe endpoint read
+// tableId from the url) but generics are not yet supported by ts-interface-builder
+export interface WebhookSubscribe {
+  url: string;
+  eventTypes: Array<"add"|"update">;
+  enabled?: boolean;
+  isReadyColumn?: string|null;
+}
+
+// Describes fields to update a webhook
+export interface WebhookUpdate {
+  id: string;
+  fields: WebhookPatch;
+}
+
+// WebhookPatch should be `Partial<WebhookFields>` but generics are not yet supported by
+// ts-interface-builder
+export interface WebhookPatch {
+  url?: string;
+  eventTypes?: Array<"add"|"update">;
+  tableId?: string;
+  enabled?: boolean;
+  isReadyColumn?: string|null;
+}

app/common/UserAPI.ts

@@ -14,6 +14,7 @@ import {OrgPrefs, UserOrgPrefs, UserPrefs} from 'app/common/Prefs';
 import * as roles from 'app/common/roles';
 import {addCurrentOrgToPath} from 'app/common/urlUtils';
 import {encodeQueryParams} from 'app/common/gutil';
+import {WebhookUpdate} from 'app/common/Triggers';
 
 export type {FullUser, UserProfile};
 
@@ -451,6 +452,9 @@ export interface DocAPI {
 
   // Get users that are worth proposing to "View As" for access control purposes.
   getUsersForViewAs(): Promise<PermissionDataWithExtraUsers>;
+
+  // Update webhook
+  updateWebhook(webhook: WebhookUpdate): Promise<void>;
 }
 
 // Operations that are supported by a doc worker.
@@ -900,6 +904,13 @@ export class DocAPIImpl extends BaseAPI implements DocAPI {
     return this.requestJson(`${this._url}/usersForViewAs`);
   }
 
+  public async updateWebhook(webhook: WebhookUpdate): Promise<void> {
+    return this.requestJson(`${this._url}/webhooks/${webhook.id}`, {
+      method: 'PATCH',
+      body: JSON.stringify(webhook.fields),
+    });
+  }
+
   public async forceReload(): Promise<void> {
     await this.request(`${this._url}/force-reload`, {
       method: 'POST'

app/gen-server/lib/HomeDBManager.ts

@@ -1851,6 +1851,18 @@ export class HomeDBManager extends EventEmitter {
     });
   }
 
+  // Updates the secret matching id and docId, to the new value.
+  public async updateSecret(id: string, docId: string, value: string, manager?: EntityManager): Promise<void> {
+    const res = await (manager || this._connection).createQueryBuilder()
+      .update(Secret)
+      .set({value})
+      .where("id = :id AND doc_id = :docId", {id, docId})
+      .execute();
+    if (res.affected !== 1) {
+      throw new ApiError('secret with given id not found', 404);
+    }
+  }
+
   public async getSecret(id: string, docId: string, manager?: EntityManager): Promise<string | undefined> {
     const secret = await (manager || this._connection).createQueryBuilder()
       .select('secrets')
@@ -1860,6 +1872,20 @@ export class HomeDBManager extends EventEmitter {
     return secret?.value;
   }
 
+  // Update the webhook url in the webhook's corresponding secret (note: the webhook identifier is
+  // its secret identifier).
+  public async updateWebhookUrl(id: string, docId: string, url: string, outerManager?: EntityManager) {
+    return await this._runInTransaction(outerManager, async manager => {
+      const value = await this.getSecret(id, docId, manager);
+      if (!value) {
+        throw new ApiError('Webhook with given id not found', 404);
+      }
+      const webhookSecret = JSON.parse(value);
+      webhookSecret.url = url;
+      await this.updateSecret(id, docId, JSON.stringify(webhookSecret), manager);
+    });
+  }
+
   public async removeWebhook(id: string, docId: string, unsubscribeKey: string, checkKey: boolean): Promise<void> {
     if (!id) {
       throw new ApiError('Bad request: id required', 400);
@@ -1881,7 +1907,7 @@ export class HomeDBManager extends EventEmitter {
       await manager.createQueryBuilder()
         .delete()
         .from(Secret)
-        .where('id = :id', {id})
+        .where('id = :id AND doc_id = :docId', {id, docId})
         .execute();
     });
   }

app/server/lib/ActiveDoc.ts

@@ -347,6 +347,7 @@ export class ActiveDoc extends EventEmitter {
 
   public get isShuttingDown(): boolean { return this._shuttingDown; }
 
+  public get triggers(): DocTriggers { return this._triggers; }
 
   public get rowLimitRatio(): number {
     return getUsageRatio(
@@ -1119,7 +1120,7 @@ export class ActiveDoc extends EventEmitter {
    * @param options: As for applyUserActions.
    * @returns Promise of retValues, see applyUserActions.
    */
-  public async applyUserActionsById(docSession: DocSession,
+  public async applyUserActionsById(docSession: OptDocSession,
                                     actionNums: number[],
                                     actionHashes: string[],
                                     undo: boolean,
@@ -2445,6 +2446,21 @@ export function tableIdToRef(metaTables: { [p: string]: TableDataAction }, table
   return tableRefs[tableRowIndex];
 }
 
+// Helper that converts a Grist column colId to a ref given the corresponding table.
+export function colIdToRef(metaTables: {[p: string]: TableDataAction}, tableId: string, colId: string) {
+
+  const tableRef = tableIdToRef(metaTables, tableId);
+
+  const [, , colRefs, columnData] = metaTables._grist_Tables_column;
+  const colRowIndex = columnData.colId.findIndex((_, i) => (
+    columnData.colId[i] === colId && columnData.parentId[i] === tableRef
+  ));
+  if (colRowIndex === -1) {
+    throw new ApiError(`Column not found "${colId}"`, 404);
+  }
+  return colRefs[colRowIndex];
+}
+
 export function sanitizeApplyUAOptions(options?: ApplyUAOptions): ApplyUAOptions {
   return pick(options||{}, ['desc', 'otherId', 'linkId', 'parseStrings']);
 }

app/server/lib/DocApi.ts

@@ -1,17 +1,26 @@
 import {createEmptyActionSummary} from "app/common/ActionSummary";
 import {ApiError} from 'app/common/ApiError';
 import {BrowserSettings} from "app/common/BrowserSettings";
-import {BulkColValues, ColValues, fromTableDataAction, TableColValues, TableRecordValue} from 'app/common/DocActions';
+import {
+  BulkColValues,
+  ColValues,
+  fromTableDataAction,
+  TableColValues,
+  TableRecordValue
+} from 'app/common/DocActions';
 import {isRaisedException} from "app/common/gristTypes";
 import {buildUrlId, parseUrlId} from "app/common/gristUrls";
 import {isAffirmative} from "app/common/gutil";
+import {SchemaTypes} from "app/common/schema";
 import {SortFunc} from 'app/common/SortFunc';
 import {Sort} from 'app/common/SortSpec';
 import {MetaRowRecord} from 'app/common/TableData';
 import {DocReplacementOptions, DocState, DocStateComparison, DocStates, NEW_DOCUMENT_CODE} from 'app/common/UserAPI';
+import TriggersTI from 'app/common/Triggers-ti';
 import {HomeDBManager, makeDocAuthResult} from 'app/gen-server/lib/HomeDBManager';
 import * as Types from "app/plugin/DocApiTypes";
 import DocApiTypesTI from "app/plugin/DocApiTypes-ti";
+import {GristObjCode} from "app/plugin/GristData";
 import GristDataTI from 'app/plugin/GristData-ti';
 import {OpOptions} from "app/plugin/TableOperations";
 import {
@@ -20,7 +29,7 @@ import {
   TableOperationsPlatform
 } from 'app/plugin/TableOperationsImpl';
 import {concatenateSummaries, summarizeAction} from "app/server/lib/ActionSummary";
-import {ActiveDoc, tableIdToRef} from "app/server/lib/ActiveDoc";
+import {ActiveDoc, colIdToRef as colIdToReference, tableIdToRef} from "app/server/lib/ActiveDoc";
 import {
   assertAccess,
   getOrSetDocAuth,
@@ -44,6 +53,7 @@ import {exportToDrive} from "app/server/lib/GoogleExport";
 import {GristServer} from 'app/server/lib/GristServer';
 import {HashUtil} from 'app/server/lib/HashUtil';
 import {makeForkIds} from "app/server/lib/idUtils";
+import log from 'app/server/lib/log';
 import {
   getDocId,
   getDocScope,
@@ -58,7 +68,7 @@ import {
 } from 'app/server/lib/requestUtils';
 import {ServerColumnGetters} from 'app/server/lib/ServerColumnGetters';
 import {localeFromRequest} from "app/server/lib/ServerLocale";
-import {allowedEventTypes, isUrlAllowed, WebhookAction, WebHookSecret} from "app/server/lib/Triggers";
+import {isUrlAllowed, WebhookAction, WebHookSecret} from "app/server/lib/Triggers";
 import {handleOptionalUpload, handleUpload} from "app/server/lib/uploads";
 import * as assert from 'assert';
 import contentDisposition from 'content-disposition';
@@ -96,6 +106,12 @@ for (const checker of [RecordsPatch, RecordsPost, RecordsPut, ColumnsPost, Colum
   checker.setReportedPath("body");
 }
 
+// Schema validators for api endpoints that creates or updates records.
+const {
+  WebhookPatch,
+  WebhookSubscribe
+} = t.createCheckers(TriggersTI);
+
 /**
  * Middleware for validating request's body with a Checker instance.
  */
@@ -104,6 +120,7 @@ function validate(checker: Checker): RequestHandler {
     try {
       checker.check(req.body);
     } catch(err) {
+      log.warn(`Error during api call to ${req.path}: Invalid payload: ${String(err)}`);
       res.status(400).json({
         error : "Invalid payload",
         details: String(err)
@@ -531,34 +548,24 @@ export class DocWorkerApi {
     );
 
     // Add a new webhook and trigger
-    this._app.post('/api/docs/:docId/tables/:tableId/_subscribe', isOwner,
+    this._app.post('/api/docs/:docId/tables/:tableId/_subscribe', isOwner, validate(WebhookSubscribe),
       withDoc(async (activeDoc, req, res) => {
         const {isReadyColumn, eventTypes, url} = req.body;
 
-        if (!(Array.isArray(eventTypes) && eventTypes.length)) {
+        if (!eventTypes.length) {
           throw new ApiError(`eventTypes must be a non-empty array`, 400);
         }
-        if (!eventTypes.every(allowedEventTypes.guard)) {
-          throw new ApiError(`Allowed values in eventTypes are: ${allowedEventTypes.values}`, 400);
-        }
-        if (!url) {
-          throw new ApiError('Bad request: url required', 400);
-        }
         if (!isUrlAllowed(url)) {
           throw new ApiError('Provided url is forbidden', 403);
         }
 
+        const tableId = req.params.tableId;
         const metaTables = await getMetaTables(activeDoc, req);
-        const tableRef = tableIdToRef(metaTables, req.params.tableId);
 
+        const tableRef = tableIdToRef(metaTables, tableId);
         let isReadyColRef = 0;
         if (isReadyColumn) {
-          const [, , colRefs, columnData] = metaTables._grist_Tables_column;
-          const colRowIndex = columnData.colId.indexOf(isReadyColumn);
-          if (colRowIndex === -1) {
-            throw new ApiError(`Column not found "${isReadyColumn}"`, 404);
-          }
-          isReadyColRef = colRefs[colRowIndex];
+          isReadyColRef = colIdToReference(metaTables, tableId, isReadyColumn);
         }
 
         const unsubscribeKey = uuidv4();
@@ -566,22 +573,30 @@ export class DocWorkerApi {
         const secretValue = JSON.stringify(webhook);
         const webhookId = (await this._dbManager.addSecret(secretValue, activeDoc.docName)).id;
 
-        const webhookAction: WebhookAction = {type: "webhook", id: webhookId};
+        try {
 
-        const sandboxRes = await handleSandboxError("_grist_Triggers", [], activeDoc.applyUserActions(
-          docSessionFromRequest(req),
-          [['AddRecord', "_grist_Triggers", null, {
-            tableRef,
-            isReadyColRef,
-            eventTypes: ["L", ...eventTypes],
-            actions: JSON.stringify([webhookAction])
-          }]]));
-
-        res.json({
-          unsubscribeKey,
-          triggerId: sandboxRes.retValues[0],
-          webhookId,
-        });
+          const webhookAction: WebhookAction = {type: "webhook", id: webhookId};
+          const sandboxRes = await handleSandboxError("_grist_Triggers", [], activeDoc.applyUserActions(
+            docSessionFromRequest(req),
+            [['AddRecord', "_grist_Triggers", null, {
+              eventTypes: [GristObjCode.List, ...eventTypes],
+              isReadyColRef,
+              tableRef,
+              actions: JSON.stringify([webhookAction])
+            }]]));
+
+          res.json({
+            unsubscribeKey,
+            triggerId: sandboxRes.retValues[0],
+            webhookId,
+          });
+
+        } catch (err) {
+
+          // remove webhook
+          await this._dbManager.removeWebhook(webhookId, activeDoc.docName, '', false);
+          throw err;
+        }
       })
     );
 
@@ -595,22 +610,12 @@ export class DocWorkerApi {
         // Validate combination of triggerId, webhookId, and tableRef.
         // This is overly strict, webhookId should be enough,
         // but it should be easy to relax that later if we want.
-        const [, , triggerRowIds, triggerColData] = metaTables._grist_Triggers;
-        const triggerRowIndex = triggerColData.actions.findIndex(a => {
-          const actions: any[] = JSON.parse((a || '[]') as string);
-          return actions.some(action => action.id === webhookId && action?.type === "webhook");
-        });
-        if (triggerRowIndex === -1) {
-          throw new ApiError(`Webhook not found "${webhookId || ''}"`, 404);
-        }
-        if (triggerColData.tableRef[triggerRowIndex] !== tableRef) {
-          throw new ApiError(`Wrong table`, 400);
-        }
-        const triggerRowId = triggerRowIds[triggerRowIndex];
+        const triggerRowId = activeDoc.triggers.getWebhookTriggerRecord(webhookId, tableRef).id;
 
         const checkKey = !(await this._isOwner(req));
         // Validate unsubscribeKey before deleting trigger from document
         await this._dbManager.removeWebhook(webhookId, activeDoc.docName, unsubscribeKey, checkKey);
+        activeDoc.triggers.webhookDeleted(webhookId);
 
         // TODO handle trigger containing other actions when that becomes possible
         await handleSandboxError("_grist_Triggers", [], activeDoc.applyUserActions(
@@ -621,6 +626,78 @@ export class DocWorkerApi {
       })
     );
 
+    // Update a webhoook
+    this._app.patch(
+      '/api/docs/:docId/webhooks/:webhookId', isOwner, validate(WebhookPatch), withDoc(async (activeDoc, req, res) => {
+
+        const docId = activeDoc.docName;
+        const webhookId = req.params.webhookId;
+        const metaTables = await getMetaTables(activeDoc, req);
+        const tablesTable = activeDoc.docData!.getMetaTable("_grist_Tables");
+        const trigger = activeDoc.triggers.getWebhookTriggerRecord(webhookId);
+        let currentTableId = tablesTable.getValue(trigger.tableRef, 'tableId')!;
+        const {url, eventTypes, isReadyColumn, tableId} = req.body;
+        const fields: Partial<SchemaTypes['_grist_Triggers']> = {};
+
+        if (url && !isUrlAllowed(url)) {
+          // TODO: remove redundancy with same validation in _subscribe endpoint
+          throw new ApiError('Provided url is forbidden', 403);
+        }
+
+        if (eventTypes) {
+          // TODO: remove redundancy with same validation in _subscribe endpoint
+          if (!eventTypes.length) {
+            throw new ApiError(`eventTypes must be a non-empty array`, 400);
+          }
+          fields.eventTypes = [GristObjCode.List, ...eventTypes];
+        }
+
+        if (tableId !== undefined) {
+          fields.tableRef = tableIdToRef(metaTables, tableId);
+          currentTableId = tableId;
+        }
+
+        if (isReadyColumn !== undefined) {
+          // When isReadyColumn is defined let's explicitly changes the ready column to the new col
+          // id, null being a special case that unsets it.
+          if (isReadyColumn !== null) {
+            fields.isReadyColRef = colIdToReference(metaTables, currentTableId, isReadyColumn);
+          } else {
+            fields.isReadyColRef = 0;
+          }
+        } else if (tableId) {
+          // When isReadyColumn is undefined but tableId was changed, let's implicitely unset the ready column
+          fields.isReadyColRef = 0;
+        }
+
+        // assign other fields properties
+        Object.assign(fields, _.pick(req.body, ['enabled']));
+
+        const triggerRowId = activeDoc.triggers.getWebhookTriggerRecord(webhookId).id;
+
+        await this._dbManager.connection.transaction(async manager => {
+
+          // update url
+          if (url) {
+            await this._dbManager.updateWebhookUrl(webhookId, docId, url, manager);
+            activeDoc.triggers.webhookDeleted(webhookId); // clear cache
+          }
+
+          // then update sqlite.
+          if (Object.keys(fields).length) {
+            // In order to make sure to push a valid modification, let's update all fields since
+            // some may have changed since lookup.
+            _.defaults(fields, _.omit(trigger, 'id'));
+            await handleSandboxError("_grist_Triggers", [], activeDoc.applyUserActions(
+              docSessionFromRequest(req),
+              [['UpdateRecord', "_grist_Triggers", triggerRowId, fields]]));
+          }
+        });
+
+        res.json({success: true});
+      })
+    );
+
     // Clears all outgoing webhooks in the queue for this document.
     this._app.delete('/api/docs/:docId/webhooks/queue', isOwner,
       withDoc(async (activeDoc, req, res) => {

app/server/lib/Triggers.ts

@@ -1,5 +1,6 @@
 import {LocalActionBundle} from 'app/common/ActionBundle';
 import {ActionSummary, TableDelta} from 'app/common/ActionSummary';
+import {ApiError} from 'app/common/ApiError';
 import {MapWithTTL} from 'app/common/AsyncCreate';
 import {fromTableDataAction, RowRecord, TableColValues, TableDataAction} from 'app/common/DocActions';
 import {StringUnion} from 'app/common/StringUnion';
@@ -36,7 +37,7 @@ type RecordDeltas = Map<number, RecordDelta>;
 type TriggerAction = WebhookAction | PythonAction;
 
 type WebhookBatchStatus = 'success'|'failure'|'rejected';
-type WebhookStatus = 'idle'|'sending'|'retrying'|'postponed'|'error';
+type WebhookStatus = 'idle'|'sending'|'retrying'|'postponed'|'error'|'invalid';
 
 export interface WebhookSummary {
   id: string;
@@ -80,6 +81,7 @@ export interface WebhookAction {
 
 // Just hypothetical
 interface PythonAction {
+  id: string;
   type: "python";
   code: string;
 }
@@ -130,8 +132,7 @@ const TRIGGER_MAX_ATTEMPTS =
 // Triggers are configured in the document, while details of webhooks (URLs) are kept
 // in the Secrets table of the Home DB.
 export class DocTriggers {
-  // Converts a column ref to colId by looking it up in _grist_Tables_column
-  private _getColId: (rowId: number) => string|undefined;
+
 
   // Events that need to be sent to webhooks in FIFO order.
   // This is the primary place where events are stored and consumed,
@@ -199,7 +200,6 @@ export class DocTriggers {
 
     const triggersTable = docData.getMetaTable("_grist_Triggers");
     const getTableId = docData.getMetaTable("_grist_Tables").getRowPropFunc("tableId");
-    this._getColId = docData.getMetaTable("_grist_Tables_column").getRowPropFunc("colId");
 
     const triggersByTableRef = _.groupBy(triggersTable.getRecords(), "tableRef");
     const triggersByTableId: Array<[string, Trigger[]]> = [];
@@ -326,6 +326,22 @@ export class DocTriggers {
     return result;
   }
 
+  public getWebhookTriggerRecord(webhookId: string, tableRef?: number) {
+    const docData = this._activeDoc.docData!;
+    const triggersTable = docData.getMetaTable("_grist_Triggers");
+    const trigger = triggersTable.getRecords().find(t => {
+      const actions: TriggerAction[] = JSON.parse((t.actions || '[]') as string);
+      return actions.some(action => action.id === webhookId && action?.type === "webhook");
+    });
+    if (!trigger) {
+      throw new ApiError(`Webhook not found "${webhookId || ''}"`, 404);
+    }
+    if (tableRef && trigger.tableRef !== tableRef) {
+      throw new ApiError(`Wrong table`, 400);
+    }
+    return trigger;
+  }
+
   public webhookDeleted(id: string) {
     // We can't do much about that as the loop might be in progress and it is not safe to modify the queue.
     // But we can clear the webHook cache, so that the next time we check the webhook url it will be gone.
@@ -350,6 +366,40 @@ export class DocTriggers {
     await this._stats.clear();
   }
 
+  // Converts a table to tableId by looking it up in _grist_Tables.
+  private _getTableId(rowId: number) {
+    const docData = this._activeDoc.docData;
+    if (!docData) {
+      throw new Error("ActiveDoc not ready");
+    }
+    return docData.getMetaTable("_grist_Tables").getValue(rowId, "tableId");
+  }
+
+  // Return false if colRef does not belong to tableRef
+  private _validateColId(colRef: number, tableRef: number) {
+    const docData = this._activeDoc.docData;
+    if (!docData) {
+      throw new Error("ActiveDoc not ready");
+    }
+    return docData.getMetaTable("_grist_Tables_column").getValue(colRef, "parentId") === tableRef;
+  }
+
+  // Converts a column ref to colId by looking it up in _grist_Tables_column. If tableRef is
+  // provided, check whether col belongs to table and throws if not.
+  private _getColId(rowId: number, tableRef?: number) {
+    const docData = this._activeDoc.docData;
+    if (!docData) {
+      throw new Error("ActiveDoc not ready");
+    }
+    if (!rowId) { return ''; }
+    const colId = docData.getMetaTable("_grist_Tables_column").getValue(rowId, "colId");
+    if (tableRef !== undefined &&
+      docData.getMetaTable("_grist_Tables_column").getValue(rowId, "parentId") !== tableRef) {
+      throw new ApiError(`Column ${colId} does not belong to table ${this._getTableId(tableRef)}`, 400);
+    }
+    return colId;
+  }
+
   private get _docId() {
     return this._activeDoc.docName;
   }
@@ -427,6 +477,22 @@ export class DocTriggers {
         continue;
       }
 
+      if (trigger.isReadyColRef) {
+        if (!this._validateColId(trigger.isReadyColRef, trigger.tableRef)) {
+          // ready column does not belong to table, let's ignore trigger and log stats
+          for (const action of webhookActions) {
+            const colId = this._getColId(trigger.isReadyColRef); // no validation
+            const tableId = this._getTableId(trigger.tableRef);
+            const error = `isReadyColumn is not valid: colId ${colId} does not belong to ${tableId}`;
+            this._stats.logInvalid(action.id, error).catch(e => log.error("Webhook stats failed to log", e));
+          }
+          continue;
+        }
+      }
+
+      // TODO: would be worth checking that the trigger's fields are valid (ie: eventTypes, url,
+      // ...) as there's no guarantee that they are.
+
       const rowIndexesToSend: number[] = _.range(bulkColValues.id.length).filter(rowIndex => {
           const rowId = bulkColValues.id[rowIndex];
           return this._shouldTriggerActions(
@@ -860,9 +926,22 @@ class WebhookStatistics extends PersistedStore<StatsKey> {
    * millisecond were seen as the same update.
    */
   public async logStatus(id: string, status: WebhookStatus, now?: number|null) {
-    await this.set(id, [
+    const stats: [StatsKey, string][] = [
       ['status', status],
       ['updatedTime', (now ?? Date.now()).toString()],
+    ];
+    if (status === 'sending') {
+      // clear any error message that could have been left from an earlier bad state (ie: invalid
+      // fields)
+      stats.push(['errorMessage', '']);
+    }
+    await this.set(id, stats);
+  }
+
+  public async logInvalid(id: string, errorMessage: string) {
+    await this.logStatus(id, 'invalid');
+    await this.set(id, [
+      ['errorMessage', errorMessage]
     ]);
   }
 

test/server/lib/DocApi.ts

@@ -33,6 +33,7 @@ import {serveSomething, Serving} from 'test/server/customUtil';
 import * as testUtils from 'test/server/testUtils';
 import clone = require('lodash/clone');
 import defaultsDeep = require('lodash/defaultsDeep');
+import pick = require('lodash/pick');
 
 const chimpy = configForUser('Chimpy');
 const kiwi = configForUser('Kiwi');
@@ -2732,23 +2733,26 @@ function testDocApi() {
   });
 
   it("POST /docs/{did}/tables/{tid}/_subscribe validates inputs", async function () {
-    async function check(requestBody: any, status: number, error: string) {
+    async function check(requestBody: any, status: number, ...errors: RegExp[]) {
       const resp = await axios.post(
         `${serverUrl}/api/docs/${docIds.Timesheets}/tables/Table1/_subscribe`,
         requestBody, chimpy
       );
       assert.equal(resp.status, status);
-      assert.deepEqual(resp.data, {error});
+      for (const error of errors) {
+        assert.match(resp.data.details || resp.data.error, error);
+      }
     }
 
-    await check({}, 400, "eventTypes must be a non-empty array");
-    await check({eventTypes: 0}, 400, "eventTypes must be a non-empty array");
-    await check({eventTypes: []}, 400, "eventTypes must be a non-empty array");
-    await check({eventTypes: ["foo"]}, 400, "Allowed values in eventTypes are: add,update");
-    await check({eventTypes: ["add"]}, 400, "Bad request: url required");
-    await check({eventTypes: ["add"], url: "https://evil.com"}, 403, "Provided url is forbidden");
-    await check({eventTypes: ["add"], url: "http://example.com"}, 403, "Provided url is forbidden");  // not https
-    await check({eventTypes: ["add"], url: "https://example.com", isReadyColumn: "bar"}, 404, `Column not found "bar"`);
+    await check({}, 400, /eventTypes is missing/);
+    await check({eventTypes: 0}, 400, /url is missing/, /eventTypes is not an array/);
+    await check({eventTypes: []}, 400, /url is missing/);
+    await check({eventTypes: [], url: "https://example.com"}, 400, /eventTypes must be a non-empty array/);
+    await check({eventTypes: ["foo"], url: "https://example.com"}, 400, /eventTypes\[0\] is none of "add", "update"/);
+    await check({eventTypes: ["add"]}, 400, /url is missing/);
+    await check({eventTypes: ["add"], url: "https://evil.com"}, 403, /Provided url is forbidden/);
+    await check({eventTypes: ["add"], url: "http://example.com"}, 403, /Provided url is forbidden/);  // not https
+    await check({eventTypes: ["add"], url: "https://example.com", isReadyColumn: "bar"}, 404, /Column not found "bar"/);
   });
 
   async function userCheck(user: AxiosRequestConfig, requestBody: any, status: number, responseBody: any) {
@@ -3354,6 +3358,7 @@ function testDocApi() {
         await axios.post(`${serverUrl}/api/docs/${docId}/apply`, [
           ['ModifyColumn', 'Table1', 'B', {type: 'Bool'}],
         ], chimpy);
+        await userApi.applyUserActions(docId, [['AddTable', 'Table2', [{id: 'Foo'}, {id: 'Bar'}]]]);
       });
 
       const waitForQueue = async (length: number) => {
@@ -3892,6 +3897,90 @@ function testDocApi() {
         await unsubscribe(docId, webhook3);
         await unsubscribe(docId, webhook4);
       });
+
+      describe('webhook update', function() {
+
+        it('should work correctly', async function() {
+
+
+          async function check(fields: any, status: number, error?: RegExp|string,
+                               expectedFieldsCallback?: (fields: any) => any) {
+
+            let savedTableId = 'Table1';
+            const origFields = {
+              tableId: 'Table1',
+              eventTypes: ['add'],
+              isReadyColumn: 'B',
+            };
+
+            // subscribe
+            const webhook = await subscribe('foo', docId, origFields);
+
+            const expectedFields = {
+              url: `${serving.url}/foo`,
+              unsubscribeKey: webhook.unsubscribeKey,
+              eventTypes: ['add'],
+              isReadyColumn: 'B',
+              tableId: 'Table1',
+              enabled: true,
+            };
+
+            let stats = await readStats(docId);
+            assert.equal(stats.length, 1, 'stats=' + JSON.stringify(stats));
+            assert.equal(stats[0].id, webhook.webhookId);
+            assert.deepEqual(stats[0].fields, expectedFields);
+
+            // update
+            const resp = await axios.patch(
+              `${serverUrl}/api/docs/${docId}/webhooks/${webhook.webhookId}`, fields, chimpy
+            );
+
+            // check resp
+            assert.equal(resp.status, status, JSON.stringify(pick(resp, ['data', 'status'])));
+            if (resp.status === 200) {
+              stats = await readStats(docId);
+              assert.equal(stats.length, 1);
+              assert.equal(stats[0].id, webhook.webhookId);
+              if (expectedFieldsCallback) { expectedFieldsCallback(expectedFields); }
+              assert.deepEqual(stats[0].fields, {...expectedFields, ...fields});
+              if (fields.tableId) {
+                savedTableId = fields.tableId;
+              }
+            } else {
+              if (error instanceof RegExp) {
+                assert.match(resp.data.details || resp.data.error, error);
+              } else {
+                assert.deepEqual(resp.data, {error});
+              }
+            }
+
+            // finally  unsubscribe
+            const unsubscribeResp = await unsubscribe(docId, webhook, savedTableId);
+            assert.equal(unsubscribeResp.status, 200, JSON.stringify(pick(unsubscribeResp, ['data', 'status'])));
+            stats = await readStats(docId);
+            assert.equal(stats.length, 0, 'stats=' + JSON.stringify(stats));
+          }
+
+          await check({url: `${serving.url}/bar`}, 200);
+          await check({url: "https://evil.com"}, 403, "Provided url is forbidden");
+          await check({url: "http://example.com"}, 403, "Provided url is forbidden");  // not https
+
+          // changing table without changing the ready column should reset the latter
+          await check({tableId: 'Table2'}, 200, '', expectedFields => expectedFields.isReadyColumn = null);
+
+
+          await check({tableId: 'Santa'}, 404, `Table not found "Santa"`);
+          await check({tableId: 'Table2', isReadyColumn: 'Foo'}, 200);
+
+          await check({eventTypes: ['add', 'update']}, 200);
+          await check({eventTypes: []}, 400, "eventTypes must be a non-empty array");
+          await check({eventTypes: ["foo"]}, 400, /eventTypes\[0\] is none of "add", "update"/);
+
+          await check({isReadyColumn: null}, 200);
+          await check({isReadyColumn: "bar"}, 404, `Column not found "bar"`);
+        });
+
+      });
     });
   });