Bug 46953 - fixed PageSettingsBlock parsing logic in Sheet. Bug caused sheet EOFs to get misplaced.

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@773412 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.5-beta6" date="2009-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46953 - More tweaks to PageSettingsBlock parsing logic in Sheet constructor</action>
            <action dev="POI-DEVELOPERS" type="fix">47089 - Fixed XSSFWorkbook.createSheet to properly increment sheetId</action>
            <action dev="POI-DEVELOPERS" type="fix">46568 - Fixed XSLFPowerPointExtractor to properly process line breaks</action>
            <action dev="POI-DEVELOPERS" type="fix">39056 - Fixed POIFSFileSystem to set CLSID of root when constructing instances from InputStream</action>

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.5-beta6" date="2009-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46953 - More tweaks to PageSettingsBlock parsing logic in Sheet constructor</action>
            <action dev="POI-DEVELOPERS" type="fix">47089 - Fixed XSSFWorkbook.createSheet to properly increment sheetId</action>
            <action dev="POI-DEVELOPERS" type="fix">46568 - Fixed XSLFPowerPointExtractor to properly process line breaks</action>
            <action dev="POI-DEVELOPERS" type="fix">39056 - Fixed POIFSFileSystem to set CLSID of root when constructing instances from InputStream</action>

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

@@ -50,6 +50,7 @@ import org.apache.poi.hssf.record.PrintHeadersRecord;
 import org.apache.poi.hssf.record.ProtectRecord;
 import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.RecordBase;
+import org.apache.poi.hssf.record.RecordFormatException;
 import org.apache.poi.hssf.record.RefModeRecord;
 import org.apache.poi.hssf.record.RowRecord;
 import org.apache.poi.hssf.record.SCLRecord;
