Le 15 janv. 10 à 00:11, Adam Wiggins a écrit :

One of my favorite features of restclient is that it throws an
exception when you get a non-200 status code.  However, this whole
area is something that is a little weak in implementation, design, and
docs.

=== Overview

For those that don't know, non-2xx status codes in restclient works as
follows.  In most cases you can run a RestClient get/put/post/delete -
if the call fails it will abort, just like you would expect with (for
example) a file operation that failed.  If it succeeds your code
continues to execute.

In the cases where you care about trapping specific error responses,
you can do so.  For example, you might want to ignore a 404 error on
DELETE (meaning the resource was already deleted):

begin
 RestClient.delete "http://somewhere/resource"
rescue RestClient::ResourceNotFound
 # already deleted
end

You may also want to do something specific with the response.  For
example, printing it out:

begin
 RestClient.get "http://somewhere"
rescue RestClient::RequestFailed => e
 puts "The request failed with HTTP status code #{e.response.code}"
 puts "The body was:"
 puts e.response.body
end

=== Implementation

The implementation is lacking in that the value on e.reponse is the
Net::HTTP response, not a RestClient::Response.  This is because it
predates the creation of RestClient::Response.  Also,
RestClient::Reponse is a little wonky right now - there's this weird
mixin thing, in general it needs some cleanup and normalization.

Although we've never documented the error handling, LOTS of things
rely on it.  So we should be careful about changing the behavior of
e.response.  Most importantly is that e.response.code and
e.response.body continue to work.

This would also fix the annoying problem where e.response.body needs
to be manually unzipped.  RestClient asks for compressed responses and
uncompresses them automatically, but e.response.body is the raw
net::http response so you get gobblygook when you try to print out a
response from a webserver that uses gzip.

My proposed solution : return a RestClient::Response and add
compatibility by using a missing_method wired on the underlying
Net::HTTP response with a warning

Alos, can you tell me which response part needs cleaning ?

=== Design

I think the overall approach is sound, but there are some small design
issues that bother me.  One is that there are specific exceptions for
certain http codes (e.g., ResourceNotFound for 404); and then a
generic catch-all (RestClient::RequestFailed) for everything else.
This results in people writing code like this:

begin
 RestClient.post("http://somewhere/resource", 'post body')
rescue RestClient::RequestFailed => e
 raise unless e.http_code == 422
 raise(BadRecord, e.response.body)
end

Here we want to trap the specific response of 422 (which Rails uses
for failed activerecord validations) and follow some custom code path
to handle that case.  For other errors we want to abort normally.
It's a shame this can't be more specific, but I don't see any easy way
around this, other than trying to create an exception for every single
HTTP code.  (I think Net::HTTP does this, actually.)  Even then, it's
within the HTTP spec to use custom codes - the 422 that Rails uses is
not part of the base spec:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

(I believe they borrowed it from webdav.)

I think having a specific exception for each code is cleaner, even if
some of them will probably never be used.

For specific codes: we could probably add a hook to enable the calling
code to register them but I think that for such cases the rest-client
calls are encapsulated and custom rescue code in the encapsulating
code would be cleaner. (Unless of course someone can show me a valid
case.)

My other design niggle is that it's hard to use this from the
restclient shell.  I generally want to see the response body in the
shell, but you have to type a multiline command in order to do so.
One option would be to make the shell behave differently - it always
shows the output body.  Another would be to add a set of
get/put/post/delete methods that behave differently and can be called
anywhere.  For example:

res = RestClient.get! 'http://example.com/does-not-exist'
puts res.code   # 404
puts res        # body

The idea is nice but we need to find a better name because the ! don't
match the general ! usage in ruby

A.

As a user of RestClient for quite some time; I've found that raising
an exception
on anything other than 200 has become my least favorite feature.

In most cases, a non-200 status is not an exceptional problem.  There is
just
something more that needs to be done.

Let's take a 201 for example.  This means my request was successful but I
need
to look elsewhere for my data.  An exception is the wrong thing here.

Having exceptions thrown about prevents me from making a better decision
of
what needs to be done.

