Bug 63657: Rework the for bug #62130 to not use up twice as much memory when writing documents.

Unfortunately XMLBeans is very tricky to use here, mainly the fact that setCArray does not replace the internal objects, but copies the content into the currently held objects makes it rather hard to do this right.

Therefore we now try to keep the existing objects and only replace the content as required to
have a stable ordering of cells in the row-XML structure.

This also fixes removing cells from rows to avoid invalid situations and
correctly free CTCellImpl instances.

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

src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java

@@ -18,7 +18,10 @@
 package org.apache.poi.xssf.usermodel;
 
 import java.util.HashSet;
+import java.util.IdentityHashMap;
 import java.util.Iterator;
+import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 import java.util.TreeMap;
 
@@ -220,7 +223,15 @@ public class XSSFRow implements Row, Comparable<XSSFRow> {
             ctCell = _row.addNewC();
         }
         XSSFCell xcell = new XSSFCell(this, ctCell);
-        xcell.setCellNum(columnIndex);
+        try {
+            xcell.setCellNum(columnIndex);
+        } catch (IllegalArgumentException e) {
+            // we need to undo adding the CTCell in _row if something fails here, e.g.
+            // cell-limits are exceeded
+            _row.removeC(_row.getCList().size()-1);
+
+            throw e;
+        }
         if (type != CellType.BLANK && type != CellType.FORMULA) {
             setDefaultValue(xcell, type);
         }
