exception handling in jcr-server io subsystem

[Brian Moseley <bc...@osafoundation.org>]

i'm in the middle of updating cosmo to use the current revision of 
jackrabbit, and one thing is giving me a bit of trouble - the fact that 
jcr-server's import/export subsystem only allows IOExceptions to be 
propagated up to the DavResource level. in fact, in some places it 
actively swallows RepositoryExceptions.

DefaultHandler.importProperties has several sections of code like this:

         try {
             // if context-encoding is null -> remove the property
             contentNode.setProperty(JcrConstants.JCR_ENCODING, 
context.getEncoding());
         } catch (RepositoryException e) {
             // ignore: property may not be present on the node
         }

and in one case:

         } catch (RepositoryException e) {
             // ignore: property may not be present on the node.
             // deliberately not rethrowing as IOException.
         }

in certain other places, RepositoryExceptions are rethrown as 
IOExceptions, losing the stack trace of the original exception in the 
process.

i understand that DefaultHandler is written to assume that a certain 
node type structure is validated by the canImport/canExport methods. 
however, RepositoryException can be thrown due to operational problems 
with underlying subsystems (ex: the PersistenceManager can't store a 
node or property because the operating system won't let it). the 
swallowing and/or lossy wrapping of RepositoryExceptions makes it much 
harder for us to track down when operational problems occur.

in the best of all possible worlds, RepositoryException would be 
unchecked and i would be able to insert code to handle it at whatever 
layer i want without affecting the method signatures of intervening layers.

however, given that this is not the case, i'd like to revisit the io 
subsystem and perhaps explicitly throw RepositoryExceptions where they 
make sense.

angela, what do you think?

Re: exception handling in jcr-server io subsystem

[Brian Moseley <bc...@osafoundation.org>]

Angela Schreiber wrote:

> if you find other places e.g. during the import of data without
> an explaining comment its probably a bug.

you seem to be talking about swallowing RepositoryExceptions 
here. i understand why you did it in importProperties, tho i 
don't agree that swallowing every possible 
RepositoryException is correct.

> yes, 'a certain'... with the handlers available no check is made
> against the nodetype-definition that would reveal whether a
> certain property can be set or not :).
 >
> but of cause you may write your own handlers that assert
> the complete nodetype structure...

that's not my point. i'm not worried about structural 
exceptions, but rather operational ones. when i run out of 
disk and derby can no longer store properties, i want 
DefaultHandler.importProperties to throw that exception up 
the stack, not swallow it.

yes, i can write my own handler, but then i don't get to 
leverage the hundreds of lines of code in DefaultHandler and 
friends that are perfectly useful.

> i didn't throw RepositoryException on purpose, due to the fact,
> that the io-stuff was originally (thus already with the command-chains)
> designed to provide some abstraction of the import/export.
> currently i don't see a benefit of throwing RepositoryException(s).

you're taking the abstraction to such an extreme that we're 
losing important exception information.

IOException does not have a constructor that accepts a root 
cause exception, so even if i write a bunch of try/catch 
clauses to catch RepositoryExceptions and rethrow them as 
IOExceptions, i lose the stack trace of the original 
exception, and i have more useless lines of code to boot.

i think you've set the wrong goal by limiting exceptions in 
this way. it's not like you're somehow abstracting jcr out 
of the import/export process - ImportContext and 
ExportContext carry a node around. the only thing you're 
achieving here is throwing away information in a lower layer 
that is useful at a higher layer.

Re: exception handling in jcr-server io subsystem

[Angela Schreiber <an...@day.com>]

hi brian

> i'm in the middle of updating cosmo to use the current revision of 
> jackrabbit, and one thing is giving me a bit of trouble - the fact that 
> jcr-server's import/export subsystem only allows IOExceptions to be 
> propagated up to the DavResource level. in fact, in some places it 
> actively swallows RepositoryExceptions.

during the import of properties in the default handler, because
i wanted this to be used by all the derived handlers as well,
which use the same structure:

- 'File'-node
- 'Resource'-node
-  <data>

thus i did it on purpose during the import of properties (see below)
and only during import of properties...
if you find other places e.g. during the import of data without
an explaining comment its probably a bug.

> i understand that DefaultHandler is written to assume that a certain 
> node type structure is validated by the canImport/canExport methods. 

yes, 'a certain'... with the handlers available no check is made
against the nodetype-definition that would reveal whether a
certain property can be set or not :).

but of cause you may write your own handlers that assert
the complete nodetype structure...

> however, given that this is not the case, i'd like to revisit the io 
> subsystem and perhaps explicitly throw RepositoryExceptions where they 
> make sense.

> angela, what do you think?

i didn't throw RepositoryException on purpose, due to the fact,
that the io-stuff was originally (thus already with the command-chains)
designed to provide some abstraction of the import/export.
currently i don't see a benefit of throwing RepositoryException(s).

kind regards
angela