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

@@ -36,6 +36,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.1-beta1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">44449 - Avoid getting confused when two sheets have shared formulas for the same areas, and when the shared formula is set incorrectly</action>
            <action dev="POI-DEVELOPERS" type="fix">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</action>
            <action dev="POI-DEVELOPERS" type="add">44371 - Support for the Offset function</action>
            <action dev="POI-DEVELOPERS" type="fix">38921 - Have HSSFPalette.findSimilar() work properly</action>

src/documentation/content/xdocs/status.xml

@@ -33,6 +33,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1-beta1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">44449 - Avoid getting confused when two sheets have shared formulas for the same areas, and when the shared formula is set incorrectly</action>
            <action dev="POI-DEVELOPERS" type="fix">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</action>
            <action dev="POI-DEVELOPERS" type="add">44371 - Support for the Offset function</action>
            <action dev="POI-DEVELOPERS" type="fix">38921 - Have HSSFPalette.findSimilar() work properly</action>

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,
+                    handleMissingSharedFormulaRecord(formula);
-                	//cant really do much about that.
-                	//throw new RecordFormatException("Could not find appropriate shared 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 <tt>SharedFormulaRecord.convertSharedFormulaRecord</tt> and hence the 
+     * <tt>parsedExpression</tt> field of this <tt>FormulaRecord</tt> will not get updated.<br/>
+     * As it turns out, this is not a problem, because in these circumstances, the existing value
+     * for <tt>parsedExpression</tt> is perfectly OK.<p/>
+     * 
+     * This method may also be used for setting breakpoints to help diagnose issues regarding the
+     * abnormally-set 'shared formula' flags. 
+     * (see TestValueRecordsAggregate.testSpuriousSharedFormulaFlag()).<p/>
+     * 
+     * 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;
 

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 java.io.File;
-import org.apache.poi.hssf.record.*;
+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 (<tt>FormulaRecord.isSharedFormula()</tt>) 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).<br/>
+     * So it is important that the code which locates the SharedFormulaRecord for each 
+     * FormulaRecord does not allow matches across sheets.</br> 
+     * 
+     * Prior to bugzilla 44449 (Feb 2008), POI <tt>ValueRecordsAggregate.construct(int, List)</tt> 
+     * allowed <tt>SharedFormulaRecord</tt>s to be erroneously used across sheets.  That incorrect
+     * behaviour is shown by this test.<p/>
+     * 
+     * <b>Notes on how to produce the test spreadsheet</b>:</p>
+     * The setup for this test (AbnormalSharedFormulaFlag.xls) is rather fragile, insomuchas 
+     * re-saving the file (either with Excel or POI) clears the flag.<br/>
+     * <ol>
+     * <li>A new spreadsheet was created in Excel (File | New | Blank Workbook).</li>
+     * <li>Sheet3 was deleted.</li>
+     * <li>Sheet2!A1 formula was set to '="second formula"', and fill-dragged through A1:A8.</li>
+     * <li>Sheet1!A1 formula was set to '="first formula"', and also fill-dragged through A1:A8.</li>
+     * <li>Four rows on Sheet1 "5" through "8" were deleted ('delete rows' alt-E D, not 'clear' Del).</li>
+     * <li>The spreadsheet was saved as AbnormalSharedFormulaFlag.xls.</li>
+     * </ol>
+     * Prior to the row delete action the spreadsheet has two <tt>SharedFormulaRecord</tt>s. One 
+     * for each sheet. To expose the bug, the shared formulas have been made to overlap.<br/>
+     * The row delete action (as described here) seems to to delete the 
+     * <tt>SharedFormulaRecord</tt> from Sheet1 (but not clear the 'shared formula' flags.<br/>
+     * 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();
+    }
+
 }