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 21a9257313..5a6133ab67 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 @@ public class XSSFRow implements Row, Comparable { 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 { 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 { } // 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 { * * @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 cArrayOrig = _row.getCList(); + CTCell[] cArrayCopy = new CTCell[cArrayOrig.size()]; + IdentityHashMap 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 @@ public class XSSFRow implements Row, Comparable { 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 c7bc49d546..463322a535 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 @@ 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 cf8c526355..6985625701 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 @@ 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); - 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()); } }