Improve handling and warnings when closing OPCPackage objects

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

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 
     <changes>
         <release version="3.7-beta2" date="2010-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">Improve handling and warnings when closing OPCPackage objects</action>
            <action dev="POI-DEVELOPERS" type="fix">49702 - Correct XSSFWorkbook.getNumCellStyles to check the right styles list</action>
            <action dev="POI-DEVELOPERS" type="add">49690 - Add WorkbookUtil, which provies a way of generating valid sheet names</action>
            <action dev="POI-DEVELOPERS" type="fix">49694 - Use DataFormatter when autosizing columns, to better match the real display width of formatted cells</action>

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

@@ -338,15 +338,19 @@ public abstract class OPCPackage implements RelationshipSource {
 	}
 
 	/**
-	 * Close the package and save its content.
+	 * Close the open, writable package and save its content.
+	 * 
+	 * If your package is open read only, then you should call {@link #revert()}
+	 *  when finished with the package.
 	 *
 	 * @throws IOException
 	 *             If an IO exception occur during the saving process.
 	 */
 	public void close() throws IOException {
 		if (this.packageAccess == PackageAccess.READ) {
-			logger
-					.log(POILogger.WARN, "The close() method is intended to SAVE a package. This package is open in READ ONLY mode, use the revert() method instead !");
+			logger.log(POILogger.WARN, 
+			        "The close() method is intended to SAVE a package. This package is open in READ ONLY mode, use the revert() method instead !");
+			revert();
 			return;
 		}
 
@@ -1298,6 +1302,17 @@ public abstract class OPCPackage implements RelationshipSource {
 			throw new IllegalArgumentException("targetFile");
 
 		this.throwExceptionIfReadOnly();
+		
+		// You shouldn't save the the same file, do a close instead
+		if(targetFile.exists() && 
+		        targetFile.getAbsolutePath().equals(this.originalPackagePath)) {
+		    throw new InvalidOperationException(
+		            "You can't call save(File) to save to the currently open " +
+		            "file. To save to the current file, please just call close()"
+		    );
+		}
+		
+		// Do the save
 		FileOutputStream fos = null;
 		try {
 			fos = new FileOutputStream(targetFile);

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

@@ -295,8 +295,11 @@ public final class ZipPackage extends Package {
 				// Save the final package to a temporary file
 				try {
 					save(tempFile);
-					this.zipArchive.close(); // Close the zip archive to be
-					// able to delete it
+					
+					// Close the current zip file, so we can
+					//  overwrite it on all platforms
+					this.zipArchive.close();
+					// Copy the new file over the old one
 					FileHelper.copyFile(tempFile, targetFile);
 				} finally {
 					// Either the save operation succeed or not, we delete the

src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackage.java

@@ -32,6 +32,7 @@ import junit.framework.TestCase;
 
 import org.apache.poi.openxml4j.OpenXML4JTestDataSamples;
 import org.apache.poi.openxml4j.exceptions.InvalidFormatException;
+import org.apache.poi.openxml4j.exceptions.InvalidOperationException;
 import org.apache.poi.openxml4j.opc.internal.ContentTypeManager;
 import org.apache.poi.openxml4j.opc.internal.FileHelper;
 import org.apache.poi.util.TempFile;
@@ -444,6 +445,64 @@ public final class TestPackage extends TestCase {
 		p.revert();
 	}
 	
+	/**
+	 * Test that we can open a file by path, and then
+	 *  write changes to it.
+	 */
+	public void testOpenFileThenOverwrite() throws Exception {
+        File tempFile = File.createTempFile("poiTesting","tmp");
+        File origFile = OpenXML4JTestDataSamples.getSampleFile("TestPackageCommon.docx");
+        FileHelper.copyFile(origFile, tempFile);
+        
+        // Open the temp file
+        OPCPackage p = OPCPackage.open(tempFile.toString(), PackageAccess.READ_WRITE);
+        // Close it
+        p.close();
+        // Delete it
+        assertTrue(tempFile.delete());
+        
+        // Reset
+        FileHelper.copyFile(origFile, tempFile);
+        p = OPCPackage.open(tempFile.toString(), PackageAccess.READ_WRITE);
+        
+        // Save it to the same file - not allowed
+        try {
+            p.save(tempFile);
+            fail("You shouldn't be able to call save(File) to overwrite the current file");
+        } catch(InvalidOperationException e) {}
+        
+        // Delete it
+        assertTrue(tempFile.delete());
+        
+        
+        // Open it read only, then close and delete - allowed
+        FileHelper.copyFile(origFile, tempFile);
+        p = OPCPackage.open(tempFile.toString(), PackageAccess.READ);
+        p.close();
+        assertTrue(tempFile.delete());
+	}
+    /**
+     * Test that we can open a file by path, save it
+     *  to another file, then delete both
+     */
+    public void testOpenFileThenSaveDelete() throws Exception {
+        File tempFile = File.createTempFile("poiTesting","tmp");
+        File tempFile2 = File.createTempFile("poiTesting","tmp");
+        File origFile = OpenXML4JTestDataSamples.getSampleFile("TestPackageCommon.docx");
+        FileHelper.copyFile(origFile, tempFile);
+        
+        // Open the temp file
+        OPCPackage p = OPCPackage.open(tempFile.toString(), PackageAccess.READ_WRITE);
+
+        // Save it to a different file
+        p.save(tempFile2);
+        p.close();
+        
+        // Delete both the files
+        assertTrue(tempFile.delete());
+        assertTrue(tempFile2.delete());
+    }
+
 	private static ContentTypeManager getContentTypeManager(OPCPackage pkg) throws Exception {
 		Field f = OPCPackage.class.getDeclaredField("contentTypeManager");
 		f.setAccessible(true);