diff --git a/app/common/DocActions.ts b/app/common/DocActions.ts index 1a64afea..b89246e7 100644 --- a/app/common/DocActions.ts +++ b/app/common/DocActions.ts @@ -145,8 +145,22 @@ export interface TableRecordValue { export type UserAction = Array; -// 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 diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index f7f5c882..041129a2 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/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)); } diff --git a/app/server/lib/Sharing.ts b/app/server/lib/Sharing.ts index da48477e..77743e63 100644 --- a/app/server/lib/Sharing.ts +++ b/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, diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 30868a8e..21f4c31e 100644 --- a/sandbox/grist/useractions.py +++ b/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', + )) diff --git a/test/fixtures/docs/RemoveTransformColumns.grist b/test/fixtures/docs/RemoveTransformColumns.grist new file mode 100644 index 00000000..2ec267a4 Binary files /dev/null and b/test/fixtures/docs/RemoveTransformColumns.grist differ diff --git a/test/nbrowser/RemoveTransformColumns.ts b/test/nbrowser/RemoveTransformColumns.ts new file mode 100644 index 00000000..a84c974b --- /dev/null +++ b/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(); + }); + +}); diff --git a/test/server/testUtils.ts b/test/server/testUtils.ts index 5975511b..3f0a5cb6 100644 --- a/test/server/testUtils.ts +++ b/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 + minLevel: string, callback: (messages: string[]) => void|Promise, + options: {timestamp: boolean} = {timestamp: false} ): Promise { 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 {