jferard / fastods

A very fast and lightweight (no dependency) library for creating ODS (Open Document Spreadsheet, mainly for Calc) files in Java. It's a Martin Schulz's SimpleODS fork
GNU General Public License v3.0
36 stars 6 forks source link

"AnonymousOdsFileWriter.save()" should not close the output stream passed to it #138

Closed negora closed 5 years ago

negora commented 5 years ago

First of all, thank you for sharing FastODS with all of us ;) .

The method AnonymousOdsFileWriter.save(OutputStream) closes the output stream that is passed to it. In my opinion, it shouldn't do that, because the code that calls such method may want to continue writting to that output.

For example, the caller code may want to generate multiple ODS files and group them into a single stream (or file). Or it may want to put a single ODS file in some kind of container that requires certain meta data after the ODS data. But right now, to do so, you've to write the ODS data in a temporary location (likely a temporary file on disk) and then copy its content into the main output stream.

I've taken a look into the source code of FastODS and I've seen that the stream is closed in the method AnonymousOdsDocument.save(ZipUTF8Writer). So I guess that this part:

try {
  ...
} finally {
  writer.close();
}

Could be replaced with something like this:

...
// To finish writting the ZIP meta-data.
writer.finish ();
// To flush the buffer in case that the original OutputStream is buffered.
writer.flush ();

After this change, the methods that create their own OutputStream (i.e. AnonymousOdsFileWriter.saveAs(File)) would need to close these by themeselves.

What do you opine about this issue? Thank you!

jferard commented 5 years ago

Thank you for your proposal. I think you're right. Aside from the unusal (but possible) use cases you presented, the main reason not to close the stream is that it's not the responsibility of the save method to do that.

The try-with-resources idiom is a good clue:

try (OutputStream os = new FileOutputStream("test.ods") {
    ...
    AnonymousOdsFileWriter.save(os);
}

If it was the job of save to close os, the try-with-resources would be useless.

I'll fix it.

negora commented 5 years ago

@jferard Thank you a lot :) .

jferard commented 5 years ago

See testSaveEmpyDocumentToStreamAndAddPrePostamble, testSaveEmpyDocumentToWriterAndAddEntry and testSaveTwiceEmpyDocumentToStream for examples.

negora commented 5 years ago

Perfect. I'll try it the next week :) .