Patch from Josh from bug #43901 - Correctly update the internal last cell number when adding and removing cells (previously sometimes off-by-one)

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@634617 13f79535-47bb-0310-9956-ffa450edef68

src/documentation/content/xdocs/changes.xml

@@ -36,6 +36,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.1-beta1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">43901 - Correctly update the internal last cell number when adding and removing cells (previously sometimes off-by-one)</action>
            <action dev="POI-DEVELOPERS" type="fix">28231 - For apparently truncated files, which are somehow still valid, now issue a truncation warning but carry on, rather than giving an exception as before</action>
            <action dev="POI-DEVELOPERS" type="fix">44504 - Added initial support for recognising external functions like YEARFRAC and ISEVEN (using NameXPtg), via LinkTable support</action>
            <action dev="POI-DEVELOPERS" type="fix">44504 - Improvements to FormulaParser - operators, precedence, error literals, quotes in string literals, range checking on IntPtg, formulas with extra un-parsed stuff at the end, improved parse error handling</action>

src/documentation/content/xdocs/status.xml

@@ -33,6 +33,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1-beta1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">43901 - Correctly update the internal last cell number when adding and removing cells (previously sometimes off-by-one)</action>
            <action dev="POI-DEVELOPERS" type="fix">44504 - Added initial support for recognising external functions like YEARFRAC and ISEVEN (using NameXPtg), via LinkTable support</action>
            <action dev="POI-DEVELOPERS" type="fix">44504 - Improvements to FormulaParser - operators, precedence, error literals, quotes in string literals, range checking on IntPtg, formulas with extra un-parsed stuff at the end, improved parse error handling</action>
            <action dev="POI-DEVELOPERS" type="fix">44504 - Fixed number conversion inconsistencies in many functions, and improved RefEval</action>

src/java/org/apache/poi/hssf/usermodel/HSSFRow.java

@@ -15,11 +15,6 @@
    limitations under the License.
 ==================================================================== */
 
-/*
- * HSSFRow.java
- *
- * Created on September 30, 2001, 3:44 PM
- */
 package org.apache.poi.hssf.usermodel;
 
 import java.util.Iterator;
@@ -38,10 +33,7 @@ import org.apache.poi.hssf.record.RowRecord;
  * @author  Andrew C. Oliver (acoliver at apache dot org)
  * @author Glen Stampoultzis (glens at apache.org)
  */
