From 425ab7b6dc139a1e843ee5f28829841f99cacc52 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Mon, 9 Sep 2013 09:24:05 +0000 Subject: [PATCH] Bug 53798: Add fix for XmlValueDisconnectException during shifting rows git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1521012 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/xssf/usermodel/XSSFSheet.java | 30 ++++-- .../usermodel/TestXSSFSheetShiftRows.java | 91 +++++++++++++++++- test-data/spreadsheet/53798.xlsx | Bin 0 -> 9383 bytes 3 files changed, 109 insertions(+), 12 deletions(-) create mode 100644 test-data/spreadsheet/53798.xlsx diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java index 37ce7fba2c..fe985ac71d 100644 --- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java +++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java @@ -2341,26 +2341,26 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { */ @SuppressWarnings("deprecation") //YK: getXYZArray() array accessors are deprecated in xmlbeans with JDK 1.5 support public void shiftRows(int startRow, int endRow, int n, boolean copyRowHeight, boolean resetOriginalRowHeight) { + // first remove all rows which will be overwritten for (Iterator it = rowIterator() ; it.hasNext() ; ) { XSSFRow row = (XSSFRow)it.next(); int rownum = row.getRowNum(); - if(rownum < startRow) continue; - - if (!copyRowHeight) { - row.setHeight((short)-1); - } - + + // check if we should remove this row as it will be overwritten by the data later if (removeRow(startRow, endRow, n, rownum)) { // remove row from worksheet.getSheetData row array int idx = _rows.headMap(row.getRowNum()).size(); - worksheet.getSheetData().removeRow(idx); + worksheet.getSheetData().removeRow(idx); // remove row from _rows it.remove(); } - else if (rownum >= startRow && rownum <= endRow) { - row.shift(n); - } + } + // then do the actual moving and also adjust comments/rowHeight + for (Iterator it = rowIterator() ; it.hasNext() ; ) { + XSSFRow row = (XSSFRow)it.next(); + int rownum = row.getRowNum(); + if(sheetComments != null){ //TODO shift Note's anchor in the associated /xl/drawing/vmlDrawings#.vml CTCommentList lst = sheetComments.getCTComments().getCommentList(); @@ -2372,6 +2372,14 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { } } } + + if(rownum < startRow || rownum > endRow) continue; + + if (!copyRowHeight) { + row.setHeight((short)-1); + } + + row.shift(n); } XSSFRowShifter rowShifter = new XSSFRowShifter(this); @@ -2625,7 +2633,9 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { } private boolean removeRow(int startRow, int endRow, int n, int rownum) { + // is this row in the target-window where the moved rows will land? if (rownum >= (startRow + n) && rownum <= (endRow + n)) { + // only remove it if the current row is not part of the data that is copied if (n > 0 && rownum > endRow) { return true; } diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheetShiftRows.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheetShiftRows.java index aa2ae22809..a5050c92ac 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheetShiftRows.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheetShiftRows.java @@ -21,6 +21,9 @@ import java.io.IOException; import org.apache.poi.ss.usermodel.BaseTestSheetShiftRows; import org.apache.poi.ss.usermodel.Cell; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.usermodel.Workbook; import org.apache.poi.ss.util.CellUtil; import org.apache.poi.xssf.XSSFITestDataProvider; import org.apache.poi.xssf.XSSFTestDataSamples; @@ -34,11 +37,13 @@ public final class TestXSSFSheetShiftRows extends BaseTestSheetShiftRows { super(XSSFITestDataProvider.instance); } - public void testShiftRowBreaks() { // disabled test from superclass + @Override + public void testShiftRowBreaks() { // disabled test from superclass // TODO - support shifting of page breaks } - public void testShiftWithComments() { // disabled test from superclass + @Override + public void testShiftWithComments() { // disabled test from superclass // TODO - support shifting of comments. } @@ -54,4 +59,86 @@ public final class TestXSSFSheetShiftRows extends BaseTestSheetShiftRows { cell = CellUtil.getCell(sheet.getRow(3), 0); assertEquals("X", cell.getStringCellValue()); } + + + public void testBug53798() throws IOException { + // NOTE that for HSSF (.xls) negative shifts combined with positive ones do work as expected + Workbook wb = XSSFTestDataSamples.openSampleWorkbook("53798.xlsx"); + + Sheet testSheet = wb.getSheetAt(0); + // 1) corrupted xlsx (unreadable data in the first row of a shifted group) already comes about + // when shifted by less than -1 negative amount (try -2) + testSheet.shiftRows(3, 3, -2); + + Row newRow = null; Cell newCell = null; + // 2) attempt to create a new row IN PLACE of a removed row by a negative shift causes corrupted + // xlsx file with unreadable data in the negative shifted row. + // NOTE it's ok to create any other row. + newRow = testSheet.createRow(3); + newCell = newRow.createCell(0); + newCell.setCellValue("new Cell in row "+newRow.getRowNum()); + + // 3) once a negative shift has been made any attempt to shift another group of rows + // (note: outside of previously negative shifted rows) by a POSITIVE amount causes POI exception: + // org.apache.xmlbeans.impl.values.XmlValueDisconnectedException. + // NOTE: another negative shift on another group of rows is successful, provided no new rows in + // place of previously shifted rows were attempted to be created as explained above. + testSheet.shiftRows(6, 7, 1); // -- CHANGE the shift to positive once the behaviour of + // the above has been tested + + //saveReport(wb, new File("/tmp/53798.xlsx")); + Workbook read = XSSFTestDataSamples.writeOutAndReadBack(wb); + assertNotNull(read); + + Sheet readSheet = read.getSheetAt(0); + verifyCellContent(readSheet, 0, "0.0"); + verifyCellContent(readSheet, 1, "3.0"); + verifyCellContent(readSheet, 2, "2.0"); + verifyCellContent(readSheet, 3, "new Cell in row 3"); + verifyCellContent(readSheet, 4, "4.0"); + verifyCellContent(readSheet, 5, "5.0"); + verifyCellContent(readSheet, 6, null); + verifyCellContent(readSheet, 7, "6.0"); + verifyCellContent(readSheet, 8, "7.0"); + } + + private void verifyCellContent(Sheet readSheet, int row, String expect) { + Row readRow = readSheet.getRow(row); + if(expect == null) { + assertNull(readRow); + return; + } + Cell readCell = readRow.getCell(0); + if(readCell.getCellType() == Cell.CELL_TYPE_NUMERIC) { + assertEquals(expect, Double.toString(readCell.getNumericCellValue())); + } else { + assertEquals(expect, readCell.getStringCellValue()); + } + } + + public void testBug53798a() throws IOException { + Workbook wb = XSSFTestDataSamples.openSampleWorkbook("53798.xlsx"); + + Sheet testSheet = wb.getSheetAt(0); + testSheet.shiftRows(3, 3, -1); + for (Row r : testSheet) { + r.getRowNum(); + } + testSheet.shiftRows(6, 6, 1); + + //saveReport(wb, new File("/tmp/53798.xlsx")); + Workbook read = XSSFTestDataSamples.writeOutAndReadBack(wb); + assertNotNull(read); + + Sheet readSheet = read.getSheetAt(0); + verifyCellContent(readSheet, 0, "0.0"); + verifyCellContent(readSheet, 1, "1.0"); + verifyCellContent(readSheet, 2, "3.0"); + verifyCellContent(readSheet, 3, null); + verifyCellContent(readSheet, 4, "4.0"); + verifyCellContent(readSheet, 5, "5.0"); + verifyCellContent(readSheet, 6, null); + verifyCellContent(readSheet, 7, "6.0"); + verifyCellContent(readSheet, 8, "8.0"); + } } diff --git a/test-data/spreadsheet/53798.xlsx b/test-data/spreadsheet/53798.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..f273308ece995a1bc9e9ff114d0a881337e62b33 GIT binary patch literal 9383 zcmeHNWmp_pvmRW61x*;-CAb84f?I&#HZZt*aCdit1PcKIL4s>=_u!gf!QJgn_S@ZT z*uD3;|G(|$>F(3hUGJQ(Q*TwDs-q|a1B(rS2Ot6f08#*4tlJp~3IOnc1pu%Bh|pT1 zHr9?{Yezkm*S25>9Y$9xOOhN|XqqelH01ezxBp@d6esjsw=rXgpC?|S$JXGmI4Y^) z_=*$v;})L4$nufmMp?avNA_L{ef^FiOU&k#_amuwWDGMyD4TR=dbiMl624FY5(Sxny%SH1>WQY$A{f1&v$+)rk9>P_ zCkb8IeMW{V;642q##;bd46q=BZc z3o63H@#_A5#M2#O64$l?55$Tum$2m?Pf82XbW(bo>HLI;~a$n75 zM<`jFLE;!B*!_2J#j*Y7?LYi@;GKVpC%-fDyM&&Z z1d-~_TL5V^aYs^saonW0-dDc`AxX{|#EA}rSPA&!9r+n9|M|l5aA9bww-YSqW zr-axCI%MD>_VHKCh=`T5X=6qS+K)dYCR>saqzO^2lpN=b?C=*sMV`^^nR%-9b_q8#zH4hQ2szBZT;o{Luw-f=>_2Oc>f5*@Q zi)YVAiNe55)(|Y#Yff<`NqQ&IO`5<`#KhvgI`oA1eI{kdxn=JWybt`}u^V%x5}mlk zy%3yAYoZjc{3u7Mv-3(!^!|&At!hl;-4i0Ja4=tUiWk>CM9qIED6bW(Oe;k3B9Ob} zKL^0U6byEB_?@f2U9kMg)PU&Lur_9F@pG6vQCIi)$v_lQ<>zFzsv9t%t6BKD?jY1e#(Kx4GnXJ6`}16K|4Ov?`3XBk?A%el7Zv5^?~VhqL^o8Q35 z%S(rL$Ef2xDi2hmxP8*@)iF~Zg;W?Lus$Od`kJ}-G8{NkTx4!_q=!(+Bww}vY}HfE zpNIdY#@$YoCo!LOZp)D5(QtqicZ|M3jwE-F`0kMpNO;@jEYrdh4L09up#S&`>7O)k z4SHvl01p7X2mbiR{>=m(O~F=Rrr(agz4nfVg3TN!b_@En3yGtR6?HWb!6|IIJgr0y z*kK(^WR{wwuACz@Cglm7UGafxuQy1BYS?AAq#uE9aC}Rp6xn;UGbz}W(*!U zIT0*I=IwF6v_kH5I_n>jElNhQW;gdi=c8LlhXC<7m#Mo;(5(!RC=R*XaKKe|b}?OY ze|GYEU#ByTj^eR!nLtun?QWnnWm;m`iNTZpem+u`0lC_cGC#G)DqYaTPzxcdlSLE8 zlx^&1iV@@_?(VGF0>YwlV{?5W=T92Dq_?2nhWb*;rQ^W!W=^t;41tr8978nbU<3@k z*eTExf}bQIQ_bp5+%)!0P9QJJ;;~eLtPFIPZpNH8{SF-m53pNlOfqfDxe}oXFTW)JA@uA>SCIUig6q0XCT0 zDdnQg+W2FQF(+_xDs$J&tuc=uxs_&m-)~(ipKM*muLKWA5#Kr9d>=h*CuU$U`N|mO z|E(=~$?I%mia1v=)9d)?5O2*@qvQUnox0`b<~274t10PdIMdskvz^}XQi0pshHlCO zD_;rmi)T=e5^!36b^l86J+0mPW=DTh3nv;>K<1m8k%P`$7|l*#tAYF~(J zE6^P{3O$i%Vu`J=9JGSE!cF&r5pAM#x+Y4KV0=l#!;t$L%RyIc@4)PNhtkU+!V#`# zAi)@AM5TzcIy(P-yXT@Oycba2^b4NJnB5ES@%(t%ZN)TULo!oCG4*t*=R=oT)8&|1 zRN{C*%u1M>6U>!ae;c((++s zQA;mdZSAOYYE7Irh^_>PIcJeJ#H%^usz751!(p_7+3E4yoH~5Pyg_CAPe};Z$iIcO@{~F&5=8B~M_?jp@3;I6a}WFOEzT z9f)$Z;~F-BJg@m?2WZP19v`-|ML!K*bjM->KW;q0ROC&Kdd5ga9X_fwXTdRvPjqBA zc1C5Z64XrtRVuR}3&Q>E8DAQJdaWEkV|3Y;RwB#t9Le1F+F-XiuS^*MlXLb zA!i4%+4;2I@~8;EWl6MMF`)DF^4;dPvSM4q-C*AgO9sK4bhF5aq$g=*LZTXGa3iQJzE5onS1vF~bnw+#T|bZQ-L74>2%0JlQsENb z2v^E1)F`xCBQvTnoXeN>7LwqrmO*Zr_B@VsiU}yxn5;75LPd-Uba1cO5^ab1<|0_C zA|WQUz^|6I92DI{Y9Oiuljde~u85dzT_mLmkFsZUQ@3vh9|^s%ihVp-Xmq(~;jsL6 z&%=_{|D`n{C#szBy>>?0J4@az1;N__SbZ+k`i?p z{8A3qub;#F@!RzmF(_?g15Svi?+Xr6Z*__D)gQ|%HXEOm(qfn^W~v<@yv$^-PTF~PC-A=fczpo8vHDFMpSP;a(1N#m?91{@ zJT0w7xi>|ml4KA+VZg2Azp7D$Ntdl*ml z?lOml0{|o;Us+H77RG-D@ZxD5n^{S0_~q-wGePxcrmj_>K@ru8O-beK&R&xtd>M2E zm{)3CZ;6dfo_Ryutk4Z1I=FS;z>?Vb>~hPMAu^BUy&`@2X1 zod)?ioCfi7CElmCr4>$s!}m9aXE|@Wof*IzBPb1=wo^K43jC!EQ}$Yw$#*T_R6RGRa>}scmQ&-Mz(xo5-A)KH_0`l30#m%S5+ltg zDuAPonfyuv9xDs3?BgEQ1vI{l4dXrFWv|2cmqt5o$Ema<_lX{62&EYxU2Btp8qW5@ zE%DpmuLNmM3lQ?f)^mwGI9X5c8|-lHHTbORy}!tieVC}vm13lcj^fa1M4m}fQFvp_ zUgs)^q~zG?F;=3Us%AW4+Ews$bGXSp)+j{q9?d;9S^uAKE8Y@$0u+)&j` zb!9QI%!}qH+C%yc!$FcOkAM}eE|AR=MD)%pT7a9jo`|q^gQ|WwXU;IcjgtqVm2?%u zE`*zc;3eAA;Q2O6F+s!8-L|JXqqCcns+X=GPNaG2@rMeaomsw1L7(@j7`4%XqILH1 zU>-`Ms;u6)sE8?uI04h*(#zvc5N)NWG)1Q%K%JZDy1O=v51%Ffn#J|Jn2VU{ zG5?z){V?0XF#Tv$9*hT?h1(JOE@fBCEHGppv`w^H(pkR?9(nzshpo6`&(jd_I?*fU z7_Czl`j~m~44Nry&kaRl5GmlLgkHv=Q0%WsIxi%n+8_${SkKiOI4^E4Tx$QSJ%Zkk zyk%Gwb3s-RjJxMexd=hcv^(NH0VIzlQQ~~^NS;bA_Q&qjxATxTSoH+Wlz2F{FrJTRFVGO&h!YB9=aN@o;l#tMsL zz#5#A5q?njumhXiqz${VkPe`Ha(FC|`~dwqKLnkB?057#xg(BYNJBX9&n{nl7(|o zx*rmO|C)I~tJmBX9`Au0Ju7x9#~m}{rdgr~Pp6M3~4mp^(TUZ^_k5!$lsW7sd`8=5@%hR|blJCjp?YP0CZ`_tf z(779`1G*9yBQ?RNcpZL9dS>OB8jNUlvUK#MGA&ZM_`NtE<4aJewB??QOY-2n1Shj8W9x5UqJ+a>n`Ka`lT(r zA`Ob_O6|jvY?!^yR-Lg>jg2?0{hPYny**#W-8$nj`5~Jim;r9g_fHjdj?<(zNnF!t z9UJas{O-Goy+5r&brvo13~Jvdk>9`Rsv)SxDbFX9nYfC`;D>up>KV2e9Z5pRrHR;f;)FNx2F#bzwKPTOHasS%CXzfidEe)0 zSaDBW>jAg$9Mld4qvP(Egp9N!;i_Qr-VlLnZX=#u)XkrkKD&sPl@mM?9sEol!Cw@8 zrRQI4o7Jav{du{;_Gw?VVv8@HFWn15n{3KN^Y$_KcdE z#->p7)~VUz2T4ygVQv0s1Wy-w(rtMlvSw+&QI)KyaO_p|6k)tlc2X)&_nC-dH7)8n zdaLXQmvVQPP_dc$xR58uku_P$7x7Ta$3<2EELi{D@jjSw^s{x|CxLMjQjUOd`Sadq z5~p)9O*h!0ys+AlI$}B}Q^7M$pDxNO-O(p^vY)$31o4}2X`KZ5Us{%{^CVdf-ovM< zTu(o7E5xrf+2myHk-PS{8^{W!myVQ6^yZ7Kqd7m2;%K5^pvmIQE0iD~;IgnuyZc9v zggz1CDF{i%C_yq}k0BXTBO60SdmCE^CPN#0@b3k`e-ot;SM3p}WYJE79k_&Wjox}L zW|dVLmsb`kP{OR<)wL*QGhSvYwlH>gotiiJk_JYvabtd;h{-rD*F1-~?4z|)l&Od! ztY&4!q-xY+pH-ts?6J1YVkN%4BXlrl=dBJE3RX!V-W+~eGdvz#oU?2S0b%2F6hmsX ztXdgAQ3Sgs%}&qSS!5t%vn!mda1SQ^fwujy9eTu|n3zc(HrANOoH0PsHya9WmRA-l z?7oqx*#0>X9=xtnC761U?=a7;6{eIvFQKN;^>I6nkq{JcgYCVOsTcgdt7^}M5`dG6 zEvvrVTqPc~kz7_A#M|*fc+J8HGP4HhXQ41xFqbVr;Xl4<_Kl78jW@upL6Sg* zA%_uchsUKNY4QzxD#^o|qG*g7X8nQ&U5pKiB_cWpu?J$}|1gi?l=3$Wg4{xfa0BD7 z)^A{I`>*jsRQCIp9w%%w$BYuV4|7J0vH;}PVL|aN&XMFjDKmhIb2gF7H_Wb}2q}); znToEC?b2|Wja+qiy%LjJAmY(Gi5cxw3|-5?hJyuK;GHG9&U!Xoks(uHnXef7NE1MH zmfT$kTiMIT^g`hhJTAjzP|}nNtqvSo_FMZbjpbkxGS`?~Zk8T+ZK}tMNKx8<^eF&F zxFEY(+Qt_ot$ljZlI>IpLoBc#73y16FVo9w5n@x=!kb3k6%-ci4_CUXts+|kcm0Nt z(5Y~>QB})=ub5l?c^5g&YoY=P=JqkpF7TJ4_p0bgAjXl~^(vk4Z^^<>Z0SbRc4SGq z@GX_uCIv{rw`}WpmQaX^M(ykb{dF$74ZtP>RBdhiU>~G_K$(sVa`cPbnH1T365XLt ziJ(xIHc}xsiRZd0!YQjc2EU57_l@0~X0QldQYy2@*f1Lke@rI(z zuK>RmmH!a@aZZB};x8rUhl0P>+5Rfn3Q2AM?^U;lI1lSFKasvdR-``2VUbJOq50n*RxC^yoLhhl%=!01wk5KLKblANJ^HlH?)E!%4zV6a$Deg(&6c zl;NT1uhIOcAOLU-DL4Erwm%eq=$3y1m=XQ<&;K7M{Sfq5ANvy$062tz{@JJBJ+7h* V9K?cttZ{e*ScQC(l#u?o`X9Pq0`dR= literal 0 HcmV?d00001