Services return the HTTP call's status #52

jyurek · 2015-02-12T20:58:55Z

In order to properly record the data for ServiceEvents in codeclimate
proper, we need to have the calls to the external services return
something useful. This is the first step toward that goal.

marshally · 2015-02-12T21:00:57Z

/cc @calavera

calavera · 2015-02-17T04:28:54Z

It looks good. I guess you've already seen how to extend services to store data in codeclimate, but just in case, check the pull request service extension out: https://github.com/codeclimate/codeclimate/blob/master/lib/cc/services/extensions/github_pull_requests.rb

jyurek · 2015-02-17T16:35:37Z

I have seen that, but I think these things should happen in this gem and not in an extension. Right?

calavera · 2015-02-18T03:54:34Z

Right, the response still needs to be handled in the services gem.

calavera · 2015-02-18T03:55:25Z

lib/cc/service/http.rb

@@ -15,6 +15,34 @@ def default_http_options
    end
  end

+  def post(url, body = nil, headers = nil, &block)
+    begin


this begin is not necessary, either it is the end that goes with it.

JacobNinja · 2015-02-19T16:23:20Z

lib/cc/service/http.rb

+      endpoint_url: url,
+      exception_message: "#{e.class}: #{e.message}"
+    }
+  rescue Exception => e


This should probably only rescue StandardError type exceptions

Damnit, you're right. I'm usually the one catching this on others' code, too.

jyurek · 2015-02-19T20:34:48Z

The way this works is by moving all of the actual HTTP POSTing into a dedicated method that wraps the response and also handles failures. A successful request will be parsed for standard things like status code, the params we sent, the url we hit, and whether it worked or not. An HTTP error will also include the exception message. Any other error will forego the HTTP code. This is all tested and common to each service, so they don't need their own specific tests for it.

A successful request can be modified by adding extra attributes into the hash via a block. The response body is passed to the block (since we get both JSON and XML from various services). Relevant values can be extracted in this block. Parse errors will trigger the generic error handling and return a "this failed" message to the caller. The uniquely-added attributes are tested on a per-service basis.

Some services make multiple requests. Only the last one is cared about.

jeffrafter · 2015-02-23T18:31:15Z

lib/cc/service/http.rb

+      endpoint_url: url,
+      exception_message: "#{e.class}: #{e.message}"
+    }
+  rescue *RERAISE_ERRORS


This pattern is making me a little quesy-- can we remove this special casing for test errors and make the following rescue for StandardError more specific so it doesn't catch test assertion errors?

jyurek · 2015-03-02T18:25:26Z

The places where simple_failure exists in this change, we simply weren't doing anything. The case didn't exist, and so we returned nil -- no bugsnag. Since we want to return ok: true/false in all cases, now, the case needed to be filled in.

noahd1 · 2015-03-02T18:26:25Z

lib/cc/services/asana.rb

+  rescue CC::Service::HTTPError => ex
+    body = JSON.parse(ex.response_body)
+    ex.user_message = body["errors"].map{|e| e["message"] }.join(" ")
+    raise ex


A bit confusing to me - where does this exception get handled? Wonder if the ErrorHandling middleware is encouraging handling logic through exceptions.

It's no so much logic as it is that we need to augment the exception. There's information that the receive_test methods need to put into the replies that can't be handled any other way. That's why we're re-raising the error -- the WithErrorHandling middleware handles the errors just fine, but the since the receive_test handlers are user-facing, they want/need to put pretty messages on the data returned.

It's possible we also want to handle this in the web-side, though that will be a bit more complicated.

noahd1 · 2015-03-02T19:11:05Z

Questions:

Since all the services are changing in some way, have we / can we mitigate the risk of this release by end to end testing as much as possible? Having good unit tests is necessary but probably not sufficient in this case.
I'm a bit uncomfortable with how and where our "response" (the hash) is generated -- in http.rb, with_error_handling.rb and then mangled further by individual services. There'll always be some degree of per-service customization of our responses, but wondering if we can minimize the distinction between a successful and unsuccessful response -- aka, is there a world where we can generate the success/failure responses in the same class, maybe by making with_error_handling.rb more generic - like a response_generator.rb?
Related to above, failure cases being handled with exceptions (WithErrorHandling) right now, but they aren't super exceptional: if a user commits a bad "token" for a service, we expect an error back from the service.

/cc @jeffrafter

jyurek · 2015-03-02T19:32:21Z

I'm not sure what more is expected of these tests. Do you want them to actually hit the network? That code isn't really being touched. Only the replies are cared about. I'm all for integration testing, but without hitting the network or using something like VCR we're unable to get any more visibility.
To be honest, I'm not in love with it either, but I'm not entirely sure how we could have the WithErrorHandling class access data that a successful HTTP post gets from the server. Even if we wanted to do such a thing, I think it would require odd layer-crossings and make the code be less understandable. We could prevent mangling the hash by the receive_test handlers by letting the thing calling the handler mangle the result, but that would need to either be super general (and therefore less useful) or make the code a nest of if statements.
We've chosen to treat any non-2XX status code as an error to be raised. We can rescue certain errors in the handlers themselves and build responses for things like 401 and 422, but this will be overshadowed by those cases (see GitHubPullRequestService) where we need to see those in the raw. While I believe there is a better way to handle things, I feel as though they're outside the scope of this change, which was to give the services useful responses when their event handlers are hit.