-
+public final class HSSFRow implements Comparable {
-public class HSSFRow
-        implements Comparable
-{
 
     // used for collections
     public final static int INITIAL_CAPACITY = 5;
@@ -157,26 +149,31 @@ public class HSSFRow
      * @param cell to remove
      */
     public void removeCell(HSSFCell cell) {
-    	removeCell(cell, true);
+        if(cell == null) {
+            throw new IllegalArgumentException("cell must not be null");
+        }
+        removeCell(cell, true);
     }
     private void removeCell(HSSFCell cell, boolean alsoRemoveRecords) {
-    	if(alsoRemoveRecords) {
-	        CellValueRecordInterface cval = cell.getCellValueRecord();
-	        sheet.removeValueRecord(getRowNum(), cval);
-    	}
         
         short column=cell.getCellNum();
-        if(cell!=null && column<cells.length)
+        if(column < 0) {
-        {
+            throw new RuntimeException("Negative cell indexes not allowed");
-          cells[column]=null;
+        }
+        if(column >= cells.length || cell != cells[column]) {
+            throw new RuntimeException("Specified cell is not from this row");
+        }
+        cells[column]=null;
+        
+        if(alsoRemoveRecords) {
+            CellValueRecordInterface cval = cell.getCellValueRecord();
+            sheet.removeValueRecord(getRowNum(), cval);
         }
         
-        if (cell.getCellNum() == row.getLastCol())
+        if (cell.getCellNum()+1 == row.getLastCol()) {
-        {
+            row.setLastCol((short) (findLastCell(row.getLastCol())+1));
-            row.setLastCol(findLastCell(row.getLastCol()));
         }
-        if (cell.getCellNum() == row.getFirstCol())
+        if (cell.getCellNum() == row.getFirstCol()) {
-        {
             row.setFirstCol(findFirstCell(row.getFirstCol()));
         }
     }
@@ -234,7 +231,7 @@ public class HSSFRow
      * TODO - Should this really be public?
      */
     protected int getOutlineLevel() {
-    	return row.getOutlineLevel();
+        return row.getOutlineLevel();
     }
     
     /**
@@ -244,55 +241,48 @@ public class HSSFRow
      * @param newColumn The new column number (0 based)
      */
     public void moveCell(HSSFCell cell, short newColumn) {
-    	// Ensure the destination is free
+        // Ensure the destination is free
-    	if(cells.length > newColumn && cells[newColumn] != null) {
+        if(cells.length > newColumn && cells[newColumn] != null) {
-    		throw new IllegalArgumentException("Asked to move cell to column " + newColumn + " but there's already a cell there");
+            throw new IllegalArgumentException("Asked to move cell to column " + newColumn + " but there's already a cell there");
-    	}
+        }
         
-    	// Check it's one of ours
+        // Check it's one of ours
-    	if(! cells[cell.getCellNum()].equals(cell)) {
+        if(! cells[cell.getCellNum()].equals(cell)) {
-    		throw new IllegalArgumentException("Asked to move a cell, but it didn't belong to our row");
+            throw new IllegalArgumentException("Asked to move a cell, but it didn't belong to our row");
-    	}
+        }
         
-    	// Move the cell to the new position
+        // Move the cell to the new position
-    	// (Don't remove the records though)
+        // (Don't remove the records though)
-    	removeCell(cell, false);
+        removeCell(cell, false);
-    	cell.updateCellNum(newColumn);
+        cell.updateCellNum(newColumn);
-    	addCell(cell);
+        addCell(cell);
     }
 
     /**
      * used internally to add a cell.
      */
-    private void addCell(HSSFCell cell)
+    private void addCell(HSSFCell cell) {
-    {
-        short column=cell.getCellNum();
-        if (row.getFirstCol() == -1)
-        {
-            row.setFirstCol(column);
-        }
-        if (row.getLastCol() == -1)
-        {
-            row.setLastCol(column);
-        }
 
-        if(column>=cells.length)
+        short column=cell.getCellNum();
-        {
+        // re-allocate cells array as required.
-          HSSFCell[] oldCells=cells;
+        if(column>=cells.length) {
-          int newSize=oldCells.length*2;
+            HSSFCell[] oldCells=cells;
-          if(newSize<column+1) newSize=column+1;
+            int newSize=oldCells.length*2;
-          cells=new HSSFCell[newSize];
+            if(newSize<column+1) {
-          System.arraycopy(oldCells,0,cells,0,oldCells.length);
+                newSize=column+1;
+            }
+            cells=new HSSFCell[newSize];
+            System.arraycopy(oldCells,0,cells,0,oldCells.length);
         }
         cells[column]=cell;
         
-        if (column < row.getFirstCol())
+        // fix up firstCol and lastCol indexes
-        {
+        if (row.getFirstCol() == -1 || column < row.getFirstCol()) {
             row.setFirstCol(column);
         }
-        if (column > row.getLastCol())
+        
-        {
+        if (row.getLastCol() == -1 || column >= row.getLastCol()) {
-            row.setLastCol(column);
+            row.setLastCol((short) (column+1)); // +1 -> for one past the last index 
         }
     }
 
@@ -324,16 +314,29 @@ public class HSSFRow
     }
 
     /**
-     * gets the number of the last cell contained in this row <b>PLUS ONE</b>. 
+     * Gets the index of the last cell contained in this row <b>PLUS ONE</b>. The result also 
-     * @return short representing the last logical cell in the row <b>PLUS ONE</b>, or -1 if the row does not contain any cells.
+     * happens to be the 1-based column number of the last cell.  This value can be used as a
+     * standard upper bound when iterating over cells:
+     * <pre> 
+     * short minColIx = row.getFirstCellNum();
+     * short maxColIx = row.getLastCellNum();
+     * for(short colIx=minColIx; colIx&lt;maxColIx; colIx++) {
+     *   HSSFCell cell = row.getCell(colIx);
+     *   if(cell == null) {
+     *     continue;
+     *   }
+     *   //... do something with cell
+     * }
+     * </pre>
+     * 
+     * @return short representing the last logical cell in the row <b>PLUS ONE</b>, or -1 if the
+     *  row does not contain any cells.
      */
-
+    public short getLastCellNum() {
-    public short getLastCellNum()
+        if (getPhysicalNumberOfCells() == 0) {
-    {
-        if (getPhysicalNumberOfCells() == 0)
             return -1;
-        else
+        }
-            return row.getLastCol();
+        return row.getLastCol();
     }
 
 
@@ -493,8 +496,8 @@ public class HSSFRow
       }
 
       public Object next() {
-    	  if (!hasNext())
+          if (!hasNext())
-    		  throw new NoSuchElementException("At last element");
+              throw new NoSuchElementException("At last element");
         HSSFCell cell=cells[nextId];
         thisId=nextId;
         findNext();
@@ -502,8 +505,8 @@ public class HSSFRow
       }
 
       public void remove() {
-    	  if (thisId == -1)
+          if (thisId == -1)
-    		  throw new IllegalStateException("remove() called before next()");
+              throw new IllegalStateException("remove() called before next()");
         cells[thisId]=null;
       }
       

src/testcases/org/apache/poi/hssf/usermodel/TestBugs.java

@@ -1104,29 +1104,6 @@ extends TestCase {
         
         assertEquals(1, wb.getNumberOfSheets());
     }
-    
-    /**
-     * POI is producing files with the wrong last-column
-     *  number on the row
-     */
-    public void BROKENtest43901() {
-		HSSFWorkbook book = new HSSFWorkbook();
-		HSSFSheet sheet = book.createSheet("test");
-
-		// New row has last col -1
-		HSSFRow row = sheet.createRow(0);
-		assertEquals(-1, row.getLastCellNum());
-		if(row.getLastCellNum() == 0) {
-			fail("Identified bug 43901");
-		}
-		
-		// Create two cells, will return one higher
-		//  than that for the last number
-		row.createCell((short) 0);
-		assertEquals(1, row.getLastCellNum());
-		row.createCell((short) 255);
-		assertEquals(256, row.getLastCellNum());
-	}
 }
 
 

src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRow.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
@@ -16,33 +15,21 @@
    limitations under the License.
 ==================================================================== */
 
-
 package org.apache.poi.hssf.usermodel;
 
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+
 import junit.framework.TestCase;
 
-import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileOutputStream;
-
-import org.apache.poi.util.TempFile;
-
 /**
  * Test HSSFRow is okay.
  *
  * @author Glen Stampoultzis (glens at apache.org)
  */
-public class TestHSSFRow
+public final class TestHSSFRow extends TestCase {
-        extends TestCase
-{
-    public TestHSSFRow(String s)
-    {
-        super(s);
-    }
 
-    public void testLastAndFirstColumns()
+    public void testLastAndFirstColumns() {
-            throws Exception
-    {
         HSSFWorkbook workbook = new HSSFWorkbook();
         HSSFSheet sheet = workbook.createSheet();
         HSSFRow row = sheet.createRow((short) 0);
@@ -51,60 +38,57 @@ public class TestHSSFRow
 
         row.createCell((short) 2);
         assertEquals(2, row.getFirstCellNum());
-        assertEquals(2, row.getLastCellNum());
+        assertEquals(3, row.getLastCellNum());
 
         row.createCell((short) 1);
         assertEquals(1, row.getFirstCellNum());
-        assertEquals(2, row.getLastCellNum());
+        assertEquals(3, row.getLastCellNum());
 
         // check the exact case reported in 'bug' 43901 - notice that the cellNum is '0' based
         row.createCell((short) 3);
         assertEquals(1, row.getFirstCellNum());
-        assertEquals(3, row.getLastCellNum());
+        assertEquals(4, row.getLastCellNum());
-
     }
 
-    public void testRemoveCell()
+    public void testRemoveCell() throws Exception {
-            throws Exception
-    {
         HSSFWorkbook workbook = new HSSFWorkbook();
         HSSFSheet sheet = workbook.createSheet();
         HSSFRow row = sheet.createRow((short) 0);
         assertEquals(-1, row.getLastCellNum());
         assertEquals(-1, row.getFirstCellNum());
         row.createCell((short) 1);
-        assertEquals(1, row.getLastCellNum());
+        assertEquals(2, row.getLastCellNum());
         assertEquals(1, row.getFirstCellNum());
         row.createCell((short) 3);
-        assertEquals(3, row.getLastCellNum());
+        assertEquals(4, row.getLastCellNum());
         assertEquals(1, row.getFirstCellNum());
         row.removeCell(row.getCell((short) 3));
-        assertEquals(1, row.getLastCellNum());
+        assertEquals(2, row.getLastCellNum());
         assertEquals(1, row.getFirstCellNum());
         row.removeCell(row.getCell((short) 1));
         assertEquals(-1, row.getLastCellNum());
         assertEquals(-1, row.getFirstCellNum());
 
-        // check the row record actually writes it out as 0's
+        // all cells on this row have been removed
+        // so check the row record actually writes it out as 0's
         byte[] data = new byte[100];
         row.getRowRecord().serialize(0, data);
         assertEquals(0, data[6]);
         assertEquals(0, data[8]);
 
-        File file = TempFile.createTempFile("XXX", "XLS");
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
-        FileOutputStream stream = new FileOutputStream(file);
+        workbook.write(baos);
-        workbook.write(stream);
+        baos.close();
-        stream.close();
+        ByteArrayInputStream inputStream = new ByteArrayInputStream(baos.toByteArray());
-        FileInputStream inputStream = new FileInputStream(file);
         workbook = new HSSFWorkbook(inputStream);
         sheet = workbook.getSheetAt(0);
-        stream.close();
+        inputStream.close();
-        file.delete();
+
         assertEquals(-1, sheet.getRow((short) 0).getLastCellNum());
         assertEquals(-1, sheet.getRow((short) 0).getFirstCellNum());
     }
 
-    public void testMoveCell() throws Exception {
+    public void testMoveCell() {
         HSSFWorkbook workbook = new HSSFWorkbook();
         HSSFSheet sheet = workbook.createSheet();
         HSSFRow row = sheet.createRow((short) 0);
@@ -121,57 +105,79 @@ public class TestHSSFRow
         HSSFCell cellB4 = row.createCell((short) 3);
 
         assertEquals(1, row.getFirstCellNum());
-        assertEquals(3, row.getLastCellNum());
+        assertEquals(4, row.getLastCellNum());
 
         // Try to move to somewhere else that's used
         try {
-        	row.moveCell(cellB2, (short)3);
+            row.moveCell(cellB2, (short)3);
-        	fail();
+            fail("IllegalArgumentException should have been thrown");
-        } catch(IllegalArgumentException e) {}
+        } catch(IllegalArgumentException e) {
+            // expected during successful test
+        }
 
         // Try to move one off a different row
         try {
-        	row.moveCell(cellA2, (short)3);
+            row.moveCell(cellA2, (short)3);
-        	fail();
+            fail("IllegalArgumentException should have been thrown");
-        } catch(IllegalArgumentException e) {}
+        } catch(IllegalArgumentException e) {
+            // expected during successful test
+        }
 
         // Move somewhere spare
         assertNotNull(row.getCell((short)1));
-    	row.moveCell(cellB2, (short)5);
+        row.moveCell(cellB2, (short)5);
         assertNull(row.getCell((short)1));
         assertNotNull(row.getCell((short)5));
 
-    	assertEquals(5, cellB2.getCellNum());
+        assertEquals(5, cellB2.getCellNum());
         assertEquals(2, row.getFirstCellNum());
-        assertEquals(5, row.getLastCellNum());
+        assertEquals(6, row.getLastCellNum());
-        
     }
 
-    public void testRowBounds()
+    public void testRowBounds() {
-            throws Exception
-    {
       HSSFWorkbook workbook = new HSSFWorkbook();
       HSSFSheet sheet = workbook.createSheet();
       //Test low row bound
-      HSSFRow row = sheet.createRow( (short) 0);
+      sheet.createRow( (short) 0);
       //Test low row bound exception
-      boolean caughtException = false;
       try {
-        row = sheet.createRow(-1);        
+        sheet.createRow(-1);
+        fail("IndexOutOfBoundsException should have been thrown");
       } catch (IndexOutOfBoundsException ex) {
-        caughtException = true;
+        // expected during successful test
       }
-      assertTrue(caughtException);
+
       //Test high row bound
-      row = sheet.createRow(65535);     
+      sheet.createRow(65535);
       //Test high row bound exception
-      caughtException = false;
       try {
-        row = sheet.createRow(65536);        
+        sheet.createRow(65536);
+        fail("IndexOutOfBoundsException should have been thrown");
       } catch (IndexOutOfBoundsException ex) {
-        caughtException = true;
+        // expected during successful test
       }
-      assertTrue(caughtException);
     }
 
+    /**
+     * Prior to patch 43901, POI was producing files with the wrong last-column
+     * number on the row
+     */
+    public void testLastCellNumIsCorrectAfterAddCell_bug43901(){
+        HSSFWorkbook book = new HSSFWorkbook();
+        HSSFSheet sheet = book.createSheet("test");
+        HSSFRow row = sheet.createRow(0);
+
+        // New row has last col -1
+        assertEquals(-1, row.getLastCellNum());
+        if(row.getLastCellNum() == 0) {
+            fail("Identified bug 43901");
+        }
+
+        // Create two cells, will return one higher
+        //  than that for the last number
+        row.createCell((short) 0);
+        assertEquals(1, row.getLastCellNum());
+        row.createCell((short) 255);
+        assertEquals(256, row.getLastCellNum());
+    }
 }