Fix for bug 45699 - RowRecordsAggregate needs to tolerate MergeCellsRecords between row/cell records

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

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

@@ -22,16 +22,20 @@ import java.util.List;
 import org.apache.poi.hssf.record.BOFRecord;
 import org.apache.poi.hssf.record.CalcCountRecord;
 import org.apache.poi.hssf.record.CalcModeRecord;
+import org.apache.poi.hssf.record.DVALRecord;
 import org.apache.poi.hssf.record.DateWindow1904Record;
 import org.apache.poi.hssf.record.DefaultRowHeightRecord;
 import org.apache.poi.hssf.record.DeltaRecord;
 import org.apache.poi.hssf.record.DimensionsRecord;
+import org.apache.poi.hssf.record.DrawingRecord;
+import org.apache.poi.hssf.record.DrawingSelectionRecord;
 import org.apache.poi.hssf.record.EOFRecord;
 import org.apache.poi.hssf.record.GridsetRecord;
 import org.apache.poi.hssf.record.GutsRecord;
 import org.apache.poi.hssf.record.HyperlinkRecord;
 import org.apache.poi.hssf.record.IndexRecord;
 import org.apache.poi.hssf.record.IterationRecord;
+import org.apache.poi.hssf.record.ObjRecord;
 import org.apache.poi.hssf.record.PaneRecord;
 import org.apache.poi.hssf.record.PrecisionRecord;
 import org.apache.poi.hssf.record.PrintGridlinesRecord;
@@ -42,8 +46,10 @@ import org.apache.poi.hssf.record.RefModeRecord;
 import org.apache.poi.hssf.record.SCLRecord;
 import org.apache.poi.hssf.record.SaveRecalcRecord;
 import org.apache.poi.hssf.record.SelectionRecord;
+import org.apache.poi.hssf.record.TextObjectRecord;
 import org.apache.poi.hssf.record.UncalcedRecord;
 import org.apache.poi.hssf.record.UnknownRecord;
+import org.apache.poi.hssf.record.WindowOneRecord;
 import org.apache.poi.hssf.record.WindowTwoRecord;
 import org.apache.poi.hssf.record.aggregates.ConditionalFormattingTable;
 import org.apache.poi.hssf.record.aggregates.DataValidityTable;
