From 5209d15d1c2d12346ba57fdc41267ffd4e45a1f0 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Tue, 19 May 2015 13:13:09 +0000 Subject: [PATCH] Prevent problems reported in Bug 56574 by ensuring that Cells are properly removed when a row is overwritten by calling createRow() with it's rownum. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1680280 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/xssf/usermodel/XSSFSheet.java | 10 ++ .../poi/xssf/usermodel/TestXSSFBugs.java | 98 ++++++++++++++++++ .../ss/usermodel/BaseTestBugzillaIssues.java | 55 +++++++++- test-data/spreadsheet/56574.xlsx | Bin 0 -> 9915 bytes 4 files changed, 160 insertions(+), 3 deletions(-) create mode 100644 test-data/spreadsheet/56574.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 189922ad47..840ed624d8 100644 --- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java +++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java @@ -602,6 +602,15 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { CTRow ctRow; XSSFRow prev = _rows.get(rownum); if(prev != null){ + // the Cells in an existing row are invalidated on-purpose, in order to clean up correctly, we + // need to call the remove, so things like ArrayFormulas and CalculationChain updates are done + // correctly. + // We remove the cell this way as the internal cell-list is changed by the remove call and + // thus would cause ConcurrentModificationException otherwise + while(prev.getFirstCellNum() != -1) { + prev.removeCell(prev.getCell(prev.getFirstCellNum())); + } + ctRow = prev.getCTRow(); ctRow.set(CTRow.Factory.newInstance()); } else { @@ -2614,6 +2623,7 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { } } }); + for (Iterator it = rowIterator() ; it.hasNext() ; ) { XSSFRow row = (XSSFRow)it.next(); diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java index 5658f4c12a..60f6188a01 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java @@ -34,6 +34,9 @@ import java.io.IOException; import java.util.Arrays; import java.util.Calendar; import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; import org.apache.poi.EncryptedDocumentException; import org.apache.poi.POIDataSamples; @@ -87,6 +90,7 @@ import org.apache.poi.xssf.streaming.SXSSFWorkbook; import org.apache.poi.xssf.usermodel.extensions.XSSFCellFill; import org.junit.Ignore; import org.junit.Test; +import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTCalcCell; import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTCols; import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTDefinedName; import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTDefinedNames; @@ -2502,4 +2506,98 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { wb.close(); tmp.delete(); } + + @Test + public void test56574() throws IOException { + runTest56574(false); + runTest56574(true); + } + + @SuppressWarnings("deprecation") + private void runTest56574(boolean createRow) throws IOException { + Workbook wb = XSSFTestDataSamples.openSampleWorkbook("56574.xlsx"); + + Sheet sheet = wb.getSheet("Func"); + assertNotNull(sheet); + + Map data; + data = new TreeMap(); + data.put("1", new Object[] {"ID", "NAME", "LASTNAME"}); + data.put("2", new Object[] {2, "Amit", "Shukla"}); + data.put("3", new Object[] {1, "Lokesh", "Gupta"}); + data.put("4", new Object[] {4, "John", "Adwards"}); + data.put("5", new Object[] {2, "Brian", "Schultz"}); + + Set keyset = data.keySet(); + int rownum = 1; + for (String key : keyset) + { + final Row row; + if(createRow) { + row = sheet.createRow(rownum++); + } else { + row = sheet.getRow(rownum++); + } + assertNotNull(row); + + Object [] objArr = data.get(key); + int cellnum = 0; + for (Object obj : objArr) + { + Cell cell = row.getCell(cellnum); + if(cell == null){ + cell = row.createCell(cellnum); + } else { + if(cell.getCellType() == Cell.CELL_TYPE_FORMULA) { + cell.setCellFormula(null); + cell.getCellStyle().setDataFormat((short) 0); + } + } + if(obj instanceof String) { + cell.setCellValue((String)obj); + } else if(obj instanceof Integer) { + cell.setCellValue((Integer)obj); + } + cellnum++; + } + } + + XSSFFormulaEvaluator.evaluateAllFormulaCells((XSSFWorkbook) wb); + wb.getCreationHelper().createFormulaEvaluator().evaluateAll(); + + CalculationChain chain = ((XSSFWorkbook)wb).getCalculationChain(); + CTCalcCell[] cArray = chain.getCTCalcChain().getCArray(); + for(CTCalcCell calc : cArray) { + // A2 to A6 should be gone + assertFalse(calc.getR().equals("A2")); + assertFalse(calc.getR().equals("A3")); + assertFalse(calc.getR().equals("A4")); + assertFalse(calc.getR().equals("A5")); + assertFalse(calc.getR().equals("A6")); + } + + /*FileOutputStream out = new FileOutputStream(new File("C:\\temp\\56574.xlsx")); + try { + wb.write(out); + } finally { + out.close(); + }*/ + + Workbook wbBack = XSSFTestDataSamples.writeOutAndReadBack(wb); + Sheet sheetBack = wbBack.getSheet("Func"); + assertNotNull(sheetBack); + + chain = ((XSSFWorkbook)wbBack).getCalculationChain(); + cArray = chain.getCTCalcChain().getCArray(); + for(CTCalcCell calc : cArray) { + // A2 to A6 should be gone + assertFalse(calc.getR().equals("A2")); + assertFalse(calc.getR().equals("A3")); + assertFalse(calc.getR().equals("A4")); + assertFalse(calc.getR().equals("A5")); + assertFalse(calc.getR().equals("A6")); + } + + wb.close(); + } } diff --git a/src/testcases/org/apache/poi/ss/usermodel/BaseTestBugzillaIssues.java b/src/testcases/org/apache/poi/ss/usermodel/BaseTestBugzillaIssues.java index fe11df6589..8b6732ca45 100644 --- a/src/testcases/org/apache/poi/ss/usermodel/BaseTestBugzillaIssues.java +++ b/src/testcases/org/apache/poi/ss/usermodel/BaseTestBugzillaIssues.java @@ -18,9 +18,12 @@ package org.apache.poi.ss.usermodel; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import java.io.IOException; import java.util.HashMap; import java.util.Map; @@ -637,7 +640,7 @@ public abstract class BaseTestBugzillaIssues { // Next up, SEARCH on its own cf.setCellFormula("SEARCH(\"am\", A1)"); cf = evaluateCell(wb, cf); - assertEquals(ErrorConstants.ERROR_VALUE, cf.getErrorCellValue()); + assertEquals(FormulaError.VALUE.getCode(), cf.getErrorCellValue()); cf.setCellFormula("SEARCH(\"am\", B1)"); cf = evaluateCell(wb, cf); @@ -645,11 +648,11 @@ public abstract class BaseTestBugzillaIssues { cf.setCellFormula("SEARCH(\"am\", C1)"); cf = evaluateCell(wb, cf); - assertEquals(ErrorConstants.ERROR_VALUE, cf.getErrorCellValue()); + assertEquals(FormulaError.VALUE.getCode(), cf.getErrorCellValue()); cf.setCellFormula("SEARCH(\"am\", D1)"); cf = evaluateCell(wb, cf); - assertEquals(ErrorConstants.ERROR_VALUE, cf.getErrorCellValue()); + assertEquals(FormulaError.VALUE.getCode(), cf.getErrorCellValue()); // Finally, bring it all together @@ -757,4 +760,50 @@ public abstract class BaseTestBugzillaIssues { assertEquals(otherCellText, c1.getStringCellValue()); assertEquals(otherCellText, c2.getStringCellValue()); } + + @Test + public void test56574OverwriteExistingRow() throws IOException { + Workbook wb = _testDataProvider.createWorkbook(); + Sheet sheet = wb.createSheet(); + + { // create the Formula-Cell + Row row = sheet.createRow(0); + Cell cell = row.createCell(0); + cell.setCellFormula("A2"); + } + + { // check that it is there now + Row row = sheet.getRow(0); + + /* CTCell[] cArray = ((XSSFRow)row).getCTRow().getCArray(); + assertEquals(1, cArray.length);*/ + + Cell cell = row.getCell(0); + assertEquals(Cell.CELL_TYPE_FORMULA, cell.getCellType()); + } + + { // overwrite the row + Row row = sheet.createRow(0); + assertNotNull(row); + } + + { // creating a row in place of another should remove the existing data, + // check that the cell is gone now + Row row = sheet.getRow(0); + + /*CTCell[] cArray = ((XSSFRow)row).getCTRow().getCArray(); + assertEquals(0, cArray.length);*/ + + Cell cell = row.getCell(0); + assertNull(cell); + } + + // the calculation chain in XSSF is empty in a newly created workbook, so we cannot check if it is correctly updated + /*assertNull(((XSSFWorkbook)wb).getCalculationChain()); + assertNotNull(((XSSFWorkbook)wb).getCalculationChain().getCTCalcChain()); + assertNotNull(((XSSFWorkbook)wb).getCalculationChain().getCTCalcChain().getCArray()); + assertEquals(0, ((XSSFWorkbook)wb).getCalculationChain().getCTCalcChain().getCArray().length);*/ + + wb.close(); + } } diff --git a/test-data/spreadsheet/56574.xlsx b/test-data/spreadsheet/56574.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..9cce54cc8ff3026acdac6616da1d225a339fb3b6 GIT binary patch literal 9915 zcmeHtbyQSq`}R;H9YacYcO#wB-JJr`FodKa4U$76NGM7Oh;)Y_DIqN(-3;CE8-36F zdXAoVt?#ezpZDywWyE9ign)<#00NK!000%B$2Tp(2Mz$B1OfoK z0AzS0X%}a(jWgIx`-!WKhY7onlOtt5B0S?u06gsae~$m*Colk6Q0(Ny>)uCN$Xn5! zam1Ceo@0M)y47|8;1V+FAEjNf_VDvls}le8hNLrOp7(>$`ceoW3h=VwAJ(ymz91*~}l=?3@0 zAE9nL;B1V4uE%w!T5G%4JWrN3fyhaP@f=8kL6L{8M3z*<7cigXGyEy3x2EEtFT+(; zPZN=Pm8Brgc=Uy|+>1(Jf8RF@3(oni;el(%ifzAFWd|uC*3AAsID8h+>b2Yn4zW@b z>mpf0Mkftz5G18P(~DAiU6PR%PO7X*@KN=fft;(*AXYi@h`=7o!OMJT80oX14!5g^ zq`|jAiw751G*R5Pn1D_>1h1xrvm(CqwXi2GqL{WTa#>A(kZU1t(^n{Cz{_4Eq~ecRJ^mB{Fk0T-Apq3> zLfC2@PMQ;#7b?T}hz299nY)dn2M7E2^FKuWA7d z?^UC+d?S@Mc^mB>MfLLnT8XpMP=pXgtJ#s$_0Qr)^LuOd+7zUg9IBlSjFOc+mVT3u zeadomv0hqI+uGyx5XX^P=*`VLn)wt$vMeXB-n@^AA#Q+N(^&G7so|j=?6|*5C%Ra|^H)&eZkRAVJ_aE1H5R;3@I zz5P7_83m@6}1&Y4*iU=+VNeDpc}PP)Uf>Gk;p={hs5@FFng}u+w|D7erC-;O&VEi#a1OSKu$Z$T69RDJUw~M=jrHhNhcVYWC zY2aY$2D|ou`)GoER_x-$?%ogC$6j+D^YX->24h86=$TMZ7LL7k%z`LikJSp`DX_7g zXr6-7?;i%VDsxZowl<~)1j*QN;afhRnyXsH&4@GQh`EB@5r<`xioRzzF~E*9EiuG;>Czm zCtVNcMS8h--C9#tEcdNShk}eas5>Ocz?=H{#C$&PIHy4aUMfVRqZSuyCPYY?4LnL5?O)4Kmm~O< zP5rBRyOmCB9cHdLKMU5!Q&CL%Q23ux;fepPYjL2OZFs@lB_6f`W5KR~xz?YZDppg{ zxq}nCNpFjSbsQJ*q}v4?w|Y9vr2;Ulq+F&4I3UMuBOBr`VQ-uYyExEKh1j zKHS2gv%Bf9%ZQQHFBdv_LRdb0rl6HykF~Fflpqw)JzC@HVhyQ#9q0t<-t1?*ufgBS zR_~5zfKZpB^^koi9ghx#I=@lT9aio;rB=mD=Izqx%8zOoRJ5t+KEq7olr_fNS@C)D zar(oNPl?;~fmW5R~Ysg5#qM>=h91ffH=Vji5I{1s{1mRU!6FT=ahKDPWf;B@oJ(&MQHF4de=4u| zo~U1G6i6~L9w9>=Nou7C3jS>lY5^9gt zfRFObe75}l%*55c$;T{aS{%s=vDB=(-7rPEtd!_u3!?sh5h|_$mAc4^5Ir32Zg>i~ zxk#Of;_*jx9lWRNu{4x^e%yItlF}-pvwe|gL`~g_TX4Bi!SpKG_(0*D30|oon=}+~ zq`@qLm{lMzEfyq#C>51{#&rC;sc&K&bzYf}tJ>1Z!errk)N?cB06YlAYiG1I+@3Vj z^K!ogwteA#H@WTUSDl>$XTESxrHoZJ^W;{pEO~_W>CQ6uu5-0US>rkU%?VwG(F@h0 z5k;Fy8H`V26KPPA@karEgY6^<9|cmOCqrJfvtTID`e2aG|4y;Q!21A*$4#u3HawGe z6R0Q}p)5Gd7Ulx*FUag5WQ+_KY2b}Pi61Z4kAViTUjl|D!ttLcW-?fZ1Vk>e$H|3Q z4gy?2#_5&PEyl#7O$o_a7y-+rDP{|g{gdW+{VY)r{ z){{73L@>>r^R^}nwR~-dvQA>L0yq=2C1Y2_aFsq{*7p&NdFH-msoOn#oOoyqhgyses5=sU;SW~ZsHz9ic-nM%{JE3DKFGLe`w6zh+N1SjvL zT1$rKjm+}&Ru4Z;uF5inGnbKoW*xGIgbk;C)EKQ0_-t2R^ETIpV%U9+kZc|zeV`DC ztt931B?BFu)R?lu;2EMGg$6#RRJO!|=@DX%N?KSan!GdnL8w|!ZFFwJ6kMMHuHYC95y&jP>{NCMjR_UJwwch#;gP1 z1Tp!c+vq91t9Ez~BV4)CoU$cBTR?JoD8`jm^0f6uM^>3KmjKFR*DH(Nmck0HdmyTD z%+x##44#y5Knx!P%bg*@fUgEBM7y9$p|S<89a^pc=LI%VqQ_k-$*)4AnejU1V|(TQ zI+7@KnTFSr$L(qR%>!-8T&@(?Weetzz~F5mCnug&f_2`_=rl{9Z4Z}{RCcP-i+lrm zqdF$}di1q2O7aa|=0=UZ>*91sE2U1f#kIJW>^_4lQl;1gIrN*CzIh?ft6jG|DNG$k zl(!3n!-DBEa3k<~O`I5#NZw!8S}7wgTL*=dIt%%-X~*F(5?)qNyj913k7d&P<);cco?gCn^hR6ND^Zmos(bJm+EsLm>K|^>zKeJ@7gF+zQ^&;9Kj9 zc?XZh+&zCs?q{0LWc(N^kM4}Snnd+?ioKc*a(0_#Wske^2a9bD;092QBw19hCh@*y z8oAQ|###bOHrJ%k@4A#^`TM$ZE z<+L`ej9g~0D6H|PrQIU{>Za&cNP2RbcOttD_eop^6bOrt#5N$=7S2*MWJrzSqJ^8`c|jiCEdZE{qLcLl4|XVCM>Bi#{aLY!t;Gq3e)@D3k31P&rJCCB`UWJK1fbQ zS)q=tMlp%M{jO-w+6$uIpa$32tQCOoh5RfIbHXZY~=9QIzB}9eyU#is&Tmq zQ#77VSksa}kMHfGid+`Rqt2;@yo4k~>q5i8CwM>Ck8X&imM(OV2srp&gvufFYf9k~ zTOk$F+Hg=!zr(~xpEdfsuEAZC8~QvOj7SyJm1_+tlOpAgZ6{fhCMJn9gv@V+R{bPM zW@`%4_Xp1rT}@{-bdEZVd?ca^c<&)~cok7ikDBB`>GkVONfn+2uB_yoyd#SfFjJ5O zscI52Oyb+iX%{jl385V(X$4-J%uJODuc!HkSG^hwbm@9g5%Hgc3pE0 z=SEGG*FKpkuLGvByUX1r!7o~ zU)pWCl#kv1+!RM1=eZv8*TE%@+`yJwp2PQoGd+xlr{>1+K@uXC2rP~-%@IVs9*4bP zo_b3zT-7mzx1$^%R#^=_Rc_DOtX$Wf@<8%?*E3XB;ejMPX(juORJ@%XZzqhrv7!Su zO2wz}S^9hD)yqc)-QJ#Ga;$g@xe*Iw@IkvhDVy(qnCrRas_=g@O}%nO<}ANe(jOIvy3DuD_+tjgBn9HNkYv)8*itP9IHTVy@iEp510}?mzt+O zL{^)gvbG;?%@&<(Ei8wKNgu^%4W^jQO$G8zp;9)Upk^A?0UfIx43m1R#T49BeKy!yG4whyLC;cBM&*m_qCa|`R}X~FNJd~Qy5MKdD=ECU=E@{CnphR3DH0Jolw zrOvf0lQ%fjvPilcaZi|-XU&Z#Des*sL^n!>bk8kZxPIEO4HN~BIO75foD!$o_#5BQ zWf<=y;}M31t~Z_!l;2|qF4=CLvlThcpZLfiQFOnbg$L$8A>5usH$|$z3C-QQK@V^4 z)VeRS6EkPZhCRp^usL|PY0+pibk*jpN0-$q_x1#R-9^0Qwzmo5!KHnSt8*BoE%OJ)ytfp8w-5b4sKbvu z=O1<ngOj1@= zA5tJ|CqDwA>Eyg%5_u$Y2N3LRzwg!~D;LA#3uorjcggdjweEX8<9E%|^3E}|hD9Ng zNB{uFAF~2KakTj{okDGu_$*HBX1%ST3V$_ayHR?a3e6;0L;FTUrng^8;F9&Xv8=P4 z?8firxv-@^L^zM5k;mKSEQox@o_q01qI^LAoldOn7=Y%ryNRo3>_M&c0|QHIG94s* z!%8B7fu+Wk<%mxWauqjGToC}NX`n_@AEfX#zhFAPa52i9{DpM>zREQ~{ zf`IL|)J%GH()#_^qLfLQX$_exhPq!4V$t?83D9UV$|>R$NitDh`sNJU2evy4xGd6r zeqPf6)_O|Q!1`op{=IP`K3pL!bG19cYJ)E32Y9=n&P&-%3g(lC^V70)i8oQEZvmgt zlWF^^hk-)sz{qNHS-NfN8g1x+D}}CRuk2X8^uFE4on_jH(WYb{@*7L4C^W7LR8crr zTAVk_=^9Lq>Mj(SsPFWk31rG7XK5P+d4sIRu^@Rr3y{qW-msvaDEm8FP$^jA3v{`+ zTvZP`_`&0)I|x6UygDCVc{CUu2ubFlI&GovordKblneUk zgJd;=;W$wa$hfiKJ7B6FKA=j_%oF3f>VY_W*-|GPrAxb_A(}T4rj>tn>-&&NZqodm zixMw3nb>kK!s@8{CJ!1xEgoGfu_FvW=3$h&X<-~pff$Q*oRpU7n*lVK`$|y!Av1j-xkuY|272M z3VZTN684J*%q4MPwOwl$D|L4lR}T&=7k8WQ41@JS{we9gcn|6-NiQJuuiNeT5>4 zV`qx?)>~)>QsNUwy`Di(TFkeyfsDx(Gn*Al(&vCyUc+zRj7W%-klHP$k(S3KdUj8&A7PCmSa}0?vzs5G?s4rxHv`!EjVOyM5Qp z!t>AD?abcQd0KL+bT=cR0xy>0rbEt_7R3aGh_YTQWZDzbtf(FIT&|Y+J!bW6WWjv% z39;xa{82RQ|G`1?DeauU;k}Y(dR5yD-q^* zvIqbG_MiRE!qxR3w}biHA4hhwq{A{NcGy1J6-Dgc^Xx(h-6v)`r%j7A3|gnp#UgRC zHVSt1^qb{o)AUhQyKieVp2YX8kLZ8hBgv}99`Vtu*rc_+FSx6;T!Z8{FJgLQc|Xbh z)%$tLcFd2ObCxRnOTE0#5Ynb_ST}X@OE!P9zV|*JdKMY-H zB_n8#_?Vse!6##=8_R^Df&OcA1Ev_t^Cy%!5}ry%Ot>1+sRV3xhg{M}i4*)*(^*m* zuBa2mc94S5@#*l_C4==ZtSlU@r0p#1oxi&=I9XCH^!xJB-$p#zbG9G%QgD^y|z#bcNIBkLI2dYQ!cd>+4tJU)wN0jmJ^{ zwkh)~z^_TpPk>CE|NQ%J1+iaIel6MjMB#)rhyG~a{Tby)0q0kg|9&rZgnx_jBmMdn z<<|t^=e?-j|AF#by6`K)ufh0F1X|*MA^aya|JC%bfzwYz0HBH#0QfD8`qlie`uAsZ u7Fdes~gdL|Mq{itJHr0 literal 0 HcmV?d00001