From 375a18e8a3426850e0df84805ab4a40ba8f9e288 Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Fri, 23 May 2008 03:56:31 +0000 Subject: [PATCH] Fix for 45066 - sheet encoding size mismatch problems git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@659403 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 | 98 ++++------ .../apache/poi/hssf/record/DBCellRecord.java | 24 +-- .../poi/hssf/usermodel/HSSFWorkbook.java | 184 ++++++++---------- .../org/apache/poi/hssf/model/TestSheet.java | 21 ++ .../poi/hssf/usermodel/TestHSSFWorkbook.java | 51 ++++- .../poi/hssf/usermodel/TestSheetHiding.java | 4 +- 8 files changed, 205 insertions(+), 179 deletions(-) diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index 5c26883bab..4fdeaefb02 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,6 +37,7 @@ + 45066 - fixed sheet encoding size mismatch problems 45003 - Support embeded HDGF visio documents 45001 - Partial fix for HWPF Range.insertBefore() and Range.delete() with unicode characters 44977 - Support for AM/PM in excel date formats diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 6706d409e1..bc57530d2e 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,6 +34,7 @@ + 45066 - fixed sheet encoding size mismatch problems 45003 - Support embeded HDGF visio documents 45001 - Partial fix for HWPF Range.insertBefore() and Range.delete() with unicode characters 44977 - Support for AM/PM in excel date formats diff --git a/src/java/org/apache/poi/hssf/model/Sheet.java b/src/java/org/apache/poi/hssf/model/Sheet.java index 45090e85cf..871ecc100f 100644 --- a/src/java/org/apache/poi/hssf/model/Sheet.java +++ b/src/java/org/apache/poi/hssf/model/Sheet.java @@ -96,8 +96,8 @@ public final class Sheet implements Model { protected List condFormatting = new ArrayList(); /** Add an UncalcedRecord if not true indicating formulas have not been calculated */ - protected boolean uncalced = false; - + protected boolean _isUncalced = false; + public static final byte PANE_LOWER_RIGHT = (byte)0; public static final byte PANE_UPPER_RIGHT = (byte)1; public static final byte PANE_LOWER_LEFT = (byte)2; @@ -162,7 +162,7 @@ public final class Sheet implements Model { } } else if (rec.getSid() == UncalcedRecord.sid) { - retval.uncalced = true; + retval._isUncalced = true; } else if (rec.getSid() == DimensionsRecord.sid) { @@ -329,16 +329,8 @@ public final class Sheet implements Model { } } retval.records = records; -// if (retval.rows == null) -// { -// retval.rows = new RowRecordsAggregate(); -// } retval.checkCells(); retval.checkRows(); -// if (retval.cells == null) -// { -// retval.cells = new ValueRecordsAggregate(); -// } if (log.check( POILogger.DEBUG )) log.log(POILogger.DEBUG, "sheet createSheet (existing file) exited"); return retval; @@ -816,17 +808,17 @@ public final class Sheet implements Model { // Once the rows have been found in the list of records, start // writing out the blocked row information. This includes the DBCell references if (record instanceof RowRecordsAggregate) { - pos += ((RowRecordsAggregate)record).serialize(pos, data, cells); // rec.length; + pos += ((RowRecordsAggregate)record).serialize(pos, data, cells); } else if (record instanceof ValueRecordsAggregate) { //Do nothing here. The records were serialized during the RowRecordAggregate block serialization } else { - pos += record.serialize(pos, data ); // rec.length; + pos += record.serialize(pos, data ); } // If the BOF record was just serialized then add the IndexRecord if (record.getSid() == BOFRecord.sid) { // Add an optional UncalcedRecord - if (uncalced) { + if (_isUncalced) { UncalcedRecord rec = new UncalcedRecord(); pos += rec.serialize(pos, data); } @@ -837,31 +829,10 @@ public final class Sheet implements Model { pos += serializeIndexRecord(k, pos, data); } } - - //// uncomment to test record sizes //// -// System.out.println( record.getClass().getName() ); -// byte[] data2 = new byte[record.getRecordSize()]; -// record.serialize(0, data2 ); // rec.length; -// if (LittleEndian.getUShort(data2, 2) != record.getRecordSize() - 4 -// && record instanceof RowRecordsAggregate == false -// && record instanceof ValueRecordsAggregate == false -// && record instanceof EscherAggregate == false) -// { -// throw new RuntimeException("Blah!!! Size off by " + ( LittleEndian.getUShort(data2, 2) - record.getRecordSize() - 4) + " records."); -// } - -//asd: int len = record.serialize(pos + offset, data ); - - ///// DEBUG BEGIN ///// -//asd: if (len != record.getRecordSize()) -//asd: throw new IllegalStateException("Record size does not match serialized bytes. Serialized size = " + len + " but getRecordSize() returns " + record.getRecordSize() + ". Record object is " + record.getClass()); - ///// DEBUG END ///// - -//asd: pos += len; // rec.length; - } - if (log.check( POILogger.DEBUG )) + if (log.check( POILogger.DEBUG )) { log.log(POILogger.DEBUG, "Sheet.serialize returning "); + } return pos-offset; } @@ -875,10 +846,17 @@ public final class Sheet implements Model { for (int j = BOFRecordIndex+1; j < records.size(); j++) { Record tmpRec = (( Record ) records.get(j)); - if (tmpRec instanceof RowRecordsAggregate) - break; + if (tmpRec instanceof UncalcedRecord) { + continue; + } + if (tmpRec instanceof RowRecordsAggregate) { + break; + } sheetRecSize+= tmpRec.getRecordSize(); } + if (_isUncalced) { + sheetRecSize += UncalcedRecord.getStaticRecordSize(); + } //Add the references to the DBCells in the IndexRecord (one for each block) int blockCount = rows.getRowBlockCount(); //Calculate the size of this IndexRecord @@ -2017,31 +1995,33 @@ public final class Sheet implements Model { { int retval = 0; - for ( int k = 0; k < records.size(); k++ ) - { - retval += ( (Record) records.get( k ) ).getRecordSize(); + for ( int k = 0; k < records.size(); k++) { + Record record = (Record) records.get(k); + if (record instanceof UncalcedRecord) { + // skip the UncalcedRecord if present, it's only encoded if the isUncalced flag is set + continue; + } + retval += record.getRecordSize(); } - //Add space for the IndexRecord if (rows != null) { - final int blocks = rows.getRowBlockCount(); - retval += IndexRecord.getRecordSizeForBlockCount(blocks); - - //Add space for the DBCell records - //Once DBCell per block. - //8 bytes per DBCell (non variable section) - //2 bytes per row reference - retval += (8 * blocks); - for (Iterator itr = rows.getIterator(); itr.hasNext();) { - RowRecord row = (RowRecord)itr.next(); - if (cells != null && cells.rowHasCells(row.getRowNumber())) - retval += 2; + // Add space for the IndexRecord and DBCell records + final int nBlocks = rows.getRowBlockCount(); + int nRows = 0; + if (cells != null) { + for (Iterator itr = rows.getIterator(); itr.hasNext();) { + RowRecord row = (RowRecord)itr.next(); + if (cells.rowHasCells(row.getRowNumber())) { + nRows++; + } + } } + retval += IndexRecord.getRecordSizeForBlockCount(nBlocks); + retval += DBCellRecord.calculateSizeOfRecords(nBlocks, nRows); } // Add space for UncalcedRecord - if (uncalced) { + if (_isUncalced) { retval += UncalcedRecord.getStaticRecordSize(); } - return retval; } @@ -2518,13 +2498,13 @@ public final class Sheet implements Model { * @return whether an uncalced record must be inserted or not at generation */ public boolean getUncalced() { - return uncalced; + return _isUncalced; } /** * @param uncalced whether an uncalced record must be inserted or not at generation */ public void setUncalced(boolean uncalced) { - this.uncalced = uncalced; + this._isUncalced = uncalced; } /** diff --git a/src/java/org/apache/poi/hssf/record/DBCellRecord.java b/src/java/org/apache/poi/hssf/record/DBCellRecord.java index caeb333a52..1da6b82c73 100644 --- a/src/java/org/apache/poi/hssf/record/DBCellRecord.java +++ b/src/java/org/apache/poi/hssf/record/DBCellRecord.java @@ -1,4 +1,3 @@ - /* ==================================================================== Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with @@ -15,7 +14,6 @@ See the License for the specific language governing permissions and limitations under the License. ==================================================================== */ - package org.apache.poi.hssf.record; @@ -29,10 +27,7 @@ import org.apache.poi.util.LittleEndian; * @author Jason Height * @version 2.0-pre */ - -public class DBCellRecord - extends Record -{ +public final class DBCellRecord extends Record { public final static int BLOCK_SIZE = 32; public final static short sid = 0xd7; private int field_1_row_offset; @@ -46,7 +41,6 @@ public class DBCellRecord * Constructs a DBCellRecord and sets its fields appropriately * @param in the RecordInputstream to read the record from */ - public DBCellRecord(RecordInputStream in) { super(in); @@ -78,7 +72,6 @@ public class DBCellRecord * * @param offset offset to the start of the first cell in the next DBCell block */ - public void setRowOffset(int offset) { field_1_row_offset = offset; @@ -108,7 +101,6 @@ public class DBCellRecord * * @return rowoffset to the start of the first cell in the next DBCell block */ - public int getRowOffset() { return field_1_row_offset; @@ -120,7 +112,6 @@ public class DBCellRecord * @param index of the cell offset to retrieve * @return celloffset from the celloffset array */ - public short getCellOffsetAt(int index) { return field_2_cell_offsets[ index ]; @@ -131,7 +122,6 @@ public class DBCellRecord * * @return number of cell offsets */ - public int getNumCellOffsets() { return field_2_cell_offsets.length; @@ -175,9 +165,15 @@ public class DBCellRecord return 8 + (getNumCellOffsets() * 2); } - /** Returns the size of a DBCellRecord when it needs to reference a certain number of rows*/ - public static int getRecordSizeForRows(int rows) { - return 8 + (rows * 2); + /** + * @returns the size of the group of DBCellRecords needed to encode + * the specified number of blocks and rows + */ + public static int calculateSizeOfRecords(int nBlocks, int nRows) { + // One DBCell per block. + // 8 bytes per DBCell (non variable section) + // 2 bytes per row reference + return nBlocks * 8 + nRows * 2; } public short getSid() diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java b/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java index 896ef19c14..98db9e037b 100644 --- a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java +++ b/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java @@ -15,12 +15,6 @@ limitations under the License. ==================================================================== */ - -/* - * HSSFWorkbook.java - * - * Created on September 30, 2001, 3:37 PM - */ package org.apache.poi.hssf.usermodel; import org.apache.poi.POIDocument; @@ -64,7 +58,6 @@ import java.util.Stack; * @author Shawn Laubach (slaubach at apache dot org) * @version 2.0-pre */ - public class HSSFWorkbook extends POIDocument { private static final int DEBUG = POILogger.DEBUG; @@ -88,7 +81,7 @@ public class HSSFWorkbook extends POIDocument * this holds the HSSFSheet objects attached to this workbook */ - protected ArrayList sheets; + protected List _sheets; /** * this holds the HSSFName objects attached to this workbook @@ -142,7 +135,7 @@ public class HSSFWorkbook extends POIDocument { super(null, null); workbook = book; - sheets = new ArrayList( INITIAL_CAPACITY ); + _sheets = new ArrayList( INITIAL_CAPACITY ); names = new ArrayList( INITIAL_CAPACITY ); } @@ -233,7 +226,7 @@ public class HSSFWorkbook extends POIDocument this.directory = null; } - sheets = new ArrayList(INITIAL_CAPACITY); + _sheets = new ArrayList(INITIAL_CAPACITY); names = new ArrayList(INITIAL_CAPACITY); // Grab the data from the workbook stream, however @@ -263,7 +256,7 @@ public class HSSFWorkbook extends POIDocument HSSFSheet hsheet = new HSSFSheet(this, sheet); - sheets.add(hsheet); + _sheets.add(hsheet); // workbook.setSheetName(sheets.size() -1, "Sheet"+sheets.size()); } @@ -361,12 +354,12 @@ public class HSSFWorkbook extends POIDocument */ public void setSheetOrder(String sheetname, int pos ) { - sheets.add(pos,sheets.remove(getSheetIndex(sheetname))); + _sheets.add(pos,_sheets.remove(getSheetIndex(sheetname))); workbook.setSheetOrder(sheetname, pos); } private void validateSheetIndex(int index) { - int lastSheetIx = sheets.size() - 1; + int lastSheetIx = _sheets.size() - 1; if (index < 0 || index > lastSheetIx) { throw new IllegalArgumentException("Sheet index (" + index +") is out of range (0.." + lastSheetIx + ")"); @@ -380,7 +373,7 @@ public class HSSFWorkbook extends POIDocument public void setSelectedTab(int index) { validateSheetIndex(index); - int nSheets = sheets.size(); + int nSheets = _sheets.size(); for (int i=0; i (sheets.size() - 1)) - { - throw new RuntimeException("Sheet out of bounds"); } - - workbook.setSheetName( sheet, name); + validateSheetIndex(sheetIx); + workbook.setSheetName(sheetIx, name); } @@ -516,15 +505,12 @@ public class HSSFWorkbook extends POIDocument * or contains /\?*[] * @param sheet number (0 based) */ - public void setSheetName( int sheet, String name, short encoding ) + public void setSheetName(int sheetIx, String name, short encoding) { - if (workbook.doesContainsSheetName( name, sheet )) + if (workbook.doesContainsSheetName( name, sheetIx )) { throw new IllegalArgumentException( "The workbook already contains a sheet with this name" ); - - if (sheet > (sheets.size() - 1)) - { - throw new RuntimeException("Sheet out of bounds"); } + validateSheetIndex(sheetIx); switch ( encoding ) { case ENCODING_COMPRESSED_UNICODE: @@ -536,51 +522,39 @@ public class HSSFWorkbook extends POIDocument throw new RuntimeException( "Unsupported encoding" ); } - workbook.setSheetName( sheet, name, encoding ); + workbook.setSheetName( sheetIx, name, encoding ); } /** * get the sheet name - * @param sheet Number + * @param sheetIx Number * @return Sheet name */ - - public String getSheetName(int sheet) + public String getSheetName(int sheetIx) { - if (sheet > (sheets.size() - 1)) - { - throw new RuntimeException("Sheet out of bounds"); - } - return workbook.getSheetName(sheet); + validateSheetIndex(sheetIx); + return workbook.getSheetName(sheetIx); } /** * check whether a sheet is hidden - * @param sheet Number + * @param sheetIx Number * @return True if sheet is hidden */ - - public boolean isSheetHidden(int sheet) { - if (sheet > (sheets.size() - 1)) - { - throw new RuntimeException("Sheet out of bounds"); - } - return workbook.isSheetHidden(sheet); + public boolean isSheetHidden(int sheetIx) { + validateSheetIndex(sheetIx); + return workbook.isSheetHidden(sheetIx); } /** * Hide or unhide a sheet * - * @param sheetnum The sheet number + * @param sheetIx The sheet index * @param hidden True to mark the sheet as hidden, false otherwise */ - - public void setSheetHidden(int sheet, boolean hidden) { - if (sheet > (sheets.size() - 1)) - { - throw new RuntimeException("Sheet out of bounds"); - } - workbook.setSheetHidden(sheet,hidden); + public void setSheetHidden(int sheetIx, boolean hidden) { + validateSheetIndex(sheetIx); + workbook.setSheetHidden(sheetIx, hidden); } /* @@ -602,12 +576,12 @@ public class HSSFWorkbook extends POIDocument /** Returns the index of the given sheet * @param sheet the sheet to look up - * @return index of the sheet (0 based) + * @return index of the sheet (0 based). -1 if not found */ public int getSheetIndex(HSSFSheet sheet) { - for(int i=0; iUncalcedRecord was present.

