With Feature::Client, allow to react to Non-Success HTTP responses to the http-verbs (get, put, post ...)

[fwolfst]

Like outlined in
https://groups.google.com/forum/?fromgroups#!topic/roar-talk/G6HKrJQWArk
as a caller I want to be informed about failed PUTs or conflicting POSTs, which are usually indicated by the HTTP-Response.

Implementation for the Net:HTTP adapter should not be too difficult, for the faraday one I have no idea (yet).

I am willing to help a bit with the implementation (or create pull request once the discussion is settled) and made minimal changes to examples/example_server.rb to have a simple test-case ( https://github.com/fwolfst/roar ).

Have fun
felix

[fwolfst]

To give my motivation: I use roar as a middleman between a Datamapper sqlite and a couchdb. Put'ting new documents just works fine, put post'ing changes fails without me easily recognizing it. Otherwise, thanks for all the good stuff in there!

[apotonick]

Ok, good to see you picking up this convo again 😄 What exactly does your added test do, and why is it duplicated in the other PR, too?

[fwolfst]

What do you mean with "again"? I did not find an older related issue.

There is no test yet, just the test-infrastructure to answer a (e.g. post-)request with a status given in the requested URL - POST /forgot/the/path/status210 will be responded with a 210.

Before the test, I would like to hear what pull request you would accept :) , or if you want, just write the test, I will try to do the implemention. I do not care so much about backwards compatibility here and have no real favorite solution as of yet. Would like to hear yours and/or others.

About the duplication I probably have not understood github yet. What should I have done instead?

[apotonick]

You have two PRs and they both contain a commit that adds your code to the test server. This should go to one, only.

Anyway, thanks a lot! ❤️ Can you give me a quick heads-up about what is the problem and how you want to fix it? Is it error-handling when POST/PUT fail? What's the current behaviour? (I could actually need this myself :)

[fwolfst]

Okay, scenario is following:

I store documents in a couchdb by issuing PUT and POST requests with json bodies.

The couchdb will answer with 20x if things are alright, but might also answer with other codes indicating failure, for example 409 because an incorrect revision was passed or somebody else changed the data in the meantime.

Now, if I updated a document in the client and want to store (post) the changes in the database, it is hard for me to tell if that action succeeded.

Similarly, services might respond to a GET with a 404 (Resource not found) or really any other non-success indicating status code (basically anything not in the 2xx range).

Sofar this goes rather unnoticed (at least without faraday, have not tried with it), where I expect to have control over it. Either by a return value, or a field in the decorator, or an exception or some kind of callback. In each case, an allowed_http_errors like Mechanize employs them might be useful to 1) easily restore backwards compatibility by defaulting them to "all status codes" 2) not hinder control flow if e.g. the result is not that important or to deal with faulty server implementations (I faced that situation lately, the webpage of GLS responding with 404 to every request, but actually delivering the html I wanted).

I hope its a bit more clear now.

[apotonick]

Here's an idea.

object.post!(..) do |response|
  do_whatever if response.status == 400
end

