From 5197891427f6a44869993913a18fb7e0569507fb Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Mon, 13 Nov 2023 22:59:23 +0200 Subject: [PATCH] (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 | 18 ++++++++- app/server/lib/ActiveDoc.ts | 8 ++++ app/server/lib/Sharing.ts | 4 +- sandbox/grist/useractions.py | 31 +++++++++------- .../docs/RemoveTransformColumns.grist | Bin 0 -> 163840 bytes test/nbrowser/RemoveTransformColumns.ts | 35 ++++++++++++++++++ test/server/testUtils.ts | 10 +++-- 7 files changed, 84 insertions(+), 22 deletions(-) create mode 100644 test/fixtures/docs/RemoveTransformColumns.grist create mode 100644 test/nbrowser/RemoveTransformColumns.ts 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 0000000000000000000000000000000000000000..2ec267a4288f27630f302fe66b5bf4416740a0e5 GIT binary patch literal 163840 zcmeI5?{6DNddInzEK-sw+HoAlNt|q$%gL;*M6@j1Iybmb5^eJ=%ZfxLF^rU6aYxdg zm%GgVu&e||E6L8KFM9t#fxhgU7We)dq`;x*i+j^+-xNg;Xo~`OZ?5QFTj1_+1+IN& zmRv5$S=v75hhuyRo8rzh^UO1!=h@lWo#Alhtp&ql>bmXJHBX&N984%m;!RaeBoYJk z|2X|`z4pk(I2P2JN#+#lOeW$ zdGM0*lgzJWg^~6d5x9FJoy<;8D<9W9ZOvrW4aac3YK4Dxt2Nv7>y|8SY;L(!td!JB zadx4k=44rN>TyF?%ZruL#nQ66v|PSYT)w7WE?qmR<{FyAEU&ERLiv2@LUDDWqE3d= zCELxZi>p^k%jG$>B`&a;nHkDLmhsY+(z}&VHa<=QZ@a-{>05@r!Mv*t&#*1h+gs3Z zFHoTSrj}D@uU=g!6&FLr+$Me&2w`tYQ(I%EOr_4;nq!D6Wn$m*3^vCba>23d3xO6{ z-L7A+YtHtH?K?GIKu%3J+=i)b$I5RRu0dXnl{-GUyvxqT)rAFWyu)gmS$k)bS@Nm_ z`8;dL%I9~J2a?(Gab@p{B*dcemhKZ&{h~>zm4`#aF*v%n4Ym~*Z5?z);L96nTl1{0 zSrm0LA3539z244t$| z&x{6RPPc!=DYK@zH<>sYdt=+JZBnD(rjpbuNt5^)XVlpTwk195$QZv)XZi~BJj2@P ztvO?X8KH> zUAQ_M_B`X`fqqpbGkN|XkLJ2|&Cooit6PS*DI`%%`JJWVWVTRHb}x5PyA#^)=1+$05)s+cJ}e?LCn%$MIi znM`IcUR3tp>1t9@oN<@vvSmBlvkqz5Y{!E{n2KqrB5{9L-+wS`&jpVAD@7dzG+80o@&{sUX_vZd&_WXI}qijD<#ON4$5c&_pz4PR4 z;waCn$8#aVT)qqICC%NGd4#YQ1$ULYz1mH>GK2+#5exO}Eq`=)kqS~LW3Q`0HvY-m zG~A|lLM4Gk6e)2^=o&6g19hE71gc}(o>1j5M%`0Wn;WWIv-Rzuzlg?0>oeI@v#C$B zgpQ!z%jT2cPw-Sx9mXP9dfg{Vg2;X zbYW&jD@^IynQ2`=HC>yYIki?}Y-(nDMn8RKTC1JX^|h(mG&@^cJ2%Z{PE~Jw==1L; z2Yb!&y&U~F>F`n2`op14+M&_7K}SB{)0}U8e4YL#LRha^Ig~L+i%jQy@omI^KWklTT%ua{e`g3 z)Yhj8x^|kK)~7U%dFo8<%&GNr>}=uOq+ZYpr`9y>tUi5CpFDSNtuR&7&Q595rzX#= zt*=co?X<9w@3_iz(YAI2uXASp0eV|)(AIQpnbmAZ7x^oESMh=ofP02=h;0ZjF|)y( z>YPnv9oo|Nl*lmo=o%}u6RXSj?QUueAX$3gjYqe0aq@T7(Y9=H(jVo#$aR5-f)KjS zUG5*Ewk+^HV}OnuupvdKl7Ue{lg0cKmY_l00ck) z1V8`;KmY_l00cnbi6bzW7)+$o{Qf_|caXpf1V8`;KmY_l00ck)1V8`;KmY_l;E5#= z%>RFy$b9<5HW3X20T2KI5C8!X009sH0T2KI5CDO%Hi4p&;#2s_)H~zFb;qb_uYb$l z)GUj+YreDbVNUD=z-Y&RW=*WF(60b6Ys0X3LSx(8w5_S!o#6ie=ZVbEzuG8ZJqUmR z2!H?xfB*=900@8p2!H?xfIvtfGmxac|57O>_?gCmB)|VprE7`I4>KQSo*VnWu}{YS zApM_|fENgW00@8p2!H?xfB*=900@Ad+oB4kFlLNmu^6co3Q{Nr_ zH2KL8+rK<`iA3L`ED!L?{95_bY(B5tyXk3bCTl+NDAybG@WJgo%FHd7ij|UDDb6mG z)Lf@9rye(SwY*p}TwSQBlO(krW8;Eh zvPG@Va%!dYZbfa0$Hz(D&>WXtu${VZYBIUbOrvgi%#jLhpIY_4Z+oPB_UhGzQgJa< z&ueFHO>3|k{j8#_xF{GWbZ;6B!!kTWGu?((i>tt@*}7qE$kcCY?j@$_%yA{g@te>+8H~s!(S(iI2kkYfLoVvJrrLOelv8nW{F-QC+d|<~2_Xz;vdKcHoiaP>+wO%~#wtLx?9P z)Fo3hEY+|)w!s{=Au3jNs#!JkP)U$n)`M0qUae5y#7~a45^is_#7(v>&D<*CZ8DYe zgf-mKOrNQ<3s+~u?rwZM(66dwCeI(_(OlQA8Jfp*b<6NJg(Rvezq2%)%oYmD?&U73 z{2JH}HqNw_G7>|yCBH9Eb?#DW?y|T-zoAa%PloLh5!n+Qq1fKCm{Yb~PqS*Q#kYNN zZsyDHo=hgQ7cVM%?{qb(D9*S`G-GBv+p`X7*=)yyM3{x!?8FAfIYm&5B#N zuBC>u^@~cvuyl6Yecz<9OI7ndTO3#W0;(4Jiih{!+@H*zKd*e0?dORY9YYU7|6#ay zp1e&Q<$3jZE<~8icVWGxxtlVN5Z0pLu2Q#GyGd7uuwXDIp?b!^)csvO3sdunQPLv?Gmz8&-z(b#BxCYx$D z^=X#S5menQn!hqMl-15ZWOT&FMRIR!Fqu7jR=Iax8WC03V~y^35Q+QyMuW(5|5yll z@KJnoSlQ8wPPY4^TrfWD5Yr7nTLnJ}=?dT32}k6jf&2m+pQXl!babhU&*9iK8tI!u z^z(gleB0$0F$4@C_y4eMs#41L+|yrd+vCr&6keivJQrEK2Kwl1vhz8JD1 z??Zq!bbcO-@mcdC5{9f8sBm1Zl>8g}hO%#+xc}K57#!*CMZy5x(5LuKJ?8%(;L!%M zfB*=900@8p2!H?xfB*=900@Arq;|LWjh>^qdW zGxpJe-;VxhjpFR;JKnq@?6KW zm)24OiHd&HY3)aftz`$TsR=%@A4u;vQ!Sqjq~bIw?QP0u)$TiA{ixHu>xLP#oFQ%^ zIDaqujF_wYGbj7Llo!M?@3lzy?nro_P4z0uJmy6Zcm@c2fj*ZNn@gvFXtFJ1%hk7G35|Piw zM`FJS4CVxSC&%N~n)Go-e&^iNfu&PXOP92b)}!O9_?(qmDn$J(y+!?F?5V=1{wz=9 z>l5VuX^<81eOZ3z=wY#rW9R*-=@o5lR&$!YaB%R9)c8R5J6lh6_OtESQ^7;|ZrjQp ztGAYzkp<%LNSu8_+Rs8V!ep_4w)v0gLN%bXo{V!ylQ#wlTe36<-`d zXLnDrd?GRUM~}2LbLiNR7y|CS_<}eC@6il)dyQvtZef|PD{Jy?9lj*R+LHF8=`dF; zgOih^FHF}Bm#UYS{O^Ko>~rS z9lj`aP)pj+K|*0=+h$XXl ztjd=3<@feKCzep%n~vfl@LFw?FKKHLi=-TrLW^YR$Bo8lwEBq@_jr!%`NFX_|KcS?;`~ROMGC%tYwgM)D z00@8p2!H?xfB*=900@8p2!H?xbQ3sC_xwkXr7Qf`Fba;gRpq}W!{`6^Wqy#z{42fT z1p*)d0w4eaAOHd&00JNY0w4eaAn>T&Ey|VrMVC-Nt z$H5VP|DQPc1vUv)fdB}A00@8p2!H?xfB*=900@8p2z=oLaR2{>BS(cG00JNY0w4ea zAOHd&00JNY0wC~(6Ttod7mgeif&d7B00@8p2!H?xfB*=900@A<7fyiR|0|h4PtZTS zKmY_l00ck)1V8`;KmY_l00ck)1in-P?2xxk z{^s?bSH;-Hi6o3E-fB*=900@8p2!H?xfB*=900=yG1O}DC1pQzj z*_9qBCo;vcj}H8H^hYDF4S$^aIQh@XefxhnXbt=WiSPme5V#+Ko&8@+W=jQS=Vimv z+3o6vW4K;JS9W}Exm2u_)biqd>0Px~-Y%S&lbEt!4eBO10&8-_0enM~^D^Mm=rKWEK8a2;RTmthG{{T_~x!ATg&NH*}&> zDP1frt4qt}E5+q&>gCe4lWMN6S-xhj*p8P|7gw*8mdkVMeCa}Qb)ljb*v!lf_S({!VsU+u&k74?HHbS|^?T6WUxN>6An^a@Xwmd2*qwc%R z5k__eM1E&gO=k0XWtT<4T`)|KIW2U}qpwNfHHH1~-7?tL3afdBZN-A>2a!&1vPRad zhGAJe9(yR}cTT*N%$_}~?D)}k73UVNu=*M`ZPRG9n%9~5inNuTdHOXraGu=dwN1m6 zMMHk)&Wp+H^t7^ftjpRZ$FOQfLkq*03~8t3;%tp%>C=YzZ+P}s?IZX0KbOoF z3d+6dD6Z(Bt!Ef_ObRVhq#rlx$AckYkLSovE{y9sxQr8xE4N~NKr-3Mz)P0oQE1e) z4R(nc8=DlQva%w>JBIFU%H^I>XYtu{j|&R6C*wj&khcYgm_ z5kj4&zpa@DEz;+GRMYrK%JWk5BN_TNfR8F;`YtQxqSWVX&GY;J!OZs(ng7WAQ|52z z1708i0w4eaAOHd&00JNY0w4eaAOHeiGJ(T`$wW4tem*UpGrvu<3iMR(@IW-Nq#K^? zxKHg9iARH+MZHcBpPy2qX;-%_D$MWy69>QKW}$iz009sH0T2KI5C8!X009sH0T2Lz z&y@h~|36o}P!0kh00JNY0w4eaAOHd&00JNY0*?g&-2XooZh#RW00JNY0w4eaAOHd& z00JNY0wD0Y65#j$N~V&ae|Uia2!H?xfB*=900@8p2!H?xfB*m3 zs!-sz;QRj^0$v~h0w4eaAOHd&00JNY0w4eaAn@c8!2SP|>sz!81V8`;KmY_l00ck) O1V8`;KmY_<1pXiW+5lVt literal 0 HcmV?d00001 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 {