Re: Please Don't Eat Exceptions
Jerome,
I just know I had to track it down to two places when performing the
test case I presented. I'll check out the new change to
HttpClientHelper and see if that works for me. It could be that I was
seeing a ghost. ;)
Anyway, no problem. I'll reopen the bug if there are additional issues.
Adam
Jerome Louvel wrote:
Hi Adam,
Thanks for pointing me to your patches. I've just reviewed them.
For the first one on HttpMethodCall, the fact that there is no body doesn't
indicate an error, it can be the expected result for a HEAD request or a 304
status. I don't think that throwing a RuntimeException is necessary. Could
you explain why you needed this?
For the patch on HttpClientHelper, I agree that the getConverter() method
shouldn't eat all those exceptions. Now, the method declares a "throws
Exception", and the handle() method catch them and set the status to
CONNECTOR_INTERNAL_ERROR (as discussed before). I hope this will satisfy
you. Otherwise, let us know or reopen the bug report.
Best regards,
Jerome
-Message d'origine-
De : Adam Taft [mailto:[EMAIL PROTECTED]
Envoyé : dimanche 3 juin 2007 04:10
À : discuss@restlet.tigris.org
Objet : Re: Please Don't Eat Exceptions
Jerome,
I think I had to patch the code in two places to get it to
work. There
was another instance of this type of code in HttpMethodCall.
You might
want to look for it there (or look at my patch in the bug
report, which
should give you an idea of where to look).
Thanks,
Adam
Jerome Louvel wrote:
Hi Adam,
The fix I've just checked in SVN should solve your issue.
You now can check
whether your request generated an error by solely looking
at the response's
status code. In your case, it would return a
CONNECTOR_ERROR_INTERNAL
because we don't fully deal with invalid URIs. In the
future, we might
improve the precision of the status. In your case you could
also expect a
CLIENT_ERROR_BAD_REQUEST because:
- you provided a non HTTP URI ("a")
- you didn't provide a Request's hostRef property to
indicate the host to
connect to
I understand your preference for checked exceptions. For a
Java-centric API
I would design like you propose, but for a REST/HTTP API, I
think it's
better and more consistent to reuse and extend the HTTP
status mechanism.
Best regards,
Jerome
-Message d'origine-
De : Adam Taft [mailto:[EMAIL PROTECTED]
Envoyé : vendredi 1 juin 2007 06:05
À : discuss@restlet.tigris.org
Objet : Re: Please Don't Eat Exceptions
I don't necessarily understand where the "first stage" and
the "second
stage" are in the code you're referring to. So, I can't
guess as to
whether it's a good solution.
Maybe think about a lower level functional class (ie.
basically what is
Client right now) throwing checked exceptions out of the
handle method.
And then, having a "convenience" class that extends Client
and wraps
the exceptions to produce an appropriate error Status code.
This would
allow the best of both worlds, by allowing the checked
exceptions to be
dealt with programatically by the end user/developer, while also
allowing a non-checked configuration for those who want it.
A developer who wants the checked exceptions would do:
Client client = new Client();
A developer who wants to do if-then-else testing on the
status code
would do:
UncheckedClient client = new UncheckedClient();
Where UncheckedClient extends Client.
Regardless, down in the depths of the specification, it's
in my mind
that the handle method should throw exceptions.
Just some additional thoughts, I guess.
Adam
Thierry Boileau wrote:
Hello,
we had a discussion with Jerome, and we plainly agree
with the fact
there is a problem in HttpClientHelper#handle [1] method as
pointed out
thanksfully by Adam and Jim.
This method first builds a call object then sends this call
and gets the
server's response.
The try/catch block is very problematic in the case the
build step of
the call fails. We've decided to remove it.
That is to say that all errors detected in this first step
will be in
charge of the caller code which is "responsible to give
correct data".
A contrario, the errors detected in the second phase should
generate a
status code:
- Status.CONNECTOR_ERROR_XXX (where XXX stands for CONNECTION,
COMMUNICATION and INTERNAL) for all errors detected before
the request
is sent. This is the current behaviour.
- HTTP status received from the server.
What do you think about?
best regards,
Thierry Boileau
[1]
public void handle(Request request, Response response) {
try {
HttpClientCall httpCall =
getConverter().toSpecific(this,
request);
getConverter().commit(httpCall, request, response);
} catch (Exception e) {
RE: Please Don't Eat Exceptions
Hi Adam,
Thanks for pointing me to your patches. I've just reviewed them.
For the first one on HttpMethodCall, the fact that there is no body doesn't
indicate an error, it can be the expected result for a HEAD request or a 304
status. I don't think that throwing a RuntimeException is necessary. Could
you explain why you needed this?
For the patch on HttpClientHelper, I agree that the getConverter() method
shouldn't eat all those exceptions. Now, the method declares a "throws
Exception", and the handle() method catch them and set the status to
CONNECTOR_INTERNAL_ERROR (as discussed before). I hope this will satisfy
you. Otherwise, let us know or reopen the bug report.
Best regards,
Jerome
> -Message d'origine-
> De : Adam Taft [mailto:[EMAIL PROTECTED]
> Envoyé : dimanche 3 juin 2007 04:10
> À : discuss@restlet.tigris.org
> Objet : Re: Please Don't Eat Exceptions
>
> Jerome,
>
> I think I had to patch the code in two places to get it to
> work. There
> was another instance of this type of code in HttpMethodCall.
> You might
> want to look for it there (or look at my patch in the bug
> report, which
> should give you an idea of where to look).
>
> Thanks,
>
> Adam
>
>
> Jerome Louvel wrote:
> > Hi Adam,
> >
> > The fix I've just checked in SVN should solve your issue.
> You now can check
> > whether your request generated an error by solely looking
> at the response's
> > status code. In your case, it would return a
> CONNECTOR_ERROR_INTERNAL
> > because we don't fully deal with invalid URIs. In the
> future, we might
> > improve the precision of the status. In your case you could
> also expect a
> > CLIENT_ERROR_BAD_REQUEST because:
> > - you provided a non HTTP URI ("a")
> > - you didn't provide a Request's hostRef property to
> indicate the host to
> > connect to
> >
> > I understand your preference for checked exceptions. For a
> Java-centric API
> > I would design like you propose, but for a REST/HTTP API, I
> think it's
> > better and more consistent to reuse and extend the HTTP
> status mechanism.
> >
> > Best regards,
> > Jerome
> >
> >> -Message d'origine-
> >> De : Adam Taft [mailto:[EMAIL PROTECTED]
> >> Envoyé : vendredi 1 juin 2007 06:05
> >> À : discuss@restlet.tigris.org
> >> Objet : Re: Please Don't Eat Exceptions
> >>
> >>
> >> I don't necessarily understand where the "first stage" and
> >> the "second
> >> stage" are in the code you're referring to. So, I can't
> guess as to
> >> whether it's a good solution.
> >>
> >> Maybe think about a lower level functional class (ie.
> >> basically what is
> >> Client right now) throwing checked exceptions out of the
> >> handle method.
> >> And then, having a "convenience" class that extends Client
> >> and wraps
> >> the exceptions to produce an appropriate error Status code.
> >> This would
> >> allow the best of both worlds, by allowing the checked
> >> exceptions to be
> >> dealt with programatically by the end user/developer, while also
> >> allowing a non-checked configuration for those who want it.
> >>
> >> A developer who wants the checked exceptions would do:
> >>
> >> Client client = new Client();
> >>
> >> A developer who wants to do if-then-else testing on the
> status code
> >> would do:
> >>
> >> UncheckedClient client = new UncheckedClient();
> >>
> >> Where UncheckedClient extends Client.
> >>
> >> Regardless, down in the depths of the specification, it's
> in my mind
> >> that the handle method should throw exceptions.
> >>
> >> Just some additional thoughts, I guess.
> >>
> >> Adam
> >>
> >>
> >> Thierry Boileau wrote:
> >>> Hello,
> >>>
> >>> we had a discussion with Jerome, and we plainly agree
> with the fact
> >>> there is a problem in HttpClientHelper#handle [1] method as
> >> pointed out
> >>> thanksfully by Adam and Jim.
> >>> This method first builds a call object then sends this call
> >> and gets the
> >>> server's response.
> >>> The try/catch block is very problematic in the case the
> >> build step of
> >>>
Re: Please Don't Eat Exceptions
Jerome,
I think I had to patch the code in two places to get it to work. There
was another instance of this type of code in HttpMethodCall. You might
want to look for it there (or look at my patch in the bug report, which
should give you an idea of where to look).
Thanks,
Adam
Jerome Louvel wrote:
Hi Adam,
The fix I've just checked in SVN should solve your issue. You now can check
whether your request generated an error by solely looking at the response's
status code. In your case, it would return a CONNECTOR_ERROR_INTERNAL
because we don't fully deal with invalid URIs. In the future, we might
improve the precision of the status. In your case you could also expect a
CLIENT_ERROR_BAD_REQUEST because:
- you provided a non HTTP URI ("a")
- you didn't provide a Request's hostRef property to indicate the host to
connect to
I understand your preference for checked exceptions. For a Java-centric API
I would design like you propose, but for a REST/HTTP API, I think it's
better and more consistent to reuse and extend the HTTP status mechanism.
Best regards,
Jerome
-Message d'origine-
De : Adam Taft [mailto:[EMAIL PROTECTED]
Envoyé : vendredi 1 juin 2007 06:05
À : discuss@restlet.tigris.org
Objet : Re: Please Don't Eat Exceptions
I don't necessarily understand where the "first stage" and
the "second
stage" are in the code you're referring to. So, I can't guess as to
whether it's a good solution.
Maybe think about a lower level functional class (ie.
basically what is
Client right now) throwing checked exceptions out of the
handle method.
And then, having a "convenience" class that extends Client
and wraps
the exceptions to produce an appropriate error Status code.
This would
allow the best of both worlds, by allowing the checked
exceptions to be
dealt with programatically by the end user/developer, while also
allowing a non-checked configuration for those who want it.
A developer who wants the checked exceptions would do:
Client client = new Client();
A developer who wants to do if-then-else testing on the status code
would do:
UncheckedClient client = new UncheckedClient();
Where UncheckedClient extends Client.
Regardless, down in the depths of the specification, it's in my mind
that the handle method should throw exceptions.
Just some additional thoughts, I guess.
Adam
Thierry Boileau wrote:
Hello,
we had a discussion with Jerome, and we plainly agree with the fact
there is a problem in HttpClientHelper#handle [1] method as
pointed out
thanksfully by Adam and Jim.
This method first builds a call object then sends this call
and gets the
server's response.
The try/catch block is very problematic in the case the
build step of
the call fails. We've decided to remove it.
That is to say that all errors detected in this first step
will be in
charge of the caller code which is "responsible to give
correct data".
A contrario, the errors detected in the second phase should
generate a
status code:
- Status.CONNECTOR_ERROR_XXX (where XXX stands for CONNECTION,
COMMUNICATION and INTERNAL) for all errors detected before
the request
is sent. This is the current behaviour.
- HTTP status received from the server.
What do you think about?
best regards,
Thierry Boileau
[1]
public void handle(Request request, Response response) {
try {
HttpClientCall httpCall =
getConverter().toSpecific(this,
request);
getConverter().commit(httpCall, request, response);
} catch (Exception e) {
getLogger().log(Level.WARNING,
"Error while handling an HTTP client call: ",
e.getMessage());
getLogger().log(Level.INFO,
"Error while handling an HTTP client call", e);
}
}
I made some comments to the bug, by the way.
One thing that came to mind, about handling different 3rd
party client
apis, is to deal with it like Spring deals with JDBC
exceptions. In
essence, they wrap and catch the various jdbc/driver
exceptions and
then map it to a set of common spring defined exceptions.
In my mind, the Status code in the Response object should
be a direct
mapping to the HTTP status codes. This is really the
intent you guys
had. Of course, the status code is undefined for a request that
cannot establish a connection with the host, due to
various problems
(physical connection, dns, proxy related issues, no route
to host, etc.)
Without the ability to "detect" these kind of connectivity
issues in
developer code, the Client class is pretty worthless. Having a
generic isError() method or a bogus status code in the response
doesn't give the level of detail needed.
You're also forcing developers to use the nasty pattern of
switch or
if/else st
RE: Please Don't Eat Exceptions
Hi Adam,
The fix I've just checked in SVN should solve your issue. You now can check
whether your request generated an error by solely looking at the response's
status code. In your case, it would return a CONNECTOR_ERROR_INTERNAL
because we don't fully deal with invalid URIs. In the future, we might
improve the precision of the status. In your case you could also expect a
CLIENT_ERROR_BAD_REQUEST because:
- you provided a non HTTP URI ("a")
- you didn't provide a Request's hostRef property to indicate the host to
connect to
I understand your preference for checked exceptions. For a Java-centric API
I would design like you propose, but for a REST/HTTP API, I think it's
better and more consistent to reuse and extend the HTTP status mechanism.
Best regards,
Jerome
> -Message d'origine-
> De : Adam Taft [mailto:[EMAIL PROTECTED]
> Envoyé : vendredi 1 juin 2007 06:05
> À : discuss@restlet.tigris.org
> Objet : Re: Please Don't Eat Exceptions
>
>
> I don't necessarily understand where the "first stage" and
> the "second
> stage" are in the code you're referring to. So, I can't guess as to
> whether it's a good solution.
>
> Maybe think about a lower level functional class (ie.
> basically what is
> Client right now) throwing checked exceptions out of the
> handle method.
> And then, having a "convenience" class that extends Client
> and wraps
> the exceptions to produce an appropriate error Status code.
> This would
> allow the best of both worlds, by allowing the checked
> exceptions to be
> dealt with programatically by the end user/developer, while also
> allowing a non-checked configuration for those who want it.
>
> A developer who wants the checked exceptions would do:
>
> Client client = new Client();
>
> A developer who wants to do if-then-else testing on the status code
> would do:
>
> UncheckedClient client = new UncheckedClient();
>
> Where UncheckedClient extends Client.
>
> Regardless, down in the depths of the specification, it's in my mind
> that the handle method should throw exceptions.
>
> Just some additional thoughts, I guess.
>
> Adam
>
>
> Thierry Boileau wrote:
> > Hello,
> >
> > we had a discussion with Jerome, and we plainly agree with the fact
> > there is a problem in HttpClientHelper#handle [1] method as
> pointed out
> > thanksfully by Adam and Jim.
> > This method first builds a call object then sends this call
> and gets the
> > server's response.
> > The try/catch block is very problematic in the case the
> build step of
> > the call fails. We've decided to remove it.
> > That is to say that all errors detected in this first step
> will be in
> > charge of the caller code which is "responsible to give
> correct data".
> >
> > A contrario, the errors detected in the second phase should
> generate a
> > status code:
> > - Status.CONNECTOR_ERROR_XXX (where XXX stands for CONNECTION,
> > COMMUNICATION and INTERNAL) for all errors detected before
> the request
> > is sent. This is the current behaviour.
> > - HTTP status received from the server.
> >
> >
> > What do you think about?
> >
> > best regards,
> > Thierry Boileau
> > [1]
> >public void handle(Request request, Response response) {
> >try {
> >HttpClientCall httpCall =
> getConverter().toSpecific(this,
> > request);
> >getConverter().commit(httpCall, request, response);
> >} catch (Exception e) {
> >getLogger().log(Level.WARNING,
> >"Error while handling an HTTP client call: ",
> >e.getMessage());
> >getLogger().log(Level.INFO,
> >"Error while handling an HTTP client call", e);
> >}
> >}
> >
> >
> >>
> >> I made some comments to the bug, by the way.
> >>
> >> One thing that came to mind, about handling different 3rd
> party client
> >> apis, is to deal with it like Spring deals with JDBC
> exceptions. In
> >> essence, they wrap and catch the various jdbc/driver
> exceptions and
> >> then map it to a set of common spring defined exceptions.
> >>
> >> In my mind, the Status code in the Response object should
> be a direct
> >> mapping to the HTTP status codes. This is really the
> intent you guys
> >> had. Of course, the status code is
Re: Please Don't Eat Exceptions
I don't necessarily understand where the "first stage" and the "second
stage" are in the code you're referring to. So, I can't guess as to
whether it's a good solution.
Maybe think about a lower level functional class (ie. basically what is
Client right now) throwing checked exceptions out of the handle method.
And then, having a "convenience" class that extends Client and wraps
the exceptions to produce an appropriate error Status code. This would
allow the best of both worlds, by allowing the checked exceptions to be
dealt with programatically by the end user/developer, while also
allowing a non-checked configuration for those who want it.
A developer who wants the checked exceptions would do:
Client client = new Client();
A developer who wants to do if-then-else testing on the status code
would do:
UncheckedClient client = new UncheckedClient();
Where UncheckedClient extends Client.
Regardless, down in the depths of the specification, it's in my mind
that the handle method should throw exceptions.
Just some additional thoughts, I guess.
Adam
Thierry Boileau wrote:
Hello,
we had a discussion with Jerome, and we plainly agree with the fact
there is a problem in HttpClientHelper#handle [1] method as pointed out
thanksfully by Adam and Jim.
This method first builds a call object then sends this call and gets the
server's response.
The try/catch block is very problematic in the case the build step of
the call fails. We've decided to remove it.
That is to say that all errors detected in this first step will be in
charge of the caller code which is "responsible to give correct data".
A contrario, the errors detected in the second phase should generate a
status code:
- Status.CONNECTOR_ERROR_XXX (where XXX stands for CONNECTION,
COMMUNICATION and INTERNAL) for all errors detected before the request
is sent. This is the current behaviour.
- HTTP status received from the server.
What do you think about?
best regards,
Thierry Boileau
[1]
public void handle(Request request, Response response) {
try {
HttpClientCall httpCall = getConverter().toSpecific(this,
request);
getConverter().commit(httpCall, request, response);
} catch (Exception e) {
getLogger().log(Level.WARNING,
"Error while handling an HTTP client call: ",
e.getMessage());
getLogger().log(Level.INFO,
"Error while handling an HTTP client call", e);
}
}
I made some comments to the bug, by the way.
One thing that came to mind, about handling different 3rd party client
apis, is to deal with it like Spring deals with JDBC exceptions. In
essence, they wrap and catch the various jdbc/driver exceptions and
then map it to a set of common spring defined exceptions.
In my mind, the Status code in the Response object should be a direct
mapping to the HTTP status codes. This is really the intent you guys
had. Of course, the status code is undefined for a request that
cannot establish a connection with the host, due to various problems
(physical connection, dns, proxy related issues, no route to host, etc.)
Without the ability to "detect" these kind of connectivity issues in
developer code, the Client class is pretty worthless. Having a
generic isError() method or a bogus status code in the response
doesn't give the level of detail needed.
You're also forcing developers to use the nasty pattern of switch or
if/else statements to check for the appropriate conditions in the
Response object. If I'm making multiple requests to the server (which
is common in a RESTful design), and the first call results in a 404
Not Found, I don't want to have to keep checking under each statement
for the various status codes. I just want to have the requests one
per line and get an exception thrown for problems.
This is why I mentioned, in the bug, about the wrapper I use that
throws a UnexpectedStatusException (a custom exception class). The
use of the code looks like this:
try {
Client client = new Client(...);
client.handle(request1, response1, Status.SUCCESS_OK);
client.handle(request2, response2, Status.SUCCESS_OK);
} catch (IOException e) {
// something bad has happened
// like no connection, host name dns failure, etc.
} catch (UnexpectedStatusException e) {
// this is where you can put your if/then checking
if (e.getStatus().equals(Status.CLIENT_ERROR_NOT_FOUND)) {
// 404 Not Found message to user
return;
}
// other handling here, or just some generic handling code
}
Note the additional parameter for the "expected" status in the
handle() method.
Anyway, the point is, you could in theory force all the client related
exceptions into either IOException or UnexpectedStatusException.
Then, you'd have a consistent API for dealing with both HTTP status
code related issues as well as the other connectivity problems related
to the physical
Re: Please Don't Eat Exceptions
works for me. Will you create an issue for this.
cheers
Thierry Boileau wrote:
Hello,
we had a discussion with Jerome, and we plainly agree with the fact
there is a problem in HttpClientHelper#handle [1] method as pointed out
thanksfully by Adam and Jim.
This method first builds a call object then sends this call and gets the
server's response.
The try/catch block is very problematic in the case the build step of
the call fails. We've decided to remove it.
That is to say that all errors detected in this first step will be in
charge of the caller code which is "responsible to give correct data".
A contrario, the errors detected in the second phase should generate a
status code:
- Status.CONNECTOR_ERROR_XXX (where XXX stands for CONNECTION,
COMMUNICATION and INTERNAL) for all errors detected before the request
is sent. This is the current behaviour.
- HTTP status received from the server.
What do you think about?
best regards,
Thierry Boileau
[1]
public void handle(Request request, Response response) {
try {
HttpClientCall httpCall = getConverter().toSpecific(this,
request);
getConverter().commit(httpCall, request, response);
} catch (Exception e) {
getLogger().log(Level.WARNING,
"Error while handling an HTTP client call: ",
e.getMessage());
getLogger().log(Level.INFO,
"Error while handling an HTTP client call", e);
}
}
I made some comments to the bug, by the way.
One thing that came to mind, about handling different 3rd party client
apis, is to deal with it like Spring deals with JDBC exceptions. In
essence, they wrap and catch the various jdbc/driver exceptions and
then map it to a set of common spring defined exceptions.
In my mind, the Status code in the Response object should be a direct
mapping to the HTTP status codes. This is really the intent you guys
had. Of course, the status code is undefined for a request that
cannot establish a connection with the host, due to various problems
(physical connection, dns, proxy related issues, no route to host, etc.)
Without the ability to "detect" these kind of connectivity issues in
developer code, the Client class is pretty worthless. Having a
generic isError() method or a bogus status code in the response
doesn't give the level of detail needed.
You're also forcing developers to use the nasty pattern of switch or
if/else statements to check for the appropriate conditions in the
Response object. If I'm making multiple requests to the server (which
is common in a RESTful design), and the first call results in a 404
Not Found, I don't want to have to keep checking under each statement
for the various status codes. I just want to have the requests one
per line and get an exception thrown for problems.
This is why I mentioned, in the bug, about the wrapper I use that
throws a UnexpectedStatusException (a custom exception class). The
use of the code looks like this:
try {
Client client = new Client(...);
client.handle(request1, response1, Status.SUCCESS_OK);
client.handle(request2, response2, Status.SUCCESS_OK);
} catch (IOException e) {
// something bad has happened
// like no connection, host name dns failure, etc.
} catch (UnexpectedStatusException e) {
// this is where you can put your if/then checking
if (e.getStatus().equals(Status.CLIENT_ERROR_NOT_FOUND)) {
// 404 Not Found message to user
return;
}
// other handling here, or just some generic handling code
}
Note the additional parameter for the "expected" status in the
handle() method.
Anyway, the point is, you could in theory force all the client related
exceptions into either IOException or UnexpectedStatusException.
Then, you'd have a consistent API for dealing with both HTTP status
code related issues as well as the other connectivity problems related
to the physical connection. Defining handle() to throw both of these
is, in my mind, the right direction.
Adam
Thierry Boileau wrote:
Adam,
I just want to explain why this behaviour happens, not to plainly
justify it.
As you point, it has been decided to show the failures by the way of
the status code and not by the way of the exceptions.
One reason is that for example, the connectors are based on other
externals projects that may throw exceptions of different nature even
for equivalent issues, and the connectors may throw themselves
distinct exceptions for equivalent issues (equivalent from the API's
point of view).
Then, it has seemed difficult to potentially have to catch a growing
list of distinct exceptions in some part of the code (not the whole
code, of course). Thus, it has been decided, and it's a design
choice, to use the status codes instead of the exceptions.
However, this behaviour may be updated, the discussion is open.
best regards,
Thierry Boileau
Oops, didn't see your reply here. I just replie
Re: Please Don't Eat Exceptions
Hello,
we had a discussion with Jerome, and we plainly agree with the fact
there is a problem in HttpClientHelper#handle [1] method as pointed out
thanksfully by Adam and Jim.
This method first builds a call object then sends this call and gets the
server's response.
The try/catch block is very problematic in the case the build step of
the call fails. We've decided to remove it.
That is to say that all errors detected in this first step will be in
charge of the caller code which is "responsible to give correct data".
A contrario, the errors detected in the second phase should generate a
status code:
- Status.CONNECTOR_ERROR_XXX (where XXX stands for CONNECTION,
COMMUNICATION and INTERNAL) for all errors detected before the request
is sent. This is the current behaviour.
- HTTP status received from the server.
What do you think about?
best regards,
Thierry Boileau
[1]
public void handle(Request request, Response response) {
try {
HttpClientCall httpCall = getConverter().toSpecific(this,
request);
getConverter().commit(httpCall, request, response);
} catch (Exception e) {
getLogger().log(Level.WARNING,
"Error while handling an HTTP client call: ",
e.getMessage());
getLogger().log(Level.INFO,
"Error while handling an HTTP client call", e);
}
}
I made some comments to the bug, by the way.
One thing that came to mind, about handling different 3rd party client
apis, is to deal with it like Spring deals with JDBC exceptions. In
essence, they wrap and catch the various jdbc/driver exceptions and
then map it to a set of common spring defined exceptions.
In my mind, the Status code in the Response object should be a direct
mapping to the HTTP status codes. This is really the intent you guys
had. Of course, the status code is undefined for a request that
cannot establish a connection with the host, due to various problems
(physical connection, dns, proxy related issues, no route to host, etc.)
Without the ability to "detect" these kind of connectivity issues in
developer code, the Client class is pretty worthless. Having a
generic isError() method or a bogus status code in the response
doesn't give the level of detail needed.
You're also forcing developers to use the nasty pattern of switch or
if/else statements to check for the appropriate conditions in the
Response object. If I'm making multiple requests to the server (which
is common in a RESTful design), and the first call results in a 404
Not Found, I don't want to have to keep checking under each statement
for the various status codes. I just want to have the requests one
per line and get an exception thrown for problems.
This is why I mentioned, in the bug, about the wrapper I use that
throws a UnexpectedStatusException (a custom exception class). The
use of the code looks like this:
try {
Client client = new Client(...);
client.handle(request1, response1, Status.SUCCESS_OK);
client.handle(request2, response2, Status.SUCCESS_OK);
} catch (IOException e) {
// something bad has happened
// like no connection, host name dns failure, etc.
} catch (UnexpectedStatusException e) {
// this is where you can put your if/then checking
if (e.getStatus().equals(Status.CLIENT_ERROR_NOT_FOUND)) {
// 404 Not Found message to user
return;
}
// other handling here, or just some generic handling code
}
Note the additional parameter for the "expected" status in the
handle() method.
Anyway, the point is, you could in theory force all the client related
exceptions into either IOException or UnexpectedStatusException.
Then, you'd have a consistent API for dealing with both HTTP status
code related issues as well as the other connectivity problems related
to the physical connection. Defining handle() to throw both of these
is, in my mind, the right direction.
Adam
Thierry Boileau wrote:
Adam,
I just want to explain why this behaviour happens, not to plainly
justify it.
As you point, it has been decided to show the failures by the way of
the status code and not by the way of the exceptions.
One reason is that for example, the connectors are based on other
externals projects that may throw exceptions of different nature even
for equivalent issues, and the connectors may throw themselves
distinct exceptions for equivalent issues (equivalent from the API's
point of view).
Then, it has seemed difficult to potentially have to catch a growing
list of distinct exceptions in some part of the code (not the whole
code, of course). Thus, it has been decided, and it's a design
choice, to use the status codes instead of the exceptions.
However, this behaviour may be updated, the discussion is open.
best regards,
Thierry Boileau
Oops, didn't see your reply here. I just replied to your other thread.
Actually, I disagree. I think the problem is that the me
Re: Please Don't Eat Exceptions
On 30 May 2007, at 22:56, Jim Alateras wrote: I'm fine with the design choice. I stumbled across a place where this wasn't happening [1] but it was hard to associate a HTTP status code with the failure since the request failed to leave the client. Currently I have patched it to set the following status code response.setStatus(Status.CLIENT_ERROR_NOT_FOUND); You can't use 404 for this, a typical REST application could PUT something after the 404. Also then there would be no way to see if it was a real 404. I think wrong hostname (or network connectivity problems) means an IOException should be thrown, just like when trying to do getEntity ().getText(). After all, we are still talking about the internet, and a number of different connectivity issues could occur that can't be described using HTTP status codes. -- Stian Soiland, myGrid team School of Computer Science The University of Manchester http://www.cs.man.ac.uk/~ssoiland/
Re: Please Don't Eat Exceptions
Thierry,
I'm fine with the design choice. I stumbled across a place where this
wasn't happening [1] but it was hard to associate a HTTP status code
with the failure since the request failed to leave the client.
Currently I have patched it to set the following status code
response.setStatus(Status.CLIENT_ERROR_NOT_FOUND);
Any thoughts?
[1] http://restlet.tigris.org/servlets/ReadMsg?list=discuss&msgNo=2176
Thierry Boileau wrote:
Adam,
I just want to explain why this behaviour happens, not to plainly
justify it.
As you point, it has been decided to show the failures by the way of the
status code and not by the way of the exceptions.
One reason is that for example, the connectors are based on other
externals projects that may throw exceptions of different nature even
for equivalent issues, and the connectors may throw themselves distinct
exceptions for equivalent issues (equivalent from the API's point of view).
Then, it has seemed difficult to potentially have to catch a growing
list of distinct exceptions in some part of the code (not the whole
code, of course). Thus, it has been decided, and it's a design choice,
to use the status codes instead of the exceptions.
However, this behaviour may be updated, the discussion is open.
best regards,
Thierry Boileau
Oops, didn't see your reply here. I just replied to your other thread.
Actually, I disagree. I think the problem is that the method should
have originally been defined to through checked exceptions.
I want to be to notify the user of the exceptions inside of the client
itself. If my end user has entered a bad url, then I want to be able to
notify them about that, for example.
Setting a status code is not the way to handle this. You propose status
code 404, but that's not an appropriate response for a "host not found"
type of problem. Likewise, it's also not an appropriate status code if
the url itself was poorly formed (like, without a leading protocol
scheme).
Anyway, my point is, there are many reason which a request could fail.
I want to see these in my own code so that I can do what's appropriate
with the exception.
Adam
Jim Alateras wrote:
Adam,
I came across the same problem and I believe the problem is more
around not setting the response status code on failure.
cheers
Adam Taft wrote:
Here's a test case to look at...
public class TestClass {
public static void main(String[] args) {
try {
Request request = new Request(Method.GET,
"a");
Client client = new Client(Protocol.HTTP);
client.handle(request);
System.out.println("This code shouldn't run!");
} catch (Throwable t) {
// eat everything
}
}
}
The offending code is in HttpClientHelper#handle ...
public void handle(Request request, Response response) {
try {
HttpClientCall httpCall =
getConverter().toSpecific(this, request);
getConverter().commit(httpCall, request, response);
} catch (Exception e) {
getLogger().log(Level.WARNING,
"Error while handling an HTTP client call: ",
e.getMessage());
getLogger().log(Level.INFO,
"Error while handling an HTTP client call", e);
}
}
As soon as I get my email confirmation from tigris, I'll file a bug
report. I just wanted to mention this in case there are other areas
you know about with similar code. If the goal is to log the
exception, that's ok, but it needs to be re-thrown.
Thanks!
Adam
Re: Please Don't Eat Exceptions
I made some comments to the bug, by the way.
One thing that came to mind, about handling different 3rd party client
apis, is to deal with it like Spring deals with JDBC exceptions. In
essence, they wrap and catch the various jdbc/driver exceptions and then
map it to a set of common spring defined exceptions.
In my mind, the Status code in the Response object should be a direct
mapping to the HTTP status codes. This is really the intent you guys
had. Of course, the status code is undefined for a request that cannot
establish a connection with the host, due to various problems (physical
connection, dns, proxy related issues, no route to host, etc.)
Without the ability to "detect" these kind of connectivity issues in
developer code, the Client class is pretty worthless. Having a generic
isError() method or a bogus status code in the response doesn't give the
level of detail needed.
You're also forcing developers to use the nasty pattern of switch or
if/else statements to check for the appropriate conditions in the
Response object. If I'm making multiple requests to the server (which
is common in a RESTful design), and the first call results in a 404 Not
Found, I don't want to have to keep checking under each statement for
the various status codes. I just want to have the requests one per line
and get an exception thrown for problems.
This is why I mentioned, in the bug, about the wrapper I use that throws
a UnexpectedStatusException (a custom exception class). The use of the
code looks like this:
try {
Client client = new Client(...);
client.handle(request1, response1, Status.SUCCESS_OK);
client.handle(request2, response2, Status.SUCCESS_OK);
} catch (IOException e) {
// something bad has happened
// like no connection, host name dns failure, etc.
} catch (UnexpectedStatusException e) {
// this is where you can put your if/then checking
if (e.getStatus().equals(Status.CLIENT_ERROR_NOT_FOUND)) {
// 404 Not Found message to user
return;
}
// other handling here, or just some generic handling code
}
Note the additional parameter for the "expected" status in the handle()
method.
Anyway, the point is, you could in theory force all the client related
exceptions into either IOException or UnexpectedStatusException. Then,
you'd have a consistent API for dealing with both HTTP status code
related issues as well as the other connectivity problems related to the
physical connection. Defining handle() to throw both of these is, in my
mind, the right direction.
Adam
Thierry Boileau wrote:
Adam,
I just want to explain why this behaviour happens, not to plainly
justify it.
As you point, it has been decided to show the failures by the way of the
status code and not by the way of the exceptions.
One reason is that for example, the connectors are based on other
externals projects that may throw exceptions of different nature even
for equivalent issues, and the connectors may throw themselves distinct
exceptions for equivalent issues (equivalent from the API's point of view).
Then, it has seemed difficult to potentially have to catch a growing
list of distinct exceptions in some part of the code (not the whole
code, of course). Thus, it has been decided, and it's a design choice,
to use the status codes instead of the exceptions.
However, this behaviour may be updated, the discussion is open.
best regards,
Thierry Boileau
Oops, didn't see your reply here. I just replied to your other thread.
Actually, I disagree. I think the problem is that the method should
have originally been defined to through checked exceptions.
I want to be to notify the user of the exceptions inside of the client
itself. If my end user has entered a bad url, then I want to be able to
notify them about that, for example.
Setting a status code is not the way to handle this. You propose status
code 404, but that's not an appropriate response for a "host not found"
type of problem. Likewise, it's also not an appropriate status code if
the url itself was poorly formed (like, without a leading protocol
scheme).
Anyway, my point is, there are many reason which a request could fail.
I want to see these in my own code so that I can do what's appropriate
with the exception.
Adam
Jim Alateras wrote:
Adam,
I came across the same problem and I believe the problem is more
around not setting the response status code on failure.
cheers
Adam Taft wrote:
Here's a test case to look at...
public class TestClass {
public static void main(String[] args) {
try {
Request request = new Request(Method.GET,
"a");
Client client = new Client(Protocol.HTTP);
client.handle(request);
System.out.println("This code shouldn't run!");
} catch (Throwable t) {
// eat everything
}
}
}
The offending code is in HttpClientHelper
Re: Please Don't Eat Exceptions
Adam,
I just want to explain why this behaviour happens, not to plainly
justify it.
As you point, it has been decided to show the failures by the way of the
status code and not by the way of the exceptions.
One reason is that for example, the connectors are based on other
externals projects that may throw exceptions of different nature even
for equivalent issues, and the connectors may throw themselves distinct
exceptions for equivalent issues (equivalent from the API's point of view).
Then, it has seemed difficult to potentially have to catch a growing
list of distinct exceptions in some part of the code (not the whole
code, of course). Thus, it has been decided, and it's a design choice,
to use the status codes instead of the exceptions.
However, this behaviour may be updated, the discussion is open.
best regards,
Thierry Boileau
Oops, didn't see your reply here. I just replied to your other thread.
Actually, I disagree. I think the problem is that the method should
have originally been defined to through checked exceptions.
I want to be to notify the user of the exceptions inside of the client
itself. If my end user has entered a bad url, then I want to be able to
notify them about that, for example.
Setting a status code is not the way to handle this. You propose status
code 404, but that's not an appropriate response for a "host not found"
type of problem. Likewise, it's also not an appropriate status code if
the url itself was poorly formed (like, without a leading protocol
scheme).
Anyway, my point is, there are many reason which a request could fail.
I want to see these in my own code so that I can do what's appropriate
with the exception.
Adam
Jim Alateras wrote:
Adam,
I came across the same problem and I believe the problem is more
around not setting the response status code on failure.
cheers
Adam Taft wrote:
Here's a test case to look at...
public class TestClass {
public static void main(String[] args) {
try {
Request request = new Request(Method.GET,
"a");
Client client = new Client(Protocol.HTTP);
client.handle(request);
System.out.println("This code shouldn't run!");
} catch (Throwable t) {
// eat everything
}
}
}
The offending code is in HttpClientHelper#handle ...
public void handle(Request request, Response response) {
try {
HttpClientCall httpCall =
getConverter().toSpecific(this, request);
getConverter().commit(httpCall, request, response);
} catch (Exception e) {
getLogger().log(Level.WARNING,
"Error while handling an HTTP client call: ",
e.getMessage());
getLogger().log(Level.INFO,
"Error while handling an HTTP client call", e);
}
}
As soon as I get my email confirmation from tigris, I'll file a bug
report. I just wanted to mention this in case there are other areas
you know about with similar code. If the goal is to log the
exception, that's ok, but it needs to be re-thrown.
Thanks!
Adam
Re: Please Don't Eat Exceptions
Oops, didn't see your reply here. I just replied to your other thread.
Actually, I disagree. I think the problem is that the method should
have originally been defined to through checked exceptions.
I want to be to notify the user of the exceptions inside of the client
itself. If my end user has entered a bad url, then I want to be able to
notify them about that, for example.
Setting a status code is not the way to handle this. You propose status
code 404, but that's not an appropriate response for a "host not found"
type of problem. Likewise, it's also not an appropriate status code if
the url itself was poorly formed (like, without a leading protocol scheme).
Anyway, my point is, there are many reason which a request could fail.
I want to see these in my own code so that I can do what's appropriate
with the exception.
Adam
Jim Alateras wrote:
Adam,
I came across the same problem and I believe the problem is more around
not setting the response status code on failure.
cheers
Adam Taft wrote:
Here's a test case to look at...
public class TestClass {
public static void main(String[] args) {
try {
Request request = new Request(Method.GET,
"a");
Client client = new Client(Protocol.HTTP);
client.handle(request);
System.out.println("This code shouldn't run!");
} catch (Throwable t) {
// eat everything
}
}
}
The offending code is in HttpClientHelper#handle ...
public void handle(Request request, Response response) {
try {
HttpClientCall httpCall = getConverter().toSpecific(this,
request);
getConverter().commit(httpCall, request, response);
} catch (Exception e) {
getLogger().log(Level.WARNING,
"Error while handling an HTTP client call: ",
e.getMessage());
getLogger().log(Level.INFO,
"Error while handling an HTTP client call", e);
}
}
As soon as I get my email confirmation from tigris, I'll file a bug
report. I just wanted to mention this in case there are other areas
you know about with similar code. If the goal is to log the
exception, that's ok, but it needs to be re-thrown.
Thanks!
Adam
Re: Please Don't Eat Exceptions
Adam,
I came across the same problem and I believe the problem is more around
not setting the response status code on failure.
cheers
Adam Taft wrote:
Here's a test case to look at...
public class TestClass {
public static void main(String[] args) {
try {
Request request = new Request(Method.GET, "a");
Client client = new Client(Protocol.HTTP);
client.handle(request);
System.out.println("This code shouldn't run!");
} catch (Throwable t) {
// eat everything
}
}
}
The offending code is in HttpClientHelper#handle ...
public void handle(Request request, Response response) {
try {
HttpClientCall httpCall = getConverter().toSpecific(this,
request);
getConverter().commit(httpCall, request, response);
} catch (Exception e) {
getLogger().log(Level.WARNING,
"Error while handling an HTTP client call: ",
e.getMessage());
getLogger().log(Level.INFO,
"Error while handling an HTTP client call", e);
}
}
As soon as I get my email confirmation from tigris, I'll file a bug
report. I just wanted to mention this in case there are other areas you
know about with similar code. If the goal is to log the exception,
that's ok, but it needs to be re-thrown.
Thanks!
Adam
