Bug 57165: Avoid PartAlreadyExistsException when removing/cloning sheets

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1663153 13f79535-47bb-0310-9956-ffa450edef68

src/ooxml/java/org/apache/poi/POIXMLDocumentPart.java

@@ -349,11 +349,27 @@ public class POIXMLDocumentPart {
      * @param descriptor the part descriptor
      * @param factory the factory that will create an instance of the requested relation
      * @return the created child POIXMLDocumentPart
+     * @throws PartAlreadyExistsException
+     *             If rule M1.12 is not verified : Packages shall not contain
+     *             equivalent part names and package implementers shall neither
+     *             create nor recognize packages with equivalent part names.
      */
     public final POIXMLDocumentPart createRelationship(POIXMLRelation descriptor, POIXMLFactory factory){
         return createRelationship(descriptor, factory, -1, false);
     }
 
+    /**
+     * Create a new child POIXMLDocumentPart
+     *
+     * @param descriptor the part descriptor
+     * @param factory the factory that will create an instance of the requested relation
+     * @param idx part number
+     * @return the created child POIXMLDocumentPart
+     * @throws PartAlreadyExistsException
+     *             If rule M1.12 is not verified : Packages shall not contain
+     *             equivalent part names and package implementers shall neither
+     *             create nor recognize packages with equivalent part names.
+     */
     public final POIXMLDocumentPart createRelationship(POIXMLRelation descriptor, POIXMLFactory factory, int idx){
         return createRelationship(descriptor, factory, idx, false);
     }
@@ -366,6 +382,10 @@ public class POIXMLDocumentPart {
      * @param idx part number
      * @param noRelation if true, then no relationship is added.
      * @return the created child POIXMLDocumentPart
+     * @throws PartAlreadyExistsException
+     *             If rule M1.12 is not verified : Packages shall not contain
+     *             equivalent part names and package implementers shall neither
+     *             create nor recognize packages with equivalent part names.
      */
     protected final POIXMLDocumentPart createRelationship(POIXMLRelation descriptor, POIXMLFactory factory, int idx, boolean noRelation){
         try {

src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java

@@ -739,7 +739,7 @@ public abstract class OPCPackage implements RelationshipSource, Closeable {
 	 * @param contentType
 	 *            Part content type.
 	 * @return The newly created part.
-	 * @throws InvalidFormatException
+	 * @throws PartAlreadyExistsException
 	 *             If rule M1.12 is not verified : Packages shall not contain
 	 *             equivalent part names and package implementers shall neither
 	 *             create nor recognize packages with equivalent part names.
@@ -762,7 +762,7 @@ public abstract class OPCPackage implements RelationshipSource, Closeable {
 	 *            Specify if the existing relationship part, if any, logically
 	 *            associated to the newly created part will be loaded.
 	 * @return The newly created part.
-	 * @throws InvalidFormatException
+	 * @throws PartAlreadyExistsException
 	 *             If rule M1.12 is not verified : Packages shall not contain
 	 *             equivalent part names and package implementers shall neither
 	 *             create nor recognize packages with equivalent part names.

src/ooxml/java/org/apache/poi/openxml4j/opc/PackagePart.java

@@ -228,8 +228,13 @@ public abstract class PackagePart implements RelationshipSource {
 	 *            Relationship unique id.
 	 * @return The newly created and added relationship
 	 *
+	 * @throws InvalidOperationException
+	 *             If a writing operation is done on a read only package or 
+	 *             invalid nested relations are created.
 	 * @throws InvalidFormatException
 	 *             If the URI point to a relationship part URI.
+	 * @throws IllegalArgumentException if targetPartName, targetMode 
+	 *             or relationshipType are passed as null
 	 * @see org.apache.poi.openxml4j.opc.RelationshipSource#addRelationship(org.apache.poi.openxml4j.opc.PackagePartName,
 	 *      org.apache.poi.openxml4j.opc.TargetMode, java.lang.String, java.lang.String)
 	 */

src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java

@@ -751,8 +751,26 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Iterable<X
         CTSheet sheet = addSheet(sheetname);
 
         int sheetNumber = 1;
-        for(XSSFSheet sh : sheets) {
+        outerloop:
-            sheetNumber = (int)Math.max(sh.sheet.getSheetId() + 1, sheetNumber);
+        while(true) {
+            for(XSSFSheet sh : sheets) {
+                sheetNumber = (int)Math.max(sh.sheet.getSheetId() + 1, sheetNumber);
+            }
+            
+            // Bug 57165: We also need to check that the resulting file name is not already taken
+            // this can happen when moving/cloning sheets
+            String sheetName = XSSFRelation.WORKSHEET.getFileName(sheetNumber);
+            for(POIXMLDocumentPart relation : getRelations()) {
+                if(relation.getPackagePart() != null && 
+                        sheetName.equals(relation.getPackagePart().getPartName().getName())) {
+                    // name is taken => try next one
+                    sheetNumber++;
+                    continue outerloop;
+                }
+            }
+
+            // no duplicate found => use this one
+            break;
         }
 
         XSSFSheet wrapper = (XSSFSheet)createRelationship(XSSFRelation.WORKSHEET, XSSFFactory.getInstance(), sheetNumber);

src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestUnfixedBugs.java

@@ -17,8 +17,6 @@
 
 package org.apache.poi.xssf.usermodel;
 
-import static org.junit.Assert.assertEquals;
-
 import java.io.IOException;
 import java.nio.charset.Charset;
 import java.util.Calendar;
@@ -184,38 +182,6 @@ public final class TestUnfixedBugs extends TestCase {
         }
     }
 
-    
-    @Test
-    public void test57165() throws IOException {
-        XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx");
-        try {
-            removeAllSheetsBut(3, wb);
-            wb.cloneSheet(0); // Throws exception here
-            wb.setSheetName(1, "New Sheet");
-            //saveWorkbook(wb, fileName);
-            
-            XSSFWorkbook wbBack = XSSFTestDataSamples.writeOutAndReadBack(wb);
-            try {
-                
-            } finally {
-                wbBack.close();
-            }
-        } finally {
-            wb.close();
-        }
-    }
-
-    private static void removeAllSheetsBut(int sheetIndex, Workbook wb)
-    {
-        int sheetNb = wb.getNumberOfSheets();
-        // Move this sheet at the first position
-        wb.setSheetOrder(wb.getSheetName(sheetIndex), 0);
-        for (int sn = sheetNb - 1; sn > 0; sn--)
-        {
-            wb.removeSheetAt(sn);
-        }
-    }
-
     // When this is fixed, the test case should go to BaseTestXCell with 
     // adjustments to use _testDataProvider to also verify this for XSSF
     public void testBug57294() throws IOException {

src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java

@@ -2134,4 +2134,36 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
         assertEquals("~CIRCULAR~REF~", FormulaError.forInt(value.getErrorValue()).getString());
         assertEquals("CIRCULAR_REF", FormulaError.forInt(value.getErrorValue()).toString());
     }
+
+    
+    @Test
+    public void test57165() throws IOException {
+        XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("57171_57163_57165.xlsx");
+        try {
+            removeAllSheetsBut(3, wb);
+            wb.cloneSheet(0); // Throws exception here
+            wb.setSheetName(1, "New Sheet");
+            //saveWorkbook(wb, fileName);
+            
+            XSSFWorkbook wbBack = XSSFTestDataSamples.writeOutAndReadBack(wb);
+            try {
+                
+            } finally {
+                wbBack.close();
+            }
+        } finally {
+            wb.close();
+        }
+    }
+
+    private static void removeAllSheetsBut(int sheetIndex, Workbook wb)
+    {
+        int sheetNb = wb.getNumberOfSheets();
+        // Move this sheet at the first position
+        wb.setSheetOrder(wb.getSheetName(sheetIndex), 0);
+        for (int sn = sheetNb - 1; sn > 0; sn--)
+        {
+            wb.removeSheetAt(sn);
+        }
+    }
 }