Please note that I fully agree that using an ErrorHandler on the reading side for all sorts of things (XML parsing errors, validation errors, even IOExceptions) is a good way to handle things and to promote robustness and error tolerance.
But my post was motivated from the writing side where errors are swallowed. If there is an I/O error while writing the target file, I personally can see no real reason why the save operation should continue since you are almost guaranteed that you'll get an incomplete or inconsistent output file. If I understood Svante correctly, he suggests to use the ErrorHandler approach for the output side, too. It feels weird to me to wrap an exception during a write operation into a SAXParseException just so it fits through the ErrorHandler interface. I'd suggest to find a different approach (and disabled by default) if the save code shall be error tolerant in some way. Normally, I'd think that if the in-memory respresentation of the ODF document is consistent there should be no errors while writing the file as long as the OutputStream works. Anyway, I'll concentrate on a proposal to throw IOExceptions during save() first. I'm sure you can always refine that later on if necessary. Patch coming soon... On 23.03.2012 00:42:40 Svante Schubert wrote: > On 22.03.2012 20:45, Rob Weir wrote: > > On Thu, Mar 22, 2012 at 1:34 PM, Jeremias Maerki <[email protected]> > > wrote: > >> Hey guys, > >> is there a special reason why OdfPackage.save(OutputStream, String) > >> catches all IOExceptions, logs them and just continues? While I can see > >> the problems in the logs, my code thinks everything is fine and > >> continues normally. > >> > > I can't think of a good reason. > I can not think of a good reason, either. Already discussed it with > Jeremias this evening by phone and agreed to throwing Exception in > general, unless the user could gather more user information (robustness) > or more information about validness, in this case the ErrorHandler > <http://docs.oracle.com/javase/1.5.0/docs/api/org/xml/sax/ErrorHandler.html> > mechanism should be uses. > > In addition we found out that for the line > data = mMediaType.getBytes("UTF-8"); > should better raise an OdfValidationException for the validator, in case > this "mimetype" named file, being first in the ZIP is not being encoded > as ASCII, see > http://docs.oasis-open.org/office/v1.2/os/OpenDocument-v1.2-os-part3.html#MIME_type_stream > Well, we might here even exchange UTF-8 to ASCII, making it more > restrict - although first 128 char are similar, some might have used > other UTF-8 characters within, which would be invalid ODF. > > For those new on the list, the pkg.OdfValidationException is inheriting > from the SAXParseException and enabling the similar mechanism meant for > SAX XML parsing for the ODF Package. > > To summarize: > While parsing an XML file it is not desired that the parsing stops at > the first invalid XML by an exception. Instead the exception will be > given to an ErrorHandler, which differentiated between warning, error > and fatalerrors. > These three levels are mapped to a specification where something should > be used, shall be used and in the last case does not work at all. > > I have extended the concept of the ErrorHandler not only for XML > validation errors, but as well for arbitrary ODF validation errors, like > within the package, when certain mandatory files are missing. > I take advantage of this concept at the ODF Validator, when a single > default ErrorHandler from the JDK can be reused to gather validation > results. > pkg.OdfPackageConstraint.java and dom.OdfSchemaConstraint.java lists the > problems that are currenlty being checked and the doc.DocumentTest.java > and pkg.PackageTest.java are testing the test mechanism with some nice > test tools as utils.ErrorHandlerStub.java > > >> Furthermore, the public save() methods all throw Exception which is > >> rather bad style. > >> > > I assume you mean that they should limit themselves to IOException? > +1 > >> Can we please change that? I'll offer to write a proposal in form of a > >> patch. > >> > > If you can contribute a patch, that would be great. But please do a > > complete rebuild, to make sure the unit tests still work. > > > > So from the top directory: > > > > mvn clean install -Ppedantic > > > > Thanks! > > > > -Rob > > > > > >> Thanks, > >> Jeremias Maerki > >> > Jeremias Maerki
