#62834 FormulaEvaluator.evaluateInCell() throws Exception

added cell type = formula check when looping through the shared formula range, to ignore any non-formula cells.

Also refactored a bit to enable passing in the evaluation context, as getCellFormula() uses it behind the scenes when evaluating a shared formula cell (has to shift the formula references based on the master cell).  Review of these changes is welcome, as always.

Checked all other code referencing the "SHARED" enum, and didn't see anything else that dealt with formula cell values and thus would need to notice non-formula cells.

Added unit test based on the failing file from Bugzilla.  Test failed until the fixed code was in place.

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1844295 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/src/java/org/apache/poi/ss/formula/BaseFormulaEvaluator.java b/src/java/org/apache/poi/ss/formula/BaseFormulaEvaluator.java
index 51a99d595098c230752d87c14478148f563ab25c..ee0615057d0a714e82f3b3046ae780a898a37f73 100644 (file)
--- a/src/java/org/apache/poi/ss/formula/BaseFormulaEvaluator.java
+++ b/src/java/org/apache/poi/ss/formula/BaseFormulaEvaluator.java
@@ -64,6 +64,14 @@ public abstract class BaseFormulaEvaluator implements FormulaEvaluator, Workbook
         return _bookEvaluator;
     }
 
+    /**
+     * internal use
+     * @return evaluation workbook
+     */
+    protected EvaluationWorkbook getEvaluationWorkbook() {
+        return _bookEvaluator.getWorkbook();
+    }
+    
     /**
      * Should be called whenever there are major changes (e.g. moving sheets) to input cells
      * in the evaluated workbook.  If performance is not critical, a single call to this method
@@ -208,14 +216,19 @@ public abstract class BaseFormulaEvaluator implements FormulaEvaluator, Workbook
         return evaluateFormulaCell(cell);
     }
 
-    protected static void setCellType(Cell cell, CellValue cv) {
+    /**
+     * set the cell type
+     * @param cell
+     * @param cv
+     */
+    protected void setCellType(Cell cell, CellValue cv) {
         CellType cellType = cv.getCellType();
         switch (cellType) {
             case BOOLEAN:
             case ERROR:
             case NUMERIC:
             case STRING:
-                cell.setCellType(cellType);
+                setCellType(cell, cellType);
                 return;
             case BLANK:
                 // never happens - blanks eventually get translated to zero
@@ -227,6 +240,15 @@ public abstract class BaseFormulaEvaluator implements FormulaEvaluator, Workbook
                 throw new IllegalStateException("Unexpected cell value type (" + cellType + ")");
         }
     }
+
+    /**
+     * Override if a different variation is needed, e.g. passing the evaluator to the cell method
+     * @param cell
+     * @param cellType
+     */
+    protected void setCellType(Cell cell, CellType cellType) {
+        cell.setCellType(cellType);
+    }
     
     protected abstract RichTextString createRichTextString(String str);
     

