incubator-odf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeremias Maerki <...@jeremias-maerki.ch>
Subject Re: OdfPackage not throwing an exception on I/O errors
Date Fri, 23 Mar 2012 08:58:41 GMT
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 <dev@jeremias-maerki.ch>
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


Mime
View raw message