noahd1 · 2015-03-03T04:01:37Z

We can discuss these more tomorrow.

Yeah, wasn't commenting on the automated test suite which as far as I can tell is quite good. I more meant that in cases like this we still need to be careful and probably do manual QA and/or look at other strategies to minimize risk.
Sorry, I misspoke, I didn't mean WithErrorHandling which I agree isn't really a place for this kind of generic behavior -- I meant producing an kind of response object in a Faraday middleware, aka, replacing ResponseCheck. Downside would be you would probably have to stick the resulting object back in the env to have it be available later
I agree there's some changes that are outside the scope of this PR. Just trying to find a balance here. Easier to chat about in person.

noahd1 · 2015-03-03T04:05:52Z

lib/cc/service/http.rb

+      endpoint_url: url,
+      status: response.status,
+      message: "Success"
+    }.merge(block.call(response))


After looking at the code more, I now see that post returns a hash but get returns, as it did before, some kind of http response object. I think this mismatch in the API is confusing, which is why I'm trying to explore if there's a better place for "response generation" that exists outside of this class. See main thread.

Yes, a GET should generate the same kind of response.

jeffrafter · 2015-03-03T17:10:31Z

lib/cc/services/hipchat.rb

-    speak(formatter.format_test, "green")
-
-    { ok: true, message: "Test message sent" }
-  rescue => ex


Is this rescue handled up the callstack? I think so... just confirming

Yup! See above. :)

In order to properly record the data for ServiceEvents in codeclimate proper, we need to have the calls to the external services return something useful. The way this works is by moving all of the actual HTTP POSTing into a dedicated method that wraps the response. Failures are handled by the `WithErrorHandling` invocation middleware as normal. A successful request will be parsed for standard things like status code, the params we sent, the url we hit, and whether it worked or not, and a message. Failures will have a message explaining the error. `receive_test` methods will include a user-friendly error message. Any other error will forego HTTP information. This is all tested and common to each service, so they don't need their own specific tests for it. A successful request can be modified by adding extra attributes into the hash via a block. The response body is passed to the block to be parsed inside it (since we get both JSON and XML from various services). Relevant values can be extracted in this block. Parse errors will trigger the generic error handling and return a "this failed" message to the caller. The uniquely-added attributes are tested on a per-service basis. Some services make multiple requests. The first one to fail is what we return an error message for. If none fail, it's all good.

Both Asana and Github provide a nicer error message when something goes wrong. If we get that response body, use it by supplying it as a supplementary error message. If we have this message, prefer it to the "real" exception's message. We must still send the message we log, so that it's saved along with the event's data.

Stubbing `POST`s in the main codeclimate repo gets confused with stubbing the #post method here. So, change this method.

Services return the HTTP call's status

calavera reviewed Feb 18, 2015
View reviewed changes

jyurek force-pushed the jy-http-calls-return-useful-data branch 2 times, most recently from 82b505e to d84ea34 Compare February 19, 2015 16:02

jyurek changed the title ~~The Asana service returns the HTTP call's status~~ Services return the HTTP call's status Feb 19, 2015

jyurek force-pushed the jy-http-calls-return-useful-data branch 2 times, most recently from f6d7999 to 66215e9 Compare February 19, 2015 16:06

JacobNinja reviewed Feb 19, 2015
View reviewed changes

jeffrafter reviewed Feb 23, 2015
View reviewed changes

jyurek force-pushed the jy-http-calls-return-useful-data branch 2 times, most recently from 69ba1b4 to 103c008 Compare February 25, 2015 18:47

jyurek self-assigned this Feb 25, 2015

noahd1 reviewed Mar 2, 2015
View reviewed changes

noahd1 reviewed Mar 3, 2015
View reviewed changes

jeffrafter reviewed Mar 3, 2015
View reviewed changes

Jon Yurek added 4 commits March 5, 2015 11:49

Give a not-ok result if there is no handler

6806316

Do not set the Content-Type when it wasn't before

26312cc

jyurek force-pushed the jy-http-calls-return-useful-data branch from 6407363 to 26312cc Compare March 5, 2015 16:50

Jon Yurek added 2 commits March 11, 2015 14:33

Do not use #to_json on HipChat's params

ad97e4c

Change the name of the #post method

a58e91d

Stubbing `POST`s in the main codeclimate repo gets confused with stubbing the #post method here. So, change this method.

noahd1 added a commit that referenced this pull request Mar 12, 2015

Merge pull request #52 from codeclimate/jy-http-calls-return-useful-data

8cd9a1d

Services return the HTTP call's status

noahd1 merged commit 8cd9a1d into master Mar 12, 2015

noahd1 deleted the jy-http-calls-return-useful-data branch March 12, 2015 14:43

jyurek removed their assignment Apr 13, 2015

Services return the HTTP call's status #52

Services return the HTTP call's status #52

Uh oh!

Conversation

jyurek commented Feb 12, 2015

Uh oh!

marshally commented Feb 12, 2015

Uh oh!

calavera commented Feb 17, 2015

Uh oh!

jyurek commented Feb 17, 2015

Uh oh!

calavera commented Feb 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyurek commented Feb 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyurek commented Mar 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahd1 commented Mar 2, 2015

Uh oh!

jyurek commented Mar 2, 2015

Uh oh!

noahd1 commented Mar 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!