+ */ + public void testUncalcSize_bug45066() { + + List records = new ArrayList(); + records.add(new BOFRecord()); + records.add(new UncalcedRecord()); + records.add(new EOFRecord()); + Sheet sheet = Sheet.createSheet( records, 0, 0 ); + + int estimatedSize = sheet.getSize(); + int serializedSize = sheet.serialize(0, new byte[estimatedSize]); + if (serializedSize != estimatedSize) { + throw new AssertionFailedError("Identified bug 45066 b"); + } + assertEquals(50, serializedSize); + } } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java index cb5b3d3554..b9873fa4b6 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java @@ -20,12 +20,16 @@ package org.apache.poi.hssf.usermodel; import java.io.File; import java.io.FileOutputStream; import java.io.IOException; +import java.util.List; import junit.framework.AssertionFailedError; import junit.framework.TestCase; import org.apache.poi.hssf.HSSFTestDataSamples; +import org.apache.poi.hssf.model.Sheet; import org.apache.poi.hssf.record.NameRecord; +import org.apache.poi.hssf.record.Record; +import org.apache.poi.hssf.record.RecordInputStream; import org.apache.poi.util.TempFile; /** * @@ -376,4 +380,49 @@ public final class TestHSSFWorkbook extends TestCase { assertEquals("active", expectedActive, sheet.isActive()); assertEquals("selected", expectedSelected, sheet.isSelected()); } -} + + /** + * If Sheet.getSize() returns a different result to Sheet.serialize(), this will cause the BOF + * records to be written with invalid offset indexes. Excel does not like this, and such + * errors are particularly hard to track down. This test ensures that HSSFWorkbook throws + * a specific exception as soon as the situation is detected. See bugzilla 45066 + */ + public void testSheetSerializeSizeMismatch_bug45066() { + HSSFWorkbook wb = new HSSFWorkbook(); + Sheet sheet = wb.createSheet("Sheet1").getSheet(); + List sheetRecords = sheet.getRecords(); + // one way (of many) to cause the discrepancy is with a badly behaved record: + sheetRecords.add(new BadlyBehavedRecord()); + // There is also much logic inside Sheet that (if buggy) might also cause the discrepancy + try { + wb.getBytes(); + throw new AssertionFailedError("Identified bug 45066 a"); + } catch (IllegalStateException e) { + // Expected badly behaved sheet record to cause exception + assertTrue(e.getMessage().startsWith("Actual serialized sheet size")); + } + } + /** + * result returned by getRecordSize() differs from result returned by serialize() + */ + private static final class BadlyBehavedRecord extends Record { + public BadlyBehavedRecord() { + // + } + protected void fillFields(RecordInputStream in) { + throw new RuntimeException("Should not be called"); + } + public short getSid() { + return 0x777; + } + public int serialize(int offset, byte[] data) { + return 4; + } + protected void validateSid(short id) { + throw new RuntimeException("Should not be called"); + } + public int getRecordSize() { + return 8; + } + } + } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestSheetHiding.java b/src/testcases/org/apache/poi/hssf/usermodel/TestSheetHiding.java index fc2a24b782..62a26e90b5 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestSheetHiding.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestSheetHiding.java @@ -45,8 +45,8 @@ public final class TestSheetHiding extends TestCase { */ public void testTextSheets() throws Exception { // Both should have two sheets - assertEquals(2, wbH.sheets.size()); - assertEquals(2, wbU.sheets.size()); + assertEquals(2, wbH.getNumberOfSheets()); + assertEquals(2, wbU.getNumberOfSheets()); // All sheets should have one row assertEquals(0, wbH.getSheetAt(0).getLastRowNum()); -- 2.39.5