Index: src/java/org/apache/poi/ss/formula/FormulaParser.java =================================================================== --- src/java/org/apache/poi/ss/formula/FormulaParser.java (revision 1706948) +++ src/java/org/apache/poi/ss/formula/FormulaParser.java (working copy) @@ -50,6 +50,7 @@ import org.apache.poi.ss.formula.ptg.MultiplyPtg; import org.apache.poi.ss.formula.ptg.NamePtg; import org.apache.poi.ss.formula.ptg.NameXPtg; +import org.apache.poi.ss.formula.ptg.NameXPxg; import org.apache.poi.ss.formula.ptg.NotEqualPtg; import org.apache.poi.ss.formula.ptg.NumberPtg; import org.apache.poi.ss.formula.ptg.OperandPtg; @@ -67,9 +68,12 @@ import org.apache.poi.ss.formula.ptg.UnionPtg; import org.apache.poi.ss.formula.ptg.ValueOperatorPtg; import org.apache.poi.ss.usermodel.ErrorConstants; +import org.apache.poi.ss.usermodel.Name; import org.apache.poi.ss.util.AreaReference; import org.apache.poi.ss.util.CellReference; import org.apache.poi.ss.util.CellReference.NameType; +import org.apache.poi.util.POILogFactory; +import org.apache.poi.util.POILogger; /** * This class parses a formula string into a List of tokens in RPN order. @@ -85,6 +89,7 @@ *

*/ public final class FormulaParser { + private final static POILogger log = POILogFactory.getLogger(FormulaParser.class); private final String _formulaString; private final int _formulaLength; /** points at the next character to be read (after the {@link #look} char) */ @@ -108,10 +113,10 @@ */ private boolean _inIntersection = false; - private FormulaParsingWorkbook _book; - private SpreadsheetVersion _ssVersion; + private final FormulaParsingWorkbook _book; + private final SpreadsheetVersion _ssVersion; - private int _sheetIndex; + private final int _sheetIndex; /** @@ -137,6 +142,7 @@ /** * Parse a formula into a array of tokens + * Side effect: creates name (Workbook.createName) if formula contains unrecognized names (names are likely UDFs) * * @param formula the formula to parse * @param workbook the parent workbook @@ -927,6 +933,8 @@ * Note - Excel function names are 'case aware but not case sensitive'. This method may end * up creating a defined name record in the workbook if the specified name is not an internal * Excel function, and has not been encountered before. + * + * Side effect: creates workbook name if name is not recognized (name is probably a UDF) * * @param name case preserved function name (as it was entered/appeared in the formula). */ @@ -940,22 +948,42 @@ // Only test cases omit the book (expecting it not to be needed) throw new IllegalStateException("Need book to evaluate name '" + name + "'"); } + // Check to see if name is a named range in the workbook EvaluationName hName = _book.getName(name, _sheetIndex); - if (hName == null) { - nameToken = _book.getNameXPtg(name, null); - if (nameToken == null) { - throw new FormulaParseException("Name '" + name - + "' is completely unknown in the current workbook"); - } - } else { + if (hName != null) { if (!hName.isFunctionName()) { throw new FormulaParseException("Attempt to use name '" + name + "' as a function, but defined name in workbook does not refer to a function"); } - + // calls to user-defined functions within the workbook // get a Name token which points to a defined name record nameToken = hName.createPtg(); + } else { + // Check if name is an external names table + nameToken = _book.getNameXPtg(name, null); + if (nameToken == null) { + // name is not an internal or external name + if (log.check(POILogger.WARN)) { + log.log(POILogger.WARN, + "FormulaParser.function: Name '" + name + "' is completely unknown in the current workbook."); + } + // name is probably the name of an unregistered User-Defined Function + switch (_book.getSpreadsheetVersion()) { + case EXCEL97: + // HSSFWorkbooks require a name to be added to Workbook defined names table + addName(name); + hName = _book.getName(name, _sheetIndex); + nameToken = hName.createPtg(); + break; + case EXCEL2007: + // XSSFWorkbooks store formula names as strings. + nameToken = new NameXPxg(name); + break; + default: + throw new IllegalStateException("Unexpected spreadsheet version: " + _book.getSpreadsheetVersion().name()); + } + } } } @@ -965,6 +993,17 @@ return getFunction(name, nameToken, args); } + + /** + * Adds a name (named range or user defined function) to underlying workbook's names table + * @param functionName + */ + private final void addName(String functionName) { + final Name name = _book.createName(); + name.setFunction(true); + name.setNameName(functionName); + name.setSheetIndex(_sheetIndex); + } /** * Generates the variable function ptg for the formula. Index: src/ooxml/testcases/org/apache/poi/ss/formula/TestFormulaParser.java =================================================================== --- src/ooxml/testcases/org/apache/poi/ss/formula/TestFormulaParser.java (revision 1706948) +++ src/ooxml/testcases/org/apache/poi/ss/formula/TestFormulaParser.java (working copy) @@ -18,8 +18,17 @@ */ package org.apache.poi.ss.formula; +import java.io.File; +import java.io.FileOutputStream; +import java.util.Locale; + import org.apache.poi.hssf.usermodel.HSSFEvaluationWorkbook; import org.apache.poi.hssf.usermodel.HSSFWorkbook; +import org.apache.poi.ss.formula.ptg.AbstractFunctionPtg; +import org.apache.poi.ss.formula.ptg.NameXPxg; +import org.apache.poi.ss.formula.ptg.Ptg; +import org.apache.poi.ss.formula.ptg.StringPtg; +import org.apache.poi.xssf.XSSFTestDataSamples; import org.apache.poi.xssf.usermodel.XSSFEvaluationWorkbook; import org.apache.poi.xssf.usermodel.XSSFWorkbook; @@ -62,5 +71,92 @@ catch (FormulaParseException expected) { } } + + // copied from org.apache.poi.hssf.model.TestFormulaParser + public void testMacroFunction() throws Exception { + // testNames.xlsm contains a VB function called 'myFunc' + final String testFile = "testNames.xlsm"; + XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook(testFile); + try { + XSSFEvaluationWorkbook workbook = XSSFEvaluationWorkbook.create(wb); + + //Expected ptg stack: [NamePtg(myFunc), StringPtg(arg), (additional operands would go here...), FunctionPtg(myFunc)] + Ptg[] ptg = FormulaParser.parse("myFunc(\"arg\")", workbook, FormulaType.CELL, -1); + assertEquals(3, ptg.length); + + // the name gets encoded as the first operand on the stack + NameXPxg tname = (NameXPxg) ptg[0]; + assertEquals("myFunc", tname.toFormulaString()); + + // the function's arguments are pushed onto the stack from left-to-right as OperandPtgs + StringPtg arg = (StringPtg) ptg[1]; + assertEquals("arg", arg.getValue()); + + // The external FunctionPtg is the last Ptg added to the stack + // During formula evaluation, this Ptg pops off the the appropriate number of + // arguments (getNumberOfOperands()) and pushes the result on the stack + AbstractFunctionPtg tfunc = (AbstractFunctionPtg) ptg[2]; + assertTrue(tfunc.isExternalFunction()); + + // confirm formula parsing is case-insensitive + FormulaParser.parse("mYfUnC(\"arg\")", workbook, FormulaType.CELL, -1); + + // confirm formula parsing doesn't care about argument count or type + // this should only throw an error when evaluating the formula. + FormulaParser.parse("myFunc()", workbook, FormulaType.CELL, -1); + FormulaParser.parse("myFunc(\"arg\", 0, TRUE)", workbook, FormulaType.CELL, -1); + + // A completely unknown formula name (not saved in workbook) should still be parseable and renderable + // but will throw an NotImplementedFunctionException or return a #NAME? error value if evaluated. + FormulaParser.parse("yourFunc(\"arg\")", workbook, FormulaType.CELL, -1); + + // Make sure workbook can be written and read + XSSFTestDataSamples.writeOutAndReadBack(wb).close(); + + // Manually check to make sure file isn't corrupted + final File fileIn = XSSFTestDataSamples.getSampleFile(testFile); + final File reSavedFile = new File(fileIn.getParentFile(), fileIn.getName().replace(".xlsm", "-saved.xlsm")); + final FileOutputStream fos = new FileOutputStream(reSavedFile); + wb.write(fos); + fos.close(); + } finally { + wb.close(); + } + } + + public void testParserErrors() throws Exception { + XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("testNames.xlsm"); + try { + XSSFEvaluationWorkbook workbook = XSSFEvaluationWorkbook.create(wb); + + parseExpectedException("("); + parseExpectedException(")"); + parseExpectedException("+"); + parseExpectedException("42+"); + parseExpectedException("IF()"); + parseExpectedException("IF("); //no closing paren + parseExpectedException("myFunc(", workbook); //no closing paren + } finally { + wb.close(); + } + } + + private static void parseExpectedException(String formula) { + parseExpectedException(formula, null); + } + + /** confirm formula has invalid syntax and parsing the formula results in FormulaParseException + * @param formula + * @param wb + */ + private static void parseExpectedException(String formula, FormulaParsingWorkbook wb) { + try { + FormulaParser.parse(formula, wb, FormulaType.CELL, -1); + fail("Expected FormulaParseException: " + formula); + } catch (final FormulaParseException e) { + // expected during successful test + assertNotNull(e.getMessage()); + } + } } Index: src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java =================================================================== --- src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java (revision 1706948) +++ src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java (working copy) @@ -19,7 +19,10 @@ import static org.junit.Assert.assertArrayEquals; +import java.io.File; +import java.io.FileOutputStream; import java.io.IOException; +import java.util.Locale; import junit.framework.AssertionFailedError; import junit.framework.TestCase; @@ -111,21 +114,69 @@ assertEquals("TOTAL[", ((StringPtg)ptgs[0]).getValue()); } - public void testMacroFunction() { + public void testMacroFunction() throws IOException { // testNames.xls contains a VB function called 'myFunc' - HSSFWorkbook w = HSSFTestDataSamples.openSampleWorkbook("testNames.xls"); - HSSFEvaluationWorkbook book = HSSFEvaluationWorkbook.create(w); + final String testFile = "testNames.xls"; + HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook(testFile); + try { + HSSFEvaluationWorkbook book = HSSFEvaluationWorkbook.create(wb); - Ptg[] ptg = HSSFFormulaParser.parse("myFunc()", w); - // myFunc() actually takes 1 parameter. Don't know if POI will ever be able to detect this problem + //Expected ptg stack: [NamePtg(myFunc), StringPtg(arg), (additional operands go here...), FunctionPtg(myFunc)] + Ptg[] ptg = FormulaParser.parse("myFunc(\"arg\")", book, FormulaType.CELL, -1); + assertEquals(3, ptg.length); - // the name gets encoded as the first arg - NamePtg tname = (NamePtg) ptg[0]; - assertEquals("myFunc", tname.toFormulaString(book)); + // the name gets encoded as the first operand on the stack + NamePtg tname = (NamePtg) ptg[0]; + assertEquals("myFunc", tname.toFormulaString(book)); - AbstractFunctionPtg tfunc = (AbstractFunctionPtg) ptg[1]; - assertTrue(tfunc.isExternalFunction()); + // the function's arguments are pushed onto the stack from left-to-right as OperandPtgs + StringPtg arg = (StringPtg) ptg[1]; + assertEquals("arg", arg.getValue()); + + // The external FunctionPtg is the last Ptg added to the stack + // During formula evaluation, this Ptg pops off the the appropriate number of + // arguments (getNumberOfOperands()) and pushes the result on the stack + AbstractFunctionPtg tfunc = (AbstractFunctionPtg) ptg[2]; //FuncVarPtg + assertTrue(tfunc.isExternalFunction()); + + // confirm formula parsing is case-insensitive + FormulaParser.parse("mYfUnC(\"arg\")", book, FormulaType.CELL, -1); + + // confirm formula parsing doesn't care about argument count or type + // this should only throw an error when evaluating the formula. + FormulaParser.parse("myFunc()", book, FormulaType.CELL, -1); + FormulaParser.parse("myFunc(\"arg\", 0, TRUE)", book, FormulaType.CELL, -1); + + // A completely unknown formula name (not saved in workbook) should still be parseable and renderable + // but will throw an NotImplementedFunctionException or return a #NAME? error value if evaluated. + FormulaParser.parse("yourFunc(\"arg\")", book, FormulaType.CELL, -1); + + // Verify that myFunc and yourFunc were successfully added to Workbook names + HSSFWorkbook wb2 = HSSFTestDataSamples.writeOutAndReadBack(wb); + try { + // HSSFWorkbook/EXCEL97-specific side-effects user-defined function names must be added to Workbook's defined names in order to be saved. + assertNotNull(wb2.getName("myFunc")); + assertEqualsIgnoreCase("myFunc", wb2.getName("myFunc").getNameName()); + assertNotNull(wb2.getName("yourFunc")); + assertEqualsIgnoreCase("yourFunc", wb2.getName("yourFunc").getNameName()); + + // Manually check to make sure file isn't corrupted + final File fileIn = HSSFTestDataSamples.getSampleFile(testFile); + final File reSavedFile = new File(fileIn.getParentFile(), fileIn.getName().replace(".xls", "-saved.xls")); + FileOutputStream fos = new FileOutputStream(reSavedFile); + wb2.write(fos); + fos.close(); + } finally { + wb2.close(); + } + } finally { + wb.close(); + } } + + private final static void assertEqualsIgnoreCase(String expected, String actual) { + assertEquals(expected.toLowerCase(Locale.ROOT), actual.toLowerCase(Locale.ROOT)); + } public void testEmbeddedSlash() { confirmTokenClasses("HYPERLINK(\"http://www.jakarta.org\",\"Jakarta\")", @@ -713,12 +764,19 @@ parseExpectedException("IF(TRUE)"); parseExpectedException("countif(A1:B5, C1, D1)"); + + parseExpectedException("("); + parseExpectedException(")"); + parseExpectedException("+"); + parseExpectedException("42+"); + + parseExpectedException("IF("); } private static void parseExpectedException(String formula) { try { parseFormula(formula); - throw new AssertionFailedError("expected parse exception"); + throw new AssertionFailedError("Expected FormulaParseException: " + formula); } catch (FormulaParseException e) { // expected during successful test assertNotNull(e.getMessage()); Index: src/testcases/org/apache/poi/ss/usermodel/BaseTestCell.java =================================================================== --- src/testcases/org/apache/poi/ss/usermodel/BaseTestCell.java (revision 1706948) +++ src/testcases/org/apache/poi/ss/usermodel/BaseTestCell.java (working copy) @@ -298,6 +298,45 @@ private Cell createACell() { return _testDataProvider.createWorkbook().createSheet("Sheet1").createRow(0).createCell(0); } + + /** + * bug 58452: Copy cell formulas containing unregistered function names + * Make sure that formulas with unknown/unregistered UDFs can be written to and read back from a file. + * + * @throws IOException + */ + @Test + public void testFormulaWithUnknownUDF() throws IOException { + final Workbook wb1 = _testDataProvider.createWorkbook(); + final FormulaEvaluator evaluator1 = wb1.getCreationHelper().createFormulaEvaluator(); + try { + final Cell cell1 = wb1.createSheet().createRow(0).createCell(0); + final String formula = "myFunc(\"arg\")"; + cell1.setCellFormula(formula); + confirmFormulaWithUnknownUDF(formula, cell1, evaluator1); + + final Workbook wb2 = _testDataProvider.writeOutAndReadBack(wb1); + final FormulaEvaluator evaluator2 = wb2.getCreationHelper().createFormulaEvaluator(); + try { + final Cell cell2 = wb2.getSheetAt(0).getRow(0).getCell(0); + confirmFormulaWithUnknownUDF(formula, cell2, evaluator2); + } finally { + wb2.close(); + } + } finally { + wb1.close(); + } + } + + private static void confirmFormulaWithUnknownUDF(String expectedFormula, Cell cell, FormulaEvaluator evaluator) { + assertEquals(expectedFormula, cell.getCellFormula()); + try { + evaluator.evaluate(cell); + fail("Expected NotImplementedFunctionException/NotImplementedException"); + } catch (final org.apache.poi.ss.formula.eval.NotImplementedException e) { + // expected + } + } @Test public void testChangeTypeStringToBool() { Index: src/testcases/org/apache/poi/ss/formula/BaseTestExternalFunctions.java =================================================================== --- src/testcases/org/apache/poi/ss/formula/BaseTestExternalFunctions.java (revision 1706948) +++ src/testcases/org/apache/poi/ss/formula/BaseTestExternalFunctions.java (working copy) @@ -19,6 +19,8 @@ import junit.framework.TestCase; import org.apache.poi.ss.ITestDataProvider; import org.apache.poi.ss.formula.eval.ErrorEval; +import org.apache.poi.ss.formula.eval.NotImplementedException; +import org.apache.poi.ss.formula.eval.NotImplementedFunctionException; import org.apache.poi.ss.formula.eval.StringEval; import org.apache.poi.ss.formula.eval.ValueEval; import org.apache.poi.ss.formula.functions.FreeRefFunction; @@ -84,6 +86,7 @@ public void testExternalFunctions() { Workbook wb = _testDataProvider.createWorkbook(); + FormulaEvaluator evaluator = wb.getCreationHelper().createFormulaEvaluator(); Sheet sh = wb.createSheet(); @@ -92,11 +95,16 @@ assertEquals("ISODD(1)+ISEVEN(2)", cell1.getCellFormula()); Cell cell2 = sh.createRow(1).createCell(0); + cell2.setCellFormula("MYFUNC(\"B1\")"); //unregistered functions are parseable and renderable, but may not be evaluateable try { - cell2.setCellFormula("MYFUNC(\"B1\")"); - fail("Should fail because MYFUNC is an unknown function"); - } catch (FormulaParseException e){ - ; //expected + evaluator.evaluate(cell2); + fail("Expected NotImplementedFunctionException/NotImplementedException"); + } catch (final NotImplementedException e) { + if (!(e.getCause() instanceof NotImplementedFunctionException)) + throw e; + // expected + // Alternatively, a future implementation of evaluate could return #NAME? error to align behavior with Excel + // assertEquals(ErrorEval.NAME_INVALID, ErrorEval.valueOf(evaluator.evaluate(cell2).getErrorValue())); } wb.addToolPack(customToolpack); @@ -108,7 +116,6 @@ cell3.setCellFormula("MYFUNC2(\"C1\")&\"-\"&A2"); //where A2 is defined above assertEquals("MYFUNC2(\"C1\")&\"-\"&A2", cell3.getCellFormula()); - FormulaEvaluator evaluator = wb.getCreationHelper().createFormulaEvaluator(); assertEquals(2.0, evaluator.evaluate(cell1).getNumberValue()); assertEquals("B1abc", evaluator.evaluate(cell2).getStringValue()); assertEquals("C1abc2-B1abc", evaluator.evaluate(cell3).getStringValue());