From 1a6fcd50245ba08acc4d29dc0ca0401b38251776 Mon Sep 17 00:00:00 2001 From: Nick Burch Date: Thu, 21 Feb 2008 15:48:52 +0000 Subject: [PATCH] Patch from Josh from bug #44449 - Handle SharedFormulas better, for where there are formulas for the same area on two sheets, and when the shared formula flag is set incorrectly git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@629837 13f79535-47bb-0310-9956-ffa450edef68 --- src/documentation/content/xdocs/changes.xml | 1 + src/documentation/content/xdocs/status.xml | 1 + .../aggregates/ValueRecordsAggregate.java | 34 ++++- .../hssf/data/AbnormalSharedFormulaFlag.xls | Bin 0 -> 17920 bytes .../aggregates/TestValueRecordsAggregate.java | 134 +++++++++++++++++- 5 files changed, 161 insertions(+), 9 deletions(-) create mode 100755 src/testcases/org/apache/poi/hssf/data/AbnormalSharedFormulaFlag.xls diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index 504029c2c..f6d2de4e2 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -36,6 +36,7 @@ + 44449 - Avoid getting confused when two sheets have shared formulas for the same areas, and when the shared formula is set incorrectly 44366 - InputStreams passed to POIFSFileSystem are now automatically closed. A warning is generated for people who might've relied on them not being closed before, and a wrapper to restore the old behaviour is supplied 44371 - Support for the Offset function 38921 - Have HSSFPalette.findSimilar() work properly diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 64b255fcb..8c21cbd94 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -33,6 +33,7 @@ + 44449 - Avoid getting confused when two sheets have shared formulas for the same areas, and when the shared formula is set incorrectly 44366 - InputStreams passed to POIFSFileSystem are now automatically closed. A warning is generated for people who might've relied on them not being closed before, and a wrapper to restore the old behaviour is supplied 44371 - Support for the Offset function 38921 - Have HSSFPalette.findSimilar() work properly diff --git a/src/java/org/apache/poi/hssf/record/aggregates/ValueRecordsAggregate.java b/src/java/org/apache/poi/hssf/record/aggregates/ValueRecordsAggregate.java index e48a0a902..068896952 100644 --- a/src/java/org/apache/poi/hssf/record/aggregates/ValueRecordsAggregate.java +++ b/src/java/org/apache/poi/hssf/record/aggregates/ValueRecordsAggregate.java @@ -34,7 +34,7 @@ import java.util.List; * @author Jason Height (jheight at chariot dot net dot au) */ -public class ValueRecordsAggregate +public final class ValueRecordsAggregate extends Record { public final static short sid = -1000; @@ -127,7 +127,7 @@ public class ValueRecordsAggregate FormulaRecordAggregate lastFormulaAggregate = null; - // First up, locate all the shared formulas + // First up, locate all the shared formulas for this sheet List sharedFormulas = new java.util.ArrayList(); for (k = offset; k < records.size(); k++) { @@ -135,6 +135,10 @@ public class ValueRecordsAggregate if (rec instanceof SharedFormulaRecord) { sharedFormulas.add(rec); } + if(rec instanceof EOFRecord) { + // End of current sheet. Ignore all subsequent shared formula records (Bugzilla 44449) + break; + } } // Now do the main processing sweep @@ -156,6 +160,8 @@ public class ValueRecordsAggregate // for us boolean found = false; for (int i=sharedFormulas.size()-1;i>=0;i--) { + // TODO - there is no junit test case to justify this reversed loop + // perhaps it could just run in the normal direction? SharedFormulaRecord shrd = (SharedFormulaRecord)sharedFormulas.get(i); if (shrd.isFormulaInShared(formula)) { shrd.convertSharedFormulaRecord(formula); @@ -164,9 +170,7 @@ public class ValueRecordsAggregate } } if (!found) { - //Sometimes the shared formula flag "seems" to be errornously set, - //cant really do much about that. - //throw new RecordFormatException("Could not find appropriate shared formula"); + handleMissingSharedFormulaRecord(formula); } } @@ -185,6 +189,24 @@ public class ValueRecordsAggregate return k; } + /** + * Sometimes the shared formula flag "seems" to be erroneously set, in which case there is no + * call to SharedFormulaRecord.convertSharedFormulaRecord and hence the + * parsedExpression field of this FormulaRecord will not get updated.
+ * As it turns out, this is not a problem, because in these circumstances, the existing value + * for parsedExpression is perfectly OK.