diff --git a/src/java/org/apache/poi/ss/usermodel/Sheet.java b/src/java/org/apache/poi/ss/usermodel/Sheet.java
index 7e3eb3c574d890d4a1b7128670ff7bf80c33b7fc..d9dd4ec1328960b1316f9e8538cbcae17f0ecb3e 100644 (file)
--- a/src/java/org/apache/poi/ss/usermodel/Sheet.java
+++ b/src/java/org/apache/poi/ss/usermodel/Sheet.java
@@ -1006,7 +1006,10 @@ public interface Sheet extends Iterable<Row> {
 
     /**
      * Sets array formula to specified region for result.
-     *
+     * <p>
+     * Note if there are shared formulas this will invalidate any 
+     * {@link FormulaEvaluator} instances based on this workbook
+     *</p>
      * @param formula text representation of the formula
      * @param range Region of array formula for result.
      * @return the {@link CellRange} of cells affected by this change

diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/BaseXSSFFormulaEvaluator.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/BaseXSSFFormulaEvaluator.java
index adb951d77a2b4af2b2d2984cbc7c879fce0be083..0e41e50cb5c6de6724333261a1f3d3de56c1e162 100644 (file)
--- a/src/ooxml/java/org/apache/poi/xssf/usermodel/BaseXSSFFormulaEvaluator.java
+++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/BaseXSSFFormulaEvaluator.java
@@ -19,6 +19,7 @@ package org.apache.poi.xssf.usermodel;
 
 import org.apache.poi.ss.formula.BaseFormulaEvaluator;
 import org.apache.poi.ss.formula.EvaluationCell;
+import org.apache.poi.ss.formula.EvaluationWorkbook;
 import org.apache.poi.ss.formula.WorkbookEvaluator;
 import org.apache.poi.ss.formula.eval.BoolEval;
 import org.apache.poi.ss.formula.eval.ErrorEval;
@@ -26,6 +27,7 @@ import org.apache.poi.ss.formula.eval.NumberEval;
 import org.apache.poi.ss.formula.eval.StringEval;
 import org.apache.poi.ss.formula.eval.ValueEval;
 import org.apache.poi.ss.usermodel.Cell;
+import org.apache.poi.ss.usermodel.CellType;
 import org.apache.poi.ss.usermodel.CellValue;
 import org.apache.poi.ss.usermodel.RichTextString;
 
@@ -69,4 +71,11 @@ public abstract class BaseXSSFFormulaEvaluator extends BaseFormulaEvaluator {
         }
         throw new RuntimeException("Unexpected eval class (" + eval.getClass().getName() + ")");
     }
+    
+    protected void setCellType(Cell cell, CellType cellType) {
+        EvaluationWorkbook evaluationWorkbook = getEvaluationWorkbook();
+        BaseXSSFEvaluationWorkbook xewb = BaseXSSFEvaluationWorkbook.class.isAssignableFrom(evaluationWorkbook.getClass()) ? (BaseXSSFEvaluationWorkbook) evaluationWorkbook : null;
+        
+        ((XSSFCell) cell).setCellType(cellType, xewb);
+    }
 }

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 b3113203a635e7af0ba82f04fbd128356ee231b9..a9490694c05856c64154c88885bb093551f4752a 100644 (file)
--- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFCell.java
+++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFCell.java
@@ -37,6 +37,7 @@ import org.apache.poi.ss.usermodel.Comment;
 import org.apache.poi.ss.usermodel.DataFormatter;
 import org.apache.poi.ss.usermodel.DateUtil;
 import org.apache.poi.ss.usermodel.FormulaError;
+import org.apache.poi.ss.usermodel.FormulaEvaluator;
 import org.apache.poi.ss.usermodel.Hyperlink;
 import org.apache.poi.ss.usermodel.RichTextString;
 import org.apache.poi.ss.usermodel.Row.MissingCellPolicy;
@@ -477,7 +478,7 @@ public final class XSSFCell implements Cell {
      * @return a formula for the cell
      * @throws IllegalStateException if the cell type returned by {@link #getCellType()} is not {@link CellType#FORMULA}
      */
-    protected String getCellFormula(XSSFEvaluationWorkbook fpb) {
+    protected String getCellFormula(BaseXSSFEvaluationWorkbook fpb) {
         CellType cellType = getCellType();
         if(cellType != CellType.FORMULA) {
             throw typeMismatch(CellType.FORMULA, cellType, false);
@@ -506,7 +507,7 @@ public final class XSSFCell implements Cell {
      * @param si Shared Group Index
      * @return non shared formula created for the given shared formula and this cell
      */
-    private String convertSharedFormula(int si, XSSFEvaluationWorkbook fpb){
+    private String convertSharedFormula(int si, BaseXSSFEvaluationWorkbook fpb){
         XSSFSheet sheet = getSheet();
 
         CTCellFormula f = sheet.getSharedFormula(si);
@@ -536,6 +537,10 @@ public final class XSSFCell implements Cell {
      * Note, this method only sets the formula string and does not calculate the formula value.
      * To set the precalculated value use {@link #setCellValue(double)} or {@link #setCellValue(String)}
      * </p>
+     * <p>
+     * Note, if there are any shared formulas, his will invalidate any 
+     * {@link FormulaEvaluator} instances based on this workbook.
+     * </p>
      *
      * @param formula the formula to set, e.g. <code>"SUM(C4:E4)"</code>.
      *  If the argument is <code>null</code> then the current formula is removed.
@@ -563,7 +568,7 @@ public final class XSSFCell implements Cell {
         if (formula == null) {
             wb.onDeleteFormula(this);
             if (_cell.isSetF()) {
-                _row.getSheet().onDeleteFormula(this);
+                _row.getSheet().onDeleteFormula(this, null);
                 _cell.unsetF();
             }
             return;
@@ -962,6 +967,16 @@ public final class XSSFCell implements Cell {
      */
     @Override
     public void setCellType(CellType cellType) {
+        setCellType(cellType, null);
+    }
+    
+    /**
+     * Needed by bug #62834, which points out getCellFormula() expects an evaluation context or creates a new one,
+     * so if there is one in use, it needs to be carried on through.
+     * @param cellType
+     * @param evalWb BaseXSSFEvaluationWorkbook already in use, or null if a new implicit one should be used
+     */
+    protected void setCellType(CellType cellType, BaseXSSFEvaluationWorkbook evalWb) {
         CellType prevType = getCellType();
 
         if(isPartOfArrayFormulaGroup()){
@@ -969,7 +984,7 @@ public final class XSSFCell implements Cell {
         }
         if(prevType == CellType.FORMULA && cellType != CellType.FORMULA) {
             if (_cell.isSetF()) {
-                _row.getSheet().onDeleteFormula(this);
+                _row.getSheet().onDeleteFormula(this, evalWb);
             }
             getSheet().getWorkbook().onDeleteFormula(this);
         }

diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java
index 423561b2ce0b258c2a2438b8d99bfb0718a7946d..cd9620d79c555931e284ebbc1a804f51216b35da 100644 (file)
--- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java
+++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java
@@ -4621,8 +4621,10 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet  {
 
     /**
      *  when a cell with a 'master' shared formula is removed,  the next cell in the range becomes the master
+     * @param cell 
+     * @param evalWb BaseXSSFEvaluationWorkbook in use, if one exists
      */
-    protected void onDeleteFormula(XSSFCell cell){
+    protected void onDeleteFormula(XSSFCell cell, BaseXSSFEvaluationWorkbook evalWb){
 
         CTCellFormula f = cell.getCTCell().getF();
         if (f != null && f.getT() == STCellFormulaType.SHARED && f.isSetRef() && f.getStringValue() != null) {
@@ -4634,9 +4636,9 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet  {
                     XSSFRow row = getRow(i);
                     if(row != null) for(int j = cell.getColumnIndex(); j <= ref.getLastColumn(); j++){
                         XSSFCell nextCell = row.getCell(j);
-                        if(nextCell != null && nextCell != cell){
+                        if(nextCell != null && nextCell != cell && nextCell.getCellType() == CellType.FORMULA){
                             CTCellFormula nextF = nextCell.getCTCell().getF();
-                            nextF.setStringValue(nextCell.getCellFormula());
+                            nextF.setStringValue(nextCell.getCellFormula(evalWb));
                             CellRangeAddress nextRef = new CellRangeAddress(
                                     nextCell.getRowIndex(), ref.getLastRow(),
                                     nextCell.getColumnIndex(), ref.getLastColumn());

diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java
index f8f71686fa42b7ad107b739f2ad469e85458fc36..418eb268e8368da0c4798adfb71d5a563d619e0a 100644 (file)
--- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java
+++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java
@@ -17,14 +17,24 @@
 
 package org.apache.poi.xssf.usermodel;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.fail;
 
 import java.io.IOException;
 import java.util.HashMap;
 import java.util.Map;
 
 import org.apache.poi.hssf.HSSFTestDataSamples;
-import org.apache.poi.ss.usermodel.*;
+import org.apache.poi.ss.usermodel.BaseTestFormulaEvaluator;
+import org.apache.poi.ss.usermodel.Cell;
+import org.apache.poi.ss.usermodel.CellValue;
+import org.apache.poi.ss.usermodel.CreationHelper;
+import org.apache.poi.ss.usermodel.FormulaEvaluator;
+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.CellReference;
 import org.apache.poi.xssf.XSSFITestDataProvider;
 import org.apache.poi.xssf.XSSFTestDataSamples;
@@ -437,6 +447,10 @@ public final class TestXSSFFormulaEvaluation extends BaseTestFormulaEvaluator {
         assertEquals("D 0,068", evaluator.evaluate(wb.getSheetAt(0).getRow(1).getCell(1)));
     }
 
+    /**
+     * see bug 62275
+     * @throws IOException
+     */
     @Test
     public void testBug62275() throws IOException {
         try (Workbook wb = new XSSFWorkbook()) {
@@ -451,4 +465,29 @@ public final class TestXSSFFormulaEvaluation extends BaseTestFormulaEvaluator {
             eval.evaluate(cell);
         }
     }
+    
+    /**
+     * see bug 62834, handle when a shared formula range doesn't contain only formula cells
+     * @throws IOException 
+     */
+    @Test
+    public void testBug62834() throws IOException {
+        try (Workbook wb = XSSFTestDataSamples.openSampleWorkbook("62834.xlsx")) {
+            FormulaEvaluator evaluator = wb.getCreationHelper().createFormulaEvaluator();
+
+            Cell a2 = wb.getSheetAt(0).getRow(1).getCell(0);
+            Cell value = evaluator.evaluateInCell(a2);
+            assertEquals("wrong value A2", "a value", value.getStringCellValue());
+            
+//            evaluator.clearAllCachedResultValues();
+            
+            Cell a3 = wb.getSheetAt(0).getRow(2).getCell(0);
+            value = evaluator.evaluateInCell(a3);
+            assertEquals("wrong value A3", "a value", value.getStringCellValue());
+            
+            Cell a5 = wb.getSheetAt(0).getRow(4).getCell(0);
+            value = evaluator.evaluateInCell(a5);
+            assertEquals("wrong value A5", "another value", value.getStringCellValue());
+        }
+    }
 }

diff --git a/test-data/spreadsheet/62834.xlsx b/test-data/spreadsheet/62834.xlsx
new file mode 100644 (file)
index 0000000..2f265e6
Binary files /dev/null and b/test-data/spreadsheet/62834.xlsx differ