From bff698c9384f77df539e4b64281a1b5a29aa69c9 Mon Sep 17 00:00:00 2001 From: Nick Burch Date: Fri, 14 Oct 2016 10:44:03 +0000 Subject: [PATCH] #60255 When creating a XSSF drawing, find the next available document part, even if another type has pinched the next number git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1764863 13f79535-47bb-0310-9956-ffa450edef68 --- .../org/apache/poi/POIXMLDocumentPart.java | 50 +++++++++++++++++++ .../apache/poi/xssf/usermodel/XSSFSheet.java | 3 +- .../org/apache/poi/TestPOIXMLDocument.java | 35 +++++++++++++ .../poi/xssf/usermodel/TestXSSFBugs.java | 43 ++++++++++++++++ 4 files changed, 130 insertions(+), 1 deletion(-) diff --git a/src/ooxml/java/org/apache/poi/POIXMLDocumentPart.java b/src/ooxml/java/org/apache/poi/POIXMLDocumentPart.java index e98c5b0cf..5d66b30bf 100644 --- a/src/ooxml/java/org/apache/poi/POIXMLDocumentPart.java +++ b/src/ooxml/java/org/apache/poi/POIXMLDocumentPart.java @@ -39,6 +39,7 @@ import org.apache.poi.openxml4j.opc.TargetMode; import org.apache.poi.util.Internal; import org.apache.poi.util.POILogFactory; import org.apache.poi.util.POILogger; +import org.apache.poi.xssf.usermodel.XSSFRelation; /** * Represents an entry of a OOXML package. @@ -537,6 +538,55 @@ public class POIXMLDocumentPart { public final POIXMLDocumentPart createRelationship(POIXMLRelation descriptor, POIXMLFactory factory, int idx){ return createRelationship(descriptor, factory, idx, false).getDocumentPart(); } + + /** + * Identifies the next available part number for a part of the given type, + * if possible, otherwise -1 if none are available. + * The found (valid) index can then be safely given to + * {@link #createRelationship(POIXMLRelation, POIXMLFactory, int)} or + * {@link #createRelationship(POIXMLRelation, POIXMLFactory, int, boolean)} + * without naming clashes. + * If parts with other types are already claiming a name for this relationship + * type (eg a {@link XSSFRelation#CHART} using the drawing part namespace + * normally used by {@link XSSFRelation#DRAWINGS}), those will be considered + * when finding the next spare number. + * + * @param descriptor The relationship type to find the part number for + * @param minIdx The minimum free index to assign, use -1 for any + * @return The next free part number, or -1 if none available + */ + protected final int getNextPartNumber(POIXMLRelation descriptor, int minIdx) { + OPCPackage pkg = packagePart.getPackage(); + + try { + if (descriptor.getDefaultFileName().equals(descriptor.getFileName(9999))) { + // Non-index based, check if default is free + PackagePartName ppName = PackagingURIHelper.createPartName(descriptor.getDefaultFileName()); + if (pkg.containPart(ppName)) { + // Default name already taken, not index based, nothing free + return -1; + } else { + // Default name free + return 0; + } + } + + // Default to searching from 1, unless they asked for 0+ + int idx = minIdx; + if (minIdx < 0) idx = 1; + while (idx < 1000) { + String name = descriptor.getFileName(idx); + if (!pkg.containPart(PackagingURIHelper.createPartName(name))) { + return idx; + } + idx++; + } + } catch (InvalidFormatException e) { + // Give a general wrapped exception for the problem + throw new POIXMLException(e); + } + return -1; + } /** * Create a new child POIXMLDocumentPart 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 bc2f071be..03cfad67a 100644 --- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java +++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java @@ -554,8 +554,9 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { return getDrawingPatriarch(); } - //drawingNumber = #drawings.size() + 1 + // Default drawingNumber = #drawings.size() + 1 int drawingNumber = getPackagePart().getPackage().getPartsByContentType(XSSFRelation.DRAWINGS.getContentType()).size() + 1; + drawingNumber = getNextPartNumber(XSSFRelation.DRAWINGS, drawingNumber); RelationPart rp = createRelationship(XSSFRelation.DRAWINGS, XSSFFactory.getInstance(), drawingNumber, false); XSSFDrawing drawing = rp.getDocumentPart(); String relId = rp.getRelationship().getId(); diff --git a/src/ooxml/testcases/org/apache/poi/TestPOIXMLDocument.java b/src/ooxml/testcases/org/apache/poi/TestPOIXMLDocument.java index 77f59372d..e93e820b3 100644 --- a/src/ooxml/testcases/org/apache/poi/TestPOIXMLDocument.java +++ b/src/ooxml/testcases/org/apache/poi/TestPOIXMLDocument.java @@ -44,6 +44,8 @@ import org.apache.poi.util.PackageHelper; import org.apache.poi.util.TempFile; import org.apache.poi.xslf.usermodel.XMLSlideShow; import org.apache.poi.xslf.usermodel.XSLFShape; +import org.apache.poi.xssf.usermodel.XSSFRelation; +import org.apache.poi.xwpf.usermodel.XWPFRelation; import org.junit.Test; /** @@ -227,6 +229,39 @@ public final class TestPOIXMLDocument { doc.close(); } } + + @Test + public void testGetNextPartNumber() throws Exception { + POIDataSamples pds = POIDataSamples.getDocumentInstance(); + @SuppressWarnings("resource") + OPCPackage pkg = PackageHelper.open(pds.openResourceAsStream("WordWithAttachments.docx")); + OPCParser doc = new OPCParser(pkg); + try { + doc.parse(new TestFactory()); + + // Non-indexed parts: Word is taken, Excel is not + assertEquals(-1, doc.getNextPartNumber(XWPFRelation.DOCUMENT, 0)); + assertEquals(-1, doc.getNextPartNumber(XWPFRelation.DOCUMENT, -1)); + assertEquals(-1, doc.getNextPartNumber(XWPFRelation.DOCUMENT, 99)); + assertEquals(0, doc.getNextPartNumber(XSSFRelation.WORKBOOK, 0)); + assertEquals(0, doc.getNextPartNumber(XSSFRelation.WORKBOOK, -1)); + assertEquals(0, doc.getNextPartNumber(XSSFRelation.WORKBOOK, 99)); + + // Indexed parts: + // Has 2 headers + assertEquals(0, doc.getNextPartNumber(XWPFRelation.HEADER, 0)); + assertEquals(3, doc.getNextPartNumber(XWPFRelation.HEADER, -1)); + assertEquals(3, doc.getNextPartNumber(XWPFRelation.HEADER, 1)); + assertEquals(8, doc.getNextPartNumber(XWPFRelation.HEADER, 8)); + + // Has no Excel Sheets + assertEquals(0, doc.getNextPartNumber(XSSFRelation.WORKSHEET, 0)); + assertEquals(1, doc.getNextPartNumber(XSSFRelation.WORKSHEET, -1)); + assertEquals(1, doc.getNextPartNumber(XSSFRelation.WORKSHEET, 1)); + } finally { + doc.close(); + } + } @Test public void testCommitNullPart() throws IOException, InvalidFormatException { 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 7fb1b507d..66b26d377 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java @@ -3128,4 +3128,47 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { wb.close(); } + + /** + * Other things, including charts, may end up taking drawing part + * numbers. (Uses a test file hand-crafted with an extra non-drawing + * part with a part number) + */ + @Test + public void drawingNumbersAlreadyTaken_60255() throws Exception { + Workbook wb = XSSFTestDataSamples.openSampleWorkbook("60255_extra_drawingparts.xlsx"); + assertEquals(4, wb.getNumberOfSheets()); + + // Sheet 3 starts with a drawing + Sheet sheet = wb.getSheetAt(0); + assertNull(sheet.getDrawingPatriarch()); + sheet = wb.getSheetAt(1); + assertNull(sheet.getDrawingPatriarch()); + sheet = wb.getSheetAt(2); + assertNotNull(sheet.getDrawingPatriarch()); + sheet = wb.getSheetAt(3); + assertNull(sheet.getDrawingPatriarch()); + + // Add another sheet, and give it a drawing + sheet = wb.createSheet(); + assertNull(sheet.getDrawingPatriarch()); + sheet.createDrawingPatriarch(); + assertNotNull(sheet.getDrawingPatriarch()); + + // Save and check + wb = XSSFTestDataSamples.writeOutAndReadBack(wb); + assertEquals(5, wb.getNumberOfSheets()); + + // Sheets 3 and 5 now + sheet = wb.getSheetAt(0); + assertNull(sheet.getDrawingPatriarch()); + sheet = wb.getSheetAt(1); + assertNull(sheet.getDrawingPatriarch()); + sheet = wb.getSheetAt(2); + assertNotNull(sheet.getDrawingPatriarch()); + sheet = wb.getSheetAt(3); + assertNull(sheet.getDrawingPatriarch()); + sheet = wb.getSheetAt(4); + assertNotNull(sheet.getDrawingPatriarch()); + } }