You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by Angela Schreiber <an...@adobe.com> on 2012/04/26 17:22:29 UTC
Exception handling in oak-core
hi all
i would like to start the discussion again regarding
exception handling in oak-core and the api.
the current situation is as follows:
- runtimeException every here and there
- invalidArgumentException
- unspecific commitFailedExceptions
now that we have quite some JCR functionality present
those exceptions are pretty cumbersome and result in a
lot of test-case failure.
my preference was to just throw the jcr-exceptions where
ever this was appropriate and unambiguous. for example
namespaceexception, versionexception, constraintviolation...
kind regards
angela
Re: Exception handling in oak-core
Posted by Michael Dürig <md...@apache.org>.
On 26.4.12 17:22, Angela Schreiber wrote:
> hi all
>
> i would like to start the discussion again regarding
> exception handling in oak-core and the api.
>
> the current situation is as follows:
> - runtimeException every here and there
> - invalidArgumentException
> - unspecific commitFailedExceptions
>
> now that we have quite some JCR functionality present
> those exceptions are pretty cumbersome and result in a
> lot of test-case failure.
>
> my preference was to just throw the jcr-exceptions where
> ever this was appropriate and unambiguous. for example
> namespaceexception, versionexception, constraintviolation...
+1 if the thrower of the exception is pretty sure that no one upstream
in the call chain would be forced to catch the exception without knowing
what to do with it. Like in the case where you have to implement an
interface where you are not allowed to throw an exception which might be
thrown by some of the methods you are calling.
In the other case why not throw an exception which extends from
RuntimeException and carries an checked exception? Something along the
lines of:
public class OakException extends RuntimeException {
private final RepositoryException repositoryException;
[...]
public void throwRepositoryException() throws RepositoryException {
throw repositoryException;
}
}
Since we are having these wrappers anyway we can unwrap these exceptions
there and throw the original one.
Michael
>
> kind regards
> angela
Re: Exception handling in oak-core
Posted by Michael Dürig <mi...@gmail.com>.
On 3.5.12 13:43, Julian Reschke wrote:
> On 2012-05-03 14:26, Thomas Mueller wrote:
>> Hi,
>>
>>> I wouldn't want to catch RuntimeException. I'd prefer if oak-core would
>>> only throw OakException (extends RuntimeException), as suggested by
>>> Michael Dürig.
>>
>> +1
>>
>>> I also think it would be good if these Oak exceptions could carry
>>> sufficient information to generate a JCR exception.
>>
>> +1
>>
>>> The mappings that have been suggested by Michael and above would help,
>>> but we'd lose correctness in the call stack, right?
>>
>> Do you mean instead of one simple stack trace you would get two (the
>> original one as "Caused by:")? I think that's OK. If it's really a
>> problem
>> we could still use Exception.setStackTrace(..), but I don't think that's
>> necessary.
>
> If the OakException wraps a RepositoryException, we extract that one and
> rethrow it, it will come with an incorrect stack trace, no? So it seems
> we need to re-wrap.
Just for the fun of it: instead of wrapping we could also trick the
compiler into thinking we throw unchecked exceptions where in reality we
throw checked exceptions:
public static void throwUnchecked(Throwable e) {
OakException.<RuntimeException>throwIt(e);
}
@SuppressWarnings("unchecked")
private static <E extends Throwable> void throwIt(Throwable e) throws E {
throw (E)e;
}
now
public void foo() {
throwUnchecked(new NamespaceException("foo"));
}
throws a NamespaceException without the compiler complaining ;-)
Credits to Alexey [1]
Michael
[1]
http://blog.ragozin.info/2011/10/java-how-to-throw-undeclared-checked.html
>
> It seems we're converging on
>
> - OakException extends RuntimeException
>
> - OakExpception can carry RepositoryException
>
> Should we start with that, and see how this works? (Open a issue for it?)
>
> Best regards, Julian
>
Re: Exception handling in oak-core
Posted by Michael Dürig <md...@apache.org>.
On 3.5.12 13:43, Julian Reschke wrote:
> On 2012-05-03 14:26, Thomas Mueller wrote:
>> Hi,
>>
>>> I wouldn't want to catch RuntimeException. I'd prefer if oak-core would
>>> only throw OakException (extends RuntimeException), as suggested by
>>> Michael Dürig.
>>
>> +1
>>
>>> I also think it would be good if these Oak exceptions could carry
>>> sufficient information to generate a JCR exception.
>>
>> +1
>>
>>> The mappings that have been suggested by Michael and above would help,
>>> but we'd lose correctness in the call stack, right?
>>
>> Do you mean instead of one simple stack trace you would get two (the
>> original one as "Caused by:")? I think that's OK. If it's really a
>> problem
>> we could still use Exception.setStackTrace(..), but I don't think that's
>> necessary.
>
> If the OakException wraps a RepositoryException, we extract that one and
> rethrow it, it will come with an incorrect stack trace, no? So it seems
> we need to re-wrap.
That depends on what you consider correct ;-) With the approach I
proposed earlier, clients would only observer the original stack trace
(that is the one from the wrapped exception) which I consider good since
this is the path to the origin of the problem.
Michael
>
> It seems we're converging on
>
> - OakException extends RuntimeException
>
> - OakExpception can carry RepositoryException
>
> Should we start with that, and see how this works? (Open a issue for it?)
>
> Best regards, Julian
>
Re: Exception handling in oak-core
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
>>Not sure what you mean with re-wrap actually...
>
>Throw a RepositoryException that wraps whatever was sent from below.
OK... I guess this should work:
RepositoryException x = ...
x.setStackTrace(this.getStackTrace()).
return x;
Regards,
Thomas
Re: Exception handling in oak-core
Posted by Julian Reschke <ju...@gmx.de>.
On 2012-05-03 14:54, Thomas Mueller wrote:
> Hi,
>
>> If the OakException wraps a RepositoryException, we extract that one and
>> rethrow it, it will come with an incorrect stack trace, no?
>
> You mean, if we use "throw ((OakException) e).getOriginalException()" or
> something similar when unwrapping?
Yes.
>> So it seems
>> we need to re-wrap.
>
> Not sure what you mean with re-wrap actually...
Throw a RepositoryException that wraps whatever was sent from below.
>>
>> It seems we're converging on
>>
>> - OakException extends RuntimeException
>>
>> - OakExpception can carry RepositoryException
>>
>> Should we start with that, and see how this works? (Open a issue for it?)
>
> Sounds good to me.
>
> Regards,
> Thomas
>
>
Re: Exception handling in oak-core
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
>If the OakException wraps a RepositoryException, we extract that one and
>rethrow it, it will come with an incorrect stack trace, no?
You mean, if we use "throw ((OakException) e).getOriginalException()" or
something similar when unwrapping?
> So it seems
>we need to re-wrap.
Not sure what you mean with re-wrap actually...
>
>It seems we're converging on
>
>- OakException extends RuntimeException
>
>- OakExpception can carry RepositoryException
>
>Should we start with that, and see how this works? (Open a issue for it?)
Sounds good to me.
Regards,
Thomas
Re: Exception handling in oak-core
Posted by Julian Reschke <ju...@gmx.de>.
On 2012-05-03 14:26, Thomas Mueller wrote:
> Hi,
>
>> I wouldn't want to catch RuntimeException. I'd prefer if oak-core would
>> only throw OakException (extends RuntimeException), as suggested by
>> Michael Dürig.
>
> +1
>
>> I also think it would be good if these Oak exceptions could carry
>> sufficient information to generate a JCR exception.
>
> +1
>
>> The mappings that have been suggested by Michael and above would help,
>> but we'd lose correctness in the call stack, right?
>
> Do you mean instead of one simple stack trace you would get two (the
> original one as "Caused by:")? I think that's OK. If it's really a problem
> we could still use Exception.setStackTrace(..), but I don't think that's
> necessary.
If the OakException wraps a RepositoryException, we extract that one and
rethrow it, it will come with an incorrect stack trace, no? So it seems
we need to re-wrap.
It seems we're converging on
- OakException extends RuntimeException
- OakExpception can carry RepositoryException
Should we start with that, and see how this works? (Open a issue for it?)
Best regards, Julian
Re: Exception handling in oak-core
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
>I wouldn't want to catch RuntimeException. I'd prefer if oak-core would
>only throw OakException (extends RuntimeException), as suggested by
>Michael Dürig.
+1
>I also think it would be good if these Oak exceptions could carry
>sufficient information to generate a JCR exception.
+1
>The mappings that have been suggested by Michael and above would help,
>but we'd lose correctness in the call stack, right?
Do you mean instead of one simple stack trace you would get two (the
original one as "Caused by:")? I think that's OK. If it's really a problem
we could still use Exception.setStackTrace(..), but I don't think that's
necessary.
Regards,
Thomas
Re: Exception handling in oak-core
Posted by Julian Reschke <ju...@gmx.de>.
On 2012-04-27 15:17, Jukka Zitting wrote:
> Hi,
>
> Instead of discussing this in the abstract, how about we look at a few
> concrete cases to see how those would be best handled?
>
> For example here's what the JCR spec says that the Session.save()
> method [1] can throw:
>
>> AccessDeniedException - if any of the changes to be persisted would violate the access
>> privileges of the this Session. Also thrown if any of the changes to be persisted would
>> cause the removal of a node that is currently referenced by a REFERENCE property
>> that this Session does not have read access to.
>>
>> ItemExistsException - if any of the changes to be persisted would be prevented by the
>> presence of an already existing item in the workspace.
>>
>> ConstraintViolationException - if any of the changes to be persisted would violate
>> a node type or restriction. Additionally, a repository may use this exception to
>> enforce implementation- or configuration-dependent restrictions.
>>
>> InvalidItemStateException - if any of the changes to be persisted conflicts with
>> a change already persisted through another session and the implementation is
>> such that this conflict can only be detected at save-time and therefore was not
>> detected earlier, at change-time.
>>
>> ReferentialIntegrityException - if any of the changes to be persisted would cause
>> the removal of a node that is currently referenced by a REFERENCE property
>> that this Session has read access to.
>>
>> VersionException - if the save would make a result in a change to persistent
>> storage that would violate the read-only status of a checked-in node.
>>
>> LockException - if the save would result in a change to persistent storage
>> that would violate a lock.
>>
>> NoSuchNodeTypeException - if the save would result in the addition of a node
>> with an unrecognized node type.
>>
>> RepositoryException - if another error occurs.
>
> Our current implementation of save only throws generic
> RepositoryExceptions and doesn't handle possible RuntimeExceptions
> thrown from below:
>
> public void save() throws RepositoryException {
> try {
> root.commit();
> } catch (CommitFailedException e) {
> throw new RepositoryException(e);
> }
> }
>
> Here's how I'd fix this:
>
> public void save() throws RepositoryException {
> try {
> root.commit();
> } catch (CommitFailedException e) {
> RepositoryException re =
> exceptionMapper.getRepositoryException(e);
> if (re == null) {
> re = new RepositoryException("Failed to save transient
> changes", e);
> }
> throw re;
> } catch (RuntimeException e) {
> throw new RepositoryException("Unexpected save() failure", e);
> }
> }
I wouldn't want to catch RuntimeException. I'd prefer if oak-core would
only throw OakException (extends RuntimeException), as suggested by
Michael Dürig (*).
I also think it would be good if these Oak exceptions could carry
sufficient information to generate a JCR exception.
The mappings that have been suggested by Michael and above would help,
but we'd lose correctness in the call stack, right?
> ...
Best regards, Julian
(*) My *actual* preference would be to throw the proper
RepositoryExceptions right away, as suggested by Angela.
Re: Exception handling in oak-core
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
Instead of discussing this in the abstract, how about we look at a few
concrete cases to see how those would be best handled?
For example here's what the JCR spec says that the Session.save()
method [1] can throw:
> AccessDeniedException - if any of the changes to be persisted would violate the access
> privileges of the this Session. Also thrown if any of the changes to be persisted would
> cause the removal of a node that is currently referenced by a REFERENCE property
> that this Session does not have read access to.
>
> ItemExistsException - if any of the changes to be persisted would be prevented by the
> presence of an already existing item in the workspace.
>
> ConstraintViolationException - if any of the changes to be persisted would violate
> a node type or restriction. Additionally, a repository may use this exception to
> enforce implementation- or configuration-dependent restrictions.
>
> InvalidItemStateException - if any of the changes to be persisted conflicts with
> a change already persisted through another session and the implementation is
> such that this conflict can only be detected at save-time and therefore was not
> detected earlier, at change-time.
>
> ReferentialIntegrityException - if any of the changes to be persisted would cause
> the removal of a node that is currently referenced by a REFERENCE property
> that this Session has read access to.
>
> VersionException - if the save would make a result in a change to persistent
> storage that would violate the read-only status of a checked-in node.
>
> LockException - if the save would result in a change to persistent storage
> that would violate a lock.
>
> NoSuchNodeTypeException - if the save would result in the addition of a node
> with an unrecognized node type.
>
> RepositoryException - if another error occurs.
Our current implementation of save only throws generic
RepositoryExceptions and doesn't handle possible RuntimeExceptions
thrown from below:
public void save() throws RepositoryException {
try {
root.commit();
} catch (CommitFailedException e) {
throw new RepositoryException(e);
}
}
Here's how I'd fix this:
public void save() throws RepositoryException {
try {
root.commit();
} catch (CommitFailedException e) {
RepositoryException re =
exceptionMapper.getRepositoryException(e);
if (re == null) {
re = new RepositoryException("Failed to save transient
changes", e);
}
throw re;
} catch (RuntimeException e) {
throw new RepositoryException("Unexpected save() failure", e);
}
}
This approach avoids having to build JCR-level information into the
core Oak API or related exceptions. Instead the various validator
plugins that enforce JCR-level constraints and throw
CommitFailedExceptions when called by the core can later map those
exceptions to appropriate JCR exceptions. To do this, such a validator
plugin would implement both the Validator interface in oak-core and a
new ExceptionMapper interface in oak-jcr, and connect to those
extension points.
Such an approach is obviously more complicated than simply allowing
RepositoryExceptions either directly in the Oak API or wrapped into
other exceptions, but the benefit is added flexibility.
For example, a direct HTTP->Oak mapping for something like a
client-facing JSOP access point (i.e. JSOP with all the constraint
checks, etc. in place), could easily provide it's own ExceptionMapper
extension point that instead of first going through JCR exceptions
would allow a validator component to directly craft an appropriate
HTTP response based on the kind of problem that was encountered.
[1] http://www.day.com/maven/javax.jcr/javadocs/jcr-2.0/javax/jcr/Session.html#save()
BR,
Jukka Zitting
Re: Exception handling in oak-core
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
>>let's assume CoreValue.getString() could throw a
>> RepositoryException (when there is an error converting the value to a
>> string). If we do that, then we would have to add exception handling to
>>a
>
>That examples seems a bit academic right now, as CoreValue.getString()
>indeed never throws (it delegates to toString()).
I know, that's what it does right now. I wasn't speaking about what it
does right now.
The question is, if we change CoreValue.getString() so values of other
types (non-string) are converted to a string, will we change
CoreValue.getString() to throw a checked exception? I suggest we don't,
and throw a runtime exception instead if needed.
Regards,
Thomas
Re: Exception handling in oak-core
Posted by Julian Reschke <ju...@gmx.de>.
On 2012-04-27 12:29, Thomas Mueller wrote:
> Hi,
>
>> my preference was to just throw the jcr-exceptions where
>> ever this was appropriate and unambiguous. for example
>> namespaceexception, versionexception, constraintviolation...
>
> I can't comment on NamespaceException, VersionException, and so on.
>
> What I find problematic is, if almost all methods can throw a checked
> exception. As an example, let's assume CoreValue.getString() could throw a
> RepositoryException (when there is an error converting the value to a
> string). If we do that, then we would have to add exception handling to a
That examples seems a bit academic right now, as CoreValue.getString()
indeed never throws (it delegates to toString()). The interesting case
are of course binary properties, and we'll have to think about what
needs to be done here.
> *lot* of places. Within iterator implementations, within compareTo
> methods, within toString methods,... It would make the code hard to read
> and maintain. I find runtime exceptions are the better solution in many
> cases. But yes, we should avoid using RuntimeException itself. I used it
> quite a lot in the query engine actually, I will find a better way.
Best regards, Julian
Re: Exception handling in oak-core
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
>my preference was to just throw the jcr-exceptions where
>ever this was appropriate and unambiguous. for example
>namespaceexception, versionexception, constraintviolation...
I can't comment on NamespaceException, VersionException, and so on.
What I find problematic is, if almost all methods can throw a checked
exception. As an example, let's assume CoreValue.getString() could throw a
RepositoryException (when there is an error converting the value to a
string). If we do that, then we would have to add exception handling to a
*lot* of places. Within iterator implementations, within compareTo
methods, within toString methods,... It would make the code hard to read
and maintain. I find runtime exceptions are the better solution in many
cases. But yes, we should avoid using RuntimeException itself. I used it
quite a lot in the query engine actually, I will find a better way.
Regards,
Thomas