From cc363f369d5619a3249b8194ca2a166346e2b01f Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Sat, 3 May 2008 03:59:32 +0000 Subject: [PATCH] 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 | 1 + src/documentation/content/xdocs/status.xml | 1 + .../apache/poi/hssf/model/FormulaParser.java | 26 ++++++- .../poi/hssf/record/formula/FuncPtg.java | 8 ++- .../poi/hssf/record/formula/FuncVarPtg.java | 4 +- .../formula/function/FunctionDataBuilder.java | 6 +- .../formula/function/FunctionMetadata.java | 13 +++- .../function/FunctionMetadataReader.java | 68 +++++++++++++++++-- .../poi/hssf/model/TestFormulaParser.java | 19 +++++- .../hssf/record/formula/AllFormulaTests.java | 1 + .../hssf/record/formula/TestFuncVarPtg.java | 53 +++++++++++++++ 11 files changed, 186 insertions(+), 14 deletions(-) create mode 100644 src/testcases/org/apache/poi/hssf/record/formula/TestFuncVarPtg.java diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index e0fdf3966d..ce08eff1a9 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,6 +37,7 @@ + 44675 - Parameter operand classes (function metadata) required to encode SUM() etc properly. Added parse validation for number of parameters 44921 - allow Ptg.writeBytes() to be called on relative ref Ptgs (RefN* and AreaN*) 44914 - Fix/suppress warning message "WARN. Unread n bytes of record 0xNN" 44892 - made HSSFWorkbook.getSheet(String) case insensitive diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index f1a5cd1d8e..7e4277acb0 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,6 +34,7 @@ + 44675 - Parameter operand classes (function metadata) required to encode SUM() etc properly. Added parse validation for number of parameters 44921 - allow Ptg.writeBytes() to be called on relative ref Ptgs (RefN* and AreaN*) 44914 - Fix/suppress warning message "WARN. Unread n bytes of record 0xNN" 44892 - made HSSFWorkbook.getSheet(String) case insensitive diff --git a/src/java/org/apache/poi/hssf/model/FormulaParser.java b/src/java/org/apache/poi/hssf/model/FormulaParser.java index 05b4d80f4a..f16ab45129 100644 --- a/src/java/org/apache/poi/hssf/model/FormulaParser.java +++ b/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 == ')'; } diff --git a/src/java/org/apache/poi/hssf/record/formula/FuncPtg.java b/src/java/org/apache/poi/hssf/record/formula/FuncPtg.java index 69edf88aae..7901fb675d 100644 --- a/src/java/org/apache/poi/hssf/record/formula/FuncPtg.java +++ b/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; - paramClass = new byte[] { Ptg.CLASS_VALUE, }; // TODO + FunctionMetadata fm = FunctionMetadataRegistry.getFunctionByIndex(functionIndex); + 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) { diff --git a/src/java/org/apache/poi/hssf/record/formula/FuncVarPtg.java b/src/java/org/apache/poi/hssf/record/formula/FuncVarPtg.java index fb65271393..431dc5717b 100644 --- a/src/java/org/apache/poi/hssf/record/formula/FuncVarPtg.java +++ b/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; - paramClass = new byte[] {Ptg.CLASS_VALUE}; + returnClass = fm.getReturnClassCode(); + paramClass = fm.getParameterClassCodes(); } } diff --git a/src/java/org/apache/poi/hssf/record/formula/function/FunctionDataBuilder.java b/src/java/org/apache/poi/hssf/record/formula/function/FunctionDataBuilder.java index 2009ebae1f..789be63240 100644 --- a/src/java/org/apache/poi/hssf/record/formula/function/FunctionDataBuilder.java +++ b/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) { - FunctionMetadata fm = new FunctionMetadata(functionIndex, functionName, minParams, maxParams); + public void add(int functionIndex, String functionName, int minParams, int maxParams, + byte returnClassCode, byte[] parameterClassCodes, boolean hasFootnote) { + FunctionMetadata fm = new FunctionMetadata(functionIndex, functionName, minParams, maxParams, + returnClassCode, parameterClassCodes); Integer indexKey = new Integer(functionIndex); diff --git a/src/java/org/apache/poi/hssf/record/formula/function/FunctionMetadata.java b/src/java/org/apache/poi/hssf/record/formula/function/FunctionMetadata.java index 94f1659b51..fc5f845437 100644 --- a/src/java/org/apache/poi/hssf/record/formula/function/FunctionMetadata.java +++ b/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(" ["); diff --git a/src/java/org/apache/poi/hssf/record/formula/function/FunctionMetadataReader.java b/src/java/org/apache/poi/hssf/record/formula/function/FunctionMetadataReader.java index bd72a92233..2cdc540e6f 100644 --- a/src/java/org/apache/poi/hssf/record/formula/function/FunctionMetadataReader.java +++ b/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 FunctionMetadataRegistry * @@ -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 - // 5 parameterClasses + byte returnClassCode = parseReturnTypeCode(parts[4]); + byte[] parameterClassCodes = parseOperandTypeCodes(parts[5]); // 6 isVolatile boolean hasNote = parts[7].length() > 0; validateFunctionName(functionName); - // TODO - make POI use returnClass, parameterClasses, isVolatile - fdb.add(functionIndex, functionName, minParams, maxParams, hasNote); + // TODO - make POI use isVolatile + 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 + "'"); } /** diff --git a/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java b/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java index 31b4dd180a..9ce862b9b6 100644 --- a/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java +++ b/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()); + } + } } diff --git a/src/testcases/org/apache/poi/hssf/record/formula/AllFormulaTests.java b/src/testcases/org/apache/poi/hssf/record/formula/AllFormulaTests.java index 3a70450223..80f6616fa6 100644 --- a/src/testcases/org/apache/poi/hssf/record/formula/AllFormulaTests.java +++ b/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); diff --git a/src/testcases/org/apache/poi/hssf/record/formula/TestFuncVarPtg.java b/src/testcases/org/apache/poi/hssf/record/formula/TestFuncVarPtg.java new file mode 100644 index 0000000000..58ae55f26b --- /dev/null +++ b/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.

+ * This functionality is related to the setParameterRVA() methods of FormulaParser + */ + 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"); + } + } +} -- 2.39.5