Bug 56688: Fix border cases in EDATE function: handle RefEval and BlankEval and also return #VALUE, not #REF if case of error

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

src/java/org/apache/poi/ss/formula/functions/EDate.java

@@ -36,8 +36,7 @@ public class EDate implements FreeRefFunction {
         }
         try {
             double startDateAsNumber = getValue(args[0]);
-            NumberEval offsetInYearsValue = (NumberEval) args[1];
+            int offsetInMonthAsNumber = (int) getValue(args[1]);
-            int offsetInMonthAsNumber = (int) offsetInYearsValue.getNumberValue();
 
             Date startDate = DateUtil.getJavaDate(startDateAsNumber);
             Calendar calendar = Calendar.getInstance();
@@ -53,10 +52,18 @@ public class EDate implements FreeRefFunction {
         if (arg instanceof NumberEval) {
             return ((NumberEval) arg).getNumberValue();
         }
+        if(arg instanceof BlankEval) {
+            return 0;
+        }
         if (arg instanceof RefEval) {
             ValueEval innerValueEval = ((RefEval) arg).getInnerValueEval();
-            return ((NumberEval) innerValueEval).getNumberValue();
+            if(innerValueEval instanceof NumberEval) {
+                return ((NumberEval) innerValueEval).getNumberValue();
+            }
+            if(innerValueEval instanceof BlankEval) {
+                return 0;
+            }
         }
-        throw new EvaluationException(ErrorEval.REF_INVALID);
+        throw new EvaluationException(ErrorEval.VALUE_INVALID);
     }
 }

src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java

@@ -18,13 +18,7 @@
 package org.apache.poi.xssf.usermodel;
 
 import static org.hamcrest.core.IsEqual.equalTo;
-import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.*;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
@@ -48,25 +42,7 @@ import org.apache.poi.ss.formula.WorkbookEvaluator;
 import org.apache.poi.ss.formula.eval.ErrorEval;
 import org.apache.poi.ss.formula.eval.ValueEval;
 import org.apache.poi.ss.formula.functions.Function;
-import org.apache.poi.ss.usermodel.BaseTestBugzillaIssues;
+import org.apache.poi.ss.usermodel.*;
-import org.apache.poi.ss.usermodel.Cell;
-import org.apache.poi.ss.usermodel.CellStyle;
-import org.apache.poi.ss.usermodel.CellValue;
-import org.apache.poi.ss.usermodel.ClientAnchor;
-import org.apache.poi.ss.usermodel.Comment;
-import org.apache.poi.ss.usermodel.CreationHelper;
-import org.apache.poi.ss.usermodel.DataFormatter;
-import org.apache.poi.ss.usermodel.DateUtil;
-import org.apache.poi.ss.usermodel.Drawing;
-import org.apache.poi.ss.usermodel.Font;
-import org.apache.poi.ss.usermodel.FormulaError;
-import org.apache.poi.ss.usermodel.FormulaEvaluator;
-import org.apache.poi.ss.usermodel.IndexedColors;
-import org.apache.poi.ss.usermodel.Name;
-import org.apache.poi.ss.usermodel.Row;
-import org.apache.poi.ss.usermodel.Sheet;
-import org.apache.poi.ss.usermodel.Workbook;
-import org.apache.poi.ss.usermodel.WorkbookFactory;
 import org.apache.poi.ss.util.AreaReference;
 import org.apache.poi.ss.util.CellRangeAddress;
 import org.apache.poi.ss.util.CellReference;
@@ -589,6 +565,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
        r = s.getRow(0);
        c = r.getCell(0);
        assertEquals("hello world", c.getRichStringCellValue().toString());