I think Adams was not talking about non 200 but about non 2XX, like in
second paragraph

My proposal is, let's have our cake and eat it too.

We break the API.  Follow me on this:

    RestClient.get "http://sinatrarb.com/a304" # =>
<RestClient::Response#... @code=304 ...>
    RestClient.get! "http://sinatrarb.com/a304" # raises exception

The main point here is that a 304 is not an exceptional problem.  In
fact, most of the time,
I don't care; it's a noop.  Just keep on going.

Going a step further:

    RestClient.get! "http://sinatrarb.com/asdf", :ok => [200, 304] #
=> raises if anything other than :ok

This last example gives more flexibility and power to users.
Sometimes a 304 is ok.  It simply means
the user should already have what they're looking for.

IMHO, anything other than a 500 isn't an exceptional problem.   In
cases where it is,
let the user decide the executions fate.

I'm half-convinced to manage the 304 as a 2XX because it would break the
behavior's coherency for a rare case.

I like the :ok parameter because it's explicit, but I would add it to
RestClient.get method instead of RestClient.get!, as the idea behind get!
(I would still prefer a better name) is to return a response for all
codes.

So I propose

RestClient.get "http://sinatrarb.com/asdf", :ok => [200, 304]
-> RestClient::Response for 201 and 304, exeption for anything else

RestClient.get! "http://sinatrarb.com/asdf"
-> Exception in all cases

And the exceptions hold a properly-decoded RestClient::Response (with
compatibility to http response for existing code)

btw: you said "anything other than a 500 isn't an exceptional problem":
including the 4XX ?

A.

Correct. Anything other than a 500 is "exceptional".

I feel that 400's are "exceptional" in some cases but not all. I want
to make the decision, not me client.

That is why I, like you, am leaning towards the :ok param.  I noticed
a few days ago that Excon http://github.com/geemus/excon/ has an
:expects param which I find useful but not compelling enough to move
from RestClient.

Maybe borrowing :expects from Excon could be the right answer here.

Thoughts?



On Sunday, January 17, 2010,  <code@...> wrote:

As a user of RestClient for quite some time; I've found that raising
an exception
on anything other than 200 has become my least favorite feature.

In most cases, a non-200 status is not an exceptional problem.  There is
just
something more that needs to be done.

Let's take a 201 for example.  This means my request was successful but I
need
to look elsewhere for my data.  An exception is the wrong thing here.

Having exceptions thrown about prevents me from making a better decision
of
what needs to be done.

I think Adams was not talking about non 200 but about non 2XX, like in
second paragraph

My proposal is, let's have our cake and eat it too.

We break the API.  Follow me on this:

    RestClient.get "http://sinatrarb.com/a304" # =>
<RestClient::Response#... @code=304 ...>
    RestClient.get! "http://sinatrarb.com/a304" # raises exception

The main point here is that a 304 is not an exceptional problem.  In
fact, most of the time,
I don't care; it's a noop.  Just keep on going.

Going a step further:

    RestClient.get! "http://sinatrarb.com/asdf", :ok => [200, 304] #
=> raises if anything other than :ok

This last example gives more flexibility and power to users.
Sometimes a 304 is ok.  It simply means
the user should already have what they're looking for.

IMHO, anything other than a 500 isn't an exceptional problem.   In
cases where it is,
let the user decide the executions fate.

I'm half-convinced to manage the 304 as a 2XX because it would break the
behavior's coherency for a rare case.

I like the :ok parameter because it's explicit, but I would add it to
RestClient.get method instead of RestClient.get!, as the idea behind get!
(I would still prefer a better name) is to return a response for all
codes.

So I propose

RestClient.get "http://sinatrarb.com/asdf", :ok => [200, 304]
-> RestClient::Response for 201 and 304, exeption for anything else

RestClient.get! "http://sinatrarb.com/asdf"
-> Exception in all cases

And the exceptions hold a properly-decoded RestClient::Response (with
compatibility to http response for existing code)

btw: you said "anything other than a 500 isn't an exceptional problem":
including the 4XX ?

A.

--
Blake Mizerany
blake.mizerany@...