From 45287875d7af3e57ea834f4b0ffcd4b371aa8c1f Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Fri, 28 Nov 2008 23:24:06 +0000 Subject: [PATCH] Fix for bug 46206 - tolerate missing DIMENSION records git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@721586 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 | 49 ++++---- .../aggregates/RowRecordsAggregate.java | 9 ++ .../org/apache/poi/hssf/model/TestSheet.java | 107 ++++++++++++------ 5 files changed, 116 insertions(+), 51 deletions(-) diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index b8cb613c2..5e4388bc5 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,6 +37,7 @@ + 46206 - Fixed Sheet to tolerate missing DIMENSION records 46301 - added pivot table records: SXDI, SXVDEX, SXPI, SXIDSTM, SXVIEW, SXVD, SXVS, et al 46280 - Fixed RowRecordsAggregate etc to properly skip PivotTable records diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 4d75382b3..99a82583a 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,6 +34,7 @@ + 46206 - Fixed Sheet to tolerate missing DIMENSION records 46301 - added pivot table records: SXDI, SXVDEX, SXPI, SXIDSTM, SXVIEW, SXVD, SXVS, et al 46280 - Fixed RowRecordsAggregate etc to properly skip PivotTable records diff --git a/src/java/org/apache/poi/hssf/model/Sheet.java b/src/java/org/apache/poi/hssf/model/Sheet.java index 7010368b9..834d33d74 100644 --- a/src/java/org/apache/poi/hssf/model/Sheet.java +++ b/src/java/org/apache/poi/hssf/model/Sheet.java @@ -93,7 +93,6 @@ import org.apache.poi.util.POILogger; * * @see org.apache.poi.hssf.model.Workbook * @see org.apache.poi.hssf.usermodel.HSSFSheet - * @version 1.0-pre */ public final class Sheet implements Model { public static final short LeftMargin = 0; @@ -103,7 +102,7 @@ public final class Sheet implements Model { private static POILogger log = POILogFactory.getLogger(Sheet.class); - protected ArrayList records = null; + private List records; protected PrintGridlinesRecord printGridlines = null; protected GridsetRecord gridset = null; private GutsRecord _gutsRecord; @@ -162,7 +161,7 @@ public final class Sheet implements Model { _mergedCellsTable = new MergedCellsTable(); RowRecordsAggregate rra = null; - records = new ArrayList(128); + records = new ArrayList(128); // TODO - take chart streams off into separate java objects int bofEofNestingLevel = 0; // nesting level can only get to 2 (when charts are present) int dimsloc = -1; @@ -304,12 +303,25 @@ public final class Sheet implements Model { records.add(rec); } - if (_dimensions == null) { - throw new RuntimeException("DimensionsRecord was not found"); - } if (windowTwo == null) { throw new RuntimeException("WINDOW2 was not found"); } + if (_dimensions == null) { + // Excel seems to always write the DIMENSION record, but tolerates when it is not present + // in all cases Excel (2007) adds the missing DIMENSION record + if (rra == null) { + // bug 46206 alludes to files which skip the DIMENSION record + // when there are no row/cell records. + // Not clear which application wrote these files. + rra = new RowRecordsAggregate(); + } else { + log.log(POILogger.WARN, "DIMENSION record not found even though row/cells present"); + // Not sure if any tools write files like this, but Excel reads them OK + } + dimsloc = findFirstRecordLocBySid(WindowTwoRecord.sid); + _dimensions = rra.createDimensions(); + records.add(dimsloc, _dimensions); + } if (rra == null) { rra = new RowRecordsAggregate(); records.add(dimsloc + 1, rra); @@ -323,13 +335,13 @@ public final class Sheet implements Model { private static final class RecordCloner implements RecordVisitor { - private final List _destList; + private final List _destList; - public RecordCloner(List destList) { + public RecordCloner(List destList) { _destList = destList; } public void visitRecord(Record r) { - _destList.add(r.clone()); + _destList.add((RecordBase)r.clone()); } } @@ -341,9 +353,9 @@ public final class Sheet implements Model { * belongs to a sheet. */ public Sheet cloneSheet() { - List clonedRecords = new ArrayList(this.records.size()); - for (int i = 0; i < this.records.size(); i++) { - RecordBase rb = (RecordBase) this.records.get(i); + List clonedRecords = new ArrayList(records.size()); + for (int i = 0; i < records.size(); i++) { + RecordBase rb = records.get(i); if (rb instanceof RecordAggregate) { ((RecordAggregate) rb).visitContainedRecords(new RecordCloner(clonedRecords)); continue; @@ -366,7 +378,7 @@ public final class Sheet implements Model { } private Sheet() { _mergedCellsTable = new MergedCellsTable(); - records = new ArrayList(32); + records = new ArrayList(32); if (log.check( POILogger.DEBUG )) log.log(POILogger.DEBUG, "Sheet createsheet from scratch called"); @@ -516,9 +528,8 @@ public final class Sheet implements Model { boolean haveSerializedIndex = false; - for (int k = 0; k < records.size(); k++) - { - RecordBase record = (RecordBase) records.get(k); + for (int k = 0; k < records.size(); k++) { + RecordBase record = records.get(k); if (record instanceof RecordAggregate) { RecordAggregate agg = (RecordAggregate) record; @@ -560,7 +571,7 @@ public final class Sheet implements Model { int result = 0; // start just after BOF record (INDEX is not present in this list) for (int j = bofRecordIndex + 1; j < records.size(); j++) { - RecordBase tmpRec = (RecordBase) records.get(j); + RecordBase tmpRec = records.get(j); if (tmpRec instanceof RowRecordsAggregate) { break; } @@ -1203,7 +1214,7 @@ public final class Sheet implements Model { } } - public List getRecords() + public List getRecords() { return records; } @@ -1564,7 +1575,7 @@ public final class Sheet implements Model { } else { - List records = getRecords(); + List records = getRecords(); EscherAggregate r = EscherAggregate.createAggregate( records, loc, drawingManager ); int startloc = loc; while ( loc + 1 < records.size() diff --git a/src/java/org/apache/poi/hssf/record/aggregates/RowRecordsAggregate.java b/src/java/org/apache/poi/hssf/record/aggregates/RowRecordsAggregate.java index 5d249640e..35b766ed5 100644 --- a/src/java/org/apache/poi/hssf/record/aggregates/RowRecordsAggregate.java +++ b/src/java/org/apache/poi/hssf/record/aggregates/RowRecordsAggregate.java @@ -28,6 +28,7 @@ import org.apache.poi.hssf.record.ArrayRecord; import org.apache.poi.hssf.record.CellValueRecordInterface; import org.apache.poi.hssf.record.ContinueRecord; import org.apache.poi.hssf.record.DBCellRecord; +import org.apache.poi.hssf.record.DimensionsRecord; import org.apache.poi.hssf.record.FormulaRecord; import org.apache.poi.hssf.record.IndexRecord; import org.apache.poi.hssf.record.MergeCellsRecord; @@ -487,4 +488,12 @@ public final class RowRecordsAggregate extends RecordAggregate { public void updateFormulasAfterRowShift(FormulaShifter formulaShifter, int currentExternSheetIndex) { _valuesAgg.updateFormulasAfterRowShift(formulaShifter, currentExternSheetIndex); } + public DimensionsRecord createDimensions() { + DimensionsRecord result = new DimensionsRecord(); + result.setFirstRow(_firstrow); + result.setLastRow(_lastrow); + result.setFirstCol((short) _valuesAgg.getFirstCellNum()); + result.setLastCol((short) _valuesAgg.getLastCellNum()); + return result; + } } diff --git a/src/testcases/org/apache/poi/hssf/model/TestSheet.java b/src/testcases/org/apache/poi/hssf/model/TestSheet.java index f19af7cb3..bd8678720 100644 --- a/src/testcases/org/apache/poi/hssf/model/TestSheet.java +++ b/src/testcases/org/apache/poi/hssf/model/TestSheet.java @@ -34,21 +34,20 @@ 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; +import org.apache.poi.hssf.record.NumberRecord; 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.record.aggregates.RecordAggregate.RecordVisitor; 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; +import org.apache.poi.hssf.usermodel.RecordInspector.RecordCollector; +import org.apache.poi.ss.util.CellRangeAddress; /** * Unit test for the Sheet class. @@ -59,25 +58,29 @@ public final class TestSheet extends TestCase { private static Sheet createSheet(List inRecs) { return Sheet.createSheet(new RecordStream(inRecs, 0)); } - - + + private static Record[] getSheetRecords(Sheet s, int offset) { + RecordCollector rc = new RecordCollector(); + s.visitContainedRecords(rc, offset); + return rc.getRecords(); + } + public void testCreateSheet() { // Check we're adding row and cell aggregates - List records = new ArrayList(); + List records = new ArrayList(); records.add( new BOFRecord() ); records.add( new DimensionsRecord() ); records.add(createWindow2Record()); records.add(EOFRecord.instance); Sheet sheet = createSheet(records); + Record[] outRecs = getSheetRecords(sheet, 0); int pos = 0; - assertTrue( sheet.records.get(pos++) instanceof BOFRecord ); - 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 ); + assertTrue(outRecs[pos++] instanceof BOFRecord ); + assertTrue(outRecs[pos++] instanceof IndexRecord ); + assertTrue(outRecs[pos++] instanceof DimensionsRecord ); + assertTrue(outRecs[pos++] instanceof WindowTwoRecord ); + assertTrue(outRecs[pos++] instanceof EOFRecord ); } private static Record createWindow2Record() { @@ -106,7 +109,7 @@ public final class TestSheet extends TestCase { return _count; } } - + public void testAddMergedRegion() { Sheet sheet = Sheet.createSheet(); int regionsToAdd = 4096; @@ -128,7 +131,7 @@ public final class TestSheet extends TestCase { int recordsExpected = regionsToAdd/1027; if ((regionsToAdd % 1027) != 0) recordsExpected++; - assertTrue("The " + regionsToAdd + " merged regions should have been spread out over " + assertTrue("The " + regionsToAdd + " merged regions should have been spread out over " + recordsExpected + " records, not " + recordsAdded, recordsAdded == recordsExpected); // Check we can't add one with invalid date try { @@ -164,9 +167,9 @@ public final class TestSheet extends TestCase { //assert they have been deleted assertEquals("Num of regions", regionsToAdd - n - 1, sheet.getNumMergedRegions()); } - - // merge records are removed from within the MergedCellsTable, - // so the sheet record count should not change + + // merge records are removed from within the MergedCellsTable, + // so the sheet record count should not change assertEquals("Sheet Records", nSheetRecords, sheet.getRecords().size()); } @@ -178,7 +181,7 @@ public final class TestSheet extends TestCase { * */ public void testMovingMergedRegion() { - List records = new ArrayList(); + List records = new ArrayList(); CellRangeAddress[] cras = { new CellRangeAddress(0, 1, 0, 2), @@ -193,7 +196,7 @@ public final class TestSheet extends TestCase { records.add(merged); Sheet sheet = createSheet(records); - sheet.records.remove(0); + sheet.getRecords().remove(0); // TODO - what does this line do? //stub object to throw off list INDEX operations sheet.removeMergedRegion(0); @@ -213,7 +216,7 @@ public final class TestSheet extends TestCase { * */ public void testRowAggregation() { - List records = new ArrayList(); + List records = new ArrayList(); records.add(Sheet.createBOF()); records.add(new DimensionsRecord()); @@ -221,7 +224,7 @@ public final class TestSheet extends TestCase { records.add(new RowRecord(1)); FormulaRecord formulaRecord = new FormulaRecord(); formulaRecord.setCachedResultTypeString(); - records.add(formulaRecord); + records.add(formulaRecord); records.add(new StringRecord()); records.add(new RowRecord(2)); records.add(createWindow2Record()); @@ -430,7 +433,7 @@ public final class TestSheet extends TestCase { byte[] buf = new byte[estimatedSize]; int serializedSize = r.serialize(0, buf); if (estimatedSize != serializedSize) { - throw new AssertionFailedError("serialized size mismatch for record (" + throw new AssertionFailedError("serialized size mismatch for record (" + r.getClass().getName() + ")"); } _totalSize += estimatedSize; @@ -445,15 +448,15 @@ public final class TestSheet extends TestCase { */ public void testUncalcSize_bug45066() { - List records = new ArrayList(); + List records = new ArrayList(); records.add(new BOFRecord()); records.add(new UncalcedRecord()); records.add(new DimensionsRecord()); records.add(createWindow2Record()); records.add(EOFRecord.instance); Sheet sheet = createSheet(records); - - // The original bug was due to different logic for collecting records for sizing and + + // The original bug was due to different logic for collecting records for sizing and // serialization. The code has since been refactored into a single method for visiting // all contained records. Now this test is much less interesting SizeCheckingRecordVisitor scrv = new SizeCheckingRecordVisitor(); @@ -491,7 +494,7 @@ public final class TestSheet extends TestCase { if (false) { // make sure that RRA and VRA are in the right place // (Aug 2008) since the VRA is now part of the RRA, there is much less chance that - // they could get out of order. Still, one could write serialize the sheet here, + // they could get out of order. Still, one could write serialize the sheet here, // and read back with EventRecordFactory to make sure... } assertEquals(242, dbCellRecordPos); @@ -528,13 +531,13 @@ public final class TestSheet extends TestCase { } } } - + /** * Checks for bug introduced around r682282-r683880 that caused a second GUTS records * which in turn got the dimensions record out of alignment */ public void testGutsRecord_bug45640() { - + Sheet sheet = Sheet.createSheet(); sheet.addRow(new RowRecord(0)); sheet.addRow(new RowRecord(1)); @@ -552,10 +555,10 @@ 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); @@ -582,4 +585,44 @@ public final class TestSheet extends TestCase { throw e; } } + + /** + * Some apps seem to write files with missing DIMENSION records. + * Excel(2007) tolerates this, so POI should too. + */ + public void testMissingDims() { + + int rowIx = 5; + int colIx = 6; + NumberRecord nr = new NumberRecord(); + nr.setRow(rowIx); + nr.setColumn((short) colIx); + nr.setValue(3.0); + + List inRecs = new ArrayList(); + inRecs.add(new BOFRecord()); + inRecs.add(new RowRecord(rowIx)); + inRecs.add(nr); + inRecs.add(createWindow2Record()); + inRecs.add(EOFRecord.instance); + Sheet sheet; + try { + sheet = createSheet(inRecs); + } catch (RuntimeException e) { + if (e.getMessage().equals("DimensionsRecord was not found")) { + throw new AssertionFailedError("Identified bug 46206"); + } + throw e; + } + + RecordCollector rv = new RecordCollector(); + sheet.visitContainedRecords(rv, rowIx); + Record[] outRecs = rv.getRecords(); + assertEquals(8, outRecs.length); + DimensionsRecord dims = (DimensionsRecord) outRecs[5]; + assertEquals(rowIx, dims.getFirstRow()); + assertEquals(rowIx, dims.getLastRow()); + assertEquals(colIx, dims.getFirstCol()); + assertEquals(colIx, dims.getLastCol()); + } }