Fixed 44675 - Parameter operand classes (function metadata) required to encode SUM() etc properly. Added parse validation for number of parameters

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@652994 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">44675 - Parameter operand classes (function metadata) required to encode SUM() etc properly. Added parse validation for number of parameters</action>
            <action dev="POI-DEVELOPERS" type="fix">44921 - allow Ptg.writeBytes() to be called on relative ref Ptgs (RefN* and AreaN*)</action>
            <action dev="POI-DEVELOPERS" type="fix">44914 - Fix/suppress warning message "WARN. Unread n bytes of record 0xNN"</action>
            <action dev="POI-DEVELOPERS" type="fix">44892 - made HSSFWorkbook.getSheet(String) case insensitive</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">44675 - Parameter operand classes (function metadata) required to encode SUM() etc properly. Added parse validation for number of parameters</action>
            <action dev="POI-DEVELOPERS" type="fix">44921 - allow Ptg.writeBytes() to be called on relative ref Ptgs (RefN* and AreaN*)</action>
            <action dev="POI-DEVELOPERS" type="fix">44914 - Fix/suppress warning message "WARN. Unread n bytes of record 0xNN"</action>
            <action dev="POI-DEVELOPERS" type="fix">44892 - made HSSFWorkbook.getSheet(String) case insensitive</action>

src/java/org/apache/poi/hssf/model/FormulaParser.java

@@ -380,12 +380,13 @@ public final class FormulaParser {
         } else {
             isVarArgs = !fm.hasFixedArgsLength();
             funcIx = fm.getIndex();
+            validateNumArgs(numArgs, fm);
         }
         AbstractFunctionPtg retval;
         if(isVarArgs) {
             retval = new FuncVarPtg(name, (byte)numArgs);
         } else {
-            retval = new FuncPtg(funcIx, (byte)numArgs);
+            retval = new FuncPtg(funcIx);
         }
         if (!name.equals(AbstractFunctionPtg.FUNCTION_NAME_IF)) {
             // early return for everything else besides IF()
@@ -447,6 +448,29 @@ public final class FormulaParser {
         return retval;
     }
 
+    private void validateNumArgs(int numArgs, FunctionMetadata fm) {
+        if(numArgs < fm.getMinParams()) {
+            String msg = "Too few arguments to function '" + fm.getName() + "'. ";
+            if(fm.hasFixedArgsLength()) {
+                msg += "Expected " + fm.getMinParams();
+            } else {
+                msg += "At least " + fm.getMinParams() + " were expected";
+            }
+            msg += " but got " + numArgs + ".";
+            throw new FormulaParseException(msg);
+         }
+        if(numArgs > fm.getMaxParams()) {
+            String msg = "Too many arguments to function '" + fm.getName() + "'. ";
+            if(fm.hasFixedArgsLength()) {
+                msg += "Expected " + fm.getMaxParams();
+            } else {
+                msg += "At most " + fm.getMaxParams() + " were expected";
+            }
+            msg += " but got " + numArgs + ".";
+            throw new FormulaParseException(msg);
+       }
+    }
+
     private static boolean isArgumentDelimiter(char ch) {
         return ch ==  ',' || ch == ')';
     }

src/java/org/apache/poi/hssf/record/formula/FuncPtg.java

@@ -57,10 +57,12 @@ public final class FuncPtg extends AbstractFunctionPtg {
         }
         numParams = fm.getMinParams();
     }
