bug58452: set cell formulas containing unregistered function names

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1711605 13f79535-47bb-0310-9956-ffa450edef68

src/java/org/apache/poi/ss/formula/FormulaParser.java

@@ -50,6 +50,7 @@ import org.apache.poi.ss.formula.ptg.MissingArgPtg;
 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.UnaryPlusPtg;
 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 @@ import org.apache.poi.ss.util.CellReference.NameType;
  * <p/>
  */
 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 @@ public final class FormulaParser {
      */
 	private boolean _inIntersection = false;
 
-	private FormulaParsingWorkbook _book;
+	private final FormulaParsingWorkbook _book;
-	private SpreadsheetVersion _ssVersion;
+	private final SpreadsheetVersion _ssVersion;
 
-	private int _sheetIndex;
+	private final int _sheetIndex;
 
 
 	/**
@@ -137,6 +142,7 @@ public final class FormulaParser {
 
 	/**
 	 * 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 @@ public final class FormulaParser {
 	 * 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 @@ public final class FormulaParser {
 				// 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) {
+			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.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 @@ public final class FormulaParser {
 
 		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.

src/ooxml/testcases/org/apache/poi/ss/formula/TestFormulaParser.java

@@ -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 @@ public class TestFormulaParser extends TestCase {
         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());
+        }
+    }
 
 }

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

@@ -19,7 +19,10 @@ package org.apache.poi.hssf.model;
 
 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,20 +114,68 @@ public final class TestFormulaParser extends TestCase {
 		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");
+		final String testFile = "testNames.xls";
-		HSSFEvaluationWorkbook book = HSSFEvaluationWorkbook.create(w);
+		HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook(testFile);
+		try {
+			HSSFEvaluationWorkbook book = HSSFEvaluationWorkbook.create(wb);
 
-		Ptg[] ptg = HSSFFormulaParser.parse("myFunc()", w);
+			//Expected ptg stack: [NamePtg(myFunc), StringPtg(arg), (additional operands go here...), FunctionPtg(myFunc)]
-		// myFunc() actually takes 1 parameter.  Don't know if POI will ever be able to detect this problem
+			Ptg[] ptg = FormulaParser.parse("myFunc(\"arg\")", book, FormulaType.CELL, -1);
+			assertEquals(3, ptg.length); 
 
-		// the name gets encoded as the first arg
+			// the name gets encoded as the first operand on the stack
-		NamePtg tname = (NamePtg) ptg[0];
+			NamePtg tname = (NamePtg) ptg[0];
-		assertEquals("myFunc", tname.toFormulaString(book));
+			assertEquals("myFunc", tname.toFormulaString(book));
 
-		AbstractFunctionPtg tfunc = (AbstractFunctionPtg) ptg[1];
+			// the function's arguments are pushed onto the stack from left-to-right as OperandPtgs
-		assertTrue(tfunc.isExternalFunction());
+			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() {
@@ -713,12 +764,19 @@ public final class TestFormulaParser extends TestCase {
 
 		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());

src/testcases/org/apache/poi/ss/formula/BaseTestExternalFunctions.java

@@ -19,6 +19,8 @@ package org.apache.poi.ss.formula;
 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 class BaseTestExternalFunctions extends TestCase {
 
     public void testExternalFunctions() {
         Workbook wb = _testDataProvider.createWorkbook();
+        FormulaEvaluator evaluator = wb.getCreationHelper().createFormulaEvaluator();
 
         Sheet sh = wb.createSheet();
 
@@ -92,11 +95,16 @@ public class BaseTestExternalFunctions extends TestCase {
         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\")");
+            evaluator.evaluate(cell2);
-            fail("Should fail because MYFUNC is an unknown function");
+            fail("Expected NotImplementedFunctionException/NotImplementedException");
-        } catch (FormulaParseException e){
+        } catch (final NotImplementedException e) {
-            ; //expected
+            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 @@ public class BaseTestExternalFunctions extends TestCase {
         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());

src/testcases/org/apache/poi/ss/usermodel/BaseTestCell.java

@@ -298,6 +298,45 @@ public abstract class BaseTestCell {
 	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() {