(core) do not look at content of recent actions when loading documents

Summary:
This removes the need for any information drawn from the content of recent actions when loading a document.

The undo/redo system does need some facts about recent actions up front. But that system has an important restriction: only actions a particular client is known to have generated can be undone by that client.

So in this diff, as we store which client has performed an action, we also store the few pieces of metadata about that action that the undo/redo system needs: `linkId`, `otherId`, `rowIdHint`, `isUndo` fields. These are all small integers (or in one case a boolean).

An existing limitation is that information about which client has performed which action is stored in memory in the worker, and not persisted anywhere. This diff does not change that limitation, meaning that undos continue to not survive a worker transition. A reasonable way to deal with that would be to back the store with redis.

Test Plan: existing tests pass

Reviewers: dsagal

Reviewed By: dsagal

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

app/client/components/GristDoc.ts

@@ -44,7 +44,7 @@ import {startWelcomeTour} from 'app/client/ui/welcomeTour';
 import {isNarrowScreen, mediaSmall, testId} from 'app/client/ui2018/cssVars';
 import {IconName} from 'app/client/ui2018/IconList';
 import {FieldEditor} from "app/client/widgets/FieldEditor";
-import {ActionGroup} from 'app/common/ActionGroup';
+import {MinimalActionGroup} from 'app/common/ActionGroup';
 import {ClientQuery} from "app/common/ActiveDocAPI";
 import {delay} from 'app/common/delay';
 import {DisposableWithEvents} from 'app/common/DisposableWithEvents';
