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

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

diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java
index 21a925731345536d2ca6284928b7d0a24f979dec..5a6133ab676b3bb686cf0e3bf98abb9ff8aec65d 100644 (file)
--- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java
+++ b/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);

diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java
index c7bc49d546fd400a6dc73acbe49e7b160c86f093..463322a535fd3198b872ffddcfeb4b6778238d64 100644 (file)
--- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java
+++ b/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);

diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java
index cf8c526355d70adb23958a97abc44d8c1ffb5a79..698562570135ab3cc6d362cfe1299a37d5256a8e 100644 (file)
--- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java
+++ b/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());
     }
 }