From 979d1b48e7a057dbfd29af6bbe0dabad0adec65a Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Sun, 26 Apr 2020 08:23:04 +0000 Subject: [PATCH] Github-177: Avoid NullPointerException if RangeCopier encounters empty/missing rows Also expose one-parameter constructor and verify it in tests. Closes #177 git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1877010 13f79535-47bb-0310-9956-ffa450edef68 --- .../poi/hssf/usermodel/HSSFRangeCopier.java | 4 + .../apache/poi/ss/usermodel/RangeCopier.java | 117 ++++++++++-------- .../poi/xssf/usermodel/XSSFRangeCopier.java | 8 +- .../poi/ss/usermodel/TestXSSFRangeCopier.java | 6 +- .../hssf/usermodel/TestHSSFRangeCopier.java | 4 +- .../poi/ss/usermodel/TestRangeCopier.java | 55 ++++++-- 6 files changed, 119 insertions(+), 75 deletions(-) diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFRangeCopier.java b/src/java/org/apache/poi/hssf/usermodel/HSSFRangeCopier.java index f6d3fb3819..60b1a23417 100644 --- a/src/java/org/apache/poi/hssf/usermodel/HSSFRangeCopier.java +++ b/src/java/org/apache/poi/hssf/usermodel/HSSFRangeCopier.java @@ -32,6 +32,10 @@ public class HSSFRangeCopier extends RangeCopier { super(sourceSheet, destSheet); } + public HSSFRangeCopier(Sheet sheet) { + super(sheet); + } + protected void adjustCellReferencesInsideFormula(Cell cell, Sheet destSheet, int deltaX, int deltaY) { FormulaRecordAggregate fra = (FormulaRecordAggregate)((HSSFCell)cell).getCellValueRecord(); int destSheetIndex = destSheet.getWorkbook().getSheetIndex(destSheet); diff --git a/src/java/org/apache/poi/ss/usermodel/RangeCopier.java b/src/java/org/apache/poi/ss/usermodel/RangeCopier.java index 6c73530b5c..8409accc72 100644 --- a/src/java/org/apache/poi/ss/usermodel/RangeCopier.java +++ b/src/java/org/apache/poi/ss/usermodel/RangeCopier.java @@ -36,13 +36,15 @@ public abstract class RangeCopier { this.sourceSheet = sourceSheet; this.destSheet = destSheet; } + public RangeCopier(Sheet sheet) { this(sheet, sheet); } - /** Uses input pattern to tile destination region, overwriting existing content. Works in following manner : + + /** Uses input pattern to tile destination region, overwriting existing content. Works in following manner : * 1.Start from top-left of destination. * 2.Paste source but only inside of destination borders. - * 3.If there is space left on right or bottom side of copy, process it as in step 2. + * 3.If there is space left on right or bottom side of copy, process it as in step 2. * @param tilePatternRange source range which should be copied in tiled manner * @param tileDestRange destination range, which should be overridden */ @@ -50,49 +52,54 @@ public abstract class RangeCopier { Sheet sourceCopy = sourceSheet.getWorkbook().cloneSheet(sourceSheet.getWorkbook().getSheetIndex(sourceSheet)); int sourceWidthMinus1 = tilePatternRange.getLastColumn() - tilePatternRange.getFirstColumn(); int sourceHeightMinus1 = tilePatternRange.getLastRow() - tilePatternRange.getFirstRow(); - int rightLimitToCopy; + int rightLimitToCopy; int bottomLimitToCopy; int nextRowIndexToCopy = tileDestRange.getFirstRow(); - do { + do { int nextCellIndexInRowToCopy = tileDestRange.getFirstColumn(); int heightToCopyMinus1 = Math.min(sourceHeightMinus1, tileDestRange.getLastRow() - nextRowIndexToCopy); bottomLimitToCopy = tilePatternRange.getFirstRow() + heightToCopyMinus1; - do { + do { int widthToCopyMinus1 = Math.min(sourceWidthMinus1, tileDestRange.getLastColumn() - nextCellIndexInRowToCopy); rightLimitToCopy = tilePatternRange.getFirstColumn() + widthToCopyMinus1; CellRangeAddress rangeToCopy = new CellRangeAddress( tilePatternRange.getFirstRow(), bottomLimitToCopy, - tilePatternRange.getFirstColumn(), rightLimitToCopy + tilePatternRange.getFirstColumn(), rightLimitToCopy ); copyRange(rangeToCopy, nextCellIndexInRowToCopy - rangeToCopy.getFirstColumn(), nextRowIndexToCopy - rangeToCopy.getFirstRow(), sourceCopy); - nextCellIndexInRowToCopy += widthToCopyMinus1 + 1; + nextCellIndexInRowToCopy += widthToCopyMinus1 + 1; } while (nextCellIndexInRowToCopy <= tileDestRange.getLastColumn()); nextRowIndexToCopy += heightToCopyMinus1 + 1; } while (nextRowIndexToCopy <= tileDestRange.getLastRow()); - + int tempCopyIndex = sourceSheet.getWorkbook().getSheetIndex(sourceCopy); - sourceSheet.getWorkbook().removeSheetAt(tempCopyIndex); + sourceSheet.getWorkbook().removeSheetAt(tempCopyIndex); } private void copyRange(CellRangeAddress sourceRange, int deltaX, int deltaY, Sheet sourceClone) { //NOSONAR, it's a bit complex but monolith method, does not make much sense to divide it if(deltaX != 0) - horizontalFormulaShifter = FormulaShifter.createForColumnCopy(sourceSheet.getWorkbook().getSheetIndex(sourceSheet), + horizontalFormulaShifter = FormulaShifter.createForColumnCopy(sourceSheet.getWorkbook().getSheetIndex(sourceSheet), sourceSheet.getSheetName(), sourceRange.getFirstColumn(), sourceRange.getLastColumn(), deltaX, sourceSheet.getWorkbook().getSpreadsheetVersion()); if(deltaY != 0) - verticalFormulaShifter = FormulaShifter.createForRowCopy(sourceSheet.getWorkbook().getSheetIndex(sourceSheet), + verticalFormulaShifter = FormulaShifter.createForRowCopy(sourceSheet.getWorkbook().getSheetIndex(sourceSheet), sourceSheet.getSheetName(), sourceRange.getFirstRow(), sourceRange.getLastRow(), deltaY, sourceSheet.getWorkbook().getSpreadsheetVersion()); - - for(int rowNo = sourceRange.getFirstRow(); rowNo <= sourceRange.getLastRow(); rowNo++) { - Row sourceRow = sourceClone.getRow(rowNo); // copy from source copy, original source might be overridden in process! - for (int columnIndex = sourceRange.getFirstColumn(); columnIndex <= sourceRange.getLastColumn(); columnIndex++) { + + for(int rowNo = sourceRange.getFirstRow(); rowNo <= sourceRange.getLastRow(); rowNo++) { + // copy from source copy, original source might be overridden in process! + Row sourceRow = sourceClone.getRow(rowNo); + if(sourceRow == null) { + continue; + } + + for (int columnIndex = sourceRange.getFirstColumn(); columnIndex <= sourceRange.getLastColumn(); columnIndex++) { Cell sourceCell = sourceRow.getCell(columnIndex); if(sourceCell == null) continue; Row destRow = destSheet.getRow(rowNo + deltaY); if(destRow == null) destRow = destSheet.createRow(rowNo + deltaY); - + Cell newCell = destRow.getCell(columnIndex + deltaX); if(newCell == null) { newCell = destRow.createCell(columnIndex + deltaX); @@ -104,56 +111,56 @@ public abstract class RangeCopier { } } } - + protected abstract void adjustCellReferencesInsideFormula(Cell cell, Sheet destSheet, int deltaX, int deltaY); // this part is different for HSSF and XSSF - + protected boolean adjustInBothDirections(Ptg[] ptgs, int sheetIndex, int deltaX, int deltaY) { boolean adjustSucceeded = true; if(deltaY != 0) - adjustSucceeded = verticalFormulaShifter.adjustFormula(ptgs, sheetIndex); + adjustSucceeded = verticalFormulaShifter.adjustFormula(ptgs, sheetIndex); if(deltaX != 0) adjustSucceeded = adjustSucceeded && horizontalFormulaShifter.adjustFormula(ptgs, sheetIndex); return adjustSucceeded; } - - // TODO clone some more properties ? - public static void cloneCellContent(Cell srcCell, Cell destCell, Map styleMap) { - if(styleMap != null) { - if(srcCell.getSheet().getWorkbook() == destCell.getSheet().getWorkbook()){ - destCell.setCellStyle(srcCell.getCellStyle()); + + // TODO clone some more properties ? + public static void cloneCellContent(Cell srcCell, Cell destCell, Map styleMap) { + if(styleMap != null) { + if(srcCell.getSheet().getWorkbook() == destCell.getSheet().getWorkbook()){ + destCell.setCellStyle(srcCell.getCellStyle()); } else { - int stHashCode = srcCell.getCellStyle().hashCode(); - CellStyle newCellStyle = styleMap.get(stHashCode); - if(newCellStyle == null){ - newCellStyle = destCell.getSheet().getWorkbook().createCellStyle(); - newCellStyle.cloneStyleFrom(srcCell.getCellStyle()); - styleMap.put(stHashCode, newCellStyle); - } - destCell.setCellStyle(newCellStyle); - } - } - switch(srcCell.getCellType()) { - case STRING: - destCell.setCellValue(srcCell.getStringCellValue()); - break; + int stHashCode = srcCell.getCellStyle().hashCode(); + CellStyle newCellStyle = styleMap.get(stHashCode); + if(newCellStyle == null){ + newCellStyle = destCell.getSheet().getWorkbook().createCellStyle(); + newCellStyle.cloneStyleFrom(srcCell.getCellStyle()); + styleMap.put(stHashCode, newCellStyle); + } + destCell.setCellStyle(newCellStyle); + } + } + switch(srcCell.getCellType()) { + case STRING: + destCell.setCellValue(srcCell.getStringCellValue()); + break; case NUMERIC: - destCell.setCellValue(srcCell.getNumericCellValue()); - break; - case BLANK: + destCell.setCellValue(srcCell.getNumericCellValue()); + break; + case BLANK: destCell.setBlank(); - break; - case BOOLEAN: - destCell.setCellValue(srcCell.getBooleanCellValue()); - break; - case ERROR: - destCell.setCellErrorValue(srcCell.getErrorCellValue()); - break; - case FORMULA: + break; + case BOOLEAN: + destCell.setCellValue(srcCell.getBooleanCellValue()); + break; + case ERROR: + destCell.setCellErrorValue(srcCell.getErrorCellValue()); + break; + case FORMULA: String oldFormula = srcCell.getCellFormula(); - destCell.setCellFormula(oldFormula); - break; - default: - break; - } + destCell.setCellFormula(oldFormula); + break; + default: + break; + } } } diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRangeCopier.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRangeCopier.java index 9269843828..50248fae81 100644 --- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRangeCopier.java +++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRangeCopier.java @@ -31,13 +31,17 @@ import org.apache.poi.util.Beta; @Beta public class XSSFRangeCopier extends RangeCopier { - public XSSFRangeCopier(Sheet sourceSheet, Sheet destSheet){ + public XSSFRangeCopier(Sheet sourceSheet, Sheet destSheet) { super(sourceSheet, destSheet); } + public XSSFRangeCopier(Sheet sheet) { + super(sheet); + } + protected void adjustCellReferencesInsideFormula(Cell cell, Sheet destSheet, int deltaX, int deltaY){ XSSFWorkbook hostWorkbook = (XSSFWorkbook) destSheet.getWorkbook(); - XSSFEvaluationWorkbook fpb = XSSFEvaluationWorkbook.create(hostWorkbook); + XSSFEvaluationWorkbook fpb = XSSFEvaluationWorkbook.create(hostWorkbook); Ptg[] ptgs = FormulaParser.parse(cell.getCellFormula(), fpb, FormulaType.CELL, 0); int destSheetIndex = hostWorkbook.getSheetIndex(destSheet); if(adjustInBothDirections(ptgs, destSheetIndex, deltaX, deltaY)) diff --git a/src/ooxml/testcases/org/apache/poi/ss/usermodel/TestXSSFRangeCopier.java b/src/ooxml/testcases/org/apache/poi/ss/usermodel/TestXSSFRangeCopier.java index 25b0d94022..35f8911293 100644 --- a/src/ooxml/testcases/org/apache/poi/ss/usermodel/TestXSSFRangeCopier.java +++ b/src/ooxml/testcases/org/apache/poi/ss/usermodel/TestXSSFRangeCopier.java @@ -34,9 +34,9 @@ import java.io.IOException; public class TestXSSFRangeCopier extends TestRangeCopier { public TestXSSFRangeCopier() { - super(); + super(); workbook = new XSSFWorkbook(); - testDataProvider = XSSFITestDataProvider.instance; + testDataProvider = XSSFITestDataProvider.instance; } @Before @@ -60,5 +60,5 @@ public class TestXSSFRangeCopier extends TestRangeCopier { newRow.copyRowFrom(existingRow, policy); assertEquals("$C2+B$2", newRow.getCell(1).getCellFormula()); } - + } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRangeCopier.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRangeCopier.java index 8f6b678513..8d685fc68d 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRangeCopier.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFRangeCopier.java @@ -27,9 +27,9 @@ import org.junit.Before; public class TestHSSFRangeCopier extends TestRangeCopier { public TestHSSFRangeCopier() { - super(); + super(); workbook = new HSSFWorkbook(); - testDataProvider = HSSFITestDataProvider.instance; + testDataProvider = HSSFITestDataProvider.instance; } @Before diff --git a/src/testcases/org/apache/poi/ss/usermodel/TestRangeCopier.java b/src/testcases/org/apache/poi/ss/usermodel/TestRangeCopier.java index def64a32c9..d00a820e3c 100644 --- a/src/testcases/org/apache/poi/ss/usermodel/TestRangeCopier.java +++ b/src/testcases/org/apache/poi/ss/usermodel/TestRangeCopier.java @@ -32,15 +32,15 @@ public abstract class TestRangeCopier { protected Sheet sheet1; protected Sheet sheet2; protected Workbook workbook; - protected RangeCopier rangeCopier; - protected RangeCopier transSheetRangeCopier; + protected RangeCopier rangeCopier; + protected RangeCopier transSheetRangeCopier; protected ITestDataProvider testDataProvider; protected void initSheets() { sheet1 = workbook.getSheet("sheet1"); sheet2 = workbook.getSheet("sheet2"); } - + @Test public void copySheetRangeWithoutFormulas() { CellRangeAddress rangeToCopy = CellRangeAddress.valueOf("B1:C2"); //2x2 @@ -53,21 +53,21 @@ public abstract class TestRangeCopier { @Test public void tileTheRangeAway() { CellRangeAddress tileRange = CellRangeAddress.valueOf("C4:D5"); - CellRangeAddress destRange = CellRangeAddress.valueOf("F4:K5"); + CellRangeAddress destRange = CellRangeAddress.valueOf("F4:K5"); rangeCopier.copyRange(tileRange, destRange); - assertEquals("1.3", getCellContent(sheet1, "H4")); - assertEquals("1.3", getCellContent(sheet1, "J4")); - assertEquals("$C1+G$2", getCellContent(sheet1, "G5")); - assertEquals("SUM(G3:I3)", getCellContent(sheet1, "H5")); - assertEquals("$C1+I$2", getCellContent(sheet1, "I5")); + assertEquals("1.3", getCellContent(sheet1, "H4")); + assertEquals("1.3", getCellContent(sheet1, "J4")); + assertEquals("$C1+G$2", getCellContent(sheet1, "G5")); + assertEquals("SUM(G3:I3)", getCellContent(sheet1, "H5")); + assertEquals("$C1+I$2", getCellContent(sheet1, "I5")); assertEquals("", getCellContent(sheet1, "L5")); //out of borders assertEquals("", getCellContent(sheet1, "G7")); //out of borders } - + @Test public void tileTheRangeOver() { CellRangeAddress tileRange = CellRangeAddress.valueOf("C4:D5"); - CellRangeAddress destRange = CellRangeAddress.valueOf("A4:C5"); + CellRangeAddress destRange = CellRangeAddress.valueOf("A4:C5"); rangeCopier.copyRange(tileRange, destRange); assertEquals("1.3", getCellContent(sheet1, "A4")); assertEquals("$C1+B$2", getCellContent(sheet1, "B5")); @@ -78,7 +78,7 @@ public abstract class TestRangeCopier { public void copyRangeToOtherSheet() { Sheet destSheet = sheet2; CellRangeAddress tileRange = CellRangeAddress.valueOf("C4:D5"); // on sheet1 - CellRangeAddress destRange = CellRangeAddress.valueOf("F4:J6"); // on sheet2 + CellRangeAddress destRange = CellRangeAddress.valueOf("F4:J6"); // on sheet2 transSheetRangeCopier.copyRange(tileRange, destRange); assertEquals("1.3", getCellContent(destSheet, "H4")); assertEquals("1.3", getCellContent(destSheet, "J4")); @@ -86,7 +86,36 @@ public abstract class TestRangeCopier { assertEquals("SUM(G3:I3)", getCellContent(destSheet, "H5")); assertEquals("$C1+I$2", getCellContent(destSheet, "I5")); } - + + @Test + public void testEmptyRow() { + // leave some rows empty in-between + Row row = sheet1.createRow(23); + row.createCell(0).setCellValue(1.2); + + Sheet destSheet = sheet2; + CellRangeAddress tileRange = CellRangeAddress.valueOf("A1:A100"); // on sheet1 + CellRangeAddress destRange = CellRangeAddress.valueOf("G1:G100"); // on sheet2 + transSheetRangeCopier.copyRange(tileRange, destRange); + + assertEquals("1.2", getCellContent(destSheet, "G24")); + } + + @Test + public void testSameSheet() { + // leave some rows empty in-between + Row row = sheet1.createRow(23); + row.createCell(0).setCellValue(1.2); + + CellRangeAddress tileRange = CellRangeAddress.valueOf("A1:A100"); // on sheet1 + CellRangeAddress destRange = CellRangeAddress.valueOf("G1:G100"); // on sheet2 + + // use the a RangeCopier with the same Sheet for source and dest + rangeCopier.copyRange(tileRange, destRange); + + assertEquals("1.2", getCellContent(sheet1, "G24")); + } + protected static String getCellContent(Sheet sheet, String coordinates) { try { CellReference p = new CellReference(coordinates); -- 2.39.5