From e2793ef74b0b380d0dd60f94add36dd3505853aa Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Sun, 10 May 2009 21:34:28 +0000 Subject: [PATCH] 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 | 1 + src/documentation/content/xdocs/status.xml | 1 + src/java/org/apache/poi/hssf/model/Sheet.java | 75 +++++++++++-------- .../record/aggregates/PageSettingsBlock.java | 4 + .../aggregates/AllRecordAggregateTests.java | 2 +- ...gBlock.java => TestPageSettingsBlock.java} | 45 ++++++++++- 6 files changed, 95 insertions(+), 33 deletions(-) rename src/testcases/org/apache/poi/hssf/record/aggregates/{TestPageSettingBlock.java => TestPageSettingsBlock.java} (72%) diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index 39f0cfc0e..ef0430c73 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,6 +37,7 @@ + 46953 - More tweaks to PageSettingsBlock parsing logic in Sheet constructor 47089 - Fixed XSSFWorkbook.createSheet to properly increment sheetId 46568 - Fixed XSLFPowerPointExtractor to properly process line breaks 39056 - Fixed POIFSFileSystem to set CLSID of root when constructing instances from InputStream diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 82563c9f6..7e1027f6d 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,6 +34,7 @@ + 46953 - More tweaks to PageSettingsBlock parsing logic in Sheet constructor 47089 - Fixed XSSFWorkbook.createSheet to properly increment sheetId 46568 - Fixed XSLFPowerPointExtractor to properly process line breaks 39056 - Fixed POIFSFileSystem to set CLSID of root when constructing instances from InputStream diff --git a/src/java/org/apache/poi/hssf/model/Sheet.java b/src/java/org/apache/poi/hssf/model/Sheet.java index 40a2e1e92..948b5befc 100644 --- a/src/java/org/apache/poi/hssf/model/Sheet.java +++ b/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 { - if (bofEofNestingLevel == 2) { - // It's normal for a chart to have its own PageSettingsBlock - // Fall through and add psb here, because chart records - // are stored loose among the sheet records. - // this latest psb does not clash with _psBlock - } else if (windowTwo != null) { - // probably 'Custom View Settings' sub-stream which is found between - // USERSVIEWBEGIN(01AA) and USERSVIEWEND(01AB) - // TODO - create UsersViewAggregate to hold these sub-streams, and simplify this code a bit - // This happens three times in test sample file "29982.xls" - // Also several times in bugzilla samples 46840-23373 and 46840-23374 - if (recSid == UnknownRecord.HEADER_FOOTER_089C) { - _psBlock.addLateHeaderFooter(rs.getNext()); - } else if (rs.peekNextSid() != UnknownRecord.USERSVIEWEND_01AB) { - // not quite the expected situation - throw new RuntimeException("two Page Settings Blocks found in the same sheet"); - } - } else { - // Some apps write PLS, WSBOOL, but PLS is part of - // 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; + } + if (bofEofNestingLevel == 2) { + psb = new PageSettingsBlock(rs); + // It's normal for a chart to have its own PageSettingsBlock + // Fall through and add psb here, because chart records + // are stored loose among the sheet records. + // this latest psb does not clash with _psBlock + } else if (windowTwo != null) { + // probably 'Custom View Settings' sub-stream which is found between + // USERSVIEWBEGIN(01AA) and USERSVIEWEND(01AB) + // TODO - create UsersViewAggregate to hold these sub-streams, and simplify this code a bit + // This happens three times in test sample file "29982.xls" + // Also several times in bugzilla samples 46840-23373 and 46840-23374 + if (recSid == UnknownRecord.HEADER_FOOTER_089C) { + _psBlock.addLateHeaderFooter(rs.getNext()); + continue; } + 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, but PLS is part of + // 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) { diff --git a/src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java b/src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java index 99694dcc9..1731a1c3a 100644 --- a/src/java/org/apache/poi/hssf/record/aggregates/PageSettingsBlock.java +++ b/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; } } diff --git a/src/testcases/org/apache/poi/hssf/record/aggregates/AllRecordAggregateTests.java b/src/testcases/org/apache/poi/hssf/record/aggregates/AllRecordAggregateTests.java index e19f182f7..57fe336b6 100644 --- a/src/testcases/org/apache/poi/hssf/record/aggregates/AllRecordAggregateTests.java +++ b/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; } } diff --git a/src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingBlock.java b/src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingsBlock.java similarity index 72% rename from src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingBlock.java rename to src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingsBlock.java index 07a49fd1e..076bbfeb3 100644 --- a/src/testcases/org/apache/poi/hssf/record/aggregates/TestPageSettingBlock.java +++ b/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)); }