You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@poi.apache.org by ce...@apache.org on 2019/08/12 16:42:46 UTC
svn commit: r1864977 - in /poi/trunk/src/ooxml:
java/org/apache/poi/xssf/usermodel/XSSFRow.java
testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java
testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java
Author: centic
Date: Mon Aug 12 16:42:46 2019
New Revision: 1864977
URL: http://svn.apache.org/viewvc?rev=1864977&view=rev
Log:
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.
Modified:
poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java
poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java
poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java
Modified: poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java?rev=1864977&r1=1864976&r2=1864977&view=diff
==============================================================================
--- poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java (original)
+++ poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java Mon Aug 12 16:42:46 2019
@@ -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, Com
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, Com
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, Com
}
// 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, Com
*
* @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();
+ for (CTCell ctCell : cArrayOrig) {
+ cArrayCopy[i] = (CTCell) ctCell.copy();
+ map.put(ctCell, i);
+ i++;
+ }
- // 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]);
+ // 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, Com
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);
Modified: poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java?rev=1864977&r1=1864976&r2=1864977&view=diff
==============================================================================
--- poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java (original)
+++ poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java Mon Aug 12 16:42:46 2019
@@ -415,8 +415,13 @@ public final class TestXSSFCell extends
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);
Modified: poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java?rev=1864977&r1=1864976&r2=1864977&view=diff
==============================================================================
--- poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java (original)
+++ poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFRow.java Mon Aug 12 16:42:46 2019
@@ -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 B
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());
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@poi.apache.org
For additional commands, e-mail: commits-help@poi.apache.org