You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by bu...@apache.org on 2016/03/30 19:12:57 UTC

[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: dev@poi.apache.org
          Reporter: tiparega@gmail.com

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: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #6 from Marcel M. <ma...@mail.de> ---
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: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

Sz.Noemi <no...@foconis.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |noemi.szemenyei@foconis.de

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


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

Posted by Andreas Beeker <ki...@apache.org>.
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: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


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

Posted by Javen O'Neal <ja...@gmail.com>.
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, <bu...@apache.org> wrote:

> https://bz.apache.org/bugzilla/show_bug.cgi?id=59252
>
> --- Comment #2 from Nick Burch <ap...@gagravarr.org> ---
> 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: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>

[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #2 from Nick Burch <ap...@gagravarr.org> ---
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: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

Dominik Stadler <do...@gmx.at> changed:

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

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

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

lawern <la...@agentes.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |lars.werner@agentes.de

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #7 from Francisco Castaneda <fr...@gmail.com> ---
I had the same problem, unless you call write on an outputstream, changes won't
be saved.

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

lawern <la...@agentes.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|3.14-FINAL                  |5.0.x-dev

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

Javen O'Neal <on...@apache.org> changed:

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

--- Comment #1 from Javen O'Neal <on...@apache.org> ---
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: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #5 from Javen O'Neal <on...@apache.org> ---
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: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #3 from Alex <ti...@gmail.com> ---
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: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

--- Comment #4 from Alex <ti...@gmail.com> ---
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: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


[Bug 59252] Close workbook does not save file

Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59252

Javen O'Neal <on...@apache.org> changed:

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

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

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org