From: Greg Woolsey Date: Fri, 19 Oct 2018 01:11:47 +0000 (+0000) Subject: #62834 FormulaEvaluator.evaluateInCell() throws Exception X-Git-Tag: REL_4_0_1~52 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=6b433ae8b34b8c32a0457d74fe3b023222fe7945;p=poi.git #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 51a99d5950..ee0615057d 100644 --- 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 7e3eb3c574..d9dd4ec132 100644 --- 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 { /** * Sets array formula to specified region for result. - * + *

+ * Note if there are shared formulas this will invalidate any + * {@link FormulaEvaluator} instances based on this workbook + *

* @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 adb951d77a..0e41e50cb5 100644 --- 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 b3113203a6..a9490694c0 100644 --- 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)} *

+ *

+ * Note, if there are any shared formulas, his will invalidate any + * {@link FormulaEvaluator} instances based on this workbook. + *

* * @param formula the formula to set, e.g. "SUM(C4:E4)". * If the argument is null 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 423561b2ce..cd9620d79c 100644 --- 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 f8f71686fa..418eb268e8 100644 --- 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 index 0000000000..2f265e69c2 Binary files /dev/null and b/test-data/spreadsheet/62834.xlsx differ