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
</jima>
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,
"aaaaaaaaaaaaaaaaa");
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