@@ -499,6 +510,10 @@ public class XSSFRow implements Row, Comparable<XSSFRow> {
         if (cell.getRow() != this) {
             throw new IllegalArgumentException("Specified cell does not belong to this row");
         }
+        //noinspection SuspiciousMethodCalls
+        if(!_cells.containsValue(cell)) {
+            throw new IllegalArgumentException("the row does not contain this cell");
+        }
 
         XSSFCell xcell = (XSSFCell)cell;
         if(xcell.isPartOfArrayFormulaGroup()) {
@@ -509,7 +524,18 @@ public class XSSFRow implements Row, Comparable<XSSFRow> {
         }
         // Performance optimization for bug 57840: explicit boxing is slightly faster than auto-unboxing, though may use more memory
         final Integer colI = Integer.valueOf(cell.getColumnIndex()); // NOSONAR
-        _cells.remove(colI);
+        XSSFCell removed = _cells.remove(colI);
+
+        // also remove the corresponding CTCell from the _row.cArray,
+        // it may not be at the same position right now
+        // thus search for it
+        int i = 0;
+        for (CTCell ctCell : _row.getCArray()) {
+            if(ctCell == removed.getCTCell()) {
+                _row.removeC(i);
+            }
+            i++;
+        }
     }
 
     /**
@@ -527,22 +553,39 @@ public class XSSFRow implements Row, Comparable<XSSFRow> {
      *
      * @see org.apache.poi.xssf.usermodel.XSSFSheet#write(java.io.OutputStream) ()
      */
-    protected void onDocumentWrite(){
-        CTCell[] cArray = new CTCell[_cells.size()];
+    protected void onDocumentWrite() {
+        // _row.cArray and _cells.getCTCell might be out of sync after adding/removing cells,
+        // thus we need to re-order it here to make the resulting file correct
+
+        // copy all values to 2nd array and a map for lookup of index
+        List<CTCell> cArrayOrig = _row.getCList();
+        CTCell[] cArrayCopy = new CTCell[cArrayOrig.size()];
+        IdentityHashMap<CTCell, Integer> map = new IdentityHashMap<>(_cells.size());
         int i = 0;
-        for (XSSFCell xssfCell : _cells.values()) {
-            cArray[i] = (CTCell) xssfCell.getCTCell().copy();
-
-            // we have to copy and re-create the XSSFCell here because the
-            // elements as otherwise setCArray below invalidates all the columns!
-            // see Bug 56170, XMLBeans seems to always release previous objects
-            // in the CArray, so we need to provide completely new ones here!
-            //_cells.put(entry.getKey(), new XSSFCell(this, cArray[i]));
-            xssfCell.setCTCell(cArray[i]);
+        for (CTCell ctCell : cArrayOrig) {
+            cArrayCopy[i] = (CTCell) ctCell.copy();
+            map.put(ctCell, i);
+            i++;
+        }
+
+        // populate _row.cArray correctly
+        i = 0;
+        for (XSSFCell cell : _cells.values()) {
+            // no need to change anything if position is correct
+            Integer correctPosition = map.get(cell.getCTCell());
+            Objects.requireNonNull(correctPosition, "Should find CTCell in _row");
+            if(correctPosition != i) {
+                // we need to re-populate this CTCell
+                _row.setCArray(i, cArrayCopy[correctPosition]);
+                cell.setCTCell(_row.getCArray(i));
+            }
             i++;
         }
 
-        _row.setCArray(cArray);
+        // remove any remaining illegal references in _rows.cArray
+        while(cArrayOrig.size() > _cells.size()) {
+            _row.removeC(_cells.size());
+        }
     }
 
     /**
@@ -696,7 +739,7 @@ public class XSSFRow implements Row, Comparable<XSSFRow> {
 
     private void shiftCell(int columnIndex, int step/*pass negative value for left shift*/){
         if(columnIndex + step < 0) {
-            throw new IllegalStateException("Column index less than zero : " + (Integer.valueOf(columnIndex + step)).toString());
+            throw new IllegalStateException("Column index less than zero : " + (Integer.valueOf(columnIndex + step)));
         }
 
         XSSFCell currentCell = getCell(columnIndex);

src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java

@@ -415,8 +415,13 @@ public final class TestXSSFCell extends BaseTestXCell {
 
             validateRow(row);
 
-            // once again with removing one cell
-            row.removeCell(cell1);
+            // once again with removing the same cell, this throws an exception
+            try {
+                row.removeCell(cell1);
+                fail("Should catch an exception");
+            } catch (IllegalArgumentException e) {
+                // expected here
+            }
 
             // now check again
             validateRow(row);

src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java

@@ -18,7 +18,6 @@
 package org.apache.poi.xssf.usermodel;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertSame;
 
@@ -196,24 +195,44 @@ public final class TestXSSFRow extends BaseTestXRow {
         
         workbook.close();
     }
-    
+
     @Test
     public void testMultipleEditWriteCycles() {
         final XSSFWorkbook wb1 = new XSSFWorkbook();
         final XSSFSheet sheet1 = wb1.createSheet("Sheet1");
-        final XSSFRow srcRow = sheet1.createRow(0);
+        XSSFRow srcRow = sheet1.createRow(0);
         srcRow.createCell(0).setCellValue("hello");
         srcRow.createCell(3).setCellValue("world");
-        
+
         // discard result
         XSSFTestDataSamples.writeOutAndReadBack(wb1);
+
+        assertEquals("hello", srcRow.getCell(0).getStringCellValue());
+        assertEquals("hello",
+                wb1.getSheet("Sheet1").getRow(0).getCell(0).getStringCellValue());
+        assertEquals("world", srcRow.getCell(3).getStringCellValue());
+        assertEquals("world",
+                wb1.getSheet("Sheet1").getRow(0).getCell(3).getStringCellValue());
+
         srcRow.createCell(1).setCellValue("cruel");
+
         // discard result
         XSSFTestDataSamples.writeOutAndReadBack(wb1);
 
+        assertEquals("hello", srcRow.getCell(0).getStringCellValue());
+        assertEquals("hello",
+                wb1.getSheet("Sheet1").getRow(0).getCell(0).getStringCellValue());
+        assertEquals("cruel", srcRow.getCell(1).getStringCellValue());
+        assertEquals("cruel",
+                wb1.getSheet("Sheet1").getRow(0).getCell(1).getStringCellValue());
+        assertEquals("world", srcRow.getCell(3).getStringCellValue());
+        assertEquals("world",
+                wb1.getSheet("Sheet1").getRow(0).getCell(3).getStringCellValue());
+
         srcRow.getCell(1).setCellValue((RichTextString) null);
-        
+
         XSSFWorkbook wb3 = XSSFTestDataSamples.writeOutAndReadBack(wb1);
-        assertEquals("Cell not blank", CellType.BLANK, wb3.getSheet("Sheet1").getRow(0).getCell(1).getCellType());
+        assertEquals("Cell should be blank", CellType.BLANK,
+                wb3.getSheet("Sheet1").getRow(0).getCell(1).getCellType());
     }
 }