@@ -210,40 +211,47 @@ public final class Sheet implements Model {
             }
 
             if (PageSettingsBlock.isComponentRecord(recSid)) {
-                PageSettingsBlock psb = new PageSettingsBlock(rs);
+                PageSettingsBlock psb;
                 if (_psBlock == null) {
+                    psb = new PageSettingsBlock(rs);
                     _psBlock = psb;
-                } else {
+                    records.add(psb);
-                    if (bofEofNestingLevel == 2) {
+                    continue;
-                        // It's normal for a chart to have its own PageSettingsBlock
+                }
-                        // Fall through and add psb here, because chart records
+                if (bofEofNestingLevel == 2) {
-                        // are stored loose among the sheet records.
+                    psb = new PageSettingsBlock(rs);
-                        // this latest psb does not clash with _psBlock
+                    // It's normal for a chart to have its own PageSettingsBlock
-                    } else if (windowTwo != null) {
+                    // Fall through and add psb here, because chart records
-                        // probably 'Custom View Settings' sub-stream which is found between
+                    // are stored loose among the sheet records.
-                        // USERSVIEWBEGIN(01AA) and USERSVIEWEND(01AB)
+                    // this latest psb does not clash with _psBlock
-                        // TODO - create UsersViewAggregate to hold these sub-streams, and simplify this code a bit
+                } else if (windowTwo != null) {
-                        // This happens three times in test sample file "29982.xls"
+                    // probably 'Custom View Settings' sub-stream which is found between
-                        // Also several times in bugzilla samples 46840-23373 and 46840-23374
+                    // USERSVIEWBEGIN(01AA) and USERSVIEWEND(01AB)
-                        if (recSid == UnknownRecord.HEADER_FOOTER_089C) {
+                    // TODO - create UsersViewAggregate to hold these sub-streams, and simplify this code a bit
-                            _psBlock.addLateHeaderFooter(rs.getNext());
+                    // This happens three times in test sample file "29982.xls"
-                        } else if (rs.peekNextSid() != UnknownRecord.USERSVIEWEND_01AB) {
+                    // Also several times in bugzilla samples 46840-23373 and 46840-23374
-                            // not quite the expected situation
+                    if (recSid == UnknownRecord.HEADER_FOOTER_089C) {
-                            throw new RuntimeException("two Page Settings Blocks found in the same sheet");
+                        _psBlock.addLateHeaderFooter(rs.getNext());
-                        }
+                        continue;
-                    } else {
-                            // Some apps write PLS, WSBOOL, <psb> but PLS is part of <psb>
-                            // This happens in the test sample file "NoGutsRecords.xls" and "WORKBOOK_in_capitals.xls"
-                            // In this case the first PSB is two records back
-                            int prevPsbIx = records.size()-2;
-                            if (_psBlock != records.get(prevPsbIx) || !(records.get(prevPsbIx+1) instanceof WSBoolRecord)) {
-                                // not quite the expected situation
-                                throw new RuntimeException("two Page Settings Blocks found in the same sheet");
-                            }
-                            records.remove(prevPsbIx); // WSBOOL will drop down one position.
-                            psb = mergePSBs(_psBlock, psb);
-                            _psBlock = psb;
                     }
+                    psb = new PageSettingsBlock(rs);
+                    if (rs.peekNextSid() != UnknownRecord.USERSVIEWEND_01AB) {
+                        // not quite the expected situation
+                        throw new RuntimeException("two Page Settings Blocks found in the same sheet");
+                    }
+                } else {
+                    psb = new PageSettingsBlock(rs);
+                    // Some apps write PLS, WSBOOL, <psb> but PLS is part of <psb>
+                    // This happens in the test sample file "NoGutsRecords.xls" and "WORKBOOK_in_capitals.xls"
+                    // In this case the first PSB is two records back
+                    int prevPsbIx = records.size()-2;
+                    if (_psBlock != records.get(prevPsbIx) || !(records.get(prevPsbIx+1) instanceof WSBoolRecord)) {
+                        // not quite the expected situation
+                        throw new RuntimeException("two Page Settings Blocks found in the same sheet");
+                    }
+                    records.remove(prevPsbIx); // WSBOOL will drop down one position.
+                    psb = mergePSBs(_psBlock, psb);
+                    _psBlock = psb;
                 }
                 records.add(psb);
                 continue;
@@ -274,6 +282,11 @@ public final class Sheet implements Model {
                 bofEofNestingLevel++;
                 if (log.check( POILogger.DEBUG ))
                     log.log(POILogger.DEBUG, "Hit BOF record. Nesting increased to " + bofEofNestingLevel);
+                BOFRecord bof = (BOFRecord)rec;
+                // TODO - extract chart sub-stream into record aggregate
+                if (bof.getType() != BOFRecord.TYPE_CHART) {
+                    throw new RecordFormatException("Bad BOF record type: " + bof.getType());
+                }
             }
             else if (recSid == EOFRecord.sid)
             {

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

@@ -35,6 +35,7 @@ import org.apache.poi.hssf.record.Margin;
 import org.apache.poi.hssf.record.PageBreakRecord;
 import org.apache.poi.hssf.record.PrintSetupRecord;
 import org.apache.poi.hssf.record.Record;
+import org.apache.poi.hssf.record.RecordFormatException;
 import org.apache.poi.hssf.record.RightMarginRecord;
 import org.apache.poi.hssf.record.TopMarginRecord;
 import org.apache.poi.hssf.record.UnknownRecord;
@@ -539,6 +540,9 @@ public final class PageSettingsBlock extends RecordAggregate {
 		if (_headerFooter != null) {
 			throw new IllegalStateException("This page settings block already has a header/footer record");
 		}
+		if (rec.getSid() != UnknownRecord.HEADER_FOOTER_089C) {
+			throw new RecordFormatException("Unexpected header-footer record sid: 0x" + Integer.toHexString(rec.getSid()));
+		}
 		_headerFooter = rec;
 	}
 }

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

@@ -36,7 +36,7 @@ public final class AllRecordAggregateTests {
 		result.addTestSuite(TestRowRecordsAggregate.class);
 		result.addTestSuite(TestSharedValueManager.class);
 		result.addTestSuite(TestValueRecordsAggregate.class);
-		result.addTestSuite(TestPageSettingBlock.class);
+		result.addTestSuite(TestPageSettingsBlock.class);
 		return result;
 	}
 }

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

@@ -30,6 +30,7 @@ import org.apache.poi.hssf.record.DimensionsRecord;
 import org.apache.poi.hssf.record.EOFRecord;
 import org.apache.poi.hssf.record.FooterRecord;
 import org.apache.poi.hssf.record.HeaderRecord;
+import org.apache.poi.hssf.record.IndexRecord;
 import org.apache.poi.hssf.record.NumberRecord;
 import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.UnknownRecord;
@@ -45,7 +46,7 @@ import org.apache.poi.util.HexRead;
  * 
  * @author Dmitriy Kumshayev
  */
-public final class TestPageSettingBlock extends TestCase {
+public final class TestPageSettingsBlock extends TestCase {
 	
 	public void testPrintSetup_bug46548() {
 		
@@ -109,6 +110,48 @@ public final class TestPageSettingBlock extends TestCase {
 		assertEquals(13, outRecs.length);
 	}
 
+	/**
+	 * Bug 46953 occurred because POI didn't handle late PSB records properly.
+	 */
+	public void testLateHeaderFooter_bug46953() {
+
+		int rowIx = 5;
+		int colIx = 6;
+		NumberRecord nr = new NumberRecord();
+		nr.setRow(rowIx);
+		nr.setColumn((short) colIx);
+		nr.setValue(3.0);
+
+		Record[] recs = {
+				BOFRecord.createSheetBOF(),
+				new HeaderRecord("&LSales Figures"),
+				new FooterRecord("&LJanuary"),
+				new DimensionsRecord(),
+				new WindowTwoRecord(),
+				ur(UnknownRecord.HEADER_FOOTER_089C, "9C 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 C4 60 00 00 00 00 00 00 00 00"),
+				EOFRecord.instance,
+		};
+		RecordStream rs = new RecordStream(Arrays.asList(recs), 0);
+		Sheet sheet = Sheet.createSheet(rs);
+
+		RecordCollector rv = new RecordCollector();
+		sheet.visitContainedRecords(rv, rowIx);
+		Record[] outRecs = rv.getRecords();
+		if (outRecs[4] == EOFRecord.instance) {
+			throw new AssertionFailedError("Identified bug 46953 - EOF incorrectly appended to PSB");
+		}
+		assertEquals(recs.length+1, outRecs.length); // +1 for index record
+
+		assertEquals(BOFRecord.class, outRecs[0].getClass());
+		assertEquals(IndexRecord.class, outRecs[1].getClass());
+		assertEquals(HeaderRecord.class, outRecs[2].getClass());
+		assertEquals(FooterRecord.class, outRecs[3].getClass());
+		assertEquals(UnknownRecord.HEADER_FOOTER_089C, outRecs[4].getSid());
+		assertEquals(DimensionsRecord.class, outRecs[5].getClass());
+		assertEquals(WindowTwoRecord.class, outRecs[6].getClass());
+		assertEquals(EOFRecord.instance, outRecs[7]);
+	}
+
 	private static UnknownRecord ur(int sid, String hexData) {
 		return new UnknownRecord(sid, HexRead.readFromString(hexData));
 	}