44950 - fixed HSSFFormulaEvaluator.evaluateInCell() and Area3DEval.getValue() also added validation for number of elements in AreaEvals

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@654356 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.1-beta2" date="2008-05-??">
+           <action dev="POI-DEVELOPERS" type="fix">44950 - fixed HSSFFormulaEvaluator.evaluateInCell() and Area3DEval.getValue() also added validation for number of elements in AreaEvals</action>
            <action dev="POI-DEVELOPERS" type="fix">42570 - fixed LabelRecord to use empty string instead of null when the length is zero.</action>
            <action dev="POI-DEVELOPERS" type="fix">42564 - fixed ArrayPtg to use ConstantValueParser.  Fixed a few other ArrayPtg encoding issues.</action>
            <action dev="POI-DEVELOPERS" type="fix">Follow-on from 28754 - StringPtg.toFormulaString() should escape double quotes</action>

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1-beta2" date="2008-05-??">
+           <action dev="POI-DEVELOPERS" type="fix">44950 - fixed HSSFFormulaEvaluator.evaluateInCell() and Area3DEval.getValue() also added validation for number of elements in AreaEvals</action>
            <action dev="POI-DEVELOPERS" type="fix">42570 - fixed LabelRecord to use empty string instead of null when the length is zero.</action>
            <action dev="POI-DEVELOPERS" type="fix">42564 - fixed ArrayPtg to use ConstantValueParser.  Fixed a few other ArrayPtg encoding issues.</action>
            <action dev="POI-DEVELOPERS" type="fix">Follow-on from 28754 - StringPtg.toFormulaString() should escape double quotes</action>

src/java/org/apache/poi/hssf/record/formula/eval/Area2DEval.java

@@ -24,77 +24,9 @@ import org.apache.poi.hssf.record.formula.Ptg;
  * @author Amol S. Deshmukh < amolweb at ya hoo dot com >
  * 
  */
