From: Dominik Stadler Date: Sun, 18 May 2014 19:18:27 +0000 (+0000) Subject: Bug 56170: Fix a problem with cells in workbooks becoming disconnected from XMLBeans... X-Git-Tag: REL_3_11_BETA1~133 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=ecf9194b651813d995fa70dada3f223d0dc92735;p=poi.git Bug 56170: Fix a problem with cells in workbooks becoming disconnected from XMLBeans whenever columns need to be reordered during writing the file. This happens because setCArray() disconnects any previously stored array-item but we try to re-use them. So we need to recreate the CTCell and set it in the XSSFCell to make this work in all currently tested cases. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1595659 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFCell.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFCell.java index 47df620a58..9675b9a14b 100644 --- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFCell.java +++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFCell.java @@ -70,7 +70,7 @@ public final class XSSFCell implements Cell { * the xml bean containing information about the cell's location, value, * data type, formatting, and formula */ - private final CTCell _cell; + private CTCell _cell; /** * the XSSFRow this cell belongs to @@ -940,6 +940,16 @@ public final class XSSFCell implements Cell { public CTCell getCTCell(){ return _cell; } + + /** + * Set a new internal xml bean. This is only for internal use, do not call this from outside! + * + * This is necessary in some rare cases to work around XMLBeans specialties. + */ + @Internal + public void setCTCell(CTCell cell) { + _cell = cell; + } /** * Chooses a new boolean value for the cell when its type is changing.

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 75e17092a9..f21b1aa6d2 100644 --- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java +++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java @@ -18,6 +18,7 @@ package org.apache.poi.xssf.usermodel; import java.util.Iterator; +import java.util.Map; import java.util.TreeMap; import org.apache.poi.ss.SpreadsheetVersion; @@ -441,8 +442,9 @@ public class XSSFRow implements Row, Comparable { protected void onDocumentWrite(){ // check if cells in the CTRow are ordered boolean isOrdered = true; - if(_row.sizeOfCArray() != _cells.size()) isOrdered = false; - else { + if(_row.sizeOfCArray() != _cells.size()) { + isOrdered = false; + } else { int i = 0; for (XSSFCell cell : _cells.values()) { CTCell c1 = cell.getCTCell(); @@ -460,9 +462,18 @@ public class XSSFRow implements Row, Comparable { if(!isOrdered){ CTCell[] cArray = new CTCell[_cells.size()]; int i = 0; - for (XSSFCell c : _cells.values()) { - cArray[i++] = c.getCTCell(); + for (Map.Entry entry : _cells.entrySet()) { + cArray[i] = (CTCell) entry.getValue().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])); + entry.getValue().setCTCell(cArray[i]); + i++; } + _row.setCArray(cArray); } } diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java index f0266c9f69..9af149bf56 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java @@ -18,13 +18,7 @@ package org.apache.poi.xssf.usermodel; import static org.hamcrest.core.IsEqual.equalTo; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import static org.junit.Assert.*; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -43,24 +37,7 @@ import org.apache.poi.ss.formula.WorkbookEvaluator; import org.apache.poi.ss.formula.eval.ErrorEval; import org.apache.poi.ss.formula.eval.ValueEval; import org.apache.poi.ss.formula.functions.Function; -import org.apache.poi.ss.usermodel.BaseTestBugzillaIssues; -import org.apache.poi.ss.usermodel.Cell; -import org.apache.poi.ss.usermodel.CellStyle; -import org.apache.poi.ss.usermodel.CellValue; -import org.apache.poi.ss.usermodel.ClientAnchor; -import org.apache.poi.ss.usermodel.Comment; -import org.apache.poi.ss.usermodel.CreationHelper; -import org.apache.poi.ss.usermodel.DataFormatter; -import org.apache.poi.ss.usermodel.Drawing; -import org.apache.poi.ss.usermodel.Font; -import org.apache.poi.ss.usermodel.FormulaError; -import org.apache.poi.ss.usermodel.FormulaEvaluator; -import org.apache.poi.ss.usermodel.IndexedColors; -import org.apache.poi.ss.usermodel.Name; -import org.apache.poi.ss.usermodel.Row; -import org.apache.poi.ss.usermodel.Sheet; -import org.apache.poi.ss.usermodel.Workbook; -import org.apache.poi.ss.usermodel.WorkbookFactory; +import org.apache.poi.ss.usermodel.*; import org.apache.poi.ss.util.AreaReference; import org.apache.poi.ss.util.CellRangeAddress; import org.apache.poi.ss.util.CellReference; @@ -601,47 +578,77 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { } /** - * Various ways of removing a cell formula should all zap - * the calcChain entry. + * Various ways of removing a cell formula should all zap the calcChain + * entry. */ @Test public void bug49966() throws Exception { - XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("shared_formulas.xlsx"); - XSSFSheet sheet = wb.getSheetAt(0); - - // CalcChain has lots of entries - CalculationChain cc = wb.getCalculationChain(); - assertEquals("A2", cc.getCTCalcChain().getCArray(0).getR()); - assertEquals("A3", cc.getCTCalcChain().getCArray(1).getR()); - assertEquals("A4", cc.getCTCalcChain().getCArray(2).getR()); - assertEquals("A5", cc.getCTCalcChain().getCArray(3).getR()); - assertEquals("A6", cc.getCTCalcChain().getCArray(4).getR()); - assertEquals("A7", cc.getCTCalcChain().getCArray(5).getR()); - assertEquals("A8", cc.getCTCalcChain().getCArray(6).getR()); - assertEquals(40, cc.getCTCalcChain().sizeOfCArray()); - - // Try various ways of changing the formulas - // If it stays a formula, chain entry should remain - // Otherwise should go - sheet.getRow(1).getCell(0).setCellFormula("A1"); // stay - sheet.getRow(2).getCell(0).setCellFormula(null); // go - sheet.getRow(3).getCell(0).setCellType(Cell.CELL_TYPE_FORMULA); // stay - sheet.getRow(4).getCell(0).setCellType(Cell.CELL_TYPE_STRING); // go - sheet.getRow(5).removeCell( - sheet.getRow(5).getCell(0) // go - ); - sheet.getRow(6).getCell(0).setCellType(Cell.CELL_TYPE_BLANK); // go - sheet.getRow(7).getCell(0).setCellValue((String)null); // go + XSSFWorkbook wb = XSSFTestDataSamples + .openSampleWorkbook("shared_formulas.xlsx"); + XSSFSheet sheet = wb.getSheetAt(0); + + Workbook wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + + // CalcChain has lots of entries + CalculationChain cc = wb.getCalculationChain(); + assertEquals("A2", cc.getCTCalcChain().getCArray(0).getR()); + assertEquals("A3", cc.getCTCalcChain().getCArray(1).getR()); + assertEquals("A4", cc.getCTCalcChain().getCArray(2).getR()); + assertEquals("A5", cc.getCTCalcChain().getCArray(3).getR()); + assertEquals("A6", cc.getCTCalcChain().getCArray(4).getR()); + assertEquals("A7", cc.getCTCalcChain().getCArray(5).getR()); + assertEquals("A8", cc.getCTCalcChain().getCArray(6).getR()); + assertEquals(40, cc.getCTCalcChain().sizeOfCArray()); + + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + + // Try various ways of changing the formulas + // If it stays a formula, chain entry should remain + // Otherwise should go + sheet.getRow(1).getCell(0).setCellFormula("A1"); // stay + sheet.getRow(2).getCell(0).setCellFormula(null); // go + sheet.getRow(3).getCell(0).setCellType(Cell.CELL_TYPE_FORMULA); // stay + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + sheet.getRow(4).getCell(0).setCellType(Cell.CELL_TYPE_STRING); // go + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + + validateCells(sheet); + sheet.getRow(5).removeCell(sheet.getRow(5).getCell(0)); // go + validateCells(sheet); + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + + sheet.getRow(6).getCell(0).setCellType(Cell.CELL_TYPE_BLANK); // go + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + sheet.getRow(7).getCell(0).setCellValue((String) null); // go - // Save and check - wb = XSSFTestDataSamples.writeOutAndReadBack(wb); - assertEquals(35, cc.getCTCalcChain().sizeOfCArray()); + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); - cc = wb.getCalculationChain(); - assertEquals("A2", cc.getCTCalcChain().getCArray(0).getR()); - assertEquals("A4", cc.getCTCalcChain().getCArray(1).getR()); - assertEquals("A9", cc.getCTCalcChain().getCArray(2).getR()); + // Save and check + wb = XSSFTestDataSamples.writeOutAndReadBack(wb); + assertEquals(35, cc.getCTCalcChain().sizeOfCArray()); + cc = wb.getCalculationChain(); + assertEquals("A2", cc.getCTCalcChain().getCArray(0).getR()); + assertEquals("A4", cc.getCTCalcChain().getCArray(1).getR()); + assertEquals("A9", cc.getCTCalcChain().getCArray(2).getR()); + } + + @Test + public void bug49966Row() throws Exception { + XSSFWorkbook wb = XSSFTestDataSamples + .openSampleWorkbook("shared_formulas.xlsx"); + XSSFSheet sheet = wb.getSheetAt(0); + + validateCells(sheet); + sheet.getRow(5).removeCell(sheet.getRow(5).getCell(0)); // go + validateCells(sheet); + } + + private void validateCells(XSSFSheet sheet) { + for(Row row : sheet) { + // trigger handling + ((XSSFRow)row).onDocumentWrite(); + } } @Test 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 44583fdf66..6d91fcd889 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFCell.java @@ -17,14 +17,21 @@ package org.apache.poi.xssf.usermodel; -import org.apache.poi.ss.usermodel.*; +import java.io.IOException; + +import org.apache.poi.ss.usermodel.BaseTestCell; +import org.apache.poi.ss.usermodel.Cell; +import org.apache.poi.ss.usermodel.DataFormatter; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.usermodel.Workbook; +import org.apache.poi.ss.util.CellRangeAddress; +import org.apache.poi.ss.util.CellReference; import org.apache.poi.xssf.XSSFITestDataProvider; +import org.apache.poi.xssf.XSSFTestDataSamples; import org.apache.poi.xssf.model.SharedStringsTable; -import org.openxmlformats.schemas.spreadsheetml.x2006.main.STCellType; import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTCell; - -import java.io.FileOutputStream; -import java.io.IOException; +import org.openxmlformats.schemas.spreadsheetml.x2006.main.STCellType; /** * @author Yegor Kozlov @@ -276,5 +283,93 @@ public final class TestXSSFCell extends BaseTestCell { } } } + + public void test56170() throws IOException { + final Workbook wb = XSSFTestDataSamples.openSampleWorkbook("56170.xlsx"); + final XSSFSheet sheet = (XSSFSheet) wb.getSheetAt(0); + + Workbook wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + Cell cell; + + // add some contents to table so that the table will need expansion + Row row = sheet.getRow(0); + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + cell = row.createCell(0); + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + cell.setCellValue("demo1"); + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + cell = row.createCell(1); + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + cell.setCellValue("demo2"); + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + cell = row.createCell(2); + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + cell.setCellValue("demo3"); + + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + + row = sheet.getRow(1); + cell = row.createCell(0); + cell.setCellValue("demo1"); + cell = row.createCell(1); + cell.setCellValue("demo2"); + cell = row.createCell(2); + cell.setCellValue("demo3"); + + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + + // expand table + XSSFTable table = sheet.getTables().get(0); + final CellReference startRef = table.getStartCellReference(); + final CellReference endRef = table.getEndCellReference(); + table.getCTTable().setRef(new CellRangeAddress(startRef.getRow(), 1, startRef.getCol(), endRef.getCol()).formatAsString()); + + wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb); + assertNotNull(wbRead); + + /*FileOutputStream stream = new FileOutputStream("c:\\temp\\output.xlsx"); + workbook.write(stream); + stream.close();*/ + } + + public void test56170Reproduce() throws IOException { + final Workbook wb = new XSSFWorkbook(); + final Sheet sheet = wb.createSheet(); + Row row = sheet.createRow(0); + + // by creating Cells out of order we trigger the handling in onDocumentWrite() + Cell cell1 = row.createCell(1); + Cell cell2 = row.createCell(0); + + validateRow(row); + + validateRow(row); + // once again with removing one cell + row.removeCell(cell1); + + validateRow(row); + + // once again with removing one cell + row.removeCell(cell1); + + // now check again + validateRow(row); + + // once again with removing one cell + row.removeCell(cell2); + + // now check again + validateRow(row); + } + + private void validateRow(Row row) { + // trigger bug with CArray handling + ((XSSFRow)row).onDocumentWrite(); + + for(Cell cell : row) { + cell.toString(); + } + } + } diff --git a/test-data/spreadsheet/56170.xlsx b/test-data/spreadsheet/56170.xlsx new file mode 100644 index 0000000000..f80e386cc3 Binary files /dev/null and b/test-data/spreadsheet/56170.xlsx differ