Fix for bug 45145 - made sure RowRecordsAggregate comes before ValueRecordsAggregate. Also fixed BiffViewer to show correct record offsets

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@663765 13f79535-47bb-0310-9956-ffa450edef68

src/documentation/content/xdocs/changes.xml

@@ -37,6 +37,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.1-final" date="2008-06-??">
+           <action dev="POI-DEVELOPERS" type="fix">45145 - Fixed Sheet to always enforce RowRecordsAggregate before ValueRecordsAggregate</action>
            <action dev="POI-DEVELOPERS" type="fix">45123 - Fixed SharedFormulaRecord.convertSharedFormulas() to propagate token operand classes</action>
            <action dev="POI-DEVELOPERS" type="fix">45087 - Correctly detect date formats like [Black]YYYY as being date based</action>
            <action dev="POI-DEVELOPERS" type="add">45060 - Improved token class transformation during formula parsing</action>

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1-final" date="2008-06-??">
+           <action dev="POI-DEVELOPERS" type="fix">45145 - Fixed Sheet to always enforce RowRecordsAggregate before ValueRecordsAggregate</action>
            <action dev="POI-DEVELOPERS" type="fix">45123 - Fixed SharedFormulaRecord.convertSharedFormulas() to propagate token operand classes</action>
            <action dev="POI-DEVELOPERS" type="fix">45087 - Correctly detect date formats like [Black]YYYY as being date based</action>
            <action dev="POI-DEVELOPERS" type="add">45060 - Improved token class transformation during formula parsing</action>

src/java/org/apache/poi/hssf/dev/BiffViewer.java

