Fix for bug 46053 - fixed evaluation cache dependency analysis when changing blank cells

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

src/documentation/content/xdocs/changes.xml

@@ -37,7 +37,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.5-beta4" date="2008-??-??">
-           <action dev="POI-DEVELOPERS" type="fix">YK: remove me. required to keep Forrest DTD compiler quiet</action>
+           <action dev="POI-DEVELOPERS" type="fix">46053 - fixed evaluation cache dependency analysis when changing blank cells</action>
         </release>
         <release version="3.2-FINAL" date="2008-10-19">
            <action dev="POI-DEVELOPERS" type="fix">45866 - allowed for change of unicode compression across Continue records</action>

src/documentation/content/xdocs/status.xml

@@ -34,7 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.5-beta4" date="2008-??-??">
-           <action dev="POI-DEVELOPERS" type="fix">YK: remove me. required to keep Forrest DTD compiler quiet</action>
+           <action dev="POI-DEVELOPERS" type="fix">46053 - fixed evaluation cache dependency analysis when changing blank cells</action>
         </release>
         <release version="3.2-FINAL" date="2008-10-19">
            <action dev="POI-DEVELOPERS" type="fix">45866 - allowed for change of unicode compression across Continue records</action>

src/java/org/apache/poi/ss/formula/EvaluationCache.java

