Display the error message when we have one

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.

Author: Jon Yurek

lib/cc/service/invocation/with_error_handling.rb

@@ -15,13 +15,15 @@ def call
         params: e.params,
         status: e.status,
         endpoint_url: e.endpoint_url,
-        message: error_message(e)
+        message: e.user_message || e.message,
+        log_message: error_message(e)
       }
     rescue => e
       @logger.error(error_message(e))
       {
         ok: false,
-        message: error_message(e)
+        message: e.message,
+        log_message: error_message(e)
       }
     end
 

lib/cc/service/response_check.rb

@@ -1,6 +1,7 @@
 class CC::Service
   class HTTPError < StandardError
     attr_reader :response_body, :status, :params, :endpoint_url
+    attr_accessor :user_message
 
     def initialize(message, env)
       @response_body = env[:body]

lib/cc/services/asana.rb

@@ -25,6 +25,10 @@ def receive_test
     result.merge(
       message: "Ticket <a href='#{result[:url]}'>#{result[:id]}</a> created."
     )
+  rescue CC::Service::HTTPError => ex
+    body = JSON.parse(ex.response_body)
+    ex.user_message = body["errors"].map{|e| e["message"] }.join(" ")
+    raise ex
   end
 
 

lib/cc/services/github_issues.rb

@@ -24,6 +24,10 @@ def receive_test
     result.merge(
       message: "Issue <a href='#{result[:url]}'>##{result[:number]}</a> created."
     )
+  rescue CC::Service::HTTPError => e
+    body = JSON.parse(e.response_body)
+    e.user_message = body["message"]
+    raise e
   end
 
   def receive_quality

test/invocation_error_handling_test.rb

@@ -30,7 +30,8 @@ def test_http_errors_return_relevant_data
     assert_equal 401, result[:status]
     assert_equal "params", result[:params]
     assert_equal "url", result[:endpoint_url]
-    assert_equal "Exception invoking service: [prefix] (CC::Service::HTTPError) foo. Response: <nil>", result[:message]
+    assert_equal "foo", result[:message]
+    assert_equal "Exception invoking service: [prefix] (CC::Service::HTTPError) foo. Response: <nil>", result[:log_message]
   end
 
   def test_error_returns_a_hash_with_explanations
@@ -44,6 +45,7 @@ def test_error_returns_a_hash_with_explanations
 
     result = handler.call
     assert_equal false, result[:ok]
-    assert_equal "Exception invoking service: [prefix] (ArgumentError) lol", result[:message]
+    assert_equal "lol", result[:message]
+    assert_equal "Exception invoking service: [prefix] (ArgumentError) lol", result[:log_message]
   end
 end

test/invocation_test.rb

@@ -12,7 +12,7 @@ def test_success
 
   def test_retries
     service = FakeService.new
-    service.raise_on_receive = true
+    service.fake_error = RuntimeError.new
     error_occurred = false
 
     begin
@@ -41,7 +41,7 @@ def test_metrics
   def test_metrics_on_errors
     statsd = FakeStatsd.new
     service = FakeService.new
-    service.raise_on_receive = true
+    service.fake_error = RuntimeError.new
     error_occurred = false
 
     begin
@@ -57,31 +57,45 @@ def test_metrics_on_errors
     assert_match /^services\.errors\.a_prefix/, statsd.incremented_keys.first
   end
 
+  def test_user_message
+    service = FakeService.new
+    service.fake_error = CC::Service::HTTPError.new("Boom", {})
+    service.override_user_message = "Hey do this"
+    logger = FakeLogger.new
+
+    result = CC::Service::Invocation.invoke(service) do |i|
+      i.with :error_handling, logger, "a_prefix"
+    end
+
+    assert_equal "Hey do this", result[:message]
+    assert_match /Boom/, result[:log_message]
+  end
+
   def test_error_handling
     service = FakeService.new
-    service.raise_on_receive = true
+    service.fake_error = RuntimeError.new("Boom")
     logger = FakeLogger.new
 
     result = CC::Service::Invocation.invoke(service) do |i|
       i.with :error_handling, logger, "a_prefix"
     end
 
-    assert_equal({ok: false, message: "Exception invoking service: [a_prefix] (RuntimeError) Boom"}, result)
+    assert_equal({ok: false, message: "Boom", log_message: "Exception invoking service: [a_prefix] (RuntimeError) Boom"}, result)
     assert_equal 1, logger.logged_errors.length
     assert_match /^Exception invoking service: \[a_prefix\]/, logger.logged_errors.first
   end
 
   def test_multiple_middleware
     service = FakeService.new
-    service.raise_on_receive = true
+    service.fake_error = RuntimeError.new("Boom")
     logger = FakeLogger.new
 
     result = CC::Service::Invocation.invoke(service) do |i|
       i.with :retries, 3
       i.with :error_handling, logger
     end
 
-    assert_equal({ok: false, message: "Exception invoking service: (RuntimeError) Boom"}, result)
+    assert_equal({ok: false, message: "Boom", log_message: "Exception invoking service: (RuntimeError) Boom"}, result)
     assert_equal 1 + 3, service.receive_count
     assert_equal 1, logger.logged_errors.length
   end
@@ -90,7 +104,7 @@ def test_multiple_middleware
 
   class FakeService
     attr_reader :receive_count
-    attr_accessor :raise_on_receive
+    attr_accessor :fake_error, :override_user_message
 
     def initialize(result = nil)
       @result = result
@@ -100,8 +114,13 @@ def initialize(result = nil)
     def receive
       @receive_count += 1
 
-      if @raise_on_receive
-        raise "Boom"
+      begin
+        raise @fake_error if @fake_error
+      rescue => e
+        if override_user_message
+          e.user_message = override_user_message
+        end
+        raise e
       end
 
       @result