From 688d5072d13fac55ae523eabb6a2b6fca22448c2 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Thu, 28 Sep 2017 09:56:45 +0000 Subject: [PATCH] Fix bug 61516: when copying cells with formulas we should properly check for references that are invalid afterwards. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1809967 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/ss/formula/FormulaShifter.java | 35 +++-- .../poi/xssf/usermodel/TestXSSFBugs.java | 125 +++++++++++------- 2 files changed, 95 insertions(+), 65 deletions(-) diff --git a/src/java/org/apache/poi/ss/formula/FormulaShifter.java b/src/java/org/apache/poi/ss/formula/FormulaShifter.java index 44e477f23..b2edea349 100644 --- a/src/java/org/apache/poi/ss/formula/FormulaShifter.java +++ b/src/java/org/apache/poi/ss/formula/FormulaShifter.java @@ -122,14 +122,12 @@ public final class FormulaShifter { @Override public String toString() { - StringBuffer sb = new StringBuffer(); - - sb.append(getClass().getName()); - sb.append(" ["); - sb.append(_firstMovedIndex); - sb.append(_lastMovedIndex); - sb.append(_amountToMove); - return sb.toString(); + return getClass().getName() + + " [" + + _firstMovedIndex + + _lastMovedIndex + + _amountToMove + + "]"; } /** @@ -463,18 +461,27 @@ public final class FormulaShifter { /** * Modifies rptg in-place and return a reference to rptg if the cell reference * would move due to a row copy operation - * Returns null or {@link #RefErrorPtg} if no change was made + * Returns null or {@link RefErrorPtg} if no change was made * - * @param aptg + * @param rptg The REF that is copied * @return The Ptg reference if the cell would move due to copy, otherwise null */ private Ptg rowCopyRefPtg(RefPtgBase rptg) { final int refRow = rptg.getRow(); if (rptg.isRowRelative()) { + // check new location where the ref is located final int destRowIndex = _firstMovedIndex + _amountToMove; - if (destRowIndex < 0 || _version.getLastRowIndex() < destRowIndex) + if (destRowIndex < 0 || _version.getLastRowIndex() < destRowIndex) { return createDeletedRef(rptg); - rptg.setRow(refRow + _amountToMove); + } + + // check new location where the ref points to + final int newRowIndex = refRow + _amountToMove; + if(newRowIndex < 0 || _version.getLastRowIndex() < newRowIndex) { + return createDeletedRef(rptg); + } + + rptg.setRow(newRowIndex); return rptg; } return null; @@ -483,9 +490,9 @@ public final class FormulaShifter { /** * Modifies aptg in-place and return a reference to aptg if the first or last row of * of the Area reference would move due to a row copy operation - * Returns null or {@link #AreaErrPtg} if no change was made + * Returns null or {@link AreaErrPtg} if no change was made * - * @param aptg + * @param aptg The Area that is copied * @return null, AreaErrPtg, or modified aptg */ private Ptg rowCopyAreaPtg(AreaPtgBase aptg) { 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 f7798270a..c8f76873f 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java @@ -64,17 +64,21 @@ import org.apache.poi.openxml4j.opc.PackagingURIHelper; import org.apache.poi.openxml4j.util.ZipSecureFile; import org.apache.poi.poifs.filesystem.NPOIFSFileSystem; import org.apache.poi.poifs.filesystem.POIFSFileSystem; +import org.apache.poi.ss.SpreadsheetVersion; +import org.apache.poi.ss.formula.FormulaParser; +import org.apache.poi.ss.formula.FormulaRenderer; +import org.apache.poi.ss.formula.FormulaShifter; +import org.apache.poi.ss.formula.FormulaType; import org.apache.poi.ss.formula.WorkbookEvaluator; import org.apache.poi.ss.formula.eval.ErrorEval; import org.apache.poi.ss.formula.eval.NumberEval; -import org.apache.poi.ss.formula.eval.ValueEval; import org.apache.poi.ss.formula.functions.Function; +import org.apache.poi.ss.formula.ptg.Ptg; 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; import org.apache.poi.ss.util.CellUtil; -import org.apache.poi.util.IOUtils; import org.apache.poi.util.LocaleUtil; import org.apache.poi.util.NullOutputStream; import org.apache.poi.util.TempFile; @@ -292,8 +296,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { */ @Test public void bug48539() throws IOException { - XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("48539.xlsx"); - try { + try (XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("48539.xlsx")) { assertEquals(3, wb.getNumberOfSheets()); assertEquals(0, wb.getNumberOfNames()); @@ -321,8 +324,6 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { // Now all of them XSSFFormulaEvaluator.evaluateAllFormulaCells(wb); - } finally { - wb.close(); } } @@ -358,7 +359,6 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { assertEquals("FFFF0000", cs.getFillForegroundXSSFColor().getARGBHex()); assertEquals("FFFF0000", cs.getFillForegroundColorColor().getARGBHex()); - assertNotNull(cs.getFillBackgroundColor()); assertEquals(64, cs.getFillBackgroundColor()); assertEquals(null, cs.getFillBackgroundXSSFColor().getARGBHex()); assertEquals(null, cs.getFillBackgroundColorColor().getARGBHex()); @@ -1432,7 +1432,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { // KY: SUM(B1: IZ1) /*double ky1Value =*/ - evaluator.evaluate(wb.getSheetAt(0).getRow(0).getCell(310)).getNumberValue(); + assertEquals(259.0, evaluator.evaluate(wb.getSheetAt(0).getRow(0).getCell(310)).getNumberValue(), 0.0001); // Assert assertEquals(259.0, a1Value, 0.0); @@ -1443,12 +1443,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { public void bug54436() throws IOException { Workbook wb = XSSFTestDataSamples.openSampleWorkbook("54436.xlsx"); if (!WorkbookEvaluator.getSupportedFunctionNames().contains("GETPIVOTDATA")) { - Function func = new Function() { - @Override - public ValueEval evaluate(ValueEval[] args, int srcRowIndex, int srcColumnIndex) { - return ErrorEval.NA; - } - }; + Function func = (args, srcRowIndex, srcColumnIndex) -> ErrorEval.NA; WorkbookEvaluator.registerFunction("GETPIVOTDATA", func); } @@ -1463,12 +1458,9 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { @Test(expected = EncryptedDocumentException.class) public void bug55692_poifs() throws IOException { // Via a POIFSFileSystem - POIFSFileSystem fsP = new POIFSFileSystem( - POIDataSamples.getPOIFSInstance().openResourceAsStream("protect.xlsx")); - try { + try (POIFSFileSystem fsP = new POIFSFileSystem( + POIDataSamples.getPOIFSInstance().openResourceAsStream("protect.xlsx"))) { WorkbookFactory.create(fsP); - } finally { - fsP.close(); } } @@ -1763,15 +1755,11 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { } } - FileOutputStream fileOutStream = new FileOutputStream(outFile); - try { + try (FileOutputStream fileOutStream = new FileOutputStream(outFile)) { wb.write(fileOutStream); - } finally { - fileOutStream.close(); } - FileInputStream is = new FileInputStream(outFile); - try { + try (FileInputStream is = new FileInputStream(outFile)) { Workbook newWB = null; try { if (wb instanceof XSSFWorkbook) { @@ -1789,8 +1777,6 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { newWB.close(); } } - } finally { - is.close(); } } @@ -2196,8 +2182,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { @Test public void test57165() throws IOException { - XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx"); - try { + try (XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx")) { removeAllSheetsBut(3, wb); wb.cloneSheet(0); // Throws exception here wb.setSheetName(1, "New Sheet"); @@ -2205,15 +2190,12 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { XSSFWorkbook wbBack = XSSFTestDataSamples.writeOutAndReadBack(wb); wbBack.close(); - } finally { - wb.close(); } } @Test public void test57165_create() throws IOException { - XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx"); - try { + try (XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx")) { removeAllSheetsBut(3, wb); wb.createSheet("newsheet"); // Throws exception here wb.setSheetName(1, "New Sheet"); @@ -2221,12 +2203,10 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { XSSFWorkbook wbBack = XSSFTestDataSamples.writeOutAndReadBack(wb); wbBack.close(); - } finally { - wb.close(); } } - private static void removeAllSheetsBut(int sheetIndex, Workbook wb) { + private static void removeAllSheetsBut(@SuppressWarnings("SameParameterValue") int sheetIndex, Workbook wb) { int sheetNb = wb.getNumberOfSheets(); // Move this sheet at the first position wb.setSheetOrder(wb.getSheetName(sheetIndex), 0); @@ -2258,8 +2238,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { */ @Test public void testBug56820_Formula1() throws IOException { - Workbook wb = new XSSFWorkbook(); - try { + try (Workbook wb = new XSSFWorkbook()) { FormulaEvaluator evaluator = wb.getCreationHelper().createFormulaEvaluator(); Sheet sh = wb.createSheet(); @@ -2274,8 +2253,6 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { assertEquals(2, A1, 0); assertEquals(4, A2, 0); //<-- FAILS EXPECTATIONS - } finally { - wb.close(); } } @@ -2288,8 +2265,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { */ @Test public void testBug56820_Formula2() throws IOException { - Workbook wb = new XSSFWorkbook(); - try { + try (Workbook wb = new XSSFWorkbook()) { FormulaEvaluator evaluator = wb.getCreationHelper().createFormulaEvaluator(); Sheet sh = wb.createSheet(); @@ -2304,15 +2280,12 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { assertEquals(2, A1, 0); assertEquals(4, A2, 0); - } finally { - wb.close(); } } @Test public void test56467() throws IOException { - Workbook wb = XSSFTestDataSamples.openSampleWorkbook("picture.xlsx"); - try { + try (Workbook wb = XSSFTestDataSamples.openSampleWorkbook("picture.xlsx")) { Sheet orig = wb.getSheetAt(0); assertNotNull(orig); @@ -2325,8 +2298,6 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { } } - } finally { - wb.close(); } } @@ -2966,7 +2937,6 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { @Ignore("bug 59442") @Test public void testSetRGBBackgroundColor() throws IOException { - XSSFWorkbook workbook = new XSSFWorkbook(); XSSFCell cell = workbook.createSheet().createRow(0).createCell(0); @@ -3157,7 +3127,7 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { // we currently only populate the dimension during writing out // to avoid having to iterate all rows/cells in each add/remove of a row or cell - IOUtils.write(wb, new NullOutputStream()); + wb.write(new NullOutputStream()); assertEquals("B2:I5", ((XSSFSheet) sheet).getCTWorksheet().getDimension().getRef()); @@ -3181,4 +3151,57 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { wb.close(); } -} \ No newline at end of file + + @Test + public void bug61516(){ + final String initialFormula = "A1"; + final String expectedFormula = "#REF!"; // from ms excel + + Workbook wb = new XSSFWorkbook(); + Sheet sheet = wb.createSheet("sheet1"); + sheet.createRow(0).createCell(0).setCellValue(1); // A1 = 1 + + { + Cell c3 = sheet.createRow(2).createCell(2); + c3.setCellFormula(initialFormula); // C3 = =A1 + FormulaEvaluator evaluator = wb.getCreationHelper().createFormulaEvaluator(); + CellValue cellValue = evaluator.evaluate(c3); + assertEquals(1, cellValue.getNumberValue(), 0.0001); + } + + { + FormulaShifter formulaShifter = FormulaShifter.createForRowCopy(0, "sheet1", 2/*firstRowToShift*/, 2/*lastRowToShift*/ + , -1/*step*/, SpreadsheetVersion.EXCEL2007); // parameters 2, 2, -1 should mean : move row range [2-2] one level up + XSSFEvaluationWorkbook fpb = XSSFEvaluationWorkbook.create((XSSFWorkbook) wb); + Ptg[] ptgs = FormulaParser.parse(initialFormula, fpb, FormulaType.CELL, 0); // [A1] + formulaShifter.adjustFormula(ptgs, 0); // adjusted to [A] + String shiftedFmla = FormulaRenderer.toFormulaString(fpb, ptgs); //A + //System.out.println(String.format("initial formula : A1; expected formula value after shifting up : #REF!; actual formula value : %s", shiftedFmla)); + assertEquals("On copy we expect the formula to be adjusted, in this case it would point to row -1, which is an invalid REF", + expectedFormula, shiftedFmla); + } + + { + FormulaShifter formulaShifter = FormulaShifter.createForRowShift(0, "sheet1", 2/*firstRowToShift*/, 2/*lastRowToShift*/ + , -1/*step*/, SpreadsheetVersion.EXCEL2007); // parameters 2, 2, -1 should mean : move row range [2-2] one level up + XSSFEvaluationWorkbook fpb = XSSFEvaluationWorkbook.create((XSSFWorkbook) wb); + Ptg[] ptgs = FormulaParser.parse(initialFormula, fpb, FormulaType.CELL, 0); // [A1] + formulaShifter.adjustFormula(ptgs, 0); // adjusted to [A] + String shiftedFmla = FormulaRenderer.toFormulaString(fpb, ptgs); //A + //System.out.println(String.format("initial formula : A1; expected formula value after shifting up : #REF!; actual formula value : %s", shiftedFmla)); + assertEquals("On move we expect the formula to stay the same, thus expecting the initial formula A1 here", + initialFormula, shiftedFmla); + } + + sheet.shiftRows(2, 2, -1); + { + Cell c2 = sheet.getRow(1).getCell(2); + assertNotNull("cell C2 needs to exist now", c2); + assertEquals(CellType.FORMULA, c2.getCellType()); + assertEquals(initialFormula, c2.getCellFormula()); + FormulaEvaluator evaluator = wb.getCreationHelper().createFormulaEvaluator(); + CellValue cellValue = evaluator.evaluate(c2); + assertEquals(1, cellValue.getNumberValue(), 0.0001); + } + } +}