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
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 21a9257..5a6133a 100644
--- 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 @@
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 @@
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 @@
}
// 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 @@
*
* @see org.apache.poi.xssf.usermodel.XSSFSheet#write(java.io.OutputStream) ()
*/
- protected void onDocumentWrite(){
- CTCell[] cArray = new CTCell[_cells.size()];
- int i = 0;
- for (XSSFCell xssfCell : _cells.values()) {
- cArray[i] = (CTCell) xssfCell.getCTCell().copy();
+ 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
- // 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]);
+ // 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 (CTCell ctCell : cArrayOrig) {
+ cArrayCopy[i] = (CTCell) ctCell.copy();
+ map.put(ctCell, i);
i++;
}
- _row.setCArray(cArray);
+ // 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++;
+ }
+
+ // remove any remaining illegal references in _rows.cArray
+ while(cArrayOrig.size() > _cells.size()) {
+ _row.removeC(_cells.size());
+ }
}
/**
@@ -696,7 +739,7 @@
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 c7bc49d..463322a 100644
--- 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 @@
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 cf8c526..6985625 100644
--- 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 @@
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);
- 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("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());
}
}