@@ -161,12 +167,19 @@ final class RecordOrderer {
 	private static int findInsertPosForNewMergedRecordTable(List records) {
 		for (int i = records.size() - 2; i >= 0; i--) { // -2 to skip EOF record
 			Object rb = records.get(i);
+			if (!(rb instanceof Record)) {
+				// DataValidityTable, ConditionalFormattingTable, 
+				// even PageSettingsBlock (which doesn't normally appear after 'View Settings')
+				continue; 
+			}
 			Record rec = (Record) rb;
 			switch (rec.getSid()) {
+				// 'View Settings' (4 records) 
 				case WindowTwoRecord.sid:
 				case SCLRecord.sid:
 				case PaneRecord.sid:
 				case SelectionRecord.sid:
+
 				case UnknownRecord.STANDARDWIDTH_0099:
 					return i + 1;
 			}
@@ -306,4 +319,29 @@ final class RecordOrderer {
 		}
 		return false;
 	}
+	/**
+	 * @return <code>true</code> if the specified record ID terminates a sequence of Row block records
+	 * It is assumed that at least one row or cell value record has been found prior to the current 
+	 * record
+	 */
+	public static boolean isEndOfRowBlock(short sid) {
+		switch(sid) {
+			case DrawingRecord.sid:
+			case DrawingSelectionRecord.sid:
+			case ObjRecord.sid:
+			case TextObjectRecord.sid:
+
+			case WindowOneRecord.sid:
+				// should really be part of workbook stream, but some apps seem to put this before WINDOW2
+			case WindowTwoRecord.sid:
+				return true;
+
+			case DVALRecord.sid:
+				return true;
+			case EOFRecord.sid: 
+				// WINDOW2 should always be present, so shouldn't have got this far
+				throw new RuntimeException("Found EOFRecord before WindowTwoRecord was encountered");
+		}
+		return PageSettingsBlock.isComponentRecord(sid);
+	}
 }

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

@@ -122,7 +122,8 @@ public final class Sheet implements Model {
     
     protected WindowTwoRecord            windowTwo         =     null;
     protected SelectionRecord            selection         =     null;
-    private   MergedCellsTable           _mergedCellsTable;
+    /** java object always present, but if empty no BIFF records are written */
+    private final MergedCellsTable       _mergedCellsTable;
     /** always present in this POI object, not always written to Excel file */
     /*package*/ColumnInfoRecordsAggregate _columnInfos;
     /** the DimensionsRecord is always present */
@@ -146,8 +147,8 @@ public final class Sheet implements Model {
      * Creates new Sheet with no initialization --useless at this point
      * @see #createSheet(List,int,int)
      */
-    public Sheet()
+    public Sheet() {
-    {
+    	_mergedCellsTable = new MergedCellsTable();
     }
 
     /**
@@ -158,7 +159,7 @@ public final class Sheet implements Model {
      * to the passed in records and references to those records held. This function
      * is normally called via Workbook.
      *
-     * @param recs array containing those records in the sheet in sequence (normally obtained from RecordFactory)
+     * @param inRecs array containing those records in the sheet in sequence (normally obtained from RecordFactory)
      * @param sheetnum integer specifying the sheet's number (0,1 or 2 in this release)
      * @param offset of the sheet's BOF record
      *
@@ -167,19 +168,19 @@ public final class Sheet implements Model {
      * @see org.apache.poi.hssf.model.Workbook
      * @see org.apache.poi.hssf.record.Record
      */
-    public static Sheet createSheet(List recs, int sheetnum, int offset)
+    public static Sheet createSheet(List inRecs, int sheetnum, int offset)
     {
         if (log.check( POILogger.DEBUG ))
             log.logFormatted(POILogger.DEBUG,
                     "Sheet createSheet (existing file) with %",
-                    new Integer(recs.size()));
+                    new Integer(inRecs.size()));
         Sheet     retval             = new Sheet();
-        ArrayList records            = new ArrayList(recs.size() / 5);
+        ArrayList records            = new ArrayList(inRecs.size() / 5);
-        boolean   isfirstcell        = true;
+        // TODO - take chart streams off into separate java objects
-        int       bofEofNestingLevel = 0;
+        int       bofEofNestingLevel = 0;  // nesting level can only get to 2 (when charts are present)
 
-        for (int k = offset; k < recs.size(); k++) {
+        for (int k = offset; k < inRecs.size(); k++) {
-            Record rec = ( Record ) recs.get(k);
+            Record rec = ( Record ) inRecs.get(k);
             if ( rec.getSid() == DBCellRecord.sid ) {
                 continue;
             }
@@ -193,7 +194,7 @@ public final class Sheet implements Model {
             }
             
             if ( rec.getSid() == CFHeaderRecord.sid ) {
-                RecordStream rs = new RecordStream(recs, k);
+                RecordStream rs = new RecordStream(inRecs, k);
                 retval.condFormatting = new ConditionalFormattingTable(rs);
                 k += rs.getCountRead()-1;
                 records.add(retval.condFormatting);
@@ -201,43 +202,34 @@ public final class Sheet implements Model {
             }
             
             if (rec.getSid() == ColumnInfoRecord.sid) {
-                RecordStream rs = new RecordStream(recs, k);
+                RecordStream rs = new RecordStream(inRecs, k);
                 retval._columnInfos = new ColumnInfoRecordsAggregate(rs);
                 k += rs.getCountRead()-1;
                 records.add(retval._columnInfos);
                 continue;
             }
             if ( rec.getSid() == DVALRecord.sid) {
-                RecordStream rs = new RecordStream(recs, k);
+                RecordStream rs = new RecordStream(inRecs, k);
                 retval._dataValidityTable = new DataValidityTable(rs);
                 k += rs.getCountRead() - 1; // TODO - convert this method result to be zero based
                 records.add(retval._dataValidityTable);
                 continue;
             }
             // TODO construct RowRecordsAggregate from RecordStream
-            if ( rec.getSid() == RowRecord.sid ) {
+            if ((rec.getSid() == RowRecord.sid || rec.isValue()) && bofEofNestingLevel == 1 ) {
-                RowRecord row = (RowRecord)rec;
+                //only add the aggregate once
-                if (retval._rowsAggregate == null) {
+                if (retval._rowsAggregate != null) {
-                    retval._rowsAggregate = new RowRecordsAggregate();
+                    throw new RuntimeException("row/cell records found in the wrong place");
-                    records.add(retval._rowsAggregate); //only add the aggregate once
                 }
-                retval._rowsAggregate.insertRow(row);
+                int lastRowCellRec = findEndOfRowBlock(inRecs, k, retval._mergedCellsTable);
+                retval._rowsAggregate = new RowRecordsAggregate(inRecs, k, lastRowCellRec);
+                records.add(retval._rowsAggregate); //only add the aggregate once
+                k = lastRowCellRec -1;
                 continue;
             }
-            if ( rec.isValue() && bofEofNestingLevel == 1 ) {
-                if (isfirstcell) {
-                    isfirstcell = false;
-                    if (retval._rowsAggregate == null) {
-                        retval._rowsAggregate = new RowRecordsAggregate();
-                        records.add(retval._rowsAggregate); //only add the aggregate once
-                    }
-                    retval._rowsAggregate.constructCellValues( k, recs );
-                }
-               continue;
-            }
              
             if (PageSettingsBlock.isComponentRecord(rec.getSid())) {
-                RecordStream rs = new RecordStream(recs, k);
+                RecordStream rs = new RecordStream(inRecs, k);
                 PageSettingsBlock psb = new PageSettingsBlock(rs);
                 if (bofEofNestingLevel == 1) {
                     if (retval._psBlock == null) {
@@ -253,9 +245,10 @@ public final class Sheet implements Model {
             }
             
             if (rec.getSid() == MergeCellsRecord.sid) {
-                RecordStream rs = new RecordStream(recs, k);
+            	// when the MergedCellsTable is found in the right place, we expect those records to be contiguous
-                retval._mergedCellsTable = new MergedCellsTable(rs);
+                RecordStream rs = new RecordStream(inRecs, k);
-                records.add(retval._mergedCellsTable);
+                retval._mergedCellsTable.read(rs);
+                k += rs.getCountRead()-1;
                 continue;
             }
             
@@ -337,6 +330,11 @@ public final class Sheet implements Model {
         if (retval._dimensions == null) {
             throw new RuntimeException("DimensionsRecord was not found");
         }
+        if (retval.windowTwo == null) {
+            throw new RuntimeException("WINDOW2 was not found");
+        }
+        // put merged cells table in the right place (regardless of where the first MergedCellsRecord was found */
+        RecordOrderer.addNewSheetRecord(records, retval._mergedCellsTable);
         retval.records = records;
         retval.checkRows();
         if (log.check( POILogger.DEBUG ))
@@ -344,6 +342,26 @@ public final class Sheet implements Model {
         return retval;
     }
 
+    /**
+     * Also collects any rogue MergeCellRecords
+     * @return the index one after the last row/cell record
+     */
+    private static int findEndOfRowBlock(List recs, int startIx, MergedCellsTable mergedCellsTable) {
+        for(int i=startIx; i<recs.size(); i++) {
+            Record rec = (Record) recs.get(i);
+            if (RecordOrderer.isEndOfRowBlock(rec.getSid())) {
+                return i;
+            }
+            if (rec.getSid() == MergeCellsRecord.sid) {
+                    // Some apps scatter these records between the rows/cells but they are supposed to
+                    // be well after the row/cell records.  We collect them here 
+                    // see bug 45699
+                    mergedCellsTable.add((MergeCellsRecord) rec);
+            }
+        }
+        throw new RuntimeException("Failed to find end of row/cell records");
+    }
+
     private static final class RecordCloner implements RecordVisitor {
 
         private final List _destList;
@@ -447,9 +465,12 @@ public final class Sheet implements Model {
         retval._dimensions = createDimensions();
         records.add(retval._dimensions);
         retval.dimsloc = records.size()-1;
+        // 'Sheet View Settings'
         records.add(retval.windowTwo = retval.createWindowTwo());
         retval.selection = createSelection();
         records.add(retval.selection);
+
+        records.add(retval._mergedCellsTable); // MCT comes after 'Sheet View Settings' 
         records.add(EOFRecord.instance);
 
 
@@ -468,11 +489,7 @@ public final class Sheet implements Model {
         }
     }
     private MergedCellsTable getMergedRecords() {
-        if (_mergedCellsTable == null) {
+        // always present
-            MergedCellsTable mct = new MergedCellsTable();
-            RecordOrderer.addNewSheetRecord(records, mct);
-            _mergedCellsTable = mct;
-        }
         return _mergedCellsTable;
     }
 
@@ -872,7 +889,7 @@ public final class Sheet implements Model {
     /**
      * creates the BOF record
      */
-    private static BOFRecord createBOF() {
+    /* package */ static BOFRecord createBOF() {
         BOFRecord retval = new BOFRecord();
 
         retval.setVersion(( short ) 0x600);

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

@@ -41,8 +41,12 @@ public final class MergedCellsTable extends RecordAggregate {
 		_mergedRegions = new ArrayList();
 	}
 
-	public MergedCellsTable(RecordStream rs) {
+	/**
-		List temp = new ArrayList();
+	 * reads zero or more consecutive {@link MergeCellsRecord}s
+	 * @param rs
+	 */
+	public void read(RecordStream rs) {
+		List temp = _mergedRegions;
 		while (rs.peekNextClass() == MergeCellsRecord.class) {
 			MergeCellsRecord mcr = (MergeCellsRecord) rs.getNext();
 			int nRegions = mcr.getNumAreas();
@@ -50,7 +54,6 @@ public final class MergedCellsTable extends RecordAggregate {
 				temp.add(mcr.getAreaAt(i));
 			}
 		}
-		_mergedRegions = temp;
 	}
 
 	public int getRecordSize() {
@@ -92,7 +95,10 @@ public final class MergedCellsTable extends RecordAggregate {
 	}
 
 	public void add(MergeCellsRecord mcr) {
-		_mergedRegions.add(mcr);
+		int nRegions = mcr.getNumAreas();
+		for (int i = 0; i < nRegions; i++) {
+			_mergedRegions.add(mcr.getAreaAt(i));
+		}
 	}
 
 	public CellRangeAddress get(int index) {

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

@@ -26,8 +26,10 @@ import java.util.TreeMap;
 import org.apache.poi.hssf.record.CellValueRecordInterface;
 import org.apache.poi.hssf.record.DBCellRecord;
 import org.apache.poi.hssf.record.IndexRecord;
+import org.apache.poi.hssf.record.MergeCellsRecord;
 import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.RowRecord;
+import org.apache.poi.hssf.record.UnknownRecord;
 
 /**
  *
@@ -39,6 +41,7 @@ public final class RowRecordsAggregate extends RecordAggregate {
     private int _lastrow  = -1;
     private final Map _rowRecords;
     private final ValueRecordsAggregate _valuesAgg;
+    private final List _unknownRecords;
 
     /** Creates a new instance of ValueRecordsAggregate */
 
@@ -48,8 +51,54 @@ public final class RowRecordsAggregate extends RecordAggregate {
     private RowRecordsAggregate(TreeMap rowRecords, ValueRecordsAggregate valuesAgg) {
         _rowRecords = rowRecords;
         _valuesAgg = valuesAgg;
+        _unknownRecords = new ArrayList();
     }
 
+    public RowRecordsAggregate(List recs, int startIx, int endIx) {
+        this();
+        // First up, locate all the shared formulas for this sheet
+        SharedFormulaHolder sfh = SharedFormulaHolder.create(recs, startIx, endIx);
+        for(int i=startIx; i<endIx; i++) {
+            Record rec = (Record) recs.get(i);
+            switch (rec.getSid()) {
+                case MergeCellsRecord.sid:
+                    // Some apps scatter these records between the rows/cells but they are supposed to
+                    // be well after the row/cell records.  It is assumed such rogue MergeCellRecords 
+                    // have already been collected by the caller, and can safely be ignored here. 
+                    // see bug 45699
+                    continue;
+                case RowRecord.sid:
+                    insertRow((RowRecord) rec);
+                    continue;
+                case DBCellRecord.sid:
+                    // end of 'Row Block'.  Should only occur after cell records
+                    continue;
+            }
+            if (rec instanceof UnknownRecord) {
+                addUnknownRecord((UnknownRecord)rec);
+                // might need to keep track of where exactly these belong
+                continue;
+            }
+            if (!rec.isValue()) {
+                throw new RuntimeException("Unexpected record type (" + rec.getClass().getName() + ")");
+            }
+            i += _valuesAgg.construct(recs, i, endIx, sfh);
+        }
+        "".length();
+    }
+    /**
+     * Handles UnknownRecords which appear within the row/cell records
+     */
+    private void addUnknownRecord(UnknownRecord rec) {
+        // ony a few distinct record IDs are encountered by the existing POI test cases:
+        // 0x1065 // many
+        // 0x01C2 // several
+        // 0x0034 // few
+        // No documentation could be found for these
+        
+        // keep the unknown records for re-serialization
+        _unknownRecords.add(rec);
+    }
     public void insertRow(RowRecord row) {
         // Integer integer = new Integer(row.getRowNumber());
         _rowRecords.put(new Integer(row.getRowNumber()), row);
@@ -215,6 +264,10 @@ public final class RowRecordsAggregate extends RecordAggregate {
             cellRecord.setRowOffset(pos);
             rv.visitRecord(cellRecord);
         }
+        for (int i=0; i< _unknownRecords.size(); i++) {
+            // Potentially breaking the file here since we don't know exactly where to write these records
+            rv.visitRecord((Record) _unknownRecords.get(i));
+        }
     }
 
     public Iterator getIterator() {
@@ -439,9 +492,6 @@ public final class RowRecordsAggregate extends RecordAggregate {
         }
         return result;
     }
-    public void constructCellValues(int offset, List records) {
-        _valuesAgg.construct(offset, records);
-    }
     public void insertCell(CellValueRecordInterface cvRec) {
         _valuesAgg.insertCell(cvRec);
     }

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

@@ -0,0 +1,96 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.hssf.record.aggregates;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.poi.hssf.record.FormulaRecord;
+import org.apache.poi.hssf.record.Record;
+import org.apache.poi.hssf.record.SharedFormulaRecord;
+
+/**
+ * Temporarily holds SharedFormulaRecords while constructing a <tt>RowRecordsAggregate</tt>
+ * 
+ * @author Josh Micich
+ */
+final class SharedFormulaHolder {
+
+	private static final SharedFormulaHolder EMPTY = new SharedFormulaHolder(new SharedFormulaRecord[0]);
+	private final SharedFormulaRecord[] _sfrs;
+
+	/**
+	 * @param recs list of sheet records (possibly contains records for other parts of the Excel file)
+	 * @param startIx index of first row/cell record for current sheet
+	 * @param endIx one past index of last row/cell record for current sheet.  It is important 
+	 * that this code does not inadvertently collect <tt>SharedFormulaRecord</tt>s from any other
+	 * sheet (which could happen if endIx is chosen poorly).  (see bug 44449) 
+	 */
+	public static SharedFormulaHolder create(List recs, int startIx, int endIx) {
+		List temp = new ArrayList();
+        for (int k = startIx; k < endIx; k++)
+        {
+            Record rec = ( Record ) recs.get(k);
+            if (rec instanceof SharedFormulaRecord) {
+                temp.add(rec);
+            }
+        }
+        if (temp.size() < 1) {
+        	return EMPTY;
+        }
+        SharedFormulaRecord[] sfrs = new SharedFormulaRecord[temp.size()];
+        temp.toArray(sfrs);
+        return new SharedFormulaHolder(sfrs);
+        
+	}
+	private SharedFormulaHolder(SharedFormulaRecord[] sfrs) {
+		_sfrs = sfrs;
+	}
+	public void convertSharedFormulaRecord(FormulaRecord formula) {
+        // Traverse the list of shared formulas in
+        //  reverse order, and try to find the correct one
+        //  for us
+        for (int i=0; i<_sfrs.length; i++) {
+            SharedFormulaRecord shrd = _sfrs[i];
+            if (shrd.isFormulaInShared(formula)) {
+                shrd.convertSharedFormulaRecord(formula);
+                return;
+            }
+        }
+        // not found
+        handleMissingSharedFormulaRecord(formula);
+	}
+
+    /**
+     * 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.
+    }
+}

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

@@ -22,11 +22,14 @@ import java.util.Iterator;
 import java.util.List;
 
 import org.apache.poi.hssf.record.CellValueRecordInterface;
-import org.apache.poi.hssf.record.EOFRecord;
+import org.apache.poi.hssf.record.DBCellRecord;
 import org.apache.poi.hssf.record.FormulaRecord;
+import org.apache.poi.hssf.record.MergeCellsRecord;
 import org.apache.poi.hssf.record.Record;
+import org.apache.poi.hssf.record.RowRecord;
 import org.apache.poi.hssf.record.SharedFormulaRecord;
 import org.apache.poi.hssf.record.StringRecord;
+import org.apache.poi.hssf.record.TableRecord;
 import org.apache.poi.hssf.record.UnknownRecord;
 import org.apache.poi.hssf.record.aggregates.RecordAggregate.RecordVisitor;
 
@@ -39,8 +42,8 @@ import org.apache.poi.hssf.record.aggregates.RecordAggregate.RecordVisitor;
  * @author Jason Height (jheight at chariot dot net dot au)
  */
 public final class ValueRecordsAggregate {
-    private int                       firstcell = -1;
+    private int firstcell = -1;
-    private int                       lastcell  = -1;
+    private int lastcell  = -1;
     private CellValueRecordInterface[][] records;
 
     /** Creates a new instance of ValueRecordsAggregate */
@@ -137,92 +140,81 @@ public final class ValueRecordsAggregate {
         return lastcell;
     }
 
-    public int construct(int offset, List records)
+    /**
-    {
+     * Processes a sequential group of cell value records.  Stops at endIx or the first 
+     * non-value record encountered.
+     * @param sfh used to resolve any shared formulas for the current sheet
+     * @return the number of records consumed
+     */
+    public int construct(List records, int offset, int endIx, SharedFormulaHolder sfh) {
         int k = 0;
 
         FormulaRecordAggregate lastFormulaAggregate = null;
         
-        // First up, locate all the shared formulas for this sheet
-        List sharedFormulas = new java.util.ArrayList();
-        for (k = offset; k < records.size(); k++)
-        {
-            Record rec = ( Record ) records.get(k);
-            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
-        for (k = offset; k < records.size(); k++)
+        for (k = offset; k < endIx; k++) {
-        {
             Record rec = ( Record ) records.get(k);
 
-            if (rec instanceof StringRecord == false && !rec.isInValueSection() && !(rec instanceof UnknownRecord))
+            if (rec instanceof StringRecord) {
-            {
+                if (lastFormulaAggregate == null) {
-                break;
+                    throw new RuntimeException("StringRecord found without preceding FormulaRecord");
-            } else if (rec instanceof SharedFormulaRecord) {
-                // Already handled, not to worry
-            } else if (rec instanceof FormulaRecord)
-            {
-              FormulaRecord formula = (FormulaRecord)rec;
-              if (formula.isSharedFormula()) {
-                // Traverse the list of shared formulas in
-                //  reverse order, and try to find the correct one
-                //  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);
-                        found = true;
-                        break;
-                    }
                 }
-                if (!found) {
+                if (lastFormulaAggregate.getStringRecord() != null) {
-                    handleMissingSharedFormulaRecord(formula);
+                    throw new RuntimeException("Multiple StringRecords found after FormulaRecord");
                 }
-              }
-                
-              lastFormulaAggregate = new FormulaRecordAggregate((FormulaRecord)rec, null);
-              insertCell( lastFormulaAggregate );
-            }
-            else if (rec instanceof StringRecord)
-            {
                 lastFormulaAggregate.setStringRecord((StringRecord)rec);
+                lastFormulaAggregate = null;
+                continue;
             }
-            else if (rec.isValue())
+            
-            {
+            if (rec instanceof TableRecord) {
-                insertCell(( CellValueRecordInterface ) rec);
+                // TODO - don't loose this record
+                // DATATABLE probably belongs in formula record aggregate
+                if (lastFormulaAggregate == null) {
+                    throw new RuntimeException("No preceding formula record found");
+                }
+                lastFormulaAggregate = null;
+                continue; 
             }
+            
+            if (rec instanceof SharedFormulaRecord) {
+                // Already handled, not to worry
+                continue;
+            }
+
+            if (rec instanceof UnknownRecord) {
+                break;
+            }
+            if (rec instanceof RowRecord) {
+                break; 
+            }
+            if (rec instanceof DBCellRecord) {
+                // end of 'Row Block'.  This record is ignored by POI
+                break;
+            }
+            if (rec instanceof MergeCellsRecord) {
+                // doesn't really belong here
+                // can safely be ignored, because it has been processed in a higher method
+                continue;
+            }
+            if (!rec.isValue()) {
+                throw new RuntimeException("bad record type");
+            }
+            if (rec instanceof FormulaRecord) {
+                FormulaRecord formula = (FormulaRecord)rec;
+                if (formula.isSharedFormula()) {
+                    sfh.convertSharedFormulaRecord(formula);
+                }
+                
+                lastFormulaAggregate = new FormulaRecordAggregate((FormulaRecord)rec, null);
+                insertCell( lastFormulaAggregate );
+                continue;
+            }
+            insertCell(( CellValueRecordInterface ) rec);
         }
-        return k;
+        return k - offset - 1;
     }
 
-    /**
-     * 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.
-    }
-    
     /** Tallies a count of the size of the cell records
      *  that are attached to the rows in the range specified.
      */

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

@@ -24,6 +24,7 @@ import java.util.List;
 import junit.framework.AssertionFailedError;
 import junit.framework.TestCase;
 
+import org.apache.poi.hssf.HSSFTestDataSamples;
 import org.apache.poi.hssf.eventmodel.ERFListener;
 import org.apache.poi.hssf.eventmodel.EventRecordFactory;
 import org.apache.poi.hssf.record.BOFRecord;
@@ -32,6 +33,7 @@ import org.apache.poi.hssf.record.CellValueRecordInterface;
 import org.apache.poi.hssf.record.ColumnInfoRecord;
 import org.apache.poi.hssf.record.DimensionsRecord;
 import org.apache.poi.hssf.record.EOFRecord;
+import org.apache.poi.hssf.record.FormulaRecord;
 import org.apache.poi.hssf.record.GutsRecord;
 import org.apache.poi.hssf.record.IndexRecord;
 import org.apache.poi.hssf.record.MergeCellsRecord;
@@ -39,9 +41,13 @@ import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.RowRecord;
 import org.apache.poi.hssf.record.StringRecord;
 import org.apache.poi.hssf.record.UncalcedRecord;
+import org.apache.poi.hssf.record.WindowTwoRecord;
 import org.apache.poi.hssf.record.aggregates.ColumnInfoRecordsAggregate;
+import org.apache.poi.hssf.record.aggregates.MergedCellsTable;
 import org.apache.poi.hssf.record.aggregates.PageSettingsBlock;
 import org.apache.poi.hssf.record.aggregates.RowRecordsAggregate;
+import org.apache.poi.hssf.usermodel.HSSFCell;
+import org.apache.poi.hssf.usermodel.HSSFRow;
 import org.apache.poi.hssf.usermodel.HSSFSheet;
 import org.apache.poi.hssf.usermodel.HSSFWorkbook;
 import org.apache.poi.hssf.util.CellRangeAddress;
@@ -57,6 +63,7 @@ public final class TestSheet extends TestCase {
         List records = new ArrayList();
         records.add( new BOFRecord() );
         records.add( new DimensionsRecord() );
+        records.add(createWindow2Record());
         records.add(EOFRecord.instance);
         Sheet sheet = Sheet.createSheet( records, 0, 0 );
 
@@ -65,9 +72,22 @@ public final class TestSheet extends TestCase {
         assertTrue( sheet.records.get(pos++) instanceof ColumnInfoRecordsAggregate );
         assertTrue( sheet.records.get(pos++) instanceof DimensionsRecord );
         assertTrue( sheet.records.get(pos++) instanceof RowRecordsAggregate );
+        assertTrue( sheet.records.get(pos++) instanceof WindowTwoRecord );
+        assertTrue( sheet.records.get(pos++) instanceof MergedCellsTable );
         assertTrue( sheet.records.get(pos++) instanceof EOFRecord );
     }
 
+    private static Record createWindow2Record() {
+        WindowTwoRecord result = new WindowTwoRecord();
+        result.setOptions(( short ) 0x6b6);
+        result.setTopRow(( short ) 0);
+        result.setLeftCol(( short ) 0);
+        result.setHeaderColor(0x40);
+        result.setPageBreakZoom(( short ) 0);
+        result.setNormalZoom(( short ) 0);
+        return result;
+    }
+
     private static final class MergedCellListener implements ERFListener {
 
         private int _count;
@@ -168,6 +188,8 @@ public final class TestSheet extends TestCase {
         records.add(new RowRecord(0));
         records.add(new RowRecord(1));
         records.add(new RowRecord(2));
+        records.add(createWindow2Record());
+        records.add(EOFRecord.instance);
         records.add(merged);
 
         Sheet sheet = Sheet.createSheet(records, 0);
@@ -193,11 +215,15 @@ public final class TestSheet extends TestCase {
     public void testRowAggregation() {
         List records = new ArrayList();
 
+        records.add(Sheet.createBOF());
         records.add(new DimensionsRecord());
         records.add(new RowRecord(0));
         records.add(new RowRecord(1));
+        records.add(new FormulaRecord());
         records.add(new StringRecord());
         records.add(new RowRecord(2));
+        records.add(createWindow2Record());
+        records.add(EOFRecord.instance);
 
         Sheet sheet = Sheet.createSheet(records, 0);
         assertNotNull("Row [2] was skipped", sheet.getRow(2));
@@ -400,6 +426,7 @@ public final class TestSheet extends TestCase {
         records.add(new BOFRecord());
         records.add(new UncalcedRecord());
         records.add(new DimensionsRecord());
+        records.add(createWindow2Record());
         records.add(EOFRecord.instance);
         Sheet sheet = Sheet.createSheet(records, 0, 0);
 
@@ -408,7 +435,7 @@ public final class TestSheet extends TestCase {
         if (serializedSize != estimatedSize) {
             throw new AssertionFailedError("Identified bug 45066 b");
         }
-        assertEquals(68, serializedSize);
+        assertEquals(90, serializedSize);
     }
 
     /**
@@ -502,5 +529,17 @@ public final class TestSheet extends TestCase {
         }
         assertEquals(1, count);
     }
+    
+    public void testMisplacedMergedCellsRecords_bug45699() {
+        HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("ex45698-22488.xls");
+        
+        HSSFSheet sheet = wb.getSheetAt(0);
+        HSSFRow row = sheet.getRow(3);
+        HSSFCell cell = row.getCell(4);
+        if (cell == null) {
+            throw new AssertionFailedError("Identified bug 45699");
+        }
+        assertEquals("Informations", cell.getRichStringCellValue().getString());
+    }
 }
 

src/testcases/org/apache/poi/hssf/record/aggregates/TestValueRecordsAggregate.java

@@ -17,8 +17,6 @@
 
 package org.apache.poi.hssf.record.aggregates;
 
-import java.io.File;
-import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
@@ -34,27 +32,23 @@ 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
+public final class TestValueRecordsAggregate extends TestCase {
-{
     private static final String ABNORMAL_SHARED_FORMULA_FLAG_TEST_FILE = "AbnormalSharedFormulaFlag.xls";
-    ValueRecordsAggregate valueRecord = new ValueRecordsAggregate();
+    private final ValueRecordsAggregate valueRecord = new ValueRecordsAggregate();
 
     /**
      * Make sure the shared formula DOESNT makes it to the FormulaRecordAggregate when being parsed
      * as part of the value records
      */
-    public void testSharedFormula()
+    public void testSharedFormula() {
-    {
         List records = new ArrayList();
         records.add( new FormulaRecord() );
         records.add( new SharedFormulaRecord() );
 
-        valueRecord.construct( 0, records );
+        constructValueRecord(records);
         Iterator iterator = valueRecord.getIterator();
         Record record = (Record) iterator.next();
         assertNotNull( "Row contains a value", record );
@@ -64,24 +58,25 @@ public class TestValueRecordsAggregate extends TestCase
 
     }
 
-    private List testData(){
+    private void constructValueRecord(List records) {
+        SharedFormulaHolder sfrh = SharedFormulaHolder.create(records, 0, records.size());
+        valueRecord.construct(records, 0, records.size(), sfrh );
+    }
+
+    private static List testData() {
         List records = new ArrayList();
         FormulaRecord formulaRecord = new FormulaRecord();
         BlankRecord blankRecord = new BlankRecord();
-        WindowOneRecord windowOneRecord = new WindowOneRecord();
         formulaRecord.setRow( 1 );
         formulaRecord.setColumn( (short) 1 );
         blankRecord.setRow( 2 );
         blankRecord.setColumn( (short) 2 );
         records.add( formulaRecord );
         records.add( blankRecord );
-        records.add( windowOneRecord );
         return records;
     }
 
-    public void testInsertCell()
+    public void testInsertCell() {
-            throws Exception
-    {
         Iterator iterator = valueRecord.getIterator();
         assertFalse( iterator.hasNext() );
 
@@ -103,8 +98,7 @@ public class TestValueRecordsAggregate extends TestCase
         valueRecord.removeCell( blankRecord2 );
     }
 
-    public void testGetPhysicalNumberOfCells() throws Exception
+    public void testGetPhysicalNumberOfCells() {
-    {
         assertEquals(0, valueRecord.getPhysicalNumberOfCells());
         BlankRecord blankRecord1 = newBlankRecord();
         valueRecord.insertCell( blankRecord1 );
@@ -113,8 +107,7 @@ public class TestValueRecordsAggregate extends TestCase
         assertEquals(0, valueRecord.getPhysicalNumberOfCells());
     }
 
-    public void testGetFirstCellNum() throws Exception
+    public void testGetFirstCellNum() {
-    {
         assertEquals( -1, valueRecord.getFirstCellNum() );
         valueRecord.insertCell( newBlankRecord( 2, 2 ) );
         assertEquals( 2, valueRecord.getFirstCellNum() );
@@ -126,8 +119,7 @@ public class TestValueRecordsAggregate extends TestCase
         assertEquals( 2, valueRecord.getFirstCellNum() );
     }
 
-    public void testGetLastCellNum() throws Exception
+    public void testGetLastCellNum() {
-    {
         assertEquals( -1, valueRecord.getLastCellNum() );
         valueRecord.insertCell( newBlankRecord( 2, 2 ) );
         assertEquals( 2, valueRecord.getLastCellNum() );
@@ -140,8 +132,7 @@ public class TestValueRecordsAggregate extends TestCase
 
     }
 
-    public void testSerialize() throws Exception
+    public void testSerialize() {
-    {
         byte[] actualArray = new byte[36];
         byte[] expectedArray = new byte[]
         {
@@ -156,7 +147,7 @@ public class TestValueRecordsAggregate extends TestCase
             (byte)0x02, (byte)0x00, (byte)0x00, (byte)0x00,
         };
         List records = testData();
-        valueRecord.construct( 0, records );
+        constructValueRecord(records);
         int bytesWritten = valueRecord.serializeCellRow(1, 0, actualArray );
         bytesWritten += valueRecord.serializeCellRow(2, bytesWritten, actualArray );
         assertEquals( 36, bytesWritten );
@@ -164,18 +155,12 @@ public class TestValueRecordsAggregate extends TestCase
             assertEquals( expectedArray[i], actualArray[i] );
     }
 
-    public static void main( String[] args )
+    private static BlankRecord newBlankRecord()
-    {
-        System.out.println( "Testing org.apache.poi.hssf.record.aggregates.TestValueRecordAggregate" );
-        junit.textui.TestRunner.run( TestValueRecordsAggregate.class );
-    }
-
-    private BlankRecord newBlankRecord()
     {
         return newBlankRecord( 2, 2 );
     }
 
-    private BlankRecord newBlankRecord( int col, int row)
+    private static BlankRecord newBlankRecord( int col, int row)
     {
         BlankRecord blankRecord = new BlankRecord();
         blankRecord.setRow( row );
@@ -285,5 +270,4 @@ public class TestValueRecordsAggregate extends TestCase
         
         return crc.getValue();
     }
-
 }