(core) Remove transform columns on shutdown

Summary: Call a new user action `RemoveTransformColumns` in ActiveDoc shutdown.

Test Plan: Added nbrowser test

Reviewers: georgegevoian, paulfitz

Reviewed By: georgegevoian

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

app/common/DocActions.ts

@@ -145,8 +145,22 @@ export interface TableRecordValue {
 
 export type UserAction = Array<string|number|object|boolean|null|undefined>;
 
-// Actions that trigger formula calculations in the data engine
-export const CALCULATING_USER_ACTIONS = new Set(['Calculate', 'UpdateCurrentTime', 'RespondToRequests']);
+// Actions that are performed automatically by the server
+// for things like regular maintenance or triggering formula calculations in the data engine.
+// Typically applied using `makeExceptionalDocSession("system")`.
+// They're also 'non-essential' in the sense that we don't need to worry about storing them
+// in action/undo history if they don't change anything (which they often won't)
+// and we can dismiss their result if the document is shutting down.
+export const SYSTEM_ACTIONS = new Set([
+  // Initial dummy action performed when the document laods.
+  'Calculate',
+  // Called automatically at regular intervals, again to trigger formula calculations.
+  'UpdateCurrentTime',
+  // Part of the formula calculation process for formulas that use the `REQUEST` function.
+  'RespondToRequests',
+  // Performed at shutdown to clean up temporary helper columns.
+  'RemoveTransformColumns'
+]);
 
 export function getNumRows(action: DocAction): number {
   return !isDataAction(action) ? 0

app/server/lib/ActiveDoc.ts

@@ -1994,6 +1994,14 @@ export class ActiveDoc extends EventEmitter {
         // even if the doc is only ever opened briefly, without having to slow down startup.
         await safeCallAndWait("removeUnusedAttachments", () => this.removeUnusedAttachments(true, usageOptions));
 
+        if (this._dataEngine && this._fullyLoaded) {
+          // Note that this must happen before `this._shuttingDown = true` because of this line in Sharing.ts:
+          //     if (this._activeDoc.isShuttingDown && isCalculate) {
+          await safeCallAndWait("removeTransformColumns",
+            () => this.applyUserActions(docSession, [["RemoveTransformColumns"]])
+          );
+        }
+
         // Update data size; we'll be syncing both it and attachments size to the database soon.
         await safeCallAndWait("_updateDataSize", () => this._updateDataSize(usageOptions));
       }

app/server/lib/Sharing.ts

@@ -8,7 +8,7 @@ import {
   UserActionBundle
 } from 'app/common/ActionBundle';
 import {ApplyUAExtendedOptions} from 'app/common/ActiveDocAPI';
-import {CALCULATING_USER_ACTIONS, DocAction, getNumRows, UserAction} from 'app/common/DocActions';
+import {DocAction, getNumRows, SYSTEM_ACTIONS, UserAction} from 'app/common/DocActions';
 import {allToken} from 'app/common/sharing';
 import {GranularAccessForBundle} from 'app/server/lib/GranularAccess';
 import log from 'app/server/lib/log';
@@ -246,7 +246,7 @@ export class Sharing {
 
     try {
 
-      const isCalculate = (userActions.length === 1 && CALCULATING_USER_ACTIONS.has(userActions[0][0] as string));
+      const isCalculate = (userActions.length === 1 && SYSTEM_ACTIONS.has(userActions[0][0] as string));
       // `internal` is true if users shouldn't be able to undo the actions. Applies to:
       // - Calculate/UpdateCurrentTime because it's not considered as performed by a particular client.
       // - Adding attachment metadata when uploading attachments,

sandbox/grist/useractions.py

@@ -1246,10 +1246,7 @@ class UserActions(object):
     # Remove also all autogenereted formula columns for conditional styles.
     # But not from transform columns, as those columns borrow rules from original columns
     more_removals.update([rule
-                          for col in col_recs if not col.colId.startswith((
-                            'gristHelper_Transform',
-                            'gristHelper_Converted',
-                          ))
+                          for col in col_recs if not _is_transform_col(col.colId)
                           for rule in col.rules])
 
     # Add any extra removals after removing the requested columns in the requested order.
@@ -1353,10 +1350,7 @@ class UserActions(object):
 
     transform = (
         col_id is not None and
-        col_id.startswith((
-          'gristHelper_Transform',
-          'gristHelper_Converted',
-        ))
+        _is_transform_col(col_id)
     )
 
     ret = self.doAddColumn(table_id, col_id, col_info)
@@ -1387,11 +1381,8 @@ class UserActions(object):
     table_rec = self._docmodel.get_table_rec(table_id)
 
     transform = (
-      col_id is not None and
-      col_id.startswith((
-        'gristHelper_Transform',
-        'gristHelper_Converted',
-      ))
+        col_id is not None and
+        _is_transform_col(col_id)
     )
 
     # Add a field for this column to the view(s) for this table.
@@ -1404,7 +1395,6 @@ class UserActions(object):
           self._docmodel.insert(section.fields, col_info.get('_position'), colRef=ret['colRef'])
     return ret
 
-
   @classmethod
   def _pick_col_name(cls, table_rec, col_id, old_col_id=None, avoid_extra=None):
     avoid_set = set(c.colId for c in table_rec.columns)
@@ -1490,6 +1480,12 @@ class UserActions(object):
         # Update the col's displayCol ref
         self._docmodel.update([col_rec], displayCol=display_col_ref)
 
+  @useraction
+  def RemoveTransformColumns(self):
+    self._docmodel.remove([
+      col for col in self._docmodel.columns.all if _is_transform_col(col.colId)
+    ])
+
   # Helper function to get a helper column with the given formula, or to add one if none
   # currently exist.
   def _add_or_update_helper_col(self, table_rec, display_col_rec, formula):
@@ -2208,3 +2204,10 @@ class UserActions(object):
   def GenImporterView(self, source_table_id, dest_table_id, transform_rule=None, options=None):
     return self._import_actions.DoGenImporterView(
         source_table_id, dest_table_id, transform_rule, options or {})
+
+
+def _is_transform_col(col_id):
+  return col_id.startswith((
+    'gristHelper_Transform',
+    'gristHelper_Converted',
+  ))

test/nbrowser/RemoveTransformColumns.ts

@@ -0,0 +1,35 @@
+import {assert, driver} from 'mocha-webdriver';
+
+import * as gu from 'test/nbrowser/gristUtils';
+import {server, setupTestSuite} from "test/nbrowser/testUtils";
+
+describe('RemoveTransformColumns', function () {
+  this.timeout(4000);
+  setupTestSuite();
+
+  it('should remove transform columns when the doc shuts down', async function () {
+    await server.simulateLogin("Chimpy", "chimpy@getgrist.com", 'nasa');
+    const doc = await gu.importFixturesDoc('chimpy', 'nasa', 'Horizon', 'RemoveTransformColumns.grist', false);
+    await driver.get(`${server.getHost()}/o/nasa/doc/${doc.id}`);
+    await gu.waitForDocToLoad();
+
+    assert.deepEqual(await gu.getVisibleGridCells({col: 'B', rowNums: [1]}), [
+      'manualSort, A, B, C, ' +
+      'gristHelper_Converted, gristHelper_Transform, ' +
+      'gristHelper_Converted2, gristHelper_Transform2'
+    ]);
+
+    const userAPI = gu.createHomeApi('chimpy', 'nasa');
+    await userAPI.applyUserActions(doc.id, [["Calculate"]]);  // finish loading fully
+    await userAPI.getDocAPI(doc.id).forceReload();
+    await driver.get(`${server.getHost()}/o/nasa/doc/${doc.id}`);
+    await gu.waitForDocToLoad();
+
+    assert.deepEqual(await gu.getVisibleGridCells({col: 'B', rowNums: [1]}), [
+      'manualSort, A, B, C'
+    ]);
+
+    await gu.checkForErrors();
+  });
+
+});

test/server/testUtils.ts

@@ -67,7 +67,7 @@ class CaptureTransport extends winston.Transport {
   private _captureFunc: (level: string, msg: string, meta: any) => void;
 
   public constructor(options: any) {
-    super();
+    super(options);
     this._captureFunc = options.captureFunc;
     if (options.name) {
       this.name = options.name;
@@ -125,7 +125,8 @@ export function setTmpLogLevel(level: string, optCaptureFunc?: (level: string, m
  * strings. These may be tested using testUtils.assertMatchArray(). Callback may return a promise.
  */
 export async function captureLog(
-  minLevel: string, callback: (messages: string[]) => void|Promise<void>
+  minLevel: string, callback: (messages: string[]) => void|Promise<void>,
+  options: {timestamp: boolean} = {timestamp: false}
 ): Promise<string[]> {
   const messages: string[] = [];
   const prevLogLevel = log.transports.file.level;
@@ -133,14 +134,15 @@ export async function captureLog(
 
   function capture(level: string, msg: string, meta: any) {
     if ((log as any).levels[level] <= (log as any).levels[minLevel]) {  // winston types are off?
-      messages.push(level + ': ' + msg + (meta ? ' ' + serialize(meta) : ''));
+      const timePrefix = options.timestamp ? new Date().toISOString() + ' ' : '';
+      messages.push(`${timePrefix}${level}: ${msg}${meta ? ' ' + serialize(meta) : ''}`);
     }
   }
 
   if (!process.env.VERBOSE) {
     log.transports.file.level = -1 as any;   // Suppress all log output.
   }
-  log.add(CaptureTransport as any, { captureFunc: capture, name });  // types are off.
+  log.add(CaptureTransport as any, { captureFunc: capture, name, level: minLevel});  // types are off.
   try {
     await callback(messages);
   } finally {