From d92e7179bf59e146d7aef4e1621d72aeb0fd4556 Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Mon, 30 Mar 2009 20:46:51 +0000 Subject: [PATCH] Fix for bug 46898 - Formula evaluator should not cache intermediate circular-reference error results git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@760162 13f79535-47bb-0310-9956-ffa450edef68 --- src/documentation/content/xdocs/changes.xml | 1 + src/documentation/content/xdocs/status.xml | 1 + .../poi/ss/formula/EvaluationTracker.java | 10 ++++ .../poi/ss/formula/WorkbookEvaluator.java | 10 ++-- .../formula/eval/TestCircularReferences.java | 49 +++++++++++++++++++ 5 files changed, 68 insertions(+), 3 deletions(-) diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index c1b4301227..cc2a0ce286 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,6 +37,7 @@ + 46898 - Fixed formula evaluator to not cache intermediate circular-reference error results 46917 - Fixed PageItemRecord(SXPI) to allow multiple field infos 46904 - Fix POIFS issue with duplicate block 0 references on very old BIFF5/BIFF7 files 46840 - PageSettingsBlock should include HEADERFOOTER record diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index bc383a2694..cb3c8ffa38 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,6 +34,7 @@ + 46898 - Fixed formula evaluator to not cache intermediate circular-reference error results 46917 - Fixed PageItemRecord(SXPI) to allow multiple field infos 46904 - Fix POIFS issue with duplicate block 0 references on very old BIFF5/BIFF7 files 46840 - PageSettingsBlock should include HEADERFOOTER record diff --git a/src/java/org/apache/poi/ss/formula/EvaluationTracker.java b/src/java/org/apache/poi/ss/formula/EvaluationTracker.java index 807d398a95..0d89b247d6 100755 --- a/src/java/org/apache/poi/ss/formula/EvaluationTracker.java +++ b/src/java/org/apache/poi/ss/formula/EvaluationTracker.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.Set; import org.apache.poi.hssf.record.formula.eval.BlankEval; +import org.apache.poi.hssf.record.formula.eval.ErrorEval; import org.apache.poi.hssf.record.formula.eval.ValueEval; import org.apache.poi.hssf.usermodel.HSSFCell; @@ -80,6 +81,15 @@ final class EvaluationTracker { throw new IllegalStateException("Call to endEvaluate without matching call to startEvaluate"); } CellEvaluationFrame frame = _evaluationFrames.get(nFrames-1); + if (result == ErrorEval.CIRCULAR_REF_ERROR && nFrames > 1) { + // Don't cache a circular ref error result if this cell is not the top evaluated cell. + // A true circular ref error will propagate all the way around the loop. However, it's + // possible to have parts of the formula tree (/ parts of the loop) to evaluate to + // CIRCULAR_REF_ERROR, and that value not get used in the final cell result (see the + // unit tests for a simple example). Thus, the only CIRCULAR_REF_ERROR result that can + // safely be cached is that of the top evaluated cell. + return; + } frame.updateFormulaResult(result); } diff --git a/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java b/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java index cb26cb2470..5ca223e72d 100644 --- a/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java +++ b/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java @@ -203,13 +203,13 @@ public final class WorkbookEvaluator { tracker.acceptFormulaDependency(cce); } IEvaluationListener evalListener = _evaluationListener; + ValueEval result; if (cce.getValue() == null) { if (!tracker.startEvaluate(cce)) { return ErrorEval.CIRCULAR_REF_ERROR; } try { - ValueEval result; Ptg[] ptgs = _workbook.getFormulaTokens(srcCell); if (evalListener == null) { @@ -235,9 +235,13 @@ public final class WorkbookEvaluator { if (isDebugLogEnabled()) { String sheetName = getSheetName(sheetIndex); CellReference cr = new CellReference(rowIndex, columnIndex); - logDebug("Evaluated " + sheetName + "!" + cr.formatAsString() + " to " + cce.getValue().toString()); + logDebug("Evaluated " + sheetName + "!" + cr.formatAsString() + " to " + result.toString()); } - return cce.getValue(); + // Usually (result === cce.getValue()) + // But sometimes: (result==ErrorEval.CIRCULAR_REF_ERROR, cce.getValue()==null) + // When circular references are detected, the cache entry is only updated for + // the top evaluation frame + return result; } /** diff --git a/src/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java b/src/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java index 669bb4b1b9..fb11b4c1b6 100755 --- a/src/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java +++ b/src/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java @@ -25,7 +25,9 @@ import org.apache.poi.hssf.usermodel.HSSFFormulaEvaluator; import org.apache.poi.hssf.usermodel.HSSFRow; import org.apache.poi.hssf.usermodel.HSSFSheet; import org.apache.poi.hssf.usermodel.HSSFWorkbook; +import org.apache.poi.ss.usermodel.Cell; import org.apache.poi.ss.usermodel.CellValue; +import org.apache.poi.ss.usermodel.ErrorConstants; /** * Tests HSSFFormulaEvaluator for its handling of cell formula circular references. * @@ -118,4 +120,51 @@ public final class TestCircularReferences extends TestCase { confirmCycleErrorCode(cellValue); } + + public void testIntermediateCircularReferenceResults_bug46898() { + HSSFWorkbook wb = new HSSFWorkbook(); + HSSFSheet sheet = wb.createSheet("Sheet1"); + + HSSFRow row = sheet.createRow(0); + + HSSFCell cellA1 = row.createCell(0); + HSSFCell cellB1 = row.createCell(1); + HSSFCell cellC1 = row.createCell(2); + HSSFCell cellD1 = row.createCell(3); + HSSFCell cellE1 = row.createCell(4); + + cellA1.setCellFormula("IF(FALSE, 1+B1, 42)"); + cellB1.setCellFormula("1+C1"); + cellC1.setCellFormula("1+D1"); + cellD1.setCellFormula("1+E1"); + cellE1.setCellFormula("1+A1"); + + HSSFFormulaEvaluator fe = new HSSFFormulaEvaluator(wb); + CellValue cv; + + // Happy day flow - evaluate A1 first + cv = fe.evaluate(cellA1); + assertEquals(Cell.CELL_TYPE_NUMERIC, cv.getCellType()); + assertEquals(42.0, cv.getNumberValue(), 0.0); + cv = fe.evaluate(cellB1); // no circ-ref-error because A1 result is cached + assertEquals(Cell.CELL_TYPE_NUMERIC, cv.getCellType()); + assertEquals(46.0, cv.getNumberValue(), 0.0); + + // Show the bug - evaluate another cell from the loop first + fe.clearAllCachedResultValues(); + cv = fe.evaluate(cellB1); + if (cv.getCellType() == ErrorEval.CIRCULAR_REF_ERROR.getErrorCode()) { + throw new AssertionFailedError("Identified bug 46898"); + } + assertEquals(Cell.CELL_TYPE_NUMERIC, cv.getCellType()); + assertEquals(46.0, cv.getNumberValue(), 0.0); + + // start evaluation on another cell + fe.clearAllCachedResultValues(); + cv = fe.evaluate(cellE1); + assertEquals(Cell.CELL_TYPE_NUMERIC, cv.getCellType()); + assertEquals(43.0, cv.getNumberValue(), 0.0); + + + } } -- 2.39.5