[Bug 59252] New: Close workbook does not save file

https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

            Bug ID: 59252
           Summary: Close workbook does not save file
           Product: POI
           Version: 3.14-FINAL
          Hardware: PC
            Status: NEW
          Severity: major
          Priority: P2
         Component: XSSF
          Assignee: [hidden email]
          Reporter: [hidden email]

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));

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 OS|                            |All

--- Comment #1 from Javen O'Neal <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #2 from Nick Burch <[hidden email]> ---
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

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #3 from Alex <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[Javen O'Neal]

This is a candidate for GSOC or Help Wanted.

Need WorkbookFactory.create and XSSFWorkbook constructor to open read only

Should not write to file on close if a workbook is merely opened and not
modified.
Sounds like changing close's behavior is contentious, so we'd need to
decide what direction we want to go (make close write changes to disk,
deprecate/remove FileInputStream constructors or add note that close won't
write changes back to disk; alternatively, make close non-saving or add a
close(boolean save) method)

However this is handled, we should harmonize all of POI so that close
behavior is clear and logical.
On Mar 30, 2016 10:43 AM, <[hidden email]> wrote:

> https://bz.apache.org/bugzilla/show_bug.cgi?id=59252
>
> --- Comment #2 from Nick Burch <[hidden email]> ---
> 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
>
> --
> You are receiving this mail because:
> You are the assignee for the bug.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

[kiwiwings]

I never understood why the default is to write changes to disc.
I'd prefer the default to leave the document unchanged and only write the changes when commit() is called.
close() should be callable in any case, possibly throwing away any temporary objects -
and it should be ok, to call it again.

As mentioned, harmonizing the various modules should be part of the change.

To stick with our deprecation policy, I would deprecate revert() and after two releases change the handling completely :P

So what to do with the in-place write functionality?
The default is to open documents read-only - for read/write we would work on a copy and
overwrite the original file when commit() is called.

Andi


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #4 from Alex <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #5 from Javen O'Neal <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

Dominik Stadler <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |59287

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |57919

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #6 from Marcel M. <[hidden email]> ---
I just hit this bug in 5.0.0.
A (wasteful) workaround is to write to a NullOutputStream to trigger the in
place update of the File:


try (Workbook wb = WorkbookFactory.create(new File("foo"));
     OutputStream out = OutputStream.nullOutputStream()) {
    ...
    wb.write(out);
}


This is especially interesting because the doc for
WorkbooFactory.create(InputStream) encourages the use of the File version:

"Note also that loading from an InputStream requires more memory than loading
from a File, so prefer {@link #create(File)} where possible."

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]