From: Dominik Stadler Date: Tue, 22 Jul 2014 12:31:56 +0000 (+0000) Subject: Bug 56688: Fix border cases in EDATE function: handle RefEval and BlankEval and also... X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=b6dd3bca347a264d07b321743f795477aaeae5e2;p=poi.git 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 --- diff --git a/src/java/org/apache/poi/ss/formula/functions/EDate.java b/src/java/org/apache/poi/ss/formula/functions/EDate.java index 22832690e3..75e5509940 100644 --- a/src/java/org/apache/poi/ss/formula/functions/EDate.java +++ b/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) offsetInYearsValue.getNumberValue(); + int offsetInMonthAsNumber = (int) getValue(args[1]); 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); } } diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java index 696e32c53c..fcf2b51629 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java +++ b/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.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 static org.junit.Assert.*; 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.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.usermodel.*; 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; - if(wb instanceof XSSFWorkbook) { - newWB = new XSSFWorkbook(is); - } else if(wb instanceof HSSFWorkbook) { - newWB = new HSSFWorkbook(is); - } else if(wb instanceof SXSSFWorkbook) { - newWB = new SXSSFWorkbook(new XSSFWorkbook(is)); - } else { - throw new IllegalStateException("Unknown workbook: " + wb); + Workbook newWB = null; + try { + if(wb instanceof XSSFWorkbook) { + newWB = new XSSFWorkbook(is); + } else if(wb instanceof HSSFWorkbook) { + newWB = new HSSFWorkbook(is); + } else if(wb instanceof SXSSFWorkbook) { + newWB = new SXSSFWorkbook(new XSSFWorkbook(is)); + } 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()); + } } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFDateUtil.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFDateUtil.java index 022975d9ae..f5ea7a1ccc 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFDateUtil.java +++ b/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(); } /** diff --git a/src/testcases/org/apache/poi/ss/formula/functions/TestEDate.java b/src/testcases/org/apache/poi/ss/formula/functions/TestEDate.java index 96cc31c1be..6e337003b3 100644 --- a/src/testcases/org/apache/poi/ss/formula/functions/TestEDate.java +++ b/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{ + private final class RefEvalImplementation implements RefEval { + private final ValueEval value; + + 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() { - EDate eDate = new EDate(); - NumberEval result = (NumberEval) eDate.evaluate(new ValueEval[]{new NumberEval(1000), new NumberEval(0)}, null); - assertEquals(1000d, result.getNumberValue()); + // 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()); + } } diff --git a/test-data/spreadsheet/56688_1.xlsx b/test-data/spreadsheet/56688_1.xlsx new file mode 100644 index 0000000000..4246e96791 Binary files /dev/null and b/test-data/spreadsheet/56688_1.xlsx differ diff --git a/test-data/spreadsheet/56688_2.xlsx b/test-data/spreadsheet/56688_2.xlsx new file mode 100644 index 0000000000..24098bd510 Binary files /dev/null and b/test-data/spreadsheet/56688_2.xlsx differ diff --git a/test-data/spreadsheet/56688_3.xlsx b/test-data/spreadsheet/56688_3.xlsx new file mode 100644 index 0000000000..9a707618ae Binary files /dev/null and b/test-data/spreadsheet/56688_3.xlsx differ diff --git a/test-data/spreadsheet/56688_4.xlsx b/test-data/spreadsheet/56688_4.xlsx new file mode 100644 index 0000000000..5aeb4ee4b4 Binary files /dev/null and b/test-data/spreadsheet/56688_4.xlsx differ