poi-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From j...@apache.org
Subject svn commit: r760162 - in /poi/trunk/src: documentation/content/xdocs/ java/org/apache/poi/ss/formula/ testcases/org/apache/poi/hssf/record/formula/eval/
Date Mon, 30 Mar 2009 20:46:52 GMT
Author: josh
Date: Mon Mar 30 20:46:51 2009
New Revision: 760162

URL: http://svn.apache.org/viewvc?rev=760162&view=rev
Log:
Fix for bug 46898 - Formula evaluator should not cache intermediate circular-reference error
results

Modified:
    poi/trunk/src/documentation/content/xdocs/changes.xml
    poi/trunk/src/documentation/content/xdocs/status.xml
    poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java
    poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java
    poi/trunk/src/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java

Modified: poi/trunk/src/documentation/content/xdocs/changes.xml
URL: http://svn.apache.org/viewvc/poi/trunk/src/documentation/content/xdocs/changes.xml?rev=760162&r1=760161&r2=760162&view=diff
==============================================================================
--- poi/trunk/src/documentation/content/xdocs/changes.xml (original)
+++ poi/trunk/src/documentation/content/xdocs/changes.xml Mon Mar 30 20:46:51 2009
@@ -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>

Modified: poi/trunk/src/documentation/content/xdocs/status.xml
URL: http://svn.apache.org/viewvc/poi/trunk/src/documentation/content/xdocs/status.xml?rev=760162&r1=760161&r2=760162&view=diff
==============================================================================
--- poi/trunk/src/documentation/content/xdocs/status.xml (original)
+++ poi/trunk/src/documentation/content/xdocs/status.xml Mon Mar 30 20:46:51 2009
@@ -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>

Modified: poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java?rev=760162&r1=760161&r2=760162&view=diff
==============================================================================
--- poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java (original)
+++ poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java Mon Mar 30 20:46:51
2009
@@ -23,6 +23,7 @@
 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 @@
 			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);
 	}

Modified: poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java?rev=760162&r1=760161&r2=760162&view=diff
==============================================================================
--- poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java (original)
+++ poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java Mon Mar 30 20:46:51
2009
@@ -203,13 +203,13 @@
 			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 @@
 		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;
 	}
 
 	/**

Modified: poi/trunk/src/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java?rev=760162&r1=760161&r2=760162&view=diff
==============================================================================
--- poi/trunk/src/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java
(original)
+++ poi/trunk/src/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java
Mon Mar 30 20:46:51 2009
@@ -25,7 +25,9 @@
 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 @@
 		
 		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);
+		
+		
+	}
 }



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@poi.apache.org
For additional commands, e-mail: commits-help@poi.apache.org


Mime
View raw message