From 9b36fb4dab42f1f49e6cfeae748e206576ba3684 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Sat, 23 Sep 2023 22:10:30 +0200 Subject: [PATCH] (core) Fix error in sandbox when removing multiple summary source columns Summary: Fixes a very specific bug reported here: https://grist.slack.com/archives/C069RUP71/p1694630242765769 The error occurred when: 1. Removing multiple columns simultaneously 2. Those columns were sources of groupby columns for a summary table (so removing them meant recreating the summary table and thus deleting its columns) 3. There was a display column for one of the columns that got deleted (either directly or indirectly) which was set to be automatically removed since it was no longer needed, but this failed because the column was already deleted as part of earlier table removal. I fixed this by making `apply_auto_removes` remove table records last, so removing the display column wouldn't be a problem. That fixed the original error, but then I noticed that trying to undo the removal could lead to another error (under even more specific circumstances). It's hard to see exactly why, but I can see that just 3 `RemoveColumn` user actions generated over 100 doc actions and corresponding undo actions, hence the difficulty in narrowing the problem down. This is partly because removing a single column would recreate a summary table, only for that table to be immediately replaced again when another column was removed. Making the frontend send a single `[BulkRemoveRecord, _grist_Tables_column, ...] ` leads to a more efficient and sensible process with about half as many doc actions and no undo error. I think this alone would also solve the original error, but the data engine change seems more generally helpful and worth keeping. Test Plan: Added a Python test and an nbrowser test Reviewers: jarek Reviewed By: jarek Differential Revision: https://phab.getgrist.com/D4052 --- app/client/components/GridView.js | 12 +++-- sandbox/grist/docmodel.py | 6 ++- sandbox/grist/test_summary.py | 55 ++++++++++++++++++++- test/fixtures/docs/DeleteColumnsUndo.grist | Bin 0 -> 172032 bytes test/nbrowser/DeleteColumnsUndo.ts | 33 +++++++++++++ 5 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/docs/DeleteColumnsUndo.grist create mode 100644 test/nbrowser/DeleteColumnsUndo.ts diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index b7454b0f..3ddc2f7e 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -890,10 +890,14 @@ GridView.prototype.deleteColumns = function(selection) { }); return Promise.resolve(false); } - let actions = fields.filter(col => !col.disableModify()).map(col => ['RemoveColumn', col.colId()]); - if (actions.length > 0) { - return this.tableModel.sendTableActions(actions, `Removed columns ${actions.map(a => a[1]).join(', ')} ` + - `from ${this.tableModel.tableData.tableId}.`).then(() => this.clearSelection()); + const columns = fields.filter(col => !col.disableModify()); + const colRefs = columns.map(col => col.colRef.peek()); + if (colRefs.length > 0) { + return this.gristDoc.docData.sendAction( + ['BulkRemoveRecord', '_grist_Tables_column', colRefs], + `Removed columns ${columns.map(col => col.colId.peek()).join(', ')} ` + + `from ${this.tableModel.tableData.tableId}.` + ).then(() => this.clearSelection()); } return Promise.resolve(false); }; diff --git a/sandbox/grist/docmodel.py b/sandbox/grist/docmodel.py index 8eec2726..4197c1bb 100644 --- a/sandbox/grist/docmodel.py +++ b/sandbox/grist/docmodel.py @@ -260,7 +260,11 @@ class DocModel(object): Remove the records marked for removal. """ # Sort to make sure removals are done in deterministic order. - gone_records = sorted(self._auto_remove_set) + gone_records = sorted( + self._auto_remove_set, + # Remove tables last to prevent errors trying to remove rows or columns from deleted tables. + key=lambda r: (r._table.table_id == "_grist_Tables", r) + ) self._auto_remove_set.clear() # setAutoRemove is called by formulas, notably summary tables, and shouldn't be blocked by ACL. with self._engine.user_actions.indirect_actions(): diff --git a/sandbox/grist/test_summary.py b/sandbox/grist/test_summary.py index fa19df8a..6af9e2a8 100644 --- a/sandbox/grist/test_summary.py +++ b/sandbox/grist/test_summary.py @@ -4,10 +4,12 @@ files: test_summary.py and test_summary2.py. """ import logging + import actions import summary -import testutil import test_engine +import testutil +import useractions from test_engine import Table, Column, View, Section, Field from useractions import allowed_summary_change @@ -870,6 +872,57 @@ class Address: [ 2, 2.0, [3], 1, 6 ], ]) + def test_remove_source_columns_with_display_column(self): + # Verify a fix for a specific bug: removing multiple groupby source columns + # when the summary table contains a display column. + + self.apply_user_action(["AddEmptyTable", None]) + # Group by A and B + self.apply_user_action(["CreateViewSection", 1, 0, "record", [2,3], None]) + # Add a display column for the group column + self.apply_user_action(['SetDisplayFormula', 'Table1_summary_A_B', None, 7, '$group.C']) + + # Verify metadata and initially. + self.assertTables([ + Table(1, "Table1", summarySourceTable=0, primaryViewId=1, columns=[ + Column(1, "manualSort", "ManualSortPos", False, "", 0), + Column(2, "A", "Any", True, "", 0), + Column(3, "B", "Any", True, "", 0), + Column(4, "C", "Any", True, "", 0), + ]), + Table(2, "Table1_summary_A_B", summarySourceTable=1, primaryViewId=0, columns=[ + Column(5, "A", "Any", False, "", 2), + Column(6, "B", "Any", False, "", 3), + Column(7, "group", "RefList:Table1", True, "table.getSummarySourceGroup(rec)", 0), + Column(8, "count", "Int", True, "len($group)", 0), + Column(9, "gristHelper_Display", "Any", True, "$group.C", 0), + ]) + ]) + + user_actions = [ + useractions.from_repr(ua) for ua in + [ + ['RemoveColumn', 'Table1', 'A'], + ['RemoveColumn', 'Table1', 'B'], + ] + ] + self.engine.apply_user_actions(user_actions) + + # Verify that the final structure is as expected. + self.assertTables([ + Table(1, "Table1", summarySourceTable=0, primaryViewId=1, columns=[ + Column(1, "manualSort", "ManualSortPos", False, "", 0), + Column(4, "C", "Any", True, "", 0), + ]), + # Table1_summary_A_B was removed and recreated as Table1_summary. + # This removed Table1_summary_A_B.group which automatically removed gristHelper_Display + # which led to an error in the past. + Table(4, "Table1_summary", summarySourceTable=1, primaryViewId=0, columns=[ + Column(14, "group", "RefList:Table1", True, "table.getSummarySourceGroup(rec)", 0), + Column(15, "count", "Int", True, "len($group)", 0), + ]) + ]) + #---------------------------------------------------------------------- # pylint: disable=R0915 def test_allow_select_by_change(self): diff --git a/test/fixtures/docs/DeleteColumnsUndo.grist b/test/fixtures/docs/DeleteColumnsUndo.grist new file mode 100644 index 0000000000000000000000000000000000000000..4365e365d76c3f21613c729080067126abe8ac18 GIT binary patch literal 172032 zcmeI5U2GdycE@KZN)#oLW5>xdPGWmZf5?nKB1Ms9Z8m5qiMAP2vMfM3kR$`C;Y%l%EbgIsB8s@5Vof{a}Fg zFGVlPA1OZ*RYu+yMBw(f6S36fr2Jml)m9BwTDNt_Efx8Dr&P9#des!=C1;m%*ozJcmit1QP zx@b9Rbzx;bx0Ih%o8oacJv~iD$TD8Ln0u|*l8ug%z}>7dQT{Exvd-MgHCMMx(%W58 zcg|Cw*9|SL&Ro7+$YmE=in&d^7kI+%nufN@43WwTbIP{v9a1E&o374gc|*?IR<+=3 z5gps9SF4)6xop+#GOr+}R?(fBp>2lBZ|aUtUJaGob#i%!oeL|40yW-dWz8sG-C(Bp z(7t?*)kO6(+p&>YYIIcInHPjuFy0b9f~r?Ed1}SsP_uQ8?p2-L3X8Tjx?bRm8)=zy ztg4w5bs`_x+AV&NWme`9w#3#1N;2DfhGVHrM&7v|Br#`|^X8h>=tGUvh>%4?*b7F5 zvx|v8?UXmD(XUcXYL%c#yvONP_LgM|dmR~5uToE6X0EH7 z>)nUXY%dRa7P5zd0d>Mh;f@FYP^nXScmEHyqZZ(r)5dR{l! zZm@Bt&77eSqD}c-d8)G)bF-JcLH{LnEOV;WE-xaxM%noB%1W+TsTDfD(JC+3s-!i?k=6{^j0FClj#nV}JFe=NQ!TS` z;3lqXg9EAR1zFO~3VYLe!=N*{l2&&uFJ0;hs5IVF9N&4lKbCs&Mfu%SFHeN%7`PYu z_rtxt^EPng=hPGF7Q%F<1M5Z2*${bzux35(Dh+$(8#FVtD*9(EG_E)O(dI>}NP~>E zruxPBJ9k5O8s2Fs@hzfA2~$FIxYrHTH98|uZOd{!RW@TZJT1~CEs~b7!f7MWwp+D@G|>*MuS)6 z?y=C~5jd@E8%4)<$D+phVVjsH!Ce)+6VeRdJ_rZoUIY0A8(yT$Pw8l?3ol_?bT-nn zgg4IjEb$$OPskm|X!>{iT3;-6`m}tTi5=pdb@9>P5BF$KH{6d0S`pr1TAK^#grSGH z?l8MU+~y&ER^3Z((L8B)c_?Urp_?~)7U;=#>yR8vojfUT)jQbonbO$}b1xYQA?pe6 z1FT-*{n$G{YfK`oko8VfcpNlyGA~62Qddsiy?6Tt2S$6ILD!X!rRdL|M1o&p?%{v% z0s#;J0T2KI5C8!X009sH0T2KI5O`Dx4Dp8H`u|aNFWLnHAOHd&00JNY0w4eaAOHd& z00JHXtp6hrKmY_l00ck)1V8`;KmY_l00cnb(IeTdVnN7Vg!Oopy+PU$w6X(v2(a+j6E1x~fD0^)6`KiiO|AsJ#|u_qEwwfr=35}`8xfVyaLR;$qcK;oa_9# z?Y<%ONN2sK(?jR{Ot03FY@smbz0>J=#n<^m-*v9!Z7+{Mmv5mVbzHACb<<()X`Pqj3s(7s7bd35+W6RNr995c<+E$gkByyW=T^0;waVG)=UI7Do0w!%Yip|$ zWAr<@diDkF`N_)o*!a20amG9w`BP+B#RIY1!0X(&`Uw45snO&1*b*yScE!u^pZ8bd z&wh4vPQ-`7A0t^Greu*nNqwB2&uVP?=;~(0^3{Jfr5wfryG2hLU1ww7Avhx+h3~5` z$l5U;kFxR?68+!>0w4eaAOHd&00JNY0w4eaAOHd&@W2tsMd_~8=e>6{#n=D)lpjmV ze=Gk&|L_6<5C8!X009sH0T2KI5C8!X009tq@Coegi%Q32!=k+(CB8Akbf=U^R65+q z@-l|ysI*^pHt6fwD^q;^zfbvlN%=43pXeW6AOHd&00JNY0w4eaAOHd&00JNY0uK#= zc;EiGe}TWJul;sFU$QTFGa%x9{$Kf2q943K00ck)1V8`;KmY_l00ck)1V8`;9ytP0 zDJmrrc>ez*=U6ld1V8`;KmY_l00ck)1V8`;KmY`~2>9#&A4tjvT_R+F00@8p2!H?x zfB*=900@8p2!H?xJO~7`@*rQrmnW`{X4h=JtUdRNwV|0Nb5`s2`rB#mIRGsE%``Jl zuPmoe(f@Vbgr2X;1W=-$&|4$Bm zOH%$;`JVEcGLrnyorl?Ag6x!K zc`rd%XGv2-%iHN&dWDpvzp17vm)XoLv-Bx7tr_~dS*1sUbBV#$-1IxM(c}HsU%zrl zpz_enXHz-TcZ^gXIq0iQG_B;JaWK29G8;MCKdbOU?eP;)1YYau`XpDgF@CZXGd!j$W>Qd4TI^clCfkN4Te->E{}#lY3#=7_pj;o_i(g zNe`ENsVq~_Muj~q*s+5Bs@bgu`(Nqn*3~C;)m?PW#QKj#JYAWF)t+4u?xVoiksaut zkf~Vw@F*2K+#HpvnpxM3Wy^NwTkjSvXEz(4|Nq)e+h{NdfB*=900@8p2!H?xfB*=9 zz}JvKLiw4LkUx@?l=A0^AB;RZ^yKhQ2EQBsAoha+*1r_JNTOG$$UVH2pUJ zXd-!q8G2QBnJpArb-FwIM%{8r_sr$XgYCxyG`d?WK7d)aD!REYQoo@&7nxRJwj-#Ux&7@#EHybPzgG^rrpRwMJEgK^)T?I0 z2W@#tp_|%@^l}#ArklXu1*yDREW%l#r3u7$^jp{+7Qq_RSOw{`E3A~7uq=xmlZ z{CoaXK}@aUFQ|mdZ|aUtUJaGob#i%!oeL|40@0=w2F)m6-C(Bp(7t?*)kO6(+p&>Y zYIIcInHPkZKNR?Zljspty`sreD-MU6t@GVauj=epShTg#^$Za=(lX~5-5jN;6Zy#2 zZt;UGvoen`@i>&(-ZLEI68|YI(;B*PIEOMOncsN8y-;!^~8|cc!kXwp7``>bJET z)Yq@7WF{}~m(d)@D(jldD(WrW-S8wqO_{C5cq}zOE^lAzpvvdK-C*NPn>j-vM4R%v z@>FLp=4LN>GxSU9Smso#U0y_X1xHJ4-7=Y-Hyu}_#mFY#-O0I8F0*|q7E4{YAn#o5 zXj0beaTjT=)Ur2cY|^r^8xI2EU`Rs+Meo|?hWoj98mqzTs%L?kpKCSHq;n3x`6~+pMytGBtCH3nM_Q*d%}C(?>39WlzvHTYIn^>72X5lJHaL)~UXUf-tgtto zHw-#=DQR`r^3tWAfJ);%#qph&`(vpWUzFcX_3}iBj)8lje?Q#YJ8uI=eoj4+ZXrx( zI(^9u;RW|+cA{ZOZ_heJqqA|_%bohsERL#r} z45YLd?=w0=<07_`jK)$^Q}Ue`g%MG5TvqFh2QRa~XEb<4?j8#*9{eo6aa!3nijM7$ zMQQ*1uuV)C0J|!9C!`s^eGm@Fy$13LHoQogpVHA(7hb})=xn5C32&V5S>ihmpO8C_ z(e&^3wZ2&D^lAAv6FbB^>*AxoAMUi09k}S~^wj-$pcUaArnR|%P8fQK>khxl>S$QN z1N^MIm)xRx((dw5&;Ua>Z}cqClkL_aIhHzkQr@a}u;nvl<7&1!u{x0uvR>Ky0IOGc zKlaYg8k0yXWW5s=j;ooIc_}iGx^nXFz1uf9Fxq<=eRM%T$S>-#{(ld*Hc$ivKmY_l z00ck)1V8`;KmY_l00bU#0*UxX(vb9ylw^ZHihU9*47@+Er++2-N0GhKJIQxPem?x; zp)*6rzxYOaHy?KPredjVR=#sOaF?aTm)M+=<1ez6)^ui6{JT{m$y06jUYc781S)zh zr|sTSY;HSfE=}--eP4RFm1^;7AXTSDX?H`ss&?1)>d(8}yQUi<+ZnoZ$wjFrPzbW5oTi#{W<`y%u zz)L<6W}heRWg!`1uu#C^^fOyW_Qq1rJ=eZyP$Gj2=8u(ueK20e<6$;7DttLsF7ENx z@wR`-YiPr%xi@K|Fg2qTp1DtGZF|MfZZTispiL^_;61|@L|gkZ+Xs|b>ZO-DHXpSt zYpU*SYBPUt?}jyt^=jLO<+!3jnQwj+_d~t{7B=QoXd@t<{)iOr8q+;g@y!v`ySu7o zBq{pqpKEL8z<~kp6maL@GhPq8Lo3)6)eWH8*}@XvSJsg{B*fY*+>55o9Pgv=Vsi9g zfvWECy-GBd2~XH;-7M=h+DzGng61%-Y_v83b~7X?&0Dk~QmlHn@*>|%8nOqgWnYoK zDfA+r?Tb?{3NE+@WxI<-C3f&s++ye5Bw$O&idXi-q4+xLL$VHnLJiSTlz9P z{ZDyYsP0S#aS?c}yur7$HHigs4hW$I3iRSe=V-M1i4=Evj_l+@;rP~&g`F1iddvD- z)bB-|flFy56djOuO|G+xOkdw1v8b+Bpw}5LE%MJXgV7$fv^qySx{LYwTx(xj!`rlV zaOcUSwu`|1sr^r2Mn;SIQqM?yapD6#Td`R~I{#5y%@>|LsrLHh#Mae0X z$}!~$r9b&U$&ZqMm;6!k&ys(T{O#nOWIf4}E6H4Pk}Bf`0w4eaAOHd&00JNY0w4ea zAn<4qNS+y{kBrNM34XOZe5OBnI+#8kO`Zy-PxU2F2Gb`a$!7!Up=XaIGr@Ewojegt zpHP#d!Sv{%((z+Ql1GE-qv_<4VETxfOb65HL&?Lz^x=caJ%My|Pa>%V z)5=IP8B8aKl8InC5l@Z;(tRU|whs9#%NC*qfzwTX)=2)^)Y= zMwOYaQ(Dsvmaov;ZrQW5OSx<@r{)*ta<8e~^4+@X#1Y0)TK#zD;u!G z*>p;aMtxm3=QLL<)lL14IxF2^oBpBN#hHdND+~E6EA2M57Yyi8Ewf=(dyx}0@2L68 zDV2_v*`7WcOO21qw-315s=ZRPZQ))V!ZVBdc=d&w?qB=&|Gve%Z+p5>nYBBd(QEe7qSSb{! zM%k*HuDI4*T3uL~&n@L=Tb1}RJpW&0Mvd8}Io+ul+GbjvDO{dum1?LK6ntJ#GP5Hc ziKQkdF zp&u7v?gh50nRU%rwrmQk&iEN;)6>&r*S&|9$Xs#5M6o3=RAy!;ck{0h7){$tQ~1k)C;VqbBV2qL$Pcbp)%XLm9L1a*_K6qnz?+r zkjpN#dN7aenr73G9#+rYtTD01vSqqdv4hX(D5%HN~oTD=%uc!#iGg32eS?Bj)RDXbSM`d!A(p#( zmCZ4Oxt#7CU27Hd^SRbU-taarv{ExHtrA))vvqENEOq*{yzQTcH^!Bv++v}0kzeuo zUTCGC5&EDd?&X29RS`n8>Ta#>hV@rw+m&eRJFnd{*>{Q5tt{IR6`hw=}~59tkFAOHd&00JNY0w4eaAOHd&00JNY z0$(wKebJbdN+h06c=wrKrBwyGtGBN&n3=2Su4OxqMZC=Ye#va5N;jV$lY@DeZ<$n? zU;j&czT#%#@E`yJAOHd&00JNY0w4eaAOHd&00Lhs0bKuosd%9r1V8`;KmY_l00ck) z1V8`;KmY_D2m-kNe<0idBR~KIKmY_l00ck)1V8`;KmY_l;7cXIum5G`Ly3Oy0s#;J z0T2KI5C8!X009sH0T2KI5O|0PJSz`LaeAGipX(7aye`w9g3kYTrv2;xk0qt?eSi;9 zGhr$SfB*=900@8p2!H?xfB*=900@9U2!W(L%+Brqb!=?;K*1Awmohj