From 6456105e3745acc226b87564406497d1e6965599 Mon Sep 17 00:00:00 2001 From: Javen O'Neal Date: Sat, 31 Oct 2015 10:22:19 +0000 Subject: [PATCH] bug 58443: prevent corrupted workbooks by checking for overlapping merged regions before adding a merged region to a sheet; fix unit tests that produced corrupt workbooks; remove deprecated HSSFSheet#addMergedRegion(Region) git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1711586 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/hssf/usermodel/HSSFSheet.java | 26 ++++++----- .../poi/ss/util/CellRangeAddressBase.java | 40 +++++++++++++++++ .../apache/poi/xssf/usermodel/XSSFSheet.java | 26 +++++++++++ .../ss/usermodel/BaseTestBugzillaIssues.java | 2 +- .../poi/ss/usermodel/BaseTestSheet.java | 44 ++++++++++++++++++- .../poi/ss/usermodel/BaseTestWorkbook.java | 12 ++--- .../poi/ss/util/TestCellRangeAddress.java | 30 +++++++++++++ 7 files changed, 160 insertions(+), 20 deletions(-) diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java b/src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java index f1412af42b..a97c8782f5 100644 --- a/src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java +++ b/src/java/org/apache/poi/hssf/usermodel/HSSFSheet.java @@ -673,22 +673,13 @@ public final class HSSFSheet implements org.apache.poi.ss.usermodel.Sheet { _sheet.setGridsPrinted(value); } - /** - * @deprecated (Aug-2008) use CellRangeAddress instead of Region - */ - public int addMergedRegion(org.apache.poi.ss.util.Region region) { - return _sheet.addMergedRegion(region.getRowFrom(), - region.getColumnFrom(), - //(short) region.getRowTo(), - region.getRowTo(), - region.getColumnTo()); - } - /** * adds a merged region of cells (hence those cells form one) * * @param region (rowfrom/colfrom-rowto/colto) to merge * @return index of this region + * @throws IllegalStateException if region intersects with an existing merged region + * or multi-cell array formula on this sheet */ public int addMergedRegion(CellRangeAddress region) { region.validate(SpreadsheetVersion.EXCEL97); @@ -696,6 +687,10 @@ public final class HSSFSheet implements org.apache.poi.ss.usermodel.Sheet { // throw IllegalStateException if the argument CellRangeAddress intersects with // a multi-cell array formula defined in this sheet validateArrayFormulas(region); + + // Throw IllegalStateException if the argument CellRangeAddress intersects with + // a merged region already in this sheet + validateMergedRegions(region); return _sheet.addMergedRegion(region.getFirstRow(), region.getFirstColumn(), @@ -730,6 +725,15 @@ public final class HSSFSheet implements org.apache.poi.ss.usermodel.Sheet { } } + + private void validateMergedRegions(CellRangeAddress candidateRegion) { + for (final CellRangeAddress existingRegion : getMergedRegions()) { + if (existingRegion.intersects(candidateRegion)) { + throw new IllegalStateException("Cannot add merged region " + candidateRegion.formatAsString() + + " to sheet because it overlaps with an existing merged region (" + existingRegion.formatAsString() + ")."); + } + } + } /** * Control if Excel should be asked to recalculate all formulas on this sheet diff --git a/src/java/org/apache/poi/ss/util/CellRangeAddressBase.java b/src/java/org/apache/poi/ss/util/CellRangeAddressBase.java index 9561cb9f5b..92267fce72 100644 --- a/src/java/org/apache/poi/ss/util/CellRangeAddressBase.java +++ b/src/java/org/apache/poi/ss/util/CellRangeAddressBase.java @@ -17,6 +17,8 @@ package org.apache.poi.ss.util; +import java.awt.Rectangle; + import org.apache.poi.ss.SpreadsheetVersion; @@ -123,6 +125,44 @@ public abstract class CellRangeAddressBase { return _firstRow <= rowInd && rowInd <= _lastRow && _firstCol <= colInd && colInd <= _lastCol; } + + /** + * Determines whether or not this CellRangeAddress and the specified CellRangeAddress intersect. + * + * @param other + * @return + */ + public boolean intersects(CellRangeAddressBase other) { + // see java.awt.Rectangle.intersects + // http://stackoverflow.com/questions/13390333/two-rectangles-intersection + + // TODO: Replace with an intersection code that doesn't rely on java.awt + return getRectangle().intersects(other.getRectangle()); + } + + // TODO: Replace with an intersection code that doesn't rely on java.awt + // Don't let this temporary implementation detail leak outside of this class + private final Rectangle getRectangle() { + int firstRow, firstCol, lastRow, lastCol; + + if (!isFullColumnRange()) { + firstRow = Math.min(_firstRow, _lastRow); + lastRow = Math.max(_firstRow, _lastRow); + } + else { + firstRow = 0; + lastRow = Integer.MAX_VALUE; + } + if (!isFullRowRange()) { + firstCol = Math.min(_firstCol, _lastCol); + lastCol = Math.max(_firstCol, _lastCol); + } + else { + firstCol = 0; + lastCol = Integer.MAX_VALUE; + } + return new Rectangle(firstRow, firstCol, lastRow-firstRow+1, lastCol-firstCol+1); + } /** * @param firstCol column number for the upper left hand corner diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java index 1406ee2bf1..4651a204ea 100644 --- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java +++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java @@ -327,6 +327,8 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { * * @param region (rowfrom/colfrom-rowto/colto) to merge * @return index of this region + * @throws IllegalStateException if region intersects with a multi-cell array formula + * @throws IllegalStateException if region intersects with an existing region on this sheet */ @Override public int addMergedRegion(CellRangeAddress region) { @@ -335,6 +337,10 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { // throw IllegalStateException if the argument CellRangeAddress intersects with // a multi-cell array formula defined in this sheet validateArrayFormulas(region); + + // Throw IllegalStateException if the argument CellRangeAddress intersects with + // a merged region already in this sheet + validateMergedRegions(region); CTMergeCells ctMergeCells = worksheet.isSetMergeCells() ? worksheet.getMergeCells() : worksheet.addNewMergeCells(); CTMergeCell ctMergeCell = ctMergeCells.addNewMergeCell(); @@ -342,6 +348,12 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { return ctMergeCells.sizeOfMergeCellArray(); } + /** + * Verify that the candidate region does not intersect with an existing multi-cell array formula in this sheet + * + * @param region + * @throws IllegalStateException if candidate region intersects an existing array formula in this sheet + */ private void validateArrayFormulas(CellRangeAddress region){ int firstRow = region.getFirstRow(); int firstColumn = region.getFirstColumn(); @@ -369,6 +381,20 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { } } + /** + * Verify that candidate region does not intersect with an existing merged region in this sheet + * + * @param candidateRegion + * @throws IllegalStateException if candidate region intersects an existing merged region in this sheet + */ + private void validateMergedRegions(CellRangeAddress candidateRegion) { + for (final CellRangeAddress existingRegion : getMergedRegions()) { + if (existingRegion.intersects(candidateRegion)) { + throw new IllegalStateException("Cannot add merged region " + candidateRegion.formatAsString() + + " to sheet because it overlaps with an existing merged region (" + existingRegion.formatAsString() + ")."); + } + } + } /** * Adjusts the column width to fit the contents. diff --git a/src/testcases/org/apache/poi/ss/usermodel/BaseTestBugzillaIssues.java b/src/testcases/org/apache/poi/ss/usermodel/BaseTestBugzillaIssues.java index 945bdf86a8..875a780d67 100644 --- a/src/testcases/org/apache/poi/ss/usermodel/BaseTestBugzillaIssues.java +++ b/src/testcases/org/apache/poi/ss/usermodel/BaseTestBugzillaIssues.java @@ -143,7 +143,7 @@ public abstract class BaseTestBugzillaIssues { Sheet template = wb.getSheetAt(0); template.addMergedRegion(new CellRangeAddress(0, 1, 0, 2)); - template.addMergedRegion(new CellRangeAddress(1, 2, 0, 2)); + template.addMergedRegion(new CellRangeAddress(2, 3, 0, 2)); Sheet clone = wb.cloneSheet(0); int originalMerged = template.getNumMergedRegions(); diff --git a/src/testcases/org/apache/poi/ss/usermodel/BaseTestSheet.java b/src/testcases/org/apache/poi/ss/usermodel/BaseTestSheet.java index 24c596b4ff..e4151360fb 100644 --- a/src/testcases/org/apache/poi/ss/usermodel/BaseTestSheet.java +++ b/src/testcases/org/apache/poi/ss/usermodel/BaseTestSheet.java @@ -257,6 +257,46 @@ public abstract class BaseTestSheet { assertEquals(3, sheetP.getPrintSetup().getCopies()); wb2.close(); } + + /** + * Dissallow creating wholly or partially overlapping merged regions + * as this results in a corrupted workbook + */ + @Test + public void addOverlappingMergedRegions() { + final Workbook wb = _testDataProvider.createWorkbook(); + final Sheet sheet = wb.createSheet(); + + final CellRangeAddress baseRegion = new CellRangeAddress(0, 1, 0, 1); + sheet.addMergedRegion(baseRegion); + + try { + final CellRangeAddress duplicateRegion = new CellRangeAddress(0, 1, 0, 1); + sheet.addMergedRegion(duplicateRegion); + fail("Should not be able to add a merged region if sheet already contains the same merged region"); + } catch (final IllegalStateException e) { } //expected + + try { + final CellRangeAddress partiallyOverlappingRegion = new CellRangeAddress(1, 2, 1, 2); + sheet.addMergedRegion(partiallyOverlappingRegion); + fail("Should not be able to add a merged region if it partially overlaps with an existing merged region"); + } catch (final IllegalStateException e) { } //expected + + try { + final CellRangeAddress subsetRegion = new CellRangeAddress(0, 1, 0, 0); + sheet.addMergedRegion(subsetRegion); + fail("Should not be able to add a merged region if it is a formal subset of an existing merged region"); + } catch (final IllegalStateException e) { } //expected + + try { + final CellRangeAddress supersetRegion = new CellRangeAddress(0, 2, 0, 2); + sheet.addMergedRegion(supersetRegion); + fail("Should not be able to add a merged region if it is a formal superset of an existing merged region"); + } catch (final IllegalStateException e) { } //expected + + final CellRangeAddress disjointRegion = new CellRangeAddress(10, 11, 10, 11); + sheet.addMergedRegion(disjointRegion); //allowed + } /** * Test adding merged regions. If the region's bounds are outside of the allowed range @@ -310,13 +350,13 @@ public abstract class BaseTestSheet { Sheet sheet = wb.createSheet(); CellRangeAddress region = new CellRangeAddress(0, 1, 0, 1); sheet.addMergedRegion(region); - region = new CellRangeAddress(1, 2, 0, 1); + region = new CellRangeAddress(2, 3, 0, 1); sheet.addMergedRegion(region); sheet.removeMergedRegion(0); region = sheet.getMergedRegion(0); - assertEquals("Left over region should be starting at row 1", 1, region.getFirstRow()); + assertEquals("Left over region should be starting at row 2", 2, region.getFirstRow()); sheet.removeMergedRegion(0); diff --git a/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java b/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java index ca69fa6266..9c3ca693ad 100644 --- a/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java +++ b/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java @@ -448,7 +448,7 @@ public abstract class BaseTestWorkbook { sheet.createRow(0).createCell(0).setCellValue("Test"); sheet.createRow(1).createCell(0).setCellValue(36.6); sheet.addMergedRegion(new CellRangeAddress(0, 1, 0, 2)); - sheet.addMergedRegion(new CellRangeAddress(1, 2, 0, 2)); + sheet.addMergedRegion(new CellRangeAddress(2, 3, 0, 2)); assertTrue(sheet.isSelected()); Sheet clonedSheet = book.cloneSheet(0); @@ -457,16 +457,16 @@ public abstract class BaseTestWorkbook { assertEquals(2, clonedSheet.getNumMergedRegions()); assertFalse(clonedSheet.isSelected()); - //cloned sheet is a deep copy, adding rows in the original does not affect the clone + //cloned sheet is a deep copy, adding rows or merged regions in the original does not affect the clone sheet.createRow(2).createCell(0).setCellValue(1); - sheet.addMergedRegion(new CellRangeAddress(0, 2, 0, 2)); - assertEquals(2, clonedSheet.getPhysicalNumberOfRows()); + sheet.addMergedRegion(new CellRangeAddress(4, 5, 0, 2)); assertEquals(2, clonedSheet.getPhysicalNumberOfRows()); + assertEquals(2, clonedSheet.getNumMergedRegions()); clonedSheet.createRow(2).createCell(0).setCellValue(1); - clonedSheet.addMergedRegion(new CellRangeAddress(0, 2, 0, 2)); - assertEquals(3, clonedSheet.getPhysicalNumberOfRows()); + clonedSheet.addMergedRegion(new CellRangeAddress(6, 7, 0, 2)); assertEquals(3, clonedSheet.getPhysicalNumberOfRows()); + assertEquals(3, clonedSheet.getNumMergedRegions()); } diff --git a/src/testcases/org/apache/poi/ss/util/TestCellRangeAddress.java b/src/testcases/org/apache/poi/ss/util/TestCellRangeAddress.java index 5616f79f38..5c35a2dff1 100644 --- a/src/testcases/org/apache/poi/ss/util/TestCellRangeAddress.java +++ b/src/testcases/org/apache/poi/ss/util/TestCellRangeAddress.java @@ -17,6 +17,8 @@ limitations under the License. package org.apache.poi.ss.util; +import static org.junit.Assert.fail; + import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -233,4 +235,32 @@ public final class TestCellRangeAddress extends TestCase { assertEquals(4, ref.getMinColumn()); assertEquals(10, ref.getMaxColumn()); } + + public void testIntersects() { + final CellRangeAddress baseRegion = new CellRangeAddress(0, 1, 0, 1); + + final CellRangeAddress duplicateRegion = new CellRangeAddress(0, 1, 0, 1); + assertIntersects(baseRegion, duplicateRegion); + + final CellRangeAddress partiallyOverlappingRegion = new CellRangeAddress(1, 2, 1, 2); + assertIntersects(baseRegion, partiallyOverlappingRegion); + + final CellRangeAddress subsetRegion = new CellRangeAddress(0, 1, 0, 0); + assertIntersects(baseRegion, subsetRegion); + + final CellRangeAddress supersetRegion = new CellRangeAddress(0, 2, 0, 2); + assertIntersects(baseRegion, supersetRegion); + + final CellRangeAddress disjointRegion = new CellRangeAddress(10, 11, 10, 11); + assertNotIntersects(baseRegion, disjointRegion); + } + + private static void assertIntersects(CellRangeAddress regionA, CellRangeAddress regionB) { + assertTrue(regionA.intersects(regionB)); + assertTrue(regionB.intersects(regionA)); + } + private static void assertNotIntersects(CellRangeAddress regionA, CellRangeAddress regionB) { + assertFalse(regionA.intersects(regionB)); + assertFalse(regionB.intersects(regionA)); + } } -- 2.39.5