Fix for bug 46898 - Formula evaluator should not cache intermediate circular-referenc...

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

diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml
index c1b43012273eb2dec65db1f89d512d68e0a37814..cc2a0ce2865284b4d5ec764c482bee87d3dea8cd 100644 (file)
--- a/src/documentation/content/xdocs/changes.xml
+++ b/src/documentation/content/xdocs/changes.xml
@@ -37,6 +37,7 @@
 
                <!-- Don't forget to update status.xml too! -->
         <release version="3.5-beta6" date="2009-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46898 - Fixed formula evaluator to not cache intermediate circular-reference error results</action>
            <action dev="POI-DEVELOPERS" type="fix">46917 - Fixed PageItemRecord(SXPI) to allow multiple field infos</action>
            <action dev="POI-DEVELOPERS" type="fix">46904 - Fix POIFS issue with duplicate block 0 references on very old BIFF5/BIFF7 files</action>
            <action dev="POI-DEVELOPERS" type="fix">46840 - PageSettingsBlock should include HEADERFOOTER record</action>

diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml
index bc383a2694da1eca9c1dad8dfef1bb599cdefc8a..cb3c8ffa38cf4709afdababdf1759fb51f9a30e3 100644 (file)
--- a/src/documentation/content/xdocs/status.xml
+++ b/src/documentation/content/xdocs/status.xml
@@ -34,6 +34,7 @@
        <!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.5-beta6" date="2009-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46898 - Fixed formula evaluator to not cache intermediate circular-reference error results</action>
            <action dev="POI-DEVELOPERS" type="fix">46917 - Fixed PageItemRecord(SXPI) to allow multiple field infos</action>
            <action dev="POI-DEVELOPERS" type="fix">46904 - Fix POIFS issue with duplicate block 0 references on very old BIFF5/BIFF7 files</action>
            <action dev="POI-DEVELOPERS" type="fix">46840 - PageSettingsBlock should include HEADERFOOTER record</action>

diff --git a/src/java/org/apache/poi/ss/formula/EvaluationTracker.java b/src/java/org/apache/poi/ss/formula/EvaluationTracker.java
index 807d398a95be7b837cdd8eefcdccbbd350052797..0d89b247d6da8e042ff51875b5c5b87e6c7dc352 100755 (executable)
--- 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 cb26cb2470382cf306afc6263b9f1e0513bf1c0f..5ca223e72df3ff01e263cf14807d4a0f980b527c 100644 (file)
--- 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 669bb4b1b93d545221efc5a123ed02cb519fea3d..fb11b4c1b6709b1839247d9ef99b8cf9857d367e 100755 (executable)
--- 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);
+               
+               
+       }
 }