+       wb.close();
     }
     
     /**
@@ -639,7 +616,8 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
         assertEquals("A7", cc.getCTCalcChain().getCArray(5).getR());
         assertEquals("A8", cc.getCTCalcChain().getCArray(6).getR());
         assertEquals(40, cc.getCTCalcChain().sizeOfCArray());
-
+        wbRead.close();
+        
         wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb);
 
         // Try various ways of changing the formulas
@@ -648,18 +626,23 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
         sheet.getRow(1).getCell(0).setCellFormula("A1"); // stay
         sheet.getRow(2).getCell(0).setCellFormula(null); // go
         sheet.getRow(3).getCell(0).setCellType(Cell.CELL_TYPE_FORMULA); // stay
+        wbRead.close();
         wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb);
         sheet.getRow(4).getCell(0).setCellType(Cell.CELL_TYPE_STRING); // go
+        wbRead.close();
         wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb);
 
         validateCells(sheet);
         sheet.getRow(5).removeCell(sheet.getRow(5).getCell(0)); // go
         validateCells(sheet);
+        wbRead.close();
         wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb);
         
         sheet.getRow(6).getCell(0).setCellType(Cell.CELL_TYPE_BLANK); // go
+        wbRead.close();
         wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb);
         sheet.getRow(7).getCell(0).setCellValue((String) null); // go
+        wbRead.close();
 
         wbRead = XSSFTestDataSamples.writeOutAndReadBack(wb);
 
@@ -671,6 +654,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
         assertEquals("A2", cc.getCTCalcChain().getCArray(0).getR());
         assertEquals("A4", cc.getCTCalcChain().getCArray(1).getR());
         assertEquals("A9", cc.getCTCalcChain().getCArray(2).getR());
+        wbRead.close();
     }
 
     @Test
@@ -954,14 +938,15 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
        
        String r3 = cell.getRichStringCellValue().getCTRst().getRList().get(2).getT();
        assertEquals("line.\n", r3.substring(r3.length()-6));
-       
+
        // Save and re-check
        wb = XSSFTestDataSamples.writeOutAndReadBack(wb);
        sheet = wb.getSheetAt(0);
        row = sheet.getRow(2);
        cell = row.getCell(2);
        assertEquals(text, cell.getStringCellValue());
-       
+       wb.close();
+
 //       FileOutputStream out = new FileOutputStream("/tmp/test48877.xlsx");
 //       wb.write(out);
 //       out.close();
@@ -1125,6 +1110,9 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
        assertEquals(true, s2.getCTWorksheet().isSetPageMargins());
        assertEquals(true, ps2.getValidSettings());
        assertEquals(false, ps2.getLandscape());
+       
+       wb1.close();
+       wb2.close();
     }
 
     /**
@@ -1201,6 +1189,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
        assertEquals(pinkStyle, s.getColumnStyle(0));
        assertEquals(defaultStyle, s.getColumnStyle(2));
        assertEquals(blueStyle, s.getColumnStyle(3));
+       wb.close();
     }
     
     /**
@@ -1532,6 +1521,8 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
         byte secondSave[] = bos.toByteArray();
         
         assertThat(firstSave, equalTo(secondSave));
+        
+        wb.close();
     }
     
     /**
@@ -1720,19 +1711,63 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
 
         FileInputStream is = new FileInputStream(outFile);
         try {
-            final Workbook newWB;
+            Workbook newWB = null;
-            if(wb instanceof XSSFWorkbook) {
+            try {
-                newWB = new XSSFWorkbook(is);
+                if(wb instanceof XSSFWorkbook) {
-            } else if(wb instanceof HSSFWorkbook) {
+                    newWB = new XSSFWorkbook(is);
-                newWB = new HSSFWorkbook(is);
+                } else if(wb instanceof HSSFWorkbook) {
-            } else if(wb instanceof SXSSFWorkbook) {
+                    newWB = new HSSFWorkbook(is);
-                newWB = new SXSSFWorkbook(new XSSFWorkbook(is));
+                } else if(wb instanceof SXSSFWorkbook) {
-            } else {
+                    newWB = new SXSSFWorkbook(new XSSFWorkbook(is));
-                throw new IllegalStateException("Unknown workbook: " + wb);
+                } else {
+                    throw new IllegalStateException("Unknown workbook: " + wb);
+                }
+                assertNotNull(newWB.getSheet("test"));
+            } finally {
+                newWB.close();
             }
-            assertNotNull(newWB.getSheet("test"));
         } finally {
             is.close();
         }
     }
+    
+    @Test
+    public void testBug56688_1() {
+        XSSFWorkbook excel = XSSFTestDataSamples.openSampleWorkbook("56688_1.xlsx");
+        checkValue(excel, "-1.0");  /* Not 0.0 because POI sees date "0" minus one month as invalid date, which is -1! */
+    }
+    
+    @Test
+    public void testBug56688_2() {
+        XSSFWorkbook excel = XSSFTestDataSamples.openSampleWorkbook("56688_2.xlsx");
+        checkValue(excel, "#VALUE!");
+    }
+    
+    @Test
+    public void testBug56688_3() {
+        XSSFWorkbook excel = XSSFTestDataSamples.openSampleWorkbook("56688_3.xlsx");
+        checkValue(excel, "#VALUE!");
+    }
+    
+    @Test
+    public void testBug56688_4() {
+        XSSFWorkbook excel = XSSFTestDataSamples.openSampleWorkbook("56688_4.xlsx");
+        
+//        Calendar calendar = Calendar.getInstance();
+//        calendar.add(Calendar.MONTH, 2);
+//        double excelDate = DateUtil.getExcelDate(calendar.getTime());
+//        NumberEval eval = new NumberEval(Math.floor(excelDate));
+//        checkValue(excel, eval.getStringValue() + ".0");
+        checkValue(excel, "41904.0");
+    }
+
+    private void checkValue(XSSFWorkbook excel, String expect) {
+        XSSFFormulaEvaluator evaluator = new XSSFFormulaEvaluator(excel);
+        evaluator.evaluateAll();
+        
+        XSSFCell cell = excel.getSheetAt(0).getRow(1).getCell(1);
+        CellValue value = evaluator.evaluate(cell);
+        
+        assertEquals(expect, value.formatAsString());
+    }
 }

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

