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

4 files changed, 193 insertions, 70 deletions

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.<p/>
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<XSSFRow> {
     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<XSSFRow> {
         if(!isOrdered){
             CTCell[] cArray = new CTCell[_cells.size()];
             int i = 0;
-            for (XSSFCell c : _cells.values()) {
-                cArray[i++] = c.getCTCell();
+            for (Map.Entry<Integer, XSSFCell> 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();
+        }
+    }    
+    
 }