-public final class Area2DEval implements AreaEval {
+public final class Area2DEval extends AreaEvalBase {
-// TODO -refactor with Area3DEval
-    private final AreaPtg _delegate;
-
-    private final ValueEval[] _values;
 
 	public Area2DEval(Ptg ptg, ValueEval[] values) {
-        if(ptg == null) {
+		super((AreaPtg) ptg, values);
-            throw new IllegalArgumentException("ptg must not be null");
-        }
-        if(values == null) {
-            throw new IllegalArgumentException("values must not be null");
-        }
-        for(int i=values.length-1; i>=0; i--) {
-            if(values[i] == null) {
-                throw new IllegalArgumentException("value array elements must not be null");
-            }
-        }
-        // TODO - check size of array vs size of AreaPtg
-        _delegate = (AreaPtg) ptg;
-        _values = values;
-    }
-
-    public int getFirstColumn() {
-        return _delegate.getFirstColumn();
-    }
-
-    public int getFirstRow() {
-        return _delegate.getFirstRow();
-    }
-
-    public int getLastColumn() {
-        return _delegate.getLastColumn();
-    }
-
-    public int getLastRow() {
-        return _delegate.getLastRow();
-    }
-
-    public ValueEval[] getValues() {
-        return _values;
-    }
-    
-    public ValueEval getValueAt(int row, int col) {
-        ValueEval retval;
-        int index = ((row-getFirstRow())*(getLastColumn()-getFirstColumn()+1))+(col-getFirstColumn());
-        if (index <0 || index >= _values.length)
-            retval = ErrorEval.VALUE_INVALID;
-        else 
-            retval = _values[index];
-        return retval;
-    }
-    
-    public boolean contains(int row, int col) {
-        return (getFirstRow() <= row) && (getLastRow() >= row) 
-            && (getFirstColumn() <= col) && (getLastColumn() >= col);
-    }
-    
-    public boolean containsRow(int row) {
-        return (getFirstRow() <= row) && (getLastRow() >= row);
-    }
-    
-    public boolean containsColumn(short col) {
-        return (getFirstColumn() <= col) && (getLastColumn() >= col);
-    }
-    
-    public boolean isColumn() {
-        return _delegate.getFirstColumn() == _delegate.getLastColumn();
-    }
-
-    public boolean isRow() {
-        return _delegate.getFirstRow() == _delegate.getLastRow();
 	}
 }

src/java/org/apache/poi/hssf/record/formula/eval/Area3DEval.java

@@ -24,82 +24,16 @@ import org.apache.poi.hssf.record.formula.Ptg;
  * @author Amol S. Deshmukh < amolweb at ya hoo dot com >
  * 
  */
-public final class Area3DEval implements AreaEval {
+public final class Area3DEval extends AreaEvalBase {
-	// TODO -refactor with Area3DEval
-    private final Area3DPtg _delegate;
 
-    private final ValueEval[] _values;
+	private final int _externSheetIndex;
 
 	public Area3DEval(Ptg ptg, ValueEval[] values) {
-        if(ptg == null) {
+		super((Area3DPtg) ptg, values);
-            throw new IllegalArgumentException("ptg must not be null");
+		_externSheetIndex = ((Area3DPtg) ptg).getExternSheetIndex();
-        }
-        if(values == null) {
-            throw new IllegalArgumentException("values must not be null");
-        }
-        for(int i=values.length-1; i>=0; i--) {
-            if(values[i] == null) {
-                throw new IllegalArgumentException("value array elements must not be null");
-            }
-        }
-        // TODO - check size of array vs size of AreaPtg
-        _values = values;
-        _delegate = (Area3DPtg) ptg;
-    }
-
-    public int getFirstColumn() {
-        return _delegate.getFirstColumn();
-    }
-
-    public int getFirstRow() {
-        return _delegate.getFirstRow();
-    }
-
-    public int getLastColumn() {
-        return (short) _delegate.getLastColumn();
-    }
-
-    public int getLastRow() {
-        return _delegate.getLastRow();
-    }
-
-    public ValueEval[] getValues() {
-        return _values;
-    }
-    
-    public ValueEval getValueAt(int row, int col) {
-        ValueEval retval;
-        int index = (row-getFirstRow())*(col-getFirstColumn());
-        if (index <0 || index >= _values.length)
-            retval = ErrorEval.VALUE_INVALID;
-        else 
-            retval = _values[index];
-        return retval;
-    }
-    
-    public boolean contains(int row, int col) {
-        return (getFirstRow() <= row) && (getLastRow() >= row) 
-            && (getFirstColumn() <= col) && (getLastColumn() >= col);
-    }
-    
-    public boolean containsRow(int row) {
-        return (getFirstRow() <= row) && (getLastRow() >= row);
-    }
-    
-    public boolean containsColumn(short col) {
-        return (getFirstColumn() <= col) && (getLastColumn() >= col);
-    }
-    
-    
-    public boolean isColumn() {
-        return _delegate.getFirstColumn() == _delegate.getLastColumn();
-    }
-
-    public boolean isRow() {
-        return _delegate.getFirstRow() == _delegate.getLastRow();
 	}
 
 	public int getExternSheetIndex() {
-        return _delegate.getExternSheetIndex();
+		return _externSheetIndex;
 	}
 }

src/java/org/apache/poi/hssf/record/formula/eval/AreaEvalBase.java

@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.poi.hssf.record.formula.eval;
+
+import org.apache.poi.hssf.record.formula.AreaI;
+
+/**
+ * @author Josh Micich
+ */
+abstract class AreaEvalBase implements AreaEval {
+
+	private final int _firstColumn;
+	private final int _firstRow;
+	private final int _lastColumn;
+	private final int _lastRow;
+	private final ValueEval[] _values;
+	private final int _nColumns;
+	private final int _nRows;
+
+	protected AreaEvalBase(AreaI ptg, ValueEval[] values) {
+		if (values == null) {
+			throw new IllegalArgumentException("values must not be null");
+		}
+		_firstRow = ptg.getFirstRow();
+		_firstColumn = ptg.getFirstColumn();
+		_lastRow = ptg.getLastRow();
+		_lastColumn = ptg.getLastColumn();
+		
+		_nColumns = _lastColumn - _firstColumn + 1;
+		_nRows = _lastRow - _firstRow + 1;
+		
+		int expectedItemCount = _nRows * _nColumns;
+		if ((values.length != expectedItemCount)) {
+			// Note - this math may need alteration when POI starts to support full column or full row refs
+			throw new IllegalArgumentException("Array size should be (" + expectedItemCount
+					+ ") but was (" + values.length + ")");
+		}
+		
+
+
+		for (int i = values.length - 1; i >= 0; i--) {
+			if (values[i] == null) {
+				throw new IllegalArgumentException("value array elements must not be null");
+			}
+		}
+		_values = values;
+	}
+
+	public final int getFirstColumn() {
+		return _firstColumn;
+	}
+
+	public final int getFirstRow() {
+		return _firstRow;
+	}
+
+	public final int getLastColumn() {
+		return _lastColumn;
+	}
+
+	public final int getLastRow() {
+		return _lastRow;
+	}
+
+	public final ValueEval[] getValues() {
+		// TODO - clone() - but some junits rely on not cloning at the moment
+		return _values;
+	}
+
+	public final ValueEval getValueAt(int row, int col) {
+		int rowOffsetIx = row - _firstRow;
+		int colOffsetIx = col - _firstColumn;
+		
+		if(rowOffsetIx < 0 || rowOffsetIx >= _nRows) {
+			throw new IllegalArgumentException("Specified row index (" + row 
+					+ ") is outside the allowed range (" + _firstRow + ".." + _lastRow + ")");
+		}
+		if(colOffsetIx < 0 || colOffsetIx >= _nColumns) {
+			throw new IllegalArgumentException("Specified column index (" + col 
+					+ ") is outside the allowed range (" + _firstColumn + ".." + col + ")");
+		}
+
+		int index = rowOffsetIx * _nColumns + colOffsetIx;
+		return _values[index];
+	}
+
+	public final boolean contains(int row, int col) {
+		return _firstRow <= row && _lastRow >= row 
+			&& _firstColumn <= col && _lastColumn >= col;
+	}
+
+	public final boolean containsRow(int row) {
+		return (_firstRow <= row) && (_lastRow >= row);
+	}
+
+	public final boolean containsColumn(short col) {
+		return (_firstColumn <= col) && (_lastColumn >= col);
+	}
+
+	public final boolean isColumn() {
+		return _firstColumn == _lastColumn;
+	}
+
+	public final boolean isRow() {
+		return _firstRow == _lastRow;
+	}
+}

src/java/org/apache/poi/hssf/usermodel/HSSFCell.java

@@ -817,8 +817,7 @@ public class HSSFCell
         int row=record.getRow();
         short col=record.getColumn();
         short styleIndex=record.getXFIndex();
-        if ((cellType != CELL_TYPE_ERROR) && (cellType != CELL_TYPE_FORMULA))
+        if (cellType != CELL_TYPE_ERROR) {
-        {
             setCellType(CELL_TYPE_ERROR, false, row, col, styleIndex);
         }
         (( BoolErrRecord ) record).setValue(value);

src/java/org/apache/poi/hssf/usermodel/HSSFFormulaEvaluator.java

@@ -232,8 +232,7 @@ public class HSSFFormulaEvaluator {
                     cell.setCellValue(cv.getBooleanValue());
                     break;
                 case HSSFCell.CELL_TYPE_ERROR:
-                    cell.setCellType(HSSFCell.CELL_TYPE_ERROR);
+                    cell.setCellErrorValue(cv.getErrorValue());
-                    cell.setCellValue(cv.getErrorValue());
                     break;
                 case HSSFCell.CELL_TYPE_NUMERIC:
                     cell.setCellType(HSSFCell.CELL_TYPE_NUMERIC);

src/testcases/org/apache/poi/hssf/record/formula/eval/AllFormulaEvalTests.java

@@ -28,7 +28,8 @@ import junit.framework.TestSuite;
 public class AllFormulaEvalTests {
 	
 	public static Test suite() {
-		TestSuite result = new TestSuite("Tests for org.apache.poi.hssf.record.formula.eval");
+		TestSuite result = new TestSuite(AllFormulaEvalTests.class.getName());
+		result.addTestSuite(TestAreaEval.class);
 		result.addTestSuite(TestCircularReferences.class);
 		result.addTestSuite(TestExternalFunction.class);
 		result.addTestSuite(TestFormulaBugs.class);

src/testcases/org/apache/poi/hssf/record/formula/eval/TestAreaEval.java

@@ -0,0 +1,62 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.hssf.record.formula.eval;
+
+import junit.framework.AssertionFailedError;
+import junit.framework.TestCase;
+
+import org.apache.poi.hssf.record.formula.Area3DPtg;
+
+/**
+ * Tests for <tt>AreaEval</tt>
+ *  
+ * @author Josh Micich
+ */
+public final class TestAreaEval extends TestCase {
+
+	public void testGetValue_bug44950() {
+		
+		Area3DPtg ptg = new Area3DPtg("B2:D3", (short)0);
+		NumberEval one = new NumberEval(1);
+		ValueEval[] values = {
+				one,	
+				new NumberEval(2),	
+				new NumberEval(3),	
+				new NumberEval(4),	
+				new NumberEval(5),	
+				new NumberEval(6),	
+		};
+		AreaEval ae = new Area3DEval(ptg, values);
+		if (one == ae.getValueAt(1, 2)) {
+			throw new AssertionFailedError("Identified bug 44950 a");
+		}
+		confirm(1, ae, 1, 1);
+		confirm(2, ae, 1, 2);
+		confirm(3, ae, 1, 3);
+		confirm(4, ae, 2, 1);
+		confirm(5, ae, 2, 2);
+		confirm(6, ae, 2, 3);
+		
+	}
+
+	private static void confirm(int expectedValue, AreaEval ae, int row, int col) {
+		NumberEval v = (NumberEval) ae.getValueAt(row, col);
+		assertEquals(expectedValue, v.getNumberValue(), 0.0);
+	}
+
+}

src/testcases/org/apache/poi/hssf/record/formula/functions/TestCountFuncs.java

@@ -67,7 +67,7 @@ public final class TestCountFuncs extends TestCase {
 		args = new Eval[] {
 			EvalFactory.createAreaEval("D1:F5", 3, 5),	// 15
 			EvalFactory.createRefEval("A1"),	
-			EvalFactory.createAreaEval("A1:F6", 7, 6),	// 42
+			EvalFactory.createAreaEval("A1:G6", 7, 6),	// 42
 			new NumberEval(0),
 		};
 		confirmCountA(59, args);
@@ -87,7 +87,7 @@ public final class TestCountFuncs extends TestCase {
 				BoolEval.TRUE,
 				BlankEval.INSTANCE,
 		};
-		range = createAreaEval("A1:B2", values);
+		range = createAreaEval("A1:B3", values);
 		confirmCountIf(2, range, BoolEval.TRUE);
 		
 		// when criteria is numeric
@@ -98,9 +98,8 @@ public final class TestCountFuncs extends TestCase {
 				new NumberEval(2),	
 				new NumberEval(2),	
 				BoolEval.TRUE,
-				BlankEval.INSTANCE,
 		};
-		range = createAreaEval("A1:B2", values);
+		range = createAreaEval("A1:B3", values);
 		confirmCountIf(3, range, new NumberEval(2));
 		// note - same results when criteria is a string that parses as the number with the same value
 		confirmCountIf(3, range, new StringEval("2.00"));

src/testcases/org/apache/poi/hssf/record/formula/functions/TestIndex.java

@@ -44,7 +44,7 @@ public final class TestIndex extends TestCase {
 			7, 8,
 			9, 10,
 			11, 12,
-			13, // excess array element. TODO - Area2DEval currently has no validation to ensure correct size of values array
+//			13, // excess array element. TODO - Area2DEval currently has no validation to ensure correct size of values array
 	};
 	
 	/**

src/testcases/org/apache/poi/hssf/usermodel/AllUserModelTests.java

@@ -38,10 +38,12 @@ public class AllUserModelTests {
 		result.addTestSuite(TestEscherGraphics2d.class);
 		result.addTestSuite(TestFontDetails.class);
 		result.addTestSuite(TestFormulas.class);
+		result.addTestSuite(TestFormulaEvaluatorBugs.class);
+		result.addTestSuite(TestFormulaEvaluatorDocs.class);
 		result.addTestSuite(TestHSSFCell.class);
 		result.addTestSuite(TestHSSFClientAnchor.class);
-		result.addTestSuite(TestHSSFConditionalFormatting.class);
 		result.addTestSuite(TestHSSFComment.class);
+		result.addTestSuite(TestHSSFConditionalFormatting.class);
 		result.addTestSuite(TestHSSFDateUtil.class);
 		result.addTestSuite(TestHSSFHeaderFooter.class);
 		result.addTestSuite(TestHSSFHyperlink.class);
@@ -54,9 +56,11 @@ public class AllUserModelTests {
 		result.addTestSuite(TestHSSFSheet.class);
 		result.addTestSuite(TestHSSFSheetOrder.class);
 		result.addTestSuite(TestHSSFSheetSetOrder.class);
+		result.addTestSuite(TestHSSFTextbox.class);
 		result.addTestSuite(TestHSSFWorkbook.class);
 		result.addTestSuite(TestNamedRange.class);
 		result.addTestSuite(TestOLE2Embeding.class);
+		result.addTestSuite(TestPOIFSProperties.class);
 		result.addTestSuite(TestReadWriteChart.class);
 		result.addTestSuite(TestSanityChecker.class);
 		result.addTestSuite(TestSheetHiding.class);

src/testcases/org/apache/poi/hssf/usermodel/TestFormulaEvaluatorBugs.java

@@ -22,12 +22,14 @@ import java.io.FileOutputStream;
 import java.util.Iterator;
 import java.util.List;
 
+import junit.framework.AssertionFailedError;
 import junit.framework.TestCase;
 
 import org.apache.poi.hssf.HSSFTestDataSamples;
 import org.apache.poi.hssf.record.aggregates.FormulaRecordAggregate;
 import org.apache.poi.hssf.record.formula.AreaPtg;
 import org.apache.poi.hssf.record.formula.FuncVarPtg;
+
 /**
  * 
  */
@@ -70,7 +72,6 @@ public final class TestFormulaEvaluatorBugs extends TestCase {
 		out.close();
 		System.err.println("Existing file for bug #44636 written to " + existing.toString());
 
-
 		// Now, do a new file from scratch
 		wb = new HSSFWorkbook();
 		sheet = wb.createSheet();
@@ -190,8 +191,7 @@ public final class TestFormulaEvaluatorBugs extends TestCase {
 		// Test the sum
 		HSSFCell cellSUM = rowSUM.getCell((short) 0);
 
-		FormulaRecordAggregate frec = 
+		FormulaRecordAggregate frec = (FormulaRecordAggregate) cellSUM.getCellValueRecord();
-			(FormulaRecordAggregate)cellSUM.getCellValueRecord();
 		List ops = frec.getFormulaRecord().getParsedExpression();
 		assertEquals(2, ops.size());
 		assertEquals(AreaPtg.class, ops.get(0).getClass());
@@ -216,7 +216,6 @@ public final class TestFormulaEvaluatorBugs extends TestCase {
 		// can do it properly
 		assertEquals(6, eva.evaluate(cellSUM).getNumberValue(), 0);
 
-
 		// Test the index
 		// Again, the formula string will be right but
 		// lacking row count, evaluated will be right
@@ -255,8 +254,7 @@ public final class TestFormulaEvaluatorBugs extends TestCase {
 	}
 
 	public void testClassCast_bug44861() throws Exception {
-		HSSFWorkbook wb = HSSFTestDataSamples.
+		HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("44861.xls");
-			openSampleWorkbook("44861.xls");
 
 		// Check direct
 		HSSFFormulaEvaluator.evaluateAllFormulaCells(wb);
@@ -278,4 +276,22 @@ public final class TestFormulaEvaluatorBugs extends TestCase {
 			}
 		}
 	}
+
+	public void testEvaluateInCellWithErrorCode_bug44950() {
+		HSSFWorkbook wb = new HSSFWorkbook();
+		HSSFSheet sheet = wb.createSheet("Sheet1");
+		HSSFRow row = sheet.createRow(1);
+		HSSFCell cell = row.createCell((short) 0);
+		cell.setCellFormula("na()"); // this formula evaluates to an Excel error code '#N/A'
+		HSSFFormulaEvaluator fe = new HSSFFormulaEvaluator(sheet, wb);
+		fe.setCurrentRow(row);
+		try {
+			fe.evaluateInCell(cell);
+		} catch (NumberFormatException e) {
+			if (e.getMessage().equals("You cannot get an error value from a non-error cell")) {
+				throw new AssertionFailedError("Identified bug 44950 b");
+			}
+			throw e;
+		}
+	}
 }