Fix for bug 46312 - ValueRecordsAggregate should handle removal of new empty row

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@722206 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-beta5" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46312 - Fixed ValueRecordsAggregate to handle removal of new empty row</action>
            <action dev="POI-DEVELOPERS" type="add">46269 - Improved error message when attempting to read BIFF2 file</action>
            <action dev="POI-DEVELOPERS" type="fix">46206 - Fixed Sheet to tolerate missing DIMENSION records</action>
            <action dev="POI-DEVELOPERS" type="add">46301 - added pivot table records: SXDI, SXVDEX, SXPI, SXIDSTM, SXVIEW, SXVD, SXVS, et al</action>

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.5-beta5" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46312 - Fixed ValueRecordsAggregate to handle removal of new empty row</action>
            <action dev="POI-DEVELOPERS" type="add">46269 - Improved error message when attempting to read BIFF2 file</action>
            <action dev="POI-DEVELOPERS" type="fix">46206 - Fixed Sheet to tolerate missing DIMENSION records</action>
            <action dev="POI-DEVELOPERS" type="add">46301 - added pivot table records: SXDI, SXVDEX, SXPI, SXIDSTM, SXVIEW, SXVD, SXVS, et al</action>

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

@@ -40,14 +40,14 @@ import org.apache.poi.hssf.record.formula.Ptg;
  * @author Jason Height (jheight at chariot dot net dot au)
  */
 public final class ValueRecordsAggregate {
+	private static final int MAX_ROW_INDEX = 0XFFFF;
 	private int firstcell = -1;
 	private int lastcell  = -1;
 	private CellValueRecordInterface[][] records;
 
 	/** Creates a new instance of ValueRecordsAggregate */
 
-    public ValueRecordsAggregate()
+	public ValueRecordsAggregate() {
-    {
 		records = new CellValueRecordInterface[30][]; // We start with 30 Rows.
 	}
 
@@ -57,21 +57,24 @@ public final class ValueRecordsAggregate {
 		if (row >= records.length) {
 			CellValueRecordInterface[][] oldRecords = records;
 			int newSize = oldRecords.length * 2;
-      if(newSize<row+1) newSize=row+1;
+			if (newSize < row + 1)
+				newSize = row + 1;
 			records = new CellValueRecordInterface[newSize][];
 			System.arraycopy(oldRecords, 0, records, 0, oldRecords.length);
 		}
 		CellValueRecordInterface[] rowCells = records[row];
 		if (rowCells == null) {
 			int newSize = column + 1;
-      if(newSize<10) newSize=10;
+			if (newSize < 10)
+				newSize = 10;
 			rowCells = new CellValueRecordInterface[newSize];
 			records[row] = rowCells;
 		}
 		if (column >= rowCells.length) {
 			CellValueRecordInterface[] oldRowCells = rowCells;
 			int newSize = oldRowCells.length * 2;
-      if(newSize<column+1) newSize=column+1;
+			if (newSize < column + 1)
+				newSize = column + 1;
 			// if(newSize>257) newSize=257; // activate?
 			rowCells = new CellValueRecordInterface[newSize];
 			System.arraycopy(oldRowCells, 0, rowCells, 0, oldRowCells.length);
@@ -107,34 +110,39 @@ public final class ValueRecordsAggregate {
 	}
 
 	public void removeAllCellsValuesForRow(int rowIndex) {
-        if (rowIndex >= records.length) {
+		if (rowIndex < 0 || rowIndex > MAX_ROW_INDEX) {
 			throw new IllegalArgumentException("Specified rowIndex " + rowIndex
-                    + " is outside the allowable range (0.." +records.length + ")");
+					+ " is outside the allowable range (0.." +MAX_ROW_INDEX + ")");
 		}
+		if (rowIndex >= records.length) {
+			// this can happen when the client code has created a row, 
+			// and then removes/replaces it before adding any cells. (see bug 46312) 
+			return;
+		} 
+		
 		records[rowIndex] = null;
 	}
 
 
-    public int getPhysicalNumberOfCells()
+	public int getPhysicalNumberOfCells() {
-    {
+		int count = 0;
-    int count=0;
+		for (int r = 0; r < records.length; r++) {
-    for(int r=0;r<records.length;r++) {
+			CellValueRecordInterface[] rowCells = records[r];
-      CellValueRecordInterface[] rowCells=records[r];
+			if (rowCells != null) {
-      if (rowCells != null)
+				for (int c = 0; c < rowCells.length; c++) {
-        for(short c=0;c<rowCells.length;c++) {
+					if (rowCells[c] != null)
-          if(rowCells[c]!=null) count++;
+						count++;
+				}
 			}
 		}
 		return count;
 	}
 
-    public int getFirstCellNum()
+	public int getFirstCellNum() {
-    {
 		return firstcell;
 	}
 
-    public int getLastCellNum()
+	public int getLastCellNum() {
-    {
 		return lastcell;
 	}
 
@@ -163,24 +171,27 @@ public final class ValueRecordsAggregate {
 	 *  that are attached to the rows in the range specified.
 	 */
 	public int getRowCellBlockSize(int startRow, int endRow) {
-      MyIterator itr = new MyIterator(startRow, endRow);
+		MyIterator itr = new MyIterator(records, startRow, endRow);
 		int size = 0;
 		while (itr.hasNext()) {
-        CellValueRecordInterface cell = (CellValueRecordInterface)itr.next();
+			CellValueRecordInterface cell = (CellValueRecordInterface) itr.next();
 			int row = cell.getRow();
-        if (row > endRow)
+			if (row > endRow) {
 				break;
-        if ((row >=startRow) && (row <= endRow))
+			}
-          size += ((RecordBase)cell).getRecordSize();
+			if ((row >= startRow) && (row <= endRow)) {
+				size += ((RecordBase) cell).getRecordSize();
+			}
 		}
 		return size;
 	}
 
 	/** Returns true if the row has cells attached to it */
 	public boolean rowHasCells(int row) {
-        if (row > records.length-1) //previously this said row > records.length which means if
+		if (row >= records.length) {
-            return false;  // if records.length == 60 and I pass "60" here I get array out of bounds
+			return false;
-      CellValueRecordInterface[] rowCells=records[row]; //because a 60 length array has the last index = 59
+		}
+		CellValueRecordInterface[] rowCells=records[row]; 
 		if(rowCells==null) return false;
 		for(int col=0;col<rowCells.length;col++) {
 			if(rowCells[col]!=null) return true;
@@ -191,7 +202,7 @@ public final class ValueRecordsAggregate {
 	/** Serializes the cells that are allocated to a certain row range*/
 	public int serializeCellRow(final int row, int offset, byte [] data)
 	{
-      MyIterator itr = new MyIterator(row, row);
+	  MyIterator itr = new MyIterator(records, row, row);
 		int	  pos = offset;
 
 		while (itr.hasNext())
@@ -244,7 +255,7 @@ public final class ValueRecordsAggregate {
 	}
 
 	public CellValueRecordInterface[] getValueRecords() {
-        List temp = new ArrayList();
+		List<CellValueRecordInterface> temp = new ArrayList<CellValueRecordInterface>();
 
 		for (int i = 0; i < records.length; i++) {
 			CellValueRecordInterface[] rowCells = records[i];
@@ -263,54 +274,55 @@ public final class ValueRecordsAggregate {
 		temp.toArray(result);
 		return result;
 	}
-    public Iterator getIterator()
+	public Iterator getIterator() {
-    {
+		return new MyIterator(records);
-    return new MyIterator();
 	}
 
-  private final class MyIterator implements Iterator {
+  private static final class MyIterator implements Iterator {
-    short nextColumn=-1;
+		private final CellValueRecordInterface[][] records;
-    int nextRow,lastRow;
+		private short nextColumn = -1;
+		private int nextRow, lastRow;
 
-    public MyIterator()
+		public MyIterator(CellValueRecordInterface[][] pRecords) {
-    {
+			this(pRecords, 0, pRecords.length - 1);
-      this.nextRow=0;
-      this.lastRow=records.length-1;
-      findNext();
 		}
 
-    public MyIterator(int firstRow,int lastRow)
+		public MyIterator(CellValueRecordInterface[][] pRecords, int firstRow, int lastRow) {
-    {
+			records = pRecords;
-      this.nextRow=firstRow;
+			this.nextRow = firstRow;
-      this.lastRow=lastRow;
+			this.lastRow = lastRow;
 			findNext();
 		}
 
 		public boolean hasNext() {
-      return nextRow<=lastRow;
+			return nextRow <= lastRow;
 		}
+
 		public Object next() {
-      Object o=records[nextRow][nextColumn];
+			Object o = records[nextRow][nextColumn];
 			findNext();
 			return o;
 		}
+
 		public void remove() {
 			throw new UnsupportedOperationException("gibt's noch nicht");
 		}
 
 		private void findNext() {
 			nextColumn++;
-      for(;nextRow<=lastRow;nextRow++) {
+			for (; nextRow <= lastRow; nextRow++) {
-                                           //previously this threw array out of bounds...
+				// previously this threw array out of bounds...
-        CellValueRecordInterface[] rowCells=(nextRow < records.length) ? records[nextRow] : null;
+				CellValueRecordInterface[] rowCells = (nextRow < records.length) ? records[nextRow]
-        if(rowCells==null) { // This row is empty
+						: null;
-          nextColumn=0;
+				if (rowCells == null) { // This row is empty
+					nextColumn = 0;
 					continue;
 				}
-        for(;nextColumn<rowCells.length;nextColumn++) {
+				for (; nextColumn < rowCells.length; nextColumn++) {
-          if(rowCells[nextColumn]!=null) return;
+					if (rowCells[nextColumn] != null)
+						return;
 				}
-        nextColumn=0;
+				nextColumn = 0;
 			}
 		}
 

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

@@ -37,9 +37,13 @@ import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.RecordBase;
 import org.apache.poi.hssf.record.SharedFormulaRecord;
 import org.apache.poi.hssf.record.WindowTwoRecord;
+import org.apache.poi.hssf.usermodel.HSSFRow;
 import org.apache.poi.hssf.usermodel.HSSFSheet;
 import org.apache.poi.hssf.usermodel.HSSFWorkbook;
 
+/**
+ * Tests for {@link ValueRecordsAggregate}
+ */
 public final class TestValueRecordsAggregate extends TestCase {
 	private static final String ABNORMAL_SHARED_FORMULA_FLAG_TEST_FILE = "AbnormalSharedFormulaFlag.xls";
 	private final ValueRecordsAggregate valueRecord = new ValueRecordsAggregate();
@@ -49,9 +53,9 @@ public final class TestValueRecordsAggregate extends TestCase {
 	 * as part of the value records
 	 */
 	public void testSharedFormula() {
-        List records = new ArrayList();
+		List<Record> records = new ArrayList<Record>();
-        records.add( new FormulaRecord() );
+		records.add(new FormulaRecord());
-        records.add( new SharedFormulaRecord() );
+		records.add(new SharedFormulaRecord());
 		records.add(new WindowTwoRecord());
 
 		constructValueRecord(records);
@@ -75,15 +79,15 @@ public final class TestValueRecordsAggregate extends TestCase {
 	}
 
 	private static List testData() {
-        List records = new ArrayList();
+		List<Record> records = new ArrayList<Record>();
 		FormulaRecord formulaRecord = new FormulaRecord();
 		BlankRecord blankRecord = new BlankRecord();
-        formulaRecord.setRow( 1 );
+		formulaRecord.setRow(1);
-        formulaRecord.setColumn( (short) 1 );
+		formulaRecord.setColumn((short) 1);
-        blankRecord.setRow( 2 );
+		blankRecord.setRow(2);
-        blankRecord.setColumn( (short) 2 );
+		blankRecord.setColumn((short) 2);
-        records.add( formulaRecord );
+		records.add(formulaRecord);
-        records.add( blankRecord );
+		records.add(blankRecord);
 		records.add(new WindowTwoRecord());
 		return records;
 	}
@@ -282,4 +286,33 @@ public final class TestValueRecordsAggregate extends TestCase {
 		
 		return crc.getValue();
 	}
+	public void testRemoveNewRow_bug46312() {
+		// To make bug occur, rowIndex needs to be >= ValueRecordsAggregate.records.length
+		int rowIndex = 30;
+		
+		ValueRecordsAggregate vra = new ValueRecordsAggregate();
+		try {
+			vra.removeAllCellsValuesForRow(rowIndex);
+		} catch (IllegalArgumentException e) {
+			if (e.getMessage().equals("Specified rowIndex 30 is outside the allowable range (0..30)")) {
+				throw new AssertionFailedError("Identified bug 46312");
+			}
+			throw e;
+		}
+
+		if (false) { // same bug as demonstrated through usermodel API
+
+			HSSFWorkbook wb = new HSSFWorkbook();
+			HSSFSheet sheet = wb.createSheet();
+			HSSFRow row = sheet.createRow(rowIndex);
+			if (false) { // must not add any cells to the new row if we want to see the bug
+				row.createCell(0); // this causes ValueRecordsAggregate.records to auto-extend
+			}
+			try {
+				sheet.createRow(rowIndex);
+			} catch (IllegalArgumentException e) {
+				throw new AssertionFailedError("Identified bug 46312");
+			}
+		}
+	}
 }