@@ -87,7 +87,8 @@ public final class BiffViewer {
                         records.add(record);
                         if (activeRecord != null)
                             activeRecord.dump(ps);
-                        activeRecord = new RecordDetails(recStream.getSid(), recStream.getLength(), (int)recStream.getPos(), record);
+                        int startPos = (int)(recStream.getPos()-recStream.getLength() - 4);
+                        activeRecord = new RecordDetails(recStream.getSid(), recStream.getLength(), startPos, record);
                     }
                     if (dump) {
                         recStream.dumpBytes(ps);

src/java/org/apache/poi/hssf/model/Sheet.java

@@ -66,7 +66,7 @@ public final class Sheet implements Model {
     protected ArrayList                  records           =     null;
               int                        preoffset         =     0;            // offset of the sheet in a new file
               int                        loc               =     0;
-    protected int                        dimsloc           =     0;
+    protected int                        dimsloc           =     -1;  // TODO - is it legal for dims record to be missing?
     protected DimensionsRecord           dims;
     protected DefaultColWidthRecord      defaultcolwidth   =     null;
     protected DefaultRowHeightRecord     defaultrowheight  =     null;
@@ -295,6 +295,8 @@ public final class Sheet implements Model {
             }
             else if ( rec.getSid() == IndexRecord.sid )
             {
+                // ignore INDEX record because it is only needed by Excel, 
+                // and POI always re-calculates its contents 
                 rec = null;
             }
 
@@ -329,8 +331,8 @@ public final class Sheet implements Model {
             }
         }
         retval.records = records;
-        retval.checkCells();
         retval.checkRows();
+        retval.checkCells();
         if (log.check( POILogger.DEBUG ))
             log.log(POILogger.DEBUG, "sheet createSheet (existing file) exited");
         return retval;
@@ -486,7 +488,15 @@ public final class Sheet implements Model {
         if (cells == null)
         {
             cells = new ValueRecordsAggregate();
-            records.add(getDimsLoc() + 1, cells);
+            // In the worksheet stream, the row records always occur before the cell (value) 
+            // records. Therefore POI's aggregates (RowRecordsAggregate, ValueRecordsAggregate) 
+            // should follow suit. Some methods in this class tolerate either order, while 
+            // others have been found to fail (see bug 45145).
+            int rraIndex = getDimsLoc() + 1;
+            if (records.get(rraIndex).getClass() != RowRecordsAggregate.class) {
+                throw new IllegalStateException("Cannot create value records before row records exist");
+            }
+            records.add(rraIndex+1, cells);
         }
     }
 
@@ -836,15 +846,22 @@ public final class Sheet implements Model {
         return pos-offset;
     }
 
-    private int serializeIndexRecord(final int BOFRecordIndex, final int offset, byte[] data) {
+    /**
+     * @param indexRecordOffset also happens to be the end of the BOF record
+     * @return the size of the serialized INDEX record
+     */
+    private int serializeIndexRecord(final int bofRecordIndex, final int indexRecordOffset,
+            byte[] data) {
         IndexRecord index = new IndexRecord();
         index.setFirstRow(rows.getFirstRowNum());
         index.setLastRowAdd1(rows.getLastRowNum() + 1);
         // Calculate the size of the records from the end of the BOF
         // and up to the RowRecordsAggregate...
-      int sheetRecSize = 0;
+
-      for (int j = BOFRecordIndex+1; j < records.size(); j++)
+        // 'initial sheet records' are between INDEX and first ROW record.
-      {
+        int sizeOfInitialSheetRecords = 0;
+        // start just after BOF record (INDEX is not present in this list)
+        for (int j = bofRecordIndex + 1; j < records.size(); j++) {
             Record tmpRec = ((Record) records.get(j));
             if (tmpRec instanceof UncalcedRecord) {
                 continue;
@@ -852,30 +869,38 @@ public final class Sheet implements Model {
             if (tmpRec instanceof RowRecordsAggregate) {
                 break;
             }
-        sheetRecSize+= tmpRec.getRecordSize();
+            sizeOfInitialSheetRecords += tmpRec.getRecordSize();
         }
         if (_isUncalced) {
-          sheetRecSize += UncalcedRecord.getStaticRecordSize();
+            sizeOfInitialSheetRecords += UncalcedRecord.getStaticRecordSize();
         }
+
         // Add the references to the DBCells in the IndexRecord (one for each block)
+        // Note: The offsets are relative to the Workbook BOF. Assume that this is
+        // 0 for now.....
+
         int blockCount = rows.getRowBlockCount();
         // Calculate the size of this IndexRecord
         int indexRecSize = IndexRecord.getRecordSizeForBlockCount(blockCount);
 
-      int rowBlockOffset = 0;
+        int currentOffset = indexRecordOffset + indexRecSize + sizeOfInitialSheetRecords;
-      int cellBlockOffset = 0;
+
-      int dbCellOffset = 0;
         for (int block = 0; block < blockCount; block++) {
-        rowBlockOffset += rows.getRowBlockSize(block);
+            // each row-block has a DBCELL record.
-        cellBlockOffset += null == cells ? 0 : cells.getRowCellBlockSize(rows.getStartRowNumberForBlock(block),
+            // The offset of each DBCELL record needs to be updated in the INDEX record
-                                                     rows.getEndRowNumberForBlock(block));
+
-        //Note: The offsets are relative to the Workbook BOF. Assume that this is
+            // account for row records in this row-block
-        //0 for now.....
+            currentOffset += rows.getRowBlockSize(block);
-        index.addDbcell(offset + indexRecSize + sheetRecSize + dbCellOffset + rowBlockOffset + cellBlockOffset);
+            // account for cell value records after those
-        //Add space required to write the dbcell record(s) (whose references were just added).
+            currentOffset += null == cells ? 0 : cells.getRowCellBlockSize(rows
-        dbCellOffset += (8 + (rows.getRowCountForBlock(block) * 2));
+                    .getStartRowNumberForBlock(block), rows.getEndRowNumberForBlock(block));
+
+            // currentOffset is now the location of the DBCELL record for this row-block
+            index.addDbcell(currentOffset);
+            // Add space required to write the DBCELL record (whose reference was just added).
+            currentOffset += (8 + (rows.getRowCountForBlock(block) * 2));
         }
-      return index.serialize(offset, data);
+        return index.serialize(indexRecordOffset, data);
     }
 
 

src/java/org/apache/poi/hssf/record/aggregates/ValueRecordsAggregate.java

@@ -37,7 +37,7 @@ import java.util.List;
 public final class ValueRecordsAggregate
     extends Record
 {
-    public final static short sid       = -1000;
+    public final static short sid       = -1001; // 1000 clashes with RowRecordsAggregate
     int                       firstcell = -1;
     int                       lastcell  = -1;
   CellValueRecordInterface[][] records;

src/testcases/org/apache/poi/hssf/model/TestSheet.java

@@ -19,11 +19,15 @@ package org.apache.poi.hssf.model;
 
 import junit.framework.AssertionFailedError;
 import junit.framework.TestCase;
+
+import org.apache.poi.hssf.eventmodel.ERFListener;
+import org.apache.poi.hssf.eventmodel.EventRecordFactory;
 import org.apache.poi.hssf.record.*;
 import org.apache.poi.hssf.record.aggregates.ColumnInfoRecordsAggregate;
 import org.apache.poi.hssf.record.aggregates.RowRecordsAggregate;
 import org.apache.poi.hssf.record.aggregates.ValueRecordsAggregate;
 
+import java.io.ByteArrayInputStream;
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
@@ -34,8 +38,7 @@ import java.util.List;
  * @author Glen Stampoultzis (glens at apache.org)
  */
 public final class TestSheet extends TestCase {
-    public void testCreateSheet() throws Exception
+    public void testCreateSheet() {
-    {
         // Check we're adding row and cell aggregates
         List records = new ArrayList();
         records.add( new BOFRecord() );
@@ -52,8 +55,7 @@ public final class TestSheet extends TestCase {
         assertTrue( sheet.records.get(pos++) instanceof EOFRecord );
     }
 
-    public void testAddMergedRegion()
+    public void testAddMergedRegion() {
-    {
         Sheet sheet = Sheet.createSheet();
         int regionsToAdd = 4096;
         int startRecords = sheet.getRecords().size();
@@ -91,8 +93,7 @@ public final class TestSheet extends TestCase {
         }
     }
 
-    public void testRemoveMergedRegion()
+    public void testRemoveMergedRegion() {
-    {
         Sheet sheet = Sheet.createSheet();
         int regionsToAdd = 4096;
 
@@ -139,13 +140,11 @@ public final class TestSheet extends TestCase {
         assertEquals("Should be no more merged regions", 0, sheet.getNumMergedRegions());
     }
 
-    public void testGetMergedRegionAt()
+    public void testGetMergedRegionAt() {
-    {
         //TODO
     }
 
-    public void testGetNumMergedRegions()
+    public void testGetNumMergedRegions() {
-    {
         //TODO
     }
 
@@ -163,7 +162,6 @@ public final class TestSheet extends TestCase {
 
         Sheet sheet = Sheet.createSheet(records, 0);
         assertNotNull("Row [2] was skipped", sheet.getRow(2));
-
     }
 
     /**
@@ -372,5 +370,73 @@ public final class TestSheet extends TestCase {
         }
         assertEquals(50, serializedSize);
     }
+
+    /**
+     * Prior to bug 45145 <tt>RowRecordsAggregate</tt> and <tt>ValueRecordsAggregate</tt> could
+     * sometimes occur in reverse order.  This test reproduces one of those situations and makes
+     * sure that RRA comes before VRA.<br/>
+     *
+     * The code here represents a normal POI use case where a spreadsheet is created from scratch.
+     */
+    public void testRowValueAggregatesOrder_bug45145() {
+
+        Sheet sheet = Sheet.createSheet();
+
+        RowRecord rr = new RowRecord(5);
+        sheet.addRow(rr);
+
+        CellValueRecordInterface cvr = new BlankRecord();
+        cvr.setColumn((short)0);
+        cvr.setRow(5);
+        sheet.addValueRecord(5, cvr);
+
+
+        int dbCellRecordPos = getDbCellRecordPos(sheet);
+        if (dbCellRecordPos == 264) {
+            // The overt symptom of the bug
+            // DBCELL record pos is calculated wrong if VRA comes before RRA
+            throw new AssertionFailedError("Identified  bug 45145");
+        }
+
+        // make sure that RRA and VRA are in the right place
+        int rraIx = sheet.getDimsLoc()+1;
+        List recs = sheet.getRecords();
+        assertEquals(RowRecordsAggregate.class, recs.get(rraIx).getClass());
+        assertEquals(ValueRecordsAggregate.class, recs.get(rraIx+1).getClass());
+
+        assertEquals(254, dbCellRecordPos);
+    }
+
+    /**
+     * @return the value calculated for the position of the first DBCELL record for this sheet.
+     * That value is found on the IndexRecord.
+     */
+    private static int getDbCellRecordPos(Sheet sheet) {
+        int size = sheet.getSize();
+        byte[] data = new byte[size];
+        sheet.serialize(0, data);
+        EventRecordFactory erf = new EventRecordFactory();
+        MyIndexRecordListener myIndexListener = new MyIndexRecordListener();
+        erf.registerListener(myIndexListener, new short[] { IndexRecord.sid, });
+        erf.processRecords(new ByteArrayInputStream(data));
+        IndexRecord indexRecord = myIndexListener.getIndexRecord();
+        int dbCellRecordPos = indexRecord.getDbcellAt(0);
+        return dbCellRecordPos;
+    }
+
+    private static final class MyIndexRecordListener implements ERFListener {
+
+        private IndexRecord _indexRecord;
+        public MyIndexRecordListener() {
+            // no-arg constructor
+        }
+        public boolean processRecord(Record rec) {
+            _indexRecord = (IndexRecord)rec;
+            return true;
+        }
+        public IndexRecord getIndexRecord() {
+            return _indexRecord;
+        }
+    }
 }