-    public FuncPtg(int functionIndex, int numberOfParameters) {
+    public FuncPtg(int functionIndex) {
         field_2_fnc_index = (short) functionIndex;
-        numParams = numberOfParameters;
+        FunctionMetadata fm = FunctionMetadataRegistry.getFunctionByIndex(functionIndex);
-        paramClass = new byte[] { Ptg.CLASS_VALUE, }; // TODO
+        numParams = fm.getMinParams(); // same as max since these are not var-arg funcs
+        returnClass = fm.getReturnClassCode();
+        paramClass = fm.getParameterClassCodes();
     }
 
     public void writeBytes(byte[] array, int offset) {

src/java/org/apache/poi/hssf/record/formula/FuncVarPtg.java

@@ -54,8 +54,8 @@ public final class FuncVarPtg extends AbstractFunctionPtg{
             returnClass = Ptg.CLASS_VALUE;
             paramClass = new byte[] {Ptg.CLASS_VALUE};
         } else {
-            returnClass = Ptg.CLASS_VALUE;
+            returnClass = fm.getReturnClassCode();
-            paramClass = new byte[] {Ptg.CLASS_VALUE};
+            paramClass = fm.getParameterClassCodes();
         }
     }
 

src/java/org/apache/poi/hssf/record/formula/function/FunctionDataBuilder.java

@@ -42,8 +42,10 @@ final class FunctionDataBuilder {
 		_mutatingFunctionIndexes = new HashSet();
 	}
 
-	public void add(int functionIndex, String functionName, int minParams, int maxParams, boolean hasFootnote) {
+	public void add(int functionIndex, String functionName, int minParams, int maxParams,
-		FunctionMetadata fm = new FunctionMetadata(functionIndex, functionName, minParams, maxParams);
+			byte returnClassCode, byte[] parameterClassCodes, boolean hasFootnote) {
+		FunctionMetadata fm = new FunctionMetadata(functionIndex, functionName, minParams, maxParams,
+				returnClassCode, parameterClassCodes);
 		
 		Integer indexKey = new Integer(functionIndex);
 		

src/java/org/apache/poi/hssf/record/formula/function/FunctionMetadata.java

@@ -27,12 +27,17 @@ public final class FunctionMetadata {
 	private final String _name;
 	private final int _minParams;
 	private final int _maxParams;
+	private final byte _returnClassCode;
+	private final byte[] _parameterClassCodes;
 
-	/* package */ FunctionMetadata(int index, String name, int minParams, int maxParams) {
+	/* package */ FunctionMetadata(int index, String name, int minParams, int maxParams,
+			byte returnClassCode, byte[] parameterClassCodes) {
 		_index = index;
 		_name = name;
 		_minParams = minParams;
 		_maxParams = maxParams;
+		_returnClassCode = returnClassCode;
+		_parameterClassCodes = parameterClassCodes;
 	}
 	public int getIndex() {
 		return _index;
@@ -49,6 +54,12 @@ public final class FunctionMetadata {
 	public boolean hasFixedArgsLength() {
 		return _minParams == _maxParams;
 	}
+	public byte getReturnClassCode() {
+		return _returnClassCode;
+	}
+	public byte[] getParameterClassCodes() {
+		return (byte[]) _parameterClassCodes.clone();
+	}
 	public String toString() {
 		StringBuffer sb = new StringBuffer(64);
 		sb.append(getClass().getName()).append(" [");

src/java/org/apache/poi/hssf/record/formula/function/FunctionMetadataReader.java

@@ -26,6 +26,8 @@ import java.util.HashSet;
 import java.util.Set;
 import java.util.regex.Pattern;
 
+import org.apache.poi.hssf.record.formula.Ptg;
+
 /**
  * Converts the text meta-data file into a <tt>FunctionMetadataRegistry</tt>
  * 
@@ -36,6 +38,12 @@ final class FunctionMetadataReader {
 	private static final String METADATA_FILE_NAME = "functionMetadata.txt";
 
 	private static final Pattern TAB_DELIM_PATTERN = Pattern.compile("\t");
+	private static final Pattern SPACE_DELIM_PATTERN = Pattern.compile(" ");
+	private static final byte[] EMPTY_BYTE_ARRAY = { };
+
+	// special characters from the ooo document
+	private static final int CHAR_ELLIPSIS_8230 = 8230;
+	private static final int CHAR_NDASH_8211 = 8211;
 	
 	private static final String[] DIGIT_ENDING_FUNCTION_NAMES = {
 		// Digits at the end of a function might be due to a left-over footnote marker.
@@ -86,14 +94,66 @@ final class FunctionMetadataReader {
 		String functionName = parts[1];
 		int minParams = parseInt(parts[2]);
 		int maxParams = parseInt(parts[3]);
-		// 4 returnClass
+		byte returnClassCode = parseReturnTypeCode(parts[4]);
-		// 5 parameterClasses
+		byte[] parameterClassCodes = parseOperandTypeCodes(parts[5]);
 		// 6 isVolatile
 		boolean hasNote = parts[7].length() > 0;
 
 		validateFunctionName(functionName);
-		// TODO - make POI use returnClass, parameterClasses, isVolatile
+		// TODO - make POI use isVolatile
-		fdb.add(functionIndex, functionName, minParams, maxParams, hasNote);
+		fdb.add(functionIndex, functionName, minParams, maxParams, 
+				returnClassCode, parameterClassCodes, hasNote);
+	}
+	
+
+	private static byte parseReturnTypeCode(String code) {
+		if(code.length() == 0) {
+			return Ptg.CLASS_REF; // happens for GETPIVOTDATA
+		}
+		return parseOperandTypeCode(code);
+	}
+
+	private static byte[] parseOperandTypeCodes(String codes) {
+		if(codes.length() < 1) {
+			return EMPTY_BYTE_ARRAY; // happens for GETPIVOTDATA
+		}
+		if(isDash(codes)) {
+			// '-' means empty:
+			return EMPTY_BYTE_ARRAY;
+		}
+		String[] array = SPACE_DELIM_PATTERN.split(codes);
+		int nItems = array.length;
+		if(array[nItems-1].charAt(0) == CHAR_ELLIPSIS_8230) {
+			nItems --;
+		}
+		byte[] result = new byte[nItems];
+		for (int i = 0; i < nItems; i++) {
+			result[i] = parseOperandTypeCode(array[i]);
+		}
+		return result;
+	}
+
+	private static boolean isDash(String codes) {
+		if(codes.length() == 1) {
+			switch (codes.charAt(0)) {
+				case '-':
+				case CHAR_NDASH_8211: // this is what the ooo doc has
+					return true;
+			}
+		}
+		return false;
+	}
+
+	private static byte parseOperandTypeCode(String code) {
+		if(code.length() != 1) {
+			throw new RuntimeException("Bad operand type code format '" + code  + "' expected single char");
+		}
+		switch(code.charAt(0)) {
+			case 'V': return Ptg.CLASS_VALUE;
+			case 'R': return Ptg.CLASS_REF;
+			case 'A': return Ptg.CLASS_ARRAY;
+		}
+		throw new IllegalArgumentException("Unexpected operand type code '" + code + "'");
 	}
 
 	/**

src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java

@@ -592,7 +592,7 @@ public final class TestFormulaParser extends TestCase {
     	HSSFWorkbook book = new HSSFWorkbook();
 
         Ptg[] ptgs = {
-                new FuncPtg(10, 0),
+                new FuncPtg(10),
         };
         assertEquals("NA()", FormulaParser.toFormulaString(book, ptgs));
     }
@@ -900,4 +900,21 @@ public final class TestFormulaParser extends TestCase {
         assertEquals(2, ptgs.length);
         assertEquals(FuncPtg.class, ptgs[1].getClass());
     }
+    
+    public void testWrongNumberOfFunctionArgs() {
+        confirmArgCountMsg("sin()", "Too few arguments to function 'SIN'. Expected 1 but got 0.");
+        confirmArgCountMsg("countif(1, 2, 3, 4)", "Too many arguments to function 'COUNTIF'. Expected 2 but got 4.");
+        confirmArgCountMsg("index(1, 2, 3, 4, 5, 6)", "Too many arguments to function 'INDEX'. At most 4 were expected but got 6.");
+        confirmArgCountMsg("vlookup(1, 2)", "Too few arguments to function 'VLOOKUP'. At least 3 were expected but got 2.");
+    }
+
+    private static void confirmArgCountMsg(String formula, String expectedMessage) {
+        HSSFWorkbook book = new HSSFWorkbook();
+        try {
+            FormulaParser.parse(formula, book);
+            throw new AssertionFailedError("Didn't get parse exception as expected");
+        } catch (FormulaParseException e) {
+            assertEquals(expectedMessage, e.getMessage());
+        }
+    }
 }

src/testcases/org/apache/poi/hssf/record/formula/AllFormulaTests.java

@@ -43,6 +43,7 @@ public final class AllFormulaTests {
 		result.addTestSuite(TestErrPtg.class);
 		result.addTestSuite(TestExternalFunctionFormulas.class);
 		result.addTestSuite(TestFuncPtg.class);
+		result.addTestSuite(TestFuncVarPtg.class);
 		result.addTestSuite(TestIntersectionPtg.class);
 		result.addTestSuite(TestPercentPtg.class);
 		result.addTestSuite(TestRangePtg.class);

src/testcases/org/apache/poi/hssf/record/formula/TestFuncVarPtg.java

@@ -0,0 +1,53 @@
+/* ====================================================================
+   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;
+
+import org.apache.poi.hssf.model.FormulaParser;
+import org.apache.poi.hssf.usermodel.HSSFWorkbook;
+
+import junit.framework.AssertionFailedError;
+import junit.framework.TestCase;
+/**
+ * @author Josh Micich
+ */
+public final class TestFuncVarPtg extends TestCase {
+
+	/**
+	 * The first fix for bugzilla 44675 broke the encoding of SUM formulas (and probably others).
+	 * The operand classes of the parameters to SUM() should be coerced to 'reference' not 'value'.
+	 * In the case of SUM, Excel evaluates the formula to '#VALUE!' if a parameter operand class is
+	 * wrong.  In other cases Excel seems to tolerate bad operand classes.</p>
+	 * This functionality is related to the setParameterRVA() methods of <tt>FormulaParser</tt>
+	 */
+	public void testOperandClass() {
+		HSSFWorkbook book = new HSSFWorkbook();
+		Ptg[] ptgs = FormulaParser.parse("sum(A1:A2)", book);
+		assertEquals(2, ptgs.length);
+		assertEquals(AreaPtg.class, ptgs[0].getClass());
+		
+		switch(ptgs[0].getPtgClass()) {
+			case Ptg.CLASS_REF:
+				// correct behaviour
+				break;
+			case Ptg.CLASS_VALUE:
+				throw new AssertionFailedError("Identified bug 44675b");
+			default:
+				throw new RuntimeException("Unexpected operand class");
+		}
+	}
+}