Bug 59252 - Close workbook does not save file
Summary: Close workbook does not save file
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.14-FINAL
Hardware: PC All
: P2 major (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on: 57919 59287
Blocks:
  Show dependency tree
 
Reported: 2016-03-30 17:12 UTC by Alex
Modified: 2016-06-17 08:19 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alex 2016-03-30 17:12:57 UTC
As speciffied in OPCPackage API: https://poi.apache.org/apidocs/org/apache/poi/openxml4j/opc/OPCPackage.html#close%28%29
close should save an Excel file if writable, but it doesn't.
Instead, you need to save to a second file, and then it saves also the original one.
It happens the same (at least) if created with WorkbookFactory.create or OPCPackage.open with Write permissions.

I wrote a basic test:

//Create excel file
File file= new File("C:\\file2.xlsx");
Workbook wbx= new XSSFWorkbook();
wbx.createSheet();
Sheet s= wbx.getSheetAt(0);
Row r= s.createRow(0);
Cell c= r.createCell(0);
c.setCellType(Cell.CELL_TYPE_STRING);
c.setCellValue("Wrong");
OutputStream os= new FileOutputStream(file);
wbx.write(os);
wbx.close();

//Update Excel file
wbx= WorkbookFactory.create(file, null, false);
/*OR 
OPCPackage pkg = OPCPackage.open(file,PackageAccess.READ_WRITE);
wbx= new XSSFWorkbook(pkg);
*/

s= wbx.getSheetAt(0);
r= s.getRow(0);
c= r.getCell(0);
c.setCellValue("Right");

//Without this, now it doesn't save
/*
File trash= File.createTempFile("basura", ".tmp");
FileOutputStream trahsOs= new FileOutputStream(trash);
wbx.write(trahsOs);
trahsOs.close();
trash.delete();
*/

//This should save the file
wbx.close();


//Reopen and check
wbx= WorkbookFactory.create(file, null, false);
s= wbx.getSheetAt(0);
r= s.getRow(0);
c= r.getCell(0);
String value= c.getStringCellValue();
wbx.close();

assert("Right".equals(value));
Comment 1 Javen O'Neal 2016-03-30 17:29:59 UTC
Since you're using File and not FileInputStream, it should write back to disk.

See bug 58779. If you open the file through a FileInputStream vs File with WorkbookFactory.create(File|FileInputStream) or XSSFWorkbook(File|FileInputStream), or OPCPackage(File|FileInputStream) you get different behavior. Saving to an input stream does nothing to the file system.

I'm thinking about changing the behavior of close to not save changes to disk, but have not decided yet.
Comment 2 Nick Burch 2016-03-30 17:43:04 UTC
OPCPackage has always supported in-place write for Files. OPOIFS never did, while NPOIFS now does. Because OPOIFS never did, and that's all we had for agest, the usermodel APIs never did either

Now that the default POIFS is NPOIFS which does, there has been talk of adding in-place write at the usermodel level (eg XSSFWorkbook.write() / HSSFWorkbook.write() in addition to the write(OutputStream) method), but no-one has so far volunteered to do the work to implement
Comment 3 Alex 2016-03-30 18:45:24 UTC
Ok,I think I understand your point, close() shouldn't save to file (and I actually agree with that) but what I'm saing its that it actually doesn't save on close() with 3.14 release unless you call write(FileOutputStream).
You said it should be saving to file as from now but it's not.
May be I didn't understood your answers.
I'll try to take a look to source conde (I hope I can follow it), but I would like to clarify if that test I wrote should be working at 3.14 release or not.
Comment 4 Alex 2016-03-30 20:45:05 UTC
Ok, I took a look over source code and found the problem.

This is method write(OutputStream) in POIXMLDocument:
public final void write(OutputStream stream) throws IOException {
	//force all children to commit their changes into the underlying OOXML Package
	Set<PackagePart> context = new HashSet<PackagePart>();
	onSave(context);
	context.clear();

	//save extended and custom properties
	getProperties().commit();

	getPackage().save(stream);
}

Key is the properties commit, that it's not done on close() method, so package doesn't notice any change.
To test this, I updated my code so instead doing this:
File trash= File.createTempFile("basura", ".tmp");
FileOutputStream trahsOs= new FileOutputStream(trash);
wbx.write(trahsOs);
trahsOs.close();
trash.delete();

It's enough to call with null and catch the Exception:
try {
	wbx.write(null);
} catch (Exception e) {}

Do you think it could be right to change close() from:
public void close() throws IOException {
	if (pkg != null) {
		if (pkg.getPackageAccess() == PackageAccess.READ) {
			pkg.revert();
		} else {
			pkg.close();
		}
		pkg= null;
	}
}

to:
public void close() throws IOException {
	if (pkg != null) {
		if (pkg.getPackageAccess() == PackageAccess.READ) {
			pkg.revert();
		} else {
			// force all children to commit their changes into the underlying OOXML Package
			Set<PackagePart> context= new HashSet<PackagePart>();
			onSave(context);
			context.clear();

			// save extended and custom properties
			getProperties().commit();
			pkg.close();
		}
		pkg= null;
	}
}

That leaving the close() discussion for other moment.
Comment 5 Javen O'Neal 2016-03-31 06:34:25 UTC
Testing the change to POIXMLDocument.close from comment 4, I got an error on TestXSSFBugs.bug45431 (which relates to a macro-enabled workbook):
"Rule M2.4 exception : this error should NEVER happen!"
Full stack trace: https://paste.apache.org/MU6J

ContentTypeManager.getContentType throws the exception
Code: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/openxml4j/opc/internal/ContentTypeManager.java?revision=1722433&view=markup#l323

According to the javadocs for getContentType:
@exception OpenXML4JRuntimeException
Throws if the content type manager is not able to find the content from an existing part.