@@ -52,16 +52,21 @@ final class EvaluationCache {
 
 	public void notifyUpdateCell(int bookIndex, int sheetIndex, EvaluationCell cell) {
 		FormulaCellCacheEntry fcce = _formulaCellCache.get(cell);
-		
+
 		Loc loc = new Loc(bookIndex, sheetIndex, cell.getRowIndex(), cell.getColumnIndex());
 		PlainValueCellCacheEntry pcce = _plainCellCache.get(loc);
 
 		if (cell.getCellType() == HSSFCell.CELL_TYPE_FORMULA) {
 			if (fcce == null) {
-				if (pcce == null) {
-					updateAnyBlankReferencingFormulas(bookIndex, sheetIndex, cell.getRowIndex(), cell.getColumnIndex());
-				}
 				fcce = new FormulaCellCacheEntry();
+				if (pcce == null) {
+					if (_evaluationListener != null) {
+						_evaluationListener.onChangeFromBlankValue(sheetIndex, cell.getRowIndex(),
+								cell.getColumnIndex(), cell, fcce);
+					}
+					updateAnyBlankReferencingFormulas(bookIndex, sheetIndex, cell.getRowIndex(),
+							cell.getColumnIndex());
+				}
 				_formulaCellCache.put(cell, fcce);
 			} else {
 				fcce.recurseClearCachedFormulaResults(_evaluationListener);
@@ -77,18 +82,28 @@ final class EvaluationCache {
 		} else {
 			ValueEval value = WorkbookEvaluator.getValueFromNonFormulaCell(cell);
 			if (pcce == null) {
-				if (fcce == null) {
+				if (value != BlankEval.INSTANCE) {
-					updateAnyBlankReferencingFormulas(bookIndex, sheetIndex, cell.getRowIndex(), cell.getColumnIndex());
+					// only cache non-blank values in the plain cell cache
-				}
+					// (dependencies on blank cells are managed by
-				pcce = new PlainValueCellCacheEntry(value);
+					// FormulaCellCacheEntry._usedBlankCellGroup)
-				_plainCellCache.put(loc, pcce);
+					pcce = new PlainValueCellCacheEntry(value);
-				if (_evaluationListener != null) {
+					if (fcce == null) {
-					_evaluationListener.onReadPlainValue(sheetIndex, cell.getRowIndex(), cell.getColumnIndex(), pcce);
+						if (_evaluationListener != null) {
+							_evaluationListener.onChangeFromBlankValue(sheetIndex, cell
+									.getRowIndex(), cell.getColumnIndex(), cell, pcce);
+						}
+						updateAnyBlankReferencingFormulas(bookIndex, sheetIndex,
+								cell.getRowIndex(), cell.getColumnIndex());
+					}
+					_plainCellCache.put(loc, pcce);
 				}
 			} else {
-				if(pcce.updateValue(value)) {
+				if (pcce.updateValue(value)) {
 					pcce.recurseClearCachedFormulaResults(_evaluationListener);
 				}
+				if (value == BlankEval.INSTANCE) {
+					_plainCellCache.remove(loc);
+				}
 			}
 			if (fcce == null) {
 				// was plain cell before - no change of type

src/java/org/apache/poi/ss/formula/IEvaluationListener.java

@@ -19,7 +19,6 @@ package org.apache.poi.ss.formula;
 
 import org.apache.poi.hssf.record.formula.Ptg;
 import org.apache.poi.hssf.record.formula.eval.ValueEval;
-import org.apache.poi.hssf.usermodel.HSSFCell;
 
 /**
  * Tests can implement this class to track the internal working of the {@link WorkbookEvaluator}.<br/>
@@ -51,4 +50,6 @@ interface IEvaluationListener {
 	 */
 	void sortDependentCachedValues(ICacheEntry[] formulaCells);
 	void onClearDependentCachedValue(ICacheEntry formulaCell, int depth);
+	void onChangeFromBlankValue(int sheetIndex, int rowIndex, int columnIndex,
+			EvaluationCell cell, ICacheEntry entry);
 }

src/testcases/org/apache/poi/ss/formula/EvaluationListener.java

@@ -19,7 +19,6 @@ package org.apache.poi.ss.formula;
 
 import org.apache.poi.hssf.record.formula.Ptg;
 import org.apache.poi.hssf.record.formula.eval.ValueEval;
-import org.apache.poi.hssf.usermodel.HSSFCell;
 
 /**
  * Tests should extend this class if they need to track the internal working of the {@link WorkbookEvaluator}.<br/>
@@ -47,6 +46,10 @@ public abstract class EvaluationListener implements IEvaluationListener {
 	public void onClearCachedValue(ICacheEntry entry) {
 		// do nothing 
 	}
+	public void onChangeFromBlankValue(int sheetIndex, int rowIndex, int columnIndex,
+			EvaluationCell cell, ICacheEntry entry) {
+		// do nothing 
+	}
 	public void sortDependentCachedValues(ICacheEntry[] entries) {
 		// do nothing 
 	}

src/testcases/org/apache/poi/ss/formula/TestEvaluationCache.java

@@ -38,9 +38,11 @@ import org.apache.poi.hssf.record.formula.eval.StringEval;
 import org.apache.poi.hssf.record.formula.eval.ValueEval;
 import org.apache.poi.hssf.usermodel.HSSFCell;
 import org.apache.poi.hssf.usermodel.HSSFEvaluationTestHelper;
+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.hssf.usermodel.HSSFFormulaEvaluator.CellValue;
 import org.apache.poi.hssf.util.CellReference;
 import org.apache.poi.ss.formula.PlainCellCache.Loc;
 
@@ -134,7 +136,19 @@ public class TestEvaluationCache extends TestCase {
 		}
 		public void onClearDependentCachedValue(ICacheEntry entry, int depth) {
 			EvaluationCell cell = (EvaluationCell) _formulaCellsByCacheEntry.get(entry);
-   			log("clear" + depth, cell.getRowIndex(), cell.getColumnIndex(),  entry.getValue());
+			log("clear" + depth, cell.getRowIndex(), cell.getColumnIndex(), entry.getValue());
+		}
+
+		public void onChangeFromBlankValue(int sheetIndex, int rowIndex, int columnIndex,
+				EvaluationCell cell, ICacheEntry entry) {
+			log("changeFromBlank", rowIndex, columnIndex, entry.getValue());
+			if (entry.getValue() == null) { // hack to tell the difference between formula and plain value
+				// perhaps the API could be improved: onChangeFromBlankToValue, onChangeFromBlankToFormula
+				_formulaCellsByCacheEntry.put(entry, cell);
+			} else {
+				Loc loc = new Loc(0, sheetIndex, rowIndex, columnIndex);
+				_plainCellLocsByCacheEntry.put(entry, loc);
+			}
 		}
 		private void log(String tag, int rowIndex, int columnIndex, Object value) {
 			StringBuffer sb = new StringBuffer(64);
@@ -213,6 +227,11 @@ public class TestEvaluationCache extends TestCase {
 			cell.setCellValue(value);
 			_evaluator.notifyUpdateCell(wrapCell(cell));
 		}
+		public void clearCell(String cellRefText) {
+			HSSFCell cell = getOrCreateCell(cellRefText);
+			cell.setCellType(HSSFCell.CELL_TYPE_BLANK);
+			_evaluator.notifyUpdateCell(wrapCell(cell));
+		}
 
 		public void setCellFormula(String cellRefText, String formulaText) {
 			HSSFCell cell = getOrCreateCell(cellRefText);
@@ -555,6 +574,75 @@ public class TestEvaluationCache extends TestCase {
 		});
 	}
 	
+	/**
+	 * Make sure that when blank cells are changed to value/formula cells, any dependent formulas
+	 * have their cached results cleared.
+	 */
+	public void testBlankCellChangedToValueCell_bug46053() {
+		HSSFWorkbook wb = new HSSFWorkbook();
+		HSSFSheet sheet = wb.createSheet("Sheet1");
+		HSSFRow row = sheet.createRow(0);
+		HSSFCell cellA1 = row.createCell(0);
+		HSSFCell cellB1 = row.createCell(1);
+		HSSFFormulaEvaluator fe = new HSSFFormulaEvaluator(wb);
+
+		cellA1.setCellFormula("B1+2.2");
+		cellB1.setCellValue(1.5);
+
+		fe.notifyUpdateCell(cellA1);
+		fe.notifyUpdateCell(cellB1);
+
+		CellValue cv;
+		cv = fe.evaluate(cellA1);
+		assertEquals(3.7, cv.getNumberValue(), 0.0);
+
+		cellB1.setCellType(HSSFCell.CELL_TYPE_BLANK);
+		fe.notifyUpdateCell(cellB1);
+		cv = fe.evaluate(cellA1); // B1 was used to evaluate A1
+		assertEquals(2.2, cv.getNumberValue(), 0.0);
+
+		cellB1.setCellValue(0.4);  // changing B1, so A1 cached result should be cleared 
+		fe.notifyUpdateCell(cellB1);
+		cv = fe.evaluate(cellA1);
+		if (cv.getNumberValue() == 2.2) {
+			// looks like left-over cached result from before change to B1
+			throw new AssertionFailedError("Identified bug 46053");
+		}
+		assertEquals(2.6, cv.getNumberValue(), 0.0);
+	}
+	
+	/**
+	 * same use-case as the test for bug 46053, but checking trace values too
+	 */
+	public void testBlankCellChangedToValueCell() {
+
+		MySheet ms = new MySheet();
+
+		ms.setCellFormula("A1", "B1+2.2");
+		ms.setCellValue("B1", 1.5);
+		ms.clearAllCachedResultValues();
+		ms.clearCell("B1");
+		ms.getAndClearLog();
+
+		confirmEvaluate(ms, "A1", 2.2);
+		confirmLog(ms, new String[] {
+			"start A1 B1+2.2",
+			"end A1 2.2",
+		});
+		ms.setCellValue("B1", 0.4);
+		confirmLog(ms, new String[] {
+			"changeFromBlank B1 0.4",
+			"clear A1",
+		});
+
+		confirmEvaluate(ms, "A1", 2.6);
+		confirmLog(ms, new String[] {
+			"start A1 B1+2.2",
+			"hit B1 0.4",
+			"end A1 2.6",
+		});
+	}
+	
 	private static void confirmEvaluate(MySheet ms, String cellRefText, double expectedValue) {
 		ValueEval v = ms.evaluateCell(cellRefText);
 		assertEquals(NumberEval.class, v.getClass());