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

@@ -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>

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>

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

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;
 	}
 
 	/**

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