The verb methods with bang (#post! and friends) could run the passed block if an error is returned. That is roughly following the semantics of Trailblazer's Operation, and works great.

As a second step, we could add :allowed_http_errors.

The only problem I see is that the "non-banged" methods like #post already accept a block which always yields the request object.

[fwolfst]

The restriction of not being able to define additional headers etc. is probably hurting the people who are also interested in the responses. So I would definately keep the request-manipulation feature, also in combination with response-inspection-feature.

I think by now I would prefer to change the default behaviour of post to always return the response.

Second favorite is a new post-method

    resp = object.do_post(){|req|}
    if ...
        # check object.http_response.status
    end

Or, have it in the module (is it a dynamic module? Too lazy to check right now), e.g.

    [response, new_obj] = MyRepresenter.post(object) {|req| ..}
    if !response.kind_of?(Net::HTTPSuccess)
        # darn
    end

Or a method throwing exceptions. Afaiu you do not like callbacks, otherwise they would be another alternative

    # callbacks, yay
    object.post(... , error_callback: &my_cb, allowed_error_codes: [409])

    # exceptions, huuray
    begin
      object.post(...){|req|}
    rescue Roar::ResponseCodeError
        # pretty sure its fine.
    end

[paulccarey]

Hi Guys,

I've got the same problem.

One of our end-points will respond with 404 if no resource is available.

Currently this raises a JSON:ParseError because we don't return a body just a head response.

The way I've seen this implemented in other clients is to raise an appropriate exception that describes the end-points response.

Specifically this is how rest-client handles such responses.
https://github.com/rest-client/rest-client#exceptions-see-wwww3orgprotocolsrfc2616rfc2616-sec10html

The exception it raises also has a copy of the payload that was returned upstream in case you really need to inspect it.

I've used this previously and found the approach works very well.
After all anything that is not within the 200 range is pretty much an error and so should be treated as an exception, right?
This allows such cases to be handled with standard error flows without having to deal with and inspect result objects for errors. nor having any callbacks.

[apotonick]

Cool, loving it! So, raise an exception only for specific return statuses?

[paulccarey]

Yeah, well you raise a specific exception for any non 200 responses.

This is how rest-client does it...
https://github.com/rest-client/rest-client/blob/master/lib/restclient/exceptions.rb

[apotonick]

Stupid question, but why do you use Roar and not rest-client? Are you using it for both rendering and consuming (server- or client-side? Or both?)?

[paulccarey]

Theres no such thing as a stupid question...

We are using roar on both client and server side. So we are sharing the representers in both the client and server. So we ensure we have the same Serialization on both ends.

[fwolfst]

Perfect, that's what mechanize does, too (throwing exception on non-200s, and the exception carrying the actual response, too).

[apotonick]

@paulccarey Cool, that's exactly why I wrote Representers in the first place - no redundancy!

Let me get a quick overview about this error handling today, then we (I mean, you) can start working on this.

[fwolfst]

As far as I can see, the faraday path already behaves similarly (in test: https://github.com/apotonick/roar/blob/master/test/http_verbs_test.rb#L57).

[paulccarey]

Thats interesting. I'm going to be working on this again today. It would be ideal if both faraday and net http worked the same really, that will be my aim however as our use case is net http at present this will be my focus im afraid.

Hopefully I should have a PR later today.

[fwolfst]

Mine too, I just started on some tests for the behaviour, still have 1 minutes to code before meeting ...
Its along these lines (in test/http_verbs_test.rb):

describe "non-success response handling" do
      it 'for all verbs, raises exception with response, when server replies with code other than 2xx' do
         verbs do |verb|
           # TODO: assert response content
            assert_raises(::Faraday::Error::ResourceNotFound) do
              @band.send(verb, :uri => "http://localhost:4567/bands/tesla", :as => "application/json")
             end
           end
       end
 end

[paulccarey]

ok cool. I'm not quite upto that just yet.

I'm currently creating some internal error classes using YAML from the following gist.
https://gist.github.com/paulccarey/e422a3efc7e55daff6a4

I'll dynamically create exception classes for all other than the 200 (success) responses. I'll also have a mapping (or hash) of response codes to exception classes.

Once I've done this then I was essentially going to inspect the responses http code that is returned and check if it matches one of the error class codes and raise that class if it does.

something like...

HTTP_STATUS_TO_ERROR_MAPPINGS = {404: NotFoundError, 422: UnprocessableEntityError }
....

http_error_klass = HTTP_STATUS_TO_ERROR_MAPPINGS[response.http_code]
raise http_error_klass.new unless http_error_klass.nill?

[fwolfst]

@paulccarey Arent such classes already be defined somewhere in net::http (e.g. http://stackoverflow.com/questions/6298281/net-http-get-source-code-and-status). I do not think that roar consumers need to control flow based on the exception raised. I would just throw a exception of i.e. type "Roar::BadHTTPResponse" - probably with a better name - and let it expose the original response (which contains the status code, too).

[paulccarey]

Hey @fwolfst

For my use case I really need to know what type of exception has been raised. If its a 404 then thats ok and I know that I need to return nil back from my client. If something else happens then something really bad has happened and I just let that error bubble up the stack.

By defining an error hierarchy as I've described we can have a standard set of self describing errors across different transport mechanisms and allow switching of transports without having to make changes to any consumers. Roar consumers only need to be concerned with roar errors and behaviour and are shielded from having to know roars internals or dig into net http or faraday.

[fwolfst]

@paulccarey Fair enough. My initial proposal also includes that certain error codes can be ignored (i.e. no exception raised and response consumed normally), as I discovered some services that always returned 400 (or 404), but actually processed successfully (and even sent a correct body anyways). But I do not see any issue in integrating that in your approach. Thanks for your ambitions!

[paulccarey]

Hey @fwolfst
No problem.

I'd argue that if a service is returning a 400/404 then its telling you something is wrong. However as you point out thats not to say that it cant still return a valid payload that can be parsed and dealt with...

I think so long as we expose the payload in the exception then these payloads can still be used.

Perhaps a progression in the context of roar is to allow certain response codes to be swallowed (or handled) and for the standard roar response handling and serialization to be used if you still expect the end-point to render a resource. I'll see how easy it would be to allow for that too if we think it would be beneficial?

[fwolfst]

@paulccarey In my case the 404 indicated that the service implementation or server configuration was wrong :) Yes, I meant let roar consume the payload (really in analogy to mechanize).

[apotonick]

Hey guys, can you give me a heads-up, is #109 still the way to go? I am about to release roar-1.0.0.beta1 and this excellent work will defo go into this release!