+ * + * This method may also be used for setting breakpoints to help diagnose issues regarding the + * abnormally-set 'shared formula' flags. + * (see TestValueRecordsAggregate.testSpuriousSharedFormulaFlag()).

+ * + * The method currently does nothing but do not delete it without finding a nice home for this + * comment. + */ + private static void handleMissingSharedFormulaRecord(FormulaRecord formula) { + // could log an info message here since this is a fairly unusual occurrence. + } + /** * called by the class that is responsible for writing this sucker. * Subclasses should implement this so that their data is passed back in a @@ -300,7 +322,7 @@ public class ValueRecordsAggregate return rec; } - public class MyIterator implements Iterator { + private final class MyIterator implements Iterator { short nextColumn=-1; int nextRow,lastRow; diff --git a/src/testcases/org/apache/poi/hssf/data/AbnormalSharedFormulaFlag.xls b/src/testcases/org/apache/poi/hssf/data/AbnormalSharedFormulaFlag.xls new file mode 100755 index 0000000000000000000000000000000000000000..788865b3b9788aea28563247e89fc0ade35abd0e GIT binary patch literal 17920 zcmeHOd2k$8ng4n;(u|MTk}V%`9J?*~kbF#J$#Hze@LMo7eq2d(mZb8KhwIr2f`2F7N zp6Pzm)0!b(YD4)=y`Ju`-*>$4ddIx^@xL{kd+5|j z!wsKb&*$?RD1zHdQEO3Y^B8Izbpq-{)Jdq5Q7=QCf;tryE+7r4 z(@>|QHlofzoryXNbvEkdsB=)SK%I+vB`W=eb%E#pO{B8FD^YKh3_dxr&j3GDp`MIwQgA}dt_Mdz$dBtegm|SRx%NilYZO}h7fH5f962O4`m2x z>U;G==c!iS0uDV&hLo~Eb@HqMf4A?I@~6JsL_Q&oD6XrO&(D7<_9Fo6@bcFZk*`Ld z#8u=ssC(rwo7;cOvPH>|v?#+_FuxX7wg4MNtw&ZN9Ja@Gy1ITvwR}Qduf?1F$X0W2 z`P5Zw(1M-4IkHK$`=Giusp}M6E%~~_`MH9u#F+LR+Q;QS8Z{#Cjagga2Ltef1MrW2 z`Tx}7AUto4JV7NCB)!9tGpQt99?-(ym*}`Ho`a4|E`A!+Si`s)YaF-5hH+bL9H)gP zorXS4NiA75P76VcaO+zb75!Q_xp2%3rxC#j8Q#Kz7mmkeLRlK~qH~d%F$y;=Y-(QK z+}yUWb^YEJ_hE1Ayyyaq^B9p`5Z>Qv{*!w4w=e?Q7Yz4_p$`iVsk|cuQ?B(o)-p(J9Mi zou8TRfuEle&BL;@v&eL{EJGv}nY#D|zAmz2?ZuoSpXCgx>6X+`(|YH+Gt)FLI!7Mn zG^wdOJDkEB=WAPXU?_#Rjq*88u9|iY;+?*v(}3S5|BT*Px5`3!#9jd%-iEwvOEZAY z__Rp^wN3UxMeF5OxlvNmtJ<{SDI8}naL%dOo5-KkY!Csh%2}Im&IuL#j}BNa!QY5} zh-<-ig%UCJdB%AY{iI+21^AcBz`sxi{^c_82g|^}QU?A=8TiM`!0#;sFDFkq`ll5g zt}T=w7WD9mGH@+VX?rbCDg3i#=sa2mex?lk+hySYSO)%$GVp5U`I@4`HJV>P+E2Jv zQ}B;8G~7Sw{z(!&soHbxrs3)IcjkX*zC^#S;I+tk3jSv_P{sAn)-v$rW#CH#;an>! z`bIwCI#a<5_SOCzbKyUg-MGc_LCeGSscNs`=)Q(?&Fa8y+@kvi+~CVKt!iJ;kM@7A zZyh@Oaf@ZFro(lxf*0icqN6tlKHAVQbJ9%c`2hvzdfBC~^{n+@i+O~C8+zvYTfq%E z8(sKMF(s9a1wC;6ZMMHF-Y9obL4~qW*8S?SZyt@WkfT008$__KZ&Yg-PJ7*fWC}8W zq!fWoszT5c)G9b|F;x`@u4t-4;E+}o0&P+i0-d%h1RA9(1iE=u2#l|)5SUP^LSST6 zg}~6O3W2dy6#|oAD8gnV9vJmC<>Mi$0wtBE%efP)qj8f6Q)>l8=!WHqFs>^gLf0%$ zgvqu7B6QdCM3{LhAVL=|PlUs61w`o9<%w`_aaPcY3AYY6Xe%HRZXNFM7Kz}&KbGj&Y3~TP4!4LaAQEmJ{x(qok#Os9qp|`b z;nuM$BNA?%1(gvAw+@!*<9A2@3{XA*n=2y{Zk>gd5ec`>qRNPbTW4`)M8d7Jq%tDm z)@dma;e2US>hivM7-NSo@;~_3e9<}|HU_U&w1}y}i)~*yD#1i9T)0pm5_1UOefQmY zcPAn4f!R)(;DLd*DiZ@>N8_eYOVL&f#fhCN&Q^t*VY)qg_~D03gzD_iWo;0FN>NO{QhrsY8k3GQgiR3%OiF8y##2ju+yRHI}q9c@ar_`OV20}5$ zIy>JPti=4-$4cD!;>DP06#kr5v}{S>y8)dau?j+!y2mv>BBQA??vekmQ4bL_+Se4+ zLtU1vCtV-Q=f?^}+!S=`)TxqaC(Mj?!a!RUH`=|hQk$(sTd&QKExbPK3Kf(W|L;-| zwx&IvSE8xXoVDJEV7jVxH2yYYIM@J$X!*4a^=^i$H+qPhr6FvX%xVyHP*g0HK7a1NgV{8iY_2Y4lL%qM6zH&t z!#2PFpI|mKOg5`a*|dkS(Me79WH> zn*dL14q@ZuFHwroJN|Q~1r!5R&uJ@kWMHnzzoz|-17*mymyz2n4FPKGvZv%q8%;AyKu*mymy zz2lX$_XYK4GJa0Sy0tKFZZ^ivEAz3kW2MNhd%hJ+w7EzW9%T|;U=a0s5q|pEy(5_D z!Xi=lk4bcqLDcIrA`gD^mSCcbibUZpCec=dsMkYqw5<2j!9*7qiNZHbqALudUVjjI z>m%O}Cc30Z6rNxbU1<>YT3_UUPi_k$8kZJM%eEwYkH^>gEpJ_suaPZSxb+rB;95+D zxhcTe5%0Nk;AE0|{IHKb{H@7>p5cLHu0Jz)JbsU#U}R=~lISB&JcKp?(#1n>F*TDQ zQBI6q23%Yy26Z?I%9mW#SWtuaDeilk;Cng|nTk^&_%&kd!Bo#7duwW7;CTFQL#|o* ziID3cCWAdV9%$n}1?dLR?V?oNF;J62Z;-lDao%26kV-$fQt2mGD*bdK5|w2mHTmOl z+b>FO4VY}jDLs8k5WIwKmygt;ZHv*i4riu|ZEfsix8W3qi?d=R3Y^6TPBb3}&M?lG zW!2ob$2o&ZE{TyCFcJnvEI+vf!_RAmT*cET*Bv=LkQ_|rGTA%r?$nXo@%VN>smRs& zdK~>cf+K#k{h+cxk7V8nop(c=Bk%_G(#Sz59>eFVM~Q-c5;vuB zkE57q^^=>@^pl&?^wWvRBv~uiMc*}?Qx<=h!E^>l?@~G_wYfLtGei1pPC!u-{p3ob zpIk}w6I0o`ky_|(pUz3rcYkgG+FI$ys+kJ&bi+GxFdX$& z>g^UuY)~XJIeLJx_fT`WB0+^~>qnJS`Sus6cQ=pd-nvh8u?D#oHjT zYoWz{d8hKAVhcu4A4XAbr>vI!>ZtPv(5vm-I)-QLIhac2Rub|qraKqX{y==TJm8T+ zJF?!+`wWhZf5!8Uc*o$_7Cui}aa57M7cF@e!NT1oR`}rU!-M^~br|&z48Oa7U?6EX zuiGb*$@V3Zhm$=AQ;EZw{=~tdp>!gf>d9n#6Un~5Y^pDrOARHuQ$xA8CkKX8H>y`d zo84Clyv^-7Je=*%3=i!&n9Qbnw`H<-V79YuAlcW_n#VLFE`8Wphoz(^J`)u;PMPU^ zN8}{>bkg%?%Xv#41nfZ%j5;_5*f9@`1bD1MAEU4L9iyKVY2<5F5cYB#cHjSm5$iH| zE2BFC+f7yX4Y=~}G;w+L^Dq6!jsv?p_bK>N1z)DF?^Tcyv~kNK#FU6_-Getz;@qhI z^UeEI>-VecVzf@H@?s~?=(_hU1uDPoZ1vnf#8bSWNK0CwvIj0vMhd0}Hh3c&Q5Euh=B{1@IKgU%7PKtf{fpTv_YSpD)yQ4n@E{Z zo+9bqz6*b~h*Pt#S5BFKS|B#W0Ut|IW}irz--`0-4K<{UeqlCNiOj^N7Kr`$8I6tG z3bCJo_l=-i8{{4MY?SX>lT|MxoNLHXZX@UmGu4d-Ogx zUMwRv_|AmUr%V_<#e~slCJf_k35;%Fwu6~{usT@+i1tSN^nybCHOQp*jo(1 z1`IX^unvC@bfawK6!CSGVHQO26H&e|v%l9kxDCa520sZM zg5r)L^!H%L-2E^`aM)5Q0tQtn0>p|bBAp3J5eG6VMW~j_#uR9r2&bjAr692Lyg!Du16EcpX!vTD2^Ip*@9uaqa`3yzug_gY{qha9oxxIsGi= zukuf*^bNk>gv#7^Jt_yQZq#Y0`%yXFK7xwLmb@QzIx3ek^tBg&#PfN?wNcG`P7ZhU z_hd6enRL#+?nqB+K=B%hwBP(`{C{{8-jD43-pl+iG5z+9l(7Cso_^}-(Zm(?C+H{DZ zi)=w!EakrEfeG|TQv5Bp&LcYiVZSB%45>2!ZjN{6G}-qzM( o7d%Rh7+d<&={1~GVbd8An@}E^SR;R2d+Hx9w#F~fyI30d3)=I=82|tP literal 0 HcmV?d00001 diff --git a/src/testcases/org/apache/poi/hssf/record/aggregates/TestValueRecordsAggregate.java b/src/testcases/org/apache/poi/hssf/record/aggregates/TestValueRecordsAggregate.java index a3315c297..8e8a72ece 100755 --- a/src/testcases/org/apache/poi/hssf/record/aggregates/TestValueRecordsAggregate.java +++ b/src/testcases/org/apache/poi/hssf/record/aggregates/TestValueRecordsAggregate.java @@ -17,15 +17,30 @@ package org.apache.poi.hssf.record.aggregates; -import junit.framework.TestCase; -import org.apache.poi.hssf.record.*; - +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; import java.util.ArrayList; import java.util.Iterator; import java.util.List; +import java.util.zip.CRC32; + +import junit.framework.AssertionFailedError; +import junit.framework.TestCase; + +import org.apache.poi.hssf.record.BlankRecord; +import org.apache.poi.hssf.record.FormulaRecord; +import org.apache.poi.hssf.record.Record; +import org.apache.poi.hssf.record.SharedFormulaRecord; +import org.apache.poi.hssf.record.UnknownRecord; +import org.apache.poi.hssf.record.WindowOneRecord; +import org.apache.poi.hssf.usermodel.HSSFSheet; +import org.apache.poi.hssf.usermodel.HSSFWorkbook; public class TestValueRecordsAggregate extends TestCase { + private static final String ABNORMAL_SHARED_FORMULA_FLAG_TEST_FILE = "AbnormalSharedFormulaFlag.xls"; ValueRecordsAggregate valueRecord = new ValueRecordsAggregate(); /** @@ -203,4 +218,117 @@ public class TestValueRecordsAggregate extends TestCase assertEquals( 36, valueRecord.getRecordSize() ); } + + /** + * Sometimes the 'shared formula' flag (FormulaRecord.isSharedFormula()) is set when + * there is no corresponding SharedFormulaRecord available. SharedFormulaRecord definitions do + * not span multiple sheets. They are are only defined within a sheet, and thus they do not + * have a sheet index field (only row and column range fields).
+ * So it is important that the code which locates the SharedFormulaRecord for each + * FormulaRecord does not allow matches across sheets.
+ * + * Prior to bugzilla 44449 (Feb 2008), POI ValueRecordsAggregate.construct(int, List) + * allowed SharedFormulaRecords to be erroneously used across sheets. That incorrect + * behaviour is shown by this test.

+ * + * Notes on how to produce the test spreadsheet:

+ * The setup for this test (AbnormalSharedFormulaFlag.xls) is rather fragile, insomuchas + * re-saving the file (either with Excel or POI) clears the flag.
+ *
    + *
  1. A new spreadsheet was created in Excel (File | New | Blank Workbook).
    2. + *
  2. Sheet3 was deleted.
    3. + *
  3. Sheet2!A1 formula was set to '="second formula"', and fill-dragged through A1:A8.
    4. + *
  4. Sheet1!A1 formula was set to '="first formula"', and also fill-dragged through A1:A8.
    5. + *
  5. Four rows on Sheet1 "5" through "8" were deleted ('delete rows' alt-E D, not 'clear' Del).
    6. + *
  6. The spreadsheet was saved as AbnormalSharedFormulaFlag.xls.
    7. + *
+ * Prior to the row delete action the spreadsheet has two SharedFormulaRecords. One + * for each sheet. To expose the bug, the shared formulas have been made to overlap.
+ * The row delete action (as described here) seems to to delete the + * SharedFormulaRecord from Sheet1 (but not clear the 'shared formula' flags.
+ * There are other variations on this theme to create the same effect. + * + */ + public void testSpuriousSharedFormulaFlag() { + File dataDir = new File(System.getProperty("HSSF.testdata.path")); + File testFile = new File(dataDir, ABNORMAL_SHARED_FORMULA_FLAG_TEST_FILE); + + long actualCRC = getFileCRC(testFile); + long expectedCRC = 2277445406L; + if(actualCRC != expectedCRC) { + System.err.println("Expected crc " + expectedCRC + " but got " + actualCRC); + throw failUnexpectedTestFileChange(); + } + HSSFWorkbook wb; + try { + FileInputStream in = new FileInputStream(testFile); + wb = new HSSFWorkbook(in); + } catch (IOException e) { + throw new RuntimeException(e); + } + + HSSFSheet s = wb.getSheetAt(0); // Sheet1 + + String cellFormula; + cellFormula = getFormulaFromFirstCell(s, 0); // row "1" + // the problem is not observable in the first row of the shared formula + if(!cellFormula.equals("\"first formula\"")) { + throw new RuntimeException("Something else wrong with this test case"); + } + + // but the problem is observable in rows 2,3,4 + cellFormula = getFormulaFromFirstCell(s, 1); // row "2" + if(cellFormula.equals("\"second formula\"")) { + throw new AssertionFailedError("found bug 44449 (Wrong SharedFormulaRecord was used)."); + } + if(!cellFormula.equals("\"first formula\"")) { + throw new RuntimeException("Something else wrong with this test case"); + } + } + private static String getFormulaFromFirstCell(HSSFSheet s, int rowIx) { + return s.getRow(rowIx).getCell((short)0).getCellFormula(); + } + + /** + * If someone opened this particular test file in Excel and saved it, the peculiar condition + * which causes the target bug would probably disappear. This test would then just succeed + * regardless of whether the fix was present. So a CRC check is performed to make it less easy + * for that to occur. + */ + private static RuntimeException failUnexpectedTestFileChange() { + String msg = "Test file '" + ABNORMAL_SHARED_FORMULA_FLAG_TEST_FILE + "' has changed. " + + "This junit may not be properly testing for the target bug. " + + "Either revert the test file or ensure that the new version " + + "has the right characteristics to test the target bug."; + // A breakpoint in ValueRecordsAggregate.handleMissingSharedFormulaRecord(FormulaRecord) + // should get hit during parsing of Sheet1. + // If the test spreadsheet is created as directed, this condition should occur. + // It is easy to upset the test spreadsheet (for example re-saving will destroy the + // peculiar condition we are testing for). + throw new RuntimeException(msg); + } + + /** + * gets a CRC checksum for the content of a file + */ + private static long getFileCRC(File f) { + CRC32 crc = new CRC32(); + byte[] buf = new byte[2048]; + try { + InputStream is = new FileInputStream(f); + while(true) { + int bytesRead = is.read(buf); + if(bytesRead < 1) { + break; + } + crc.update(buf, 0, bytesRead); + } + is.close(); + } catch (IOException e) { + throw new RuntimeException(e); + } + + return crc.getValue(); + } + }