#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

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
@@ -228,6 +241,15 @@ public abstract class BaseFormulaEvaluator implements FormulaEvaluator, Workbook
         }
     }
 
+    /**
+     * 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);
     
     protected void setCellValue(Cell cell, CellValue cv) {

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

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);
+    }
 }

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);
         }

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());

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());
+        }
+    }
 }