RE: Unchecked exceptions in Resource constructors
Hi Vincent,
I've improved the exception handling as suggested. Checked in SVN trunk.
Best regards,
Jerome
> -Message d'origine-
> De : news [mailto:[EMAIL PROTECTED] De la part de Vincent
> Envoyé : samedi 22 septembre 2007 05:24
> À : discuss@restlet.tigris.org
> Objet : Re: Unchecked exceptions in Resource constructors
>
>
> Hi Jerome,
>
> > > } catch (InvocationTargetException e) {
> > > if (e.getCause() instanceof Error) {
> > > throw (Error) e.getCause();
> > > } else {
> > > getLogger() [...]
> > > }
> > I spoke too fast, we should also consider RuntimeExceptions:
> > catch(InvocationTargetException e){
> > if (e.getCause() instanceof Error
> > || e.getCause() instanceof RuntimeException) {
> > throw (Error) e.getCause();
> > }
> >getLogger()
> > }
>
> This solution doesn't work because if the cause is a
> RuntimeException, we can't
> cast it to an Error (doh!). And if we rethrow it as a
> RuntimeException, it'll
> get caught by the catch(Exception e) block of the enclosing try block.
> So, the solution is to throw a RuntimeException from the
> inner try blok, and let
> it bubble up from the outer try block:
>
> public Resource createResource(Request request, Response response) {
> Resource result = null;
>
> try {
> ...
> try {
>
> } catch(InvocationTargetException e){
> if (e.getCause() instanceof Error){
> throw (Error) e.getCause();
> }
> if ( e.getCause() instanceof RuntimeException) {
>throw (RuntimeException) e.getCause();
> }
> getLogger()...
>
> }
>} catch (RuntimeException e){
> // Let unchecked exceptions bubble up
> throw e;
>}
>}catch (Exception e) {
> getLogger()...
>}
> }
>
> return result;
> }
>
>
> In short: we make sure we let all unchecked exceptions
> (errors & runtime) flow
> through the method, while all checked ones (Exceptions that
> are not runtime) are
> trapped and logged. We should also document the fact that
> Resource constructors
> should never throw checked exceptions.
>
> -Vincent.
>
Re: Unchecked exceptions in Resource constructors
Hi Jerome,
> > } catch (InvocationTargetException e) {
> > if (e.getCause() instanceof Error) {
> > throw (Error) e.getCause();
> > } else {
> > getLogger() [...]
> > }
> I spoke too fast, we should also consider RuntimeExceptions:
> catch(InvocationTargetException e){
> if (e.getCause() instanceof Error
> || e.getCause() instanceof RuntimeException) {
> throw (Error) e.getCause();
> }
>getLogger()
> }
This solution doesn't work because if the cause is a RuntimeException, we can't
cast it to an Error (doh!). And if we rethrow it as a RuntimeException, it'll
get caught by the catch(Exception e) block of the enclosing try block.
So, the solution is to throw a RuntimeException from the inner try blok, and let
it bubble up from the outer try block:
public Resource createResource(Request request, Response response) {
Resource result = null;
try {
...
try {
} catch(InvocationTargetException e){
if (e.getCause() instanceof Error){
throw (Error) e.getCause();
}
if ( e.getCause() instanceof RuntimeException) {
throw (RuntimeException) e.getCause();
}
getLogger()...
}
} catch (RuntimeException e){
// Let unchecked exceptions bubble up
throw e;
}
}catch (Exception e) {
getLogger()...
}
}
return result;
}
In short: we make sure we let all unchecked exceptions (errors & runtime) flow
through the method, while all checked ones (Exceptions that are not runtime) are
trapped and logged. We should also document the fact that Resource constructors
should never throw checked exceptions.
-Vincent.
Re: Unchecked exceptions in Resource constructors
Hi Jerome,
> } catch (InvocationTargetException e) {
> if (e.getCause() instanceof Error) {
> throw (Error) e.getCause();
> } else {
> getLogger() [...]
> }
I spoke too fast, we should also consider RuntimeExceptions:
catch(InvocationTargetException e){
if (e.getCause() instanceof Error
|| e.getCause() instanceof RuntimeException) {
throw (Error) e.getCause();
}
getLogger()
}
-Vincent.
Re: Unchecked exceptions in Resource constructors
Hi Jerome, > Your suggestion seems reasonable. I've applied this changed to SVN trunk > (only). It is slightly different from your patch. Let me know if it works > for you: Thanks. It's exactly what I needed. -Vincent.
RE: Unchecked exceptions in Resource constructors
Hi Vincent,
Your suggestion seems reasonable. I've applied this changed to SVN trunk
(only). It is slightly different from your patch. Let me know if it works
for you:
public Resource createResource(Request request, Response response) {
Resource result = null;
if (getTargetClass() != null) {
try {
[...]
} catch (InvocationTargetException e) {
if (e.getCause() instanceof Error) {
throw (Error) e.getCause();
} else {
getLogger() [...]
}
} catch (Exception e) {
getLogger() [...]
}
}
Best regards,
Jerome
> -Message d'origine-
> De : news [mailto:[EMAIL PROTECTED] De la part de Vincent
> Envoyé : jeudi 13 septembre 2007 00:41
> À : discuss@restlet.tigris.org
> Objet : Unchecked exceptions in Resource constructors
>
>
> Hello,
>
>
> My application throws unchecked exceptions; I derived
> StatusService so that
> I can decide what status code and representation to send to
> the client.
>
> It works as expected except when the exception is thrown from
> a resource
> constructor.
> Because Finder.createResource uses reflection to call my
> resource constructor,
> the unchecked exception gets wrapped into an
> InvocationTargerException;
> this (checked) exception is caught by the method's 'catch
> (Exception e)'
> block, which logs a message and returns null:
>
>
> public Resource createResource(Request request, Response response) {
> Resource result = null;
> if (getTargetClass() != null) {
> try {
> [...]
> result = constructor.newInstance(getContext(),
> request,response);
> [...]
> } catch (Exception e) {
>getLogger().log(...);
> }
> }
> return result;
> }
>
>
> My unchecked exception has been swallowed- and now,
> Finder.handle assumes it's
> a 404:
>
> public void handle(Request request, Response response) {
> [...]
> Resource target = findTarget(request, response);
> if (!response.getStatus().equals(Status.SUCCESS_OK)) {
> // Probably during the instantiation of the target resource, or
> // earlier the status was changed from the default one. Don't go
> // further.
> } else if (target == null) {
>// If the currrent status is a success but we couldn't find the
>// target resource for the request's resource URI, then we set
> // the response status to 404 (Not Found).
> response.setStatus(Status.CLIENT_ERROR_NOT_FOUND);
> }
> [...]
> }
>
> My solution is to provide my own Finder, catch
> InvocationTargetException,
> and rethrow the cause (it it's an error):
>
> class MyFinder extends Finder {
> public Resource createResource(Request request, Response response) {
> Resource result = null;
> Error error = null;
> try {
> [...]
> try {
> [...]
> } catch (NoSuchMethodException nsme) {
> [...]
> } }
> catch(InvocationTargetException e){
> if (e.getCause() instanceof Error){
> error =(Error) e.getCause();
> }
> }
> } catch (Exception e) {
> getLogger().log([...]);
> }
> }
> // rethrow the Error that was wrapped in InvocationTargetException
> if (error != null){
> throw error;
> }
> return result;
> }
>
>
> The alternative would by to catch Error in all my resource
> constructors,
> set the response status to SERVER_ERROR, and rethrow the error. Not
> very compelling.
>
> Do you see the current behavior as a design flaw? I agree
> that resource
> constructor should not throw check exceptions because we know they're
> created by reflection, but we should let unchecked exceptions
> bubble up.
>
>
>
> -Vincent.
Unchecked exceptions in Resource constructors
Hello,
My application throws unchecked exceptions; I derived StatusService so that
I can decide what status code and representation to send to the client.
It works as expected except when the exception is thrown from a resource
constructor.
Because Finder.createResource uses reflection to call my resource constructor,
the unchecked exception gets wrapped into an InvocationTargerException;
this (checked) exception is caught by the method's 'catch (Exception e)'
block, which logs a message and returns null:
public Resource createResource(Request request, Response response) {
Resource result = null;
if (getTargetClass() != null) {
try {
[...]
result = constructor.newInstance(getContext(), request,response);
[...]
} catch (Exception e) {
getLogger().log(...);
}
}
return result;
}
My unchecked exception has been swallowed- and now, Finder.handle assumes it's
a 404:
public void handle(Request request, Response response) {
[...]
Resource target = findTarget(request, response);
if (!response.getStatus().equals(Status.SUCCESS_OK)) {
// Probably during the instantiation of the target resource, or
// earlier the status was changed from the default one. Don't go
// further.
} else if (target == null) {
// If the currrent status is a success but we couldn't find the
// target resource for the request's resource URI, then we set
// the response status to 404 (Not Found).
response.setStatus(Status.CLIENT_ERROR_NOT_FOUND);
}
[...]
}
My solution is to provide my own Finder, catch InvocationTargetException,
and rethrow the cause (it it's an error):
class MyFinder extends Finder {
public Resource createResource(Request request, Response response) {
Resource result = null;
Error error = null;
try {
[...]
try {
[...]
} catch (NoSuchMethodException nsme) {
[...]
} }
catch(InvocationTargetException e){
if (e.getCause() instanceof Error){
error =(Error) e.getCause();
}
}
} catch (Exception e) {
getLogger().log([...]);
}
}
// rethrow the Error that was wrapped in InvocationTargetException
if (error != null){
throw error;
}
return result;
}
The alternative would by to catch Error in all my resource constructors,
set the response status to SERVER_ERROR, and rethrow the error. Not
very compelling.
Do you see the current behavior as a design flaw? I agree that resource
constructor should not throw check exceptions because we know they're
created by reflection, but we should let unchecked exceptions bubble up.
-Vincent.