@@ -376,7 +376,7 @@ export class GristDoc extends DisposableWithEvents {
    * Switch to the view/section and scroll to the record indicated by cursorPos. If cursorPos is
    * null, then moves to a position best suited for optActionGroup (not yet implemented).
    */
-  public async moveToCursorPos(cursorPos?: CursorPos, optActionGroup?: ActionGroup): Promise<void> {
+  public async moveToCursorPos(cursorPos?: CursorPos, optActionGroup?: MinimalActionGroup): Promise<void> {
     if (!cursorPos || cursorPos.sectionId == null) {
       // TODO We could come up with a suitable cursorPos here based on the action itself.
       // This should only come up if trying to undo/redo after reloading a page (since the cursorPos

app/client/components/UndoStack.ts

@@ -1,12 +1,12 @@
 import {CursorPos} from 'app/client/components/Cursor';
 import {GristDoc} from 'app/client/components/GristDoc';
 import * as dispose from 'app/client/lib/dispose';
-import {ActionGroup} from 'app/common/ActionGroup';
+import {MinimalActionGroup} from 'app/common/ActionGroup';
 import {PromiseChain} from 'app/common/gutil';
 import {fromKo, Observable} from 'grainjs';
 import * as ko from 'knockout';
 
-export interface ActionGroupWithCursorPos extends ActionGroup {
+export interface ActionGroupWithCursorPos extends MinimalActionGroup {
   cursorPos?: CursorPos;
 }
 
@@ -27,13 +27,13 @@ export class UndoStack extends dispose.Disposable {
   private _gristDoc: GristDoc;
   private _stack: ActionGroupWithCursorPos[];
   private _pointer: number;
-  private _linkMap: {[actionNum: number]: ActionGroup};
+  private _linkMap: {[actionNum: number]: MinimalActionGroup};
 
   // Chain of promises which send undo actions to the server. This delays the execution of the
   // next action until the current one has been received and moved the pointer index.
   private _undoChain = new PromiseChain<void>();
 
-  public create(log: ActionGroup[], options: {gristDoc: GristDoc}) {
+  public create(log: MinimalActionGroup[], options: {gristDoc: GristDoc}) {
     this._gristDoc = options.gristDoc;
 
     // TODO: _stack and _linkMap grow without bound within a single session.
@@ -65,7 +65,7 @@ export class UndoStack extends dispose.Disposable {
    * Should only be given own actions. Pays attention to actionNum, otherId, linkId, and
    * uses those to adjust undo index.
    */
-  public pushAction(ag: ActionGroup): void {
+  public pushAction(ag: MinimalActionGroup): void {
     if (!ag.fromSelf) {
       return;
     }
@@ -133,7 +133,7 @@ export class UndoStack extends dispose.Disposable {
   /**
    * Find all actionGroups in the bundle that starts with the given action group.
    */
-  private _findActionBundle(ag: ActionGroup) {
+  private _findActionBundle(ag: MinimalActionGroup) {
     const prevNums = new Set();
     const actionGroups = [];
     // Follow references through the linkMap adding items to the array bundle.

app/common/ActionGroup.ts

@@ -1,19 +1,28 @@
 import {ActionSummary} from 'app/common/ActionSummary';
 
-/** This is the action representation the client works with. */
+/**
-export interface ActionGroup {
+ * This is the action representation the client works with, for the purposes of undos/redos.
+ */
+export interface MinimalActionGroup {
   actionNum: number;
   actionHash: string;
-  desc?: string;
-  actionSummary: ActionSummary;
   fromSelf: boolean;
   linkId: number;
   otherId: number;
-  time: number;
-  user: string;
   rowIdHint: number;      // If non-zero, this is a rowId that would be a good place to put
                           // the cursor after an undo.
-  primaryAction: string;  // The name of the first user action in the ActionGroup.
   isUndo: boolean;        // True if the first user action is ApplyUndoActions.
+}
+
+/**
+ * This is the action representation the client works with, for the purposes of document
+ * history and undos/redos.
+ */
+export interface ActionGroup extends MinimalActionGroup {
+  desc?: string;
+  actionSummary: ActionSummary;
+  time: number;
+  user: string;
+  primaryAction: string;  // The name of the first user action in the ActionGroup.
   internal: boolean;      // True if it is inappropriate to log/undo the action.
 }

app/common/DocListAPI.ts

@@ -1,4 +1,4 @@
-import {ActionGroup} from 'app/common/ActionGroup';
+import {MinimalActionGroup} from 'app/common/ActionGroup';
 import {TableDataAction} from 'app/common/DocActions';
 import {Role} from 'app/common/roles';
 import {StringUnion} from 'app/common/StringUnion';
@@ -41,7 +41,7 @@ export interface OpenLocalDocResult {
   docFD: number;
   clientId: string;  // the docFD is meaningful only in the context of this session
   doc: {[tableId: string]: TableDataAction};
-  log: ActionGroup[];
+  log: MinimalActionGroup[];
   recoveryMode?: boolean;
   userOverride?: UserOverride;
 }

app/server/lib/ActionHistory.ts

@@ -14,7 +14,7 @@
  */
 
 import {LocalActionBundle} from 'app/common/ActionBundle';
-import {ActionGroup} from 'app/common/ActionGroup';
+import {ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup';
 import {createEmptyActionSummary} from 'app/common/ActionSummary';
 import {getSelectionDesc, UserAction} from 'app/common/DocActions';
 import {DocState} from 'app/common/UserAPI';
@@ -27,7 +27,7 @@ export interface ActionGroupOptions {
   summarize?: boolean;
 
   // The client for which the action group is being prepared, if known.
-  client?: {clientId: string}|null;
+  clientId?: string;
 
   // Values returned by the action, if known.
   retValues?: any[];
@@ -36,6 +36,20 @@ export interface ActionGroupOptions {
   internal?: boolean;
 }
 
+/**
+ * Metadata about an action that is needed for undo/redo stack.
+ */
+export interface ActionHistoryUndoInfoWithoutClient {
+  otherId: number;
+  linkId: number;
+  rowIdHint: number;
+  isUndo: boolean;
+}
+
+export interface ActionHistoryUndoInfo extends ActionHistoryUndoInfoWithoutClient {
+  clientId: string;
+}
+
 export abstract class ActionHistory {
   /**
    * Initialize the ActionLog by reading the database. No other methods may be used until the
@@ -112,6 +126,8 @@ export abstract class ActionHistory {
    */
   public abstract getRecentActionGroups(maxActions: number, options: ActionGroupOptions): Promise<ActionGroup[]>;
 
+  public abstract getRecentMinimalActionGroups(maxActions: number, clientId?: string): Promise<MinimalActionGroup[]>;
+
   /**
    * Get the most recent states from the history.  States are just
    * actions without any content.  Results are ordered by most recent
@@ -131,10 +147,10 @@ export abstract class ActionHistory {
    * Associates an action with a client. This association is expected to be transient, rather
    * than persistent.  It should survive a client-side reload but not a server-side restart.
    */
-  public abstract setActionClientId(actionHash: string, clientId: string): void;
+  public abstract setActionUndoInfo(actionHash: string, undoInfo: ActionHistoryUndoInfo): void;
 
   /** Check for any client associated with an action, identified by checksum */
-  public abstract getActionClientId(actionHash: string): string | undefined;
+  public abstract getActionUndoInfo(actionHash: string): ActionHistoryUndoInfo | undefined;
 
   /**
    * Remove all stored actions except the last keepN and run the VACUUM command
@@ -232,11 +248,62 @@ export function humanDescription(actions: UserAction[]): string {
 export function asActionGroup(history: ActionHistory,
                               act: LocalActionBundle,
                               options: ActionGroupOptions): ActionGroup {
-  const {summarize, client, retValues} = options;
+  const {summarize, clientId} = options;
   const info = act.info[1];
-  const fromSelf = (client && client.clientId && act.actionHash) ?
-    (history.getActionClientId(act.actionHash) === client.clientId) : false;
 
+  const fromSelf = (act.actionHash && clientId) ?
+    (history.getActionUndoInfo(act.actionHash)?.clientId === clientId) : false;
+
+  const {extra: {primaryAction}, minimal: {rowIdHint, isUndo}} =
+    getActionUndoInfoWithoutClient(act, options.retValues);
+
+  return {
+    actionNum: act.actionNum,
+    actionHash: act.actionHash || "",
+    desc: info.desc || humanDescription(act.userActions),
+    actionSummary: summarize ? summarizeAction(act) : createEmptyActionSummary(),
+    fromSelf,
+    linkId: info.linkId,
+    otherId: info.otherId,
+    time: info.time,
+    user: info.user,
+    rowIdHint,
+    primaryAction,
+    isUndo,
+    internal: options.internal || false,
+  };
+}
+
+export function asMinimalActionGroup(history: ActionHistory,
+                                     act: {actionHash: string, actionNum: number},
+                                     clientId?: string): MinimalActionGroup {
+  const undoInfo = act.actionHash ? history.getActionUndoInfo(act.actionHash) : undefined;
+  const fromSelf = clientId ? (undoInfo?.clientId === clientId) : false;
+  return {
+    actionNum: act.actionNum,
+    actionHash: act.actionHash || "",
+    fromSelf,
+    linkId: undoInfo?.linkId || 0,
+    otherId: undoInfo?.otherId || 0,
+    rowIdHint: undoInfo?.rowIdHint || 0,
+    isUndo: undoInfo?.isUndo || false,
+  };
+}
+
+export function getActionUndoInfo(act: LocalActionBundle, clientId: string,
+                                  retValues: any[]): ActionHistoryUndoInfo {
+  return {
+    ...getActionUndoInfoWithoutClient(act, retValues).minimal,
+    clientId,
+  };
+}
+
+/**
+ * Compute undo information from an action bundle and return values if available.
+ * Results are returned as {minimal, extra} where core has information needed for minimal
+ * action groups, and extra has information only needed for full action groups.
+ */
+function getActionUndoInfoWithoutClient(act: LocalActionBundle, retValues?: any[]) {
   let rowIdHint = 0;
   if (retValues) {
     // A hint for cursor position.  This logic used to live on the client, but now trying to
@@ -255,22 +322,18 @@ export function asActionGroup(history: ActionHistory,
     }
   }
 
+  const info = act.info[1];
   const primaryAction: string = String((act.userActions[0] || [""])[0]);
   const isUndo = primaryAction === 'ApplyUndoActions';
-
   return {
-    actionNum: act.actionNum,
+    minimal: {
-    actionHash: act.actionHash || "",
-    desc: info.desc || humanDescription(act.userActions),
-    actionSummary: summarize ? summarizeAction(act) : createEmptyActionSummary(),
-    fromSelf,
-    linkId: info.linkId,
-    otherId: info.otherId,
-    time: info.time,
-    user: info.user,
       rowIdHint,
-    primaryAction,
+      otherId: info.otherId,
+      linkId: info.linkId,
       isUndo,
-    internal: options.internal || false,
+    },
+    extra: {
+      primaryAction,
+    },
   };
 }

app/server/lib/ActionHistoryImpl.ts

@@ -2,14 +2,15 @@
  * Minimal ActionHistory implementation
  */
 import {LocalActionBundle} from 'app/common/ActionBundle';
-import {ActionGroup} from 'app/common/ActionGroup';
+import {ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup';
 import * as marshaller from 'app/common/marshal';
 import {DocState} from 'app/common/UserAPI';
 import {reportTimeTaken} from 'app/server/lib/reportTimeTaken';
 import * as crypto from 'crypto';
 import keyBy = require('lodash/keyBy');
 import mapValues = require('lodash/mapValues');
-import {ActionGroupOptions, ActionHistory, asActionGroup} from './ActionHistory';
+import {ActionGroupOptions, ActionHistory, ActionHistoryUndoInfo, asActionGroup,
+        asMinimalActionGroup} from './ActionHistory';
 import {ISQLiteDB, ResultRow} from './SQLiteDB';
 
 // History will from time to time be pruned back to within these limits
@@ -165,7 +166,7 @@ export class ActionHistoryImpl implements ActionHistory {
   private _haveLocalSent: boolean = false;    // cache for this.haveLocalSent()
   private _haveLocalUnsent: boolean = false;  // cache for this.haveLocalUnsent()
   private _initialized: boolean = false;      // true when initialize() has completed
-  private _actionClient = new Map<string, string>();  // transient cache of who created actions
+  private _actionUndoInfo = new Map<string, ActionHistoryUndoInfo>();  // transient cache of undo info
 
   constructor(private _db: ISQLiteDB, private _options: ActionHistoryOptions = defaultOptions) {
   }
@@ -174,7 +175,7 @@ export class ActionHistoryImpl implements ActionHistory {
   public async wipe() {
     await this._db.run("UPDATE _gristsys_ActionHistoryBranch SET actionRef = NULL");
     await this._db.run("DELETE FROM _gristsys_ActionHistory");
-    this._actionClient.clear();
+    this._actionUndoInfo.clear();
   }
 
   public async initialize(): Promise<void> {
@@ -382,6 +383,17 @@ export class ActionHistoryImpl implements ActionHistory {
       () => actions.map(row => asActionGroup(this, decodeActionFromRow(row), options)));
   }
 
+  public async getRecentMinimalActionGroups(maxActions: number, clientId?: string): Promise<MinimalActionGroup[]> {
+    // Don't look at content of actions.
+    const actions = await this._getRecentActionRows(maxActions, false);
+    return reportTimeTaken(
+      "getRecentMinimalActionGroups",
+      () => actions.map(row => asMinimalActionGroup(
+        this,
+        {actionHash: row.actionHash, actionNum: row.actionNum},
+        clientId)));
+  }
+
   public async getRecentStates(maxStates?: number): Promise<DocState[]> {
     const branches = await this._getBranches();
     const states = await this._fetchParts(null,
@@ -432,23 +444,25 @@ export class ActionHistoryImpl implements ActionHistory {
     });
   }
 
-  public setActionClientId(actionHash: string, clientId: string): void {
+  public setActionUndoInfo(actionHash: string, undoInfo: ActionHistoryUndoInfo): void {
-    this._actionClient.set(actionHash, clientId);
+    this._actionUndoInfo.set(actionHash, undoInfo);
   }
 
-  public getActionClientId(actionHash: string): string | undefined {
+  public getActionUndoInfo(actionHash: string): ActionHistoryUndoInfo | undefined {
-    return this._actionClient.get(actionHash);
+    return this._actionUndoInfo.get(actionHash);
   }
 
   /**
    * Fetches the most recent action row from the history, orderd with earlier actions first.
    * If `maxActions` is supplied, at most that number of actions are returned.
    */
-  private async _getRecentActionRows(maxActions?: number): Promise<ResultRow[]> {
+  private async _getRecentActionRows(maxActions: number|undefined,
+                                     withBody: boolean = true): Promise<ResultRow[]> {
     const branches = await this._getBranches();
+    const columns = '_gristsys_ActionHistory.id, actionNum, actionHash' + (withBody ? ', body' : '');
     const result = await this._fetchParts(null,
                                           branches.local_unsent,
-                                           "_gristsys_ActionHistory.id, actionNum, actionHash, body",
+                                          columns,
                                           maxActions,
                                           true);
     result.reverse();  // Implementation note: this could be optimized away when `maxActions`
@@ -618,7 +632,7 @@ export class ActionHistoryImpl implements ActionHistory {
     await this._db.run(`DELETE FROM _gristsys_ActionHistory
                           WHERE id ${invert ? 'NOT' : ''} IN (${idList})`);
     for (const row of rows) {
-      this._actionClient.delete(row.actionHash);
+      this._actionUndoInfo.delete(row.actionHash);
     }
     return ids;
   }

app/server/lib/ActiveDoc.ts

@@ -15,7 +15,7 @@ import fetch from 'node-fetch';
 import * as tmp from 'tmp';
 
 import {getEnvContent, LocalActionBundle, SandboxActionBundle, UserActionBundle} from 'app/common/ActionBundle';
-import {ActionGroup} from 'app/common/ActionGroup';
+import { ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup';
 import {
   ApplyUAOptions,
   ApplyUAResult,
@@ -251,7 +251,7 @@ export class ActiveDoc extends EventEmitter {
    */
   public async getRecentActions(docSession: OptDocSession, summarize: boolean): Promise<ActionGroup[]> {
     const groups = await this._actionHistory.getRecentActionGroups(MAX_RECENT_ACTIONS,
-      {client: docSession.client, summarize});
+      {clientId: docSession.client?.clientId, summarize});
     const permittedGroups: ActionGroup[] = [];
     // Process groups serially since the work is synchronous except for some
     // possible db accesses that will be serialized in any case.
@@ -263,6 +263,11 @@ export class ActiveDoc extends EventEmitter {
     return permittedGroups;
   }
 
+  public async getRecentMinimalActions(docSession: OptDocSession): Promise<MinimalActionGroup[]> {
+    return this._actionHistory.getRecentMinimalActionGroups(
+      MAX_RECENT_ACTIONS, docSession.client?.clientId);
+  }
+
   /** expose action history for tests */
   public getActionHistory(): ActionHistory {
     return this._actionHistory;

app/server/lib/DocManager.ts

@@ -317,7 +317,7 @@ export class DocManager extends EventEmitter {
 
     const [metaTables, recentActions] = await Promise.all([
       activeDoc.fetchMetaTables(docSession),
-      activeDoc.getRecentActions(docSession, false)
+      activeDoc.getRecentMinimalActions(docSession)
     ]);
 
     this.emit('open-doc', this.storageManager.getPath(activeDoc.docName));

app/server/lib/Sharing.ts

@@ -14,7 +14,7 @@ import {shortDesc} from 'app/server/lib/shortDesc';
 import * as assert from 'assert';
 import {Mutex} from 'async-mutex';
 import * as Deque from 'double-ended-queue';
-import {ActionHistory, asActionGroup} from './ActionHistory';
+import { ActionHistory, asActionGroup, getActionUndoInfo} from './ActionHistory';
 import {summarizeAction} from "./ActionSummary";
 import {ActiveDoc} from './ActiveDoc';
 import {makeExceptionalDocSession, OptDocSession} from './DocSession';
@@ -275,7 +275,9 @@ export class Sharing {
           // Check isCalculate because that's not an action we should allow undo/redo for (it's not
           // considered as performed by a particular client).
           if (client && client.clientId && !isCalculate) {
-            this._actionHistory.setActionClientId(localActionBundle.actionHash!, client.clientId);
+            this._actionHistory.setActionUndoInfo(
+              localActionBundle.actionHash!,
+              getActionUndoInfo(localActionBundle, client.clientId, sandboxActionBundle.retValues));
           }
         });
       }
@@ -286,7 +288,7 @@ export class Sharing {
 
       // Broadcast the action to connected browsers.
       const actionGroup = asActionGroup(this._actionHistory, localActionBundle, {
-        client,
+        clientId: client?.clientId,
         retValues: sandboxActionBundle.retValues,
         // Mark the on-open Calculate action as internal. In future, synchronizing fields to today's
         // date and other changes from external values may count as internal.