@@ -21,6 +21,9 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
+import java.io.IOException;
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
 import java.util.Calendar;
 import java.util.Date;
 import java.util.GregorianCalendar;
@@ -28,6 +31,7 @@ import java.util.TimeZone;
 
 import org.apache.poi.hssf.HSSFTestDataSamples;
 import org.apache.poi.hssf.model.InternalWorkbook;
+import org.apache.poi.ss.usermodel.DateUtil;
 import org.junit.Test;
 
 /**
@@ -337,9 +341,10 @@ public final class TestHSSFDateUtil {
     /**
      * Test that against a real, test file, we still do everything
      *  correctly
+     * @throws IOException 
      */
     @Test
-    public void onARealFile() {
+    public void onARealFile() throws IOException {
 
         HSSFWorkbook workbook = HSSFTestDataSamples.openSampleWorkbook("DateFormats.xls");
         HSSFSheet sheet       = workbook.getSheetAt(0);
@@ -394,6 +399,18 @@ public final class TestHSSFDateUtil {
         assertFalse(HSSFDateUtil.isInternalDateFormat(cell.getCellStyle().getDataFormat()));
         assertTrue(HSSFDateUtil.isADateFormat(style.getDataFormat(), style.getDataFormatString()));
         assertTrue(HSSFDateUtil.isCellDateFormatted(cell));
+        
+        workbook.close();
+    }
+
+    @Test
+    public void excelDateBorderCases() throws ParseException {
+        SimpleDateFormat df = new SimpleDateFormat("yyyy-MM-dd");
+        
+        assertEquals(1.0, DateUtil.getExcelDate(df.parse("1900-01-01")), 0.00001);
+        assertEquals(31.0, DateUtil.getExcelDate(df.parse("1900-01-31")), 0.00001);
+        assertEquals(32.0, DateUtil.getExcelDate(df.parse("1900-02-01")), 0.00001);
+        assertEquals(/* BAD_DATE! */ -1.0, DateUtil.getExcelDate(df.parse("1899-12-31")), 0.00001);
     }
 
     @Test
@@ -495,9 +512,10 @@ public final class TestHSSFDateUtil {
     /**
      * User reported a datetime issue in POI-2.5:
      *  Setting Cell's value to Jan 1, 1900 without a time doesn't return the same value set to
+     * @throws IOException 
      */
     @Test
-    public void bug19172()
+    public void bug19172() throws IOException
     {
         HSSFWorkbook workbook = new HSSFWorkbook();
         HSSFSheet sheet = workbook.createSheet();
@@ -505,7 +523,7 @@ public final class TestHSSFDateUtil {
 
         Calendar cal = Calendar.getInstance();
 
-        // A pseduo special Excel dates
+        // A pseudo special Excel dates
         cal.set(1900, 0, 1);
 
         Date valueToTest = cal.getTime();
@@ -515,6 +533,8 @@ public final class TestHSSFDateUtil {
         Date returnedValue = cell.getDateCellValue();
 
         assertEquals(valueToTest.getTime(), returnedValue.getTime());
+        
+        workbook.close();
     }
 
     /**

src/testcases/org/apache/poi/ss/formula/functions/TestEDate.java

@@ -17,24 +17,67 @@
 
 package org.apache.poi.ss.formula.functions;
 
+import java.util.Calendar;
+import java.util.Date;
+
 import junit.framework.TestCase;
+
+import org.apache.poi.ss.formula.eval.AreaEval;
+import org.apache.poi.ss.formula.eval.BlankEval;
 import org.apache.poi.ss.formula.eval.ErrorEval;
 import org.apache.poi.ss.formula.eval.NumberEval;
+import org.apache.poi.ss.formula.eval.RefEval;
 import org.apache.poi.ss.formula.eval.ValueEval;
 import org.apache.poi.ss.usermodel.DateUtil;
 import org.apache.poi.ss.usermodel.ErrorConstants;
 
-import java.util.Calendar;
-import java.util.Date;
-
 public class TestEDate extends TestCase{
 
-    public void testEDateProperValues() {
+    private final class RefEvalImplementation implements RefEval {
-        EDate eDate = new EDate();
+        private final ValueEval value;
-        NumberEval result = (NumberEval) eDate.evaluate(new ValueEval[]{new NumberEval(1000), new NumberEval(0)}, null);
+        
-        assertEquals(1000d, result.getNumberValue());
+        public RefEvalImplementation(ValueEval value) {
+            super();
+            this.value = value;
+        }
+
+        public AreaEval offset(int relFirstRowIx, int relLastRowIx,
+                int relFirstColIx, int relLastColIx) {
+            throw new UnsupportedOperationException();
+        }
+
+        public int getRow() {
+            throw new UnsupportedOperationException();
+        }
+
+        public ValueEval getInnerValueEval() {
+            return value;
+        }
+
+        public int getColumn() {
+            throw new UnsupportedOperationException();
+        }
     }
 
+    public void testEDateProperValues() {
+        // verify some border-case combinations of startDate and month-increase
+        checkValue(1000, 0, 1000d);
+        checkValue(1, 0, 1d);
+        checkValue(0, 1, 31d);
+        checkValue(1, 1, 32d);
+        checkValue(0, 0, /* BAD_DATE! */ -1.0d);
+        checkValue(0, -2, /* BAD_DATE! */ -1.0d);
+        checkValue(0, -3, /* BAD_DATE! */ -1.0d);
+        checkValue(49104, 0, 49104d);
+        checkValue(49104, 1, 49134d);
+    }
+
+    private void checkValue(int startDate, int monthInc, double expectedResult) {
+        EDate eDate = new EDate();
+        NumberEval result = (NumberEval) eDate.evaluate(new ValueEval[]{new NumberEval(startDate), new NumberEval(monthInc)}, null);
+        assertEquals(expectedResult, result.getNumberValue());
+    }
+    
     public void testEDateInvalidValues() {
         EDate eDate = new EDate();
         ErrorEval result = (ErrorEval) eDate.evaluate(new ValueEval[]{new NumberEval(1000)}, null);
@@ -65,4 +108,38 @@ public class TestEDate extends TestCase{
         instance.add(Calendar.MONTH, offset);
         assertEquals(resultDate, instance.getTime());
     }
+    
+    public void testBug56688() {        
+        EDate eDate = new EDate();
+        NumberEval result = (NumberEval) eDate.evaluate(new ValueEval[]{new NumberEval(1000), new RefEvalImplementation(new NumberEval(0))}, null);
+        assertEquals(1000d, result.getNumberValue());
+    }
+
+    public void testRefEvalStartDate() {
+        EDate eDate = new EDate();
+        NumberEval result = (NumberEval) eDate.evaluate(new ValueEval[]{new RefEvalImplementation(new NumberEval(1000)), new NumberEval(0)}, null);
+        assertEquals(1000d, result.getNumberValue());
+    }
+
+    public void testEDateInvalidValueEval() {
+        ValueEval evaluate = new EDate().evaluate(new ValueEval[]{new ValueEval() {}, new NumberEval(0)}, null);
+        assertTrue(evaluate instanceof ErrorEval);
+        assertEquals(ErrorEval.VALUE_INVALID, evaluate);
+    }
+
+    public void testEDateBlankValueEval() {
+        NumberEval evaluate = (NumberEval) new EDate().evaluate(new ValueEval[]{BlankEval.instance, new NumberEval(0)}, null);
+        assertEquals(-1.0d, evaluate.getNumberValue());
+    }
+
+    public void testEDateBlankRefValueEval() {
+        EDate eDate = new EDate();
+        NumberEval result = (NumberEval) eDate.evaluate(new ValueEval[]{new RefEvalImplementation(BlankEval.instance), new NumberEval(0)}, null);
+        assertEquals("0 startDate triggers BAD_DATE currently, thus -1.0!", 
+                -1.0d, result.getNumberValue());
+
+        result = (NumberEval) eDate.evaluate(new ValueEval[]{new NumberEval(1), new RefEvalImplementation(BlankEval.instance)}, null);
+        assertEquals("Blank is handled as 0 otherwise", 
+                1.0d, result.getNumberValue());
+    }
 }