Merge pull request #350 from ojab/master

dblock · web-flow · commit c320e8f97caa · 2020-11-01T17:50:55.000-05:00
Handle server errors such as timouts &amp; non-json responses
diff --git a/.rubocop.yml b/.rubocop.yml
@@ -18,7 +18,7 @@ Layout/EmptyLineAfterMagicComment:
   Enabled: false
 
 Metrics/BlockLength:
-  Max: 250
+  Enabled: false
 
 Metrics/ClassLength:
   Max: 250
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,7 @@
-### 0.15.2 (Next)
+### 0.16.0 (Next)
 
 * [#348](https://github.com/slack-ruby/slack-ruby-client/pull/348): Added `admin_conversations_archive`, `admin_conversations_convertToPrivate`, `admin_conversations_create`, `admin_conversations_delete`, `admin_conversations_disconnectShared`, `admin_conversations_getConversationPrefs`, `admin_conversations_getTeams`, `admin_conversations_invite`, `admin_conversations_rename`, `admin_conversations_search`, `admin_conversations_setConversationPrefs`, `admin_conversations_unarchive`, `admin_conversations_ekm_listOriginalConnectedChannelInfo`, `admin_users_session_invalidate`, `apps_event_authorizations_list`, `conversations_mark`, `workflows_stepCompleted`, `workflows_stepFailed`, `workflows_updateStep` endpoints - [@wasabigeek](https://github.com/wasabigeek).
+* [#350](https://github.com/slack-ruby/slack-ruby-client/pull/350): Handle server errors such as timouts & non-json responses (see [Upgrading to 0.16.0](UPGRADING.md#upgrading-to--0160)) - [@ojab](https://github.com/ojab).
 * Your contribution here.
 
 ### 0.15.1 (2020/9/3)
diff --git a/README.md b/README.md
@@ -326,7 +326,11 @@ If you exceed [Slack’s rate limits](https://api.slack.com/docs/rate-limits), a
 
 ##### Other Errors
 
-In any other case, a `Faraday::ClientError` will be raised. This may be the case if Slack is temporarily unavailable, for example.
+In case of Slack temporarily unavailability a `Slack::Web::Api::Errors::ServerError` (`Slack::Web::Api::Errors::SlackError` subclass) subclasses will be raised and original `Faraday::Error` will be accesible via `exception.cause`.
+
+Specifically `Slack::Web::Api::Errors::ParsingError` will be raised on non-json response (i. e. 200 OK with `Slack unavailable` HTML page) and `Slack::Web::Api::Errors::HttpRequestError` subclasses for connection failures (`Slack::Web::Api::Errors::TimeoutError` for read/open timeouts & `Slack::Web::Api::Errors::UnavailableError` for 5xx HTTP responses).
+
+In any other case, a `Faraday::ClientError` will be raised.
 
 ### RealTime Client
 
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -1,6 +1,12 @@
 Upgrading Slack-Ruby-Client
 ===========================
 
+### Upgrading to >= 0.16.0
+
+As of 0.16.0 `Faraday::Error` exceptions sans `Faraday::ClientError` are wrapped into `Slack::Web::Api::Errors::ServerError`, so if you're rescuing `Faraday::Error` — you should adjust your code to use `Slack::Web::Api::Errors::ServerError` and use `exception.cause` if underlying `Faraday::Error` is needed.
+
+See [README#other-errors](README.md#other-errors) and [#350](https://github.com/slack-ruby/slack-ruby-client/pull/350) for more information.
+
 ### Upgrading to >= 0.15.0
 
 As of 0.15.0, `activesupport` is no longer required. Add `gem 'activesupport'` to your Gemfile if you required ActiveSupport via this library.
@@ -130,5 +136,3 @@ gem 'celluloid-io'
 When in doubt, use `faye-websocket`.
 
 See [#5](https://github.com/slack-ruby/slack-ruby-client/issues/5) for more information.
-
-
diff --git a/lib/slack-ruby-client.rb b/lib/slack-ruby-client.rb
@@ -29,6 +29,7 @@
 require_relative 'slack/web/api/error'
 require_relative 'slack/web/api/errors'
 require_relative 'slack/web/faraday/response/raise_error'
+require_relative 'slack/web/faraday/response/wrap_error'
 require_relative 'slack/web/faraday/connection'
 require_relative 'slack/web/faraday/request'
 require_relative 'slack/web/api/mixins'
diff --git a/lib/slack/web/api/errors.rb b/lib/slack/web/api/errors.rb
@@ -161,6 +161,11 @@ class HandleAlreadyExists < SlackError; end
         class HashConflict < SlackError; end
         class InactiveCall < SlackError; end
         class InternalError < SlackError; end
+        class ServerError < InternalError; end
+        class ParsingError < ServerError; end
+        class HttpRequestError < ServerError; end
+        class TimeoutError < HttpRequestError; end
+        class UnavailableError < HttpRequestError; end
         class InvalidActor < SlackError; end
         class InvalidAppId < SlackError; end
         class InvalidArgName < SlackError; end
diff --git a/lib/slack/web/faraday/connection.rb b/lib/slack/web/faraday/connection.rb
@@ -24,7 +24,7 @@ def connection
               ::Faraday::Connection.new(endpoint, options) do |connection|
                 connection.use ::Faraday::Request::Multipart
                 connection.use ::Faraday::Request::UrlEncoded
-                connection.use ::Faraday::Response::RaiseError
+                connection.use ::Slack::Web::Faraday::Response::WrapError
                 connection.use ::Slack::Web::Faraday::Response::RaiseError
                 connection.use ::FaradayMiddleware::Mashify, mash_class: Slack::Messages::Message
                 connection.use ::FaradayMiddleware::ParseJson
diff --git a/lib/slack/web/faraday/response/raise_error.rb b/lib/slack/web/faraday/response/raise_error.rb
@@ -6,7 +6,12 @@ module Response
         class RaiseError < ::Faraday::Response::Middleware
           def on_complete(env)
             raise Slack::Web::Api::Errors::TooManyRequestsError, env.response if env.status == 429
-            return unless (body = env.body) && !body['ok']
+
+            return unless env.success?
+
+            body = env.body
+            return unless body
+            return if body['ok']
 
             error_message =
               body['error'] || body['errors'].map { |message| message['error'] }.join(',')
@@ -15,6 +20,16 @@ def on_complete(env)
             error_class ||= Slack::Web::Api::Errors::SlackError
             raise error_class.new(error_message, env.response)
           end
+
+          def call(env)
+            super
+          rescue Slack::Web::Api::Errors::SlackError, Slack::Web::Api::Errors::TooManyRequestsError
+            raise
+          rescue ::Faraday::ParsingError
+            raise Slack::Web::Api::Errors::ParsingError.new('parsing_error', env.response)
+          rescue ::Faraday::TimeoutError, ::Faraday::ConnectionFailed
+            raise Slack::Web::Api::Errors::TimeoutError.new('timeout_error', env.response)
+          end
         end
       end
     end
diff --git a/lib/slack/web/faraday/response/wrap_error.rb b/lib/slack/web/faraday/response/wrap_error.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+module Slack
+  module Web
+    module Faraday
+      module Response
+        class WrapError < ::Faraday::Response::RaiseError
+          def on_complete(env)
+            super
+          rescue Slack::Web::Api::Errors::SlackError
+            raise
+          rescue ::Faraday::ServerError
+            raise Slack::Web::Api::Errors::UnavailableError.new('unavailable_error', env.response)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/slack/real_time/client_spec.rb b/spec/slack/real_time/client_spec.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 require 'spec_helper'
 
-RSpec.describe Slack::RealTime::Client do # rubocop:disable Metrics/BlockLength
+RSpec.describe Slack::RealTime::Client do
   let(:ws) { double(Slack::RealTime::Concurrency::Mock::WebSocket, on: true) }
 
   before do
diff --git a/spec/slack/web/api/errors/service_unavailable_spec.rb b/spec/slack/web/api/errors/service_unavailable_spec.rb
diff --git a/spec/slack/web/client_spec.rb b/spec/slack/web/client_spec.rb
@@ -275,5 +275,61 @@
         end
       end
     end
+
+    context 'server failures' do
+      subject(:request) { client.api_test }
+
+      let(:stub_slack_request) { stub_request(:post, 'https://slack.com/api/api.test') }
+      let(:exception) do
+        begin
+          request
+        rescue Slack::Web::Api::Errors::ServerError => e
+          return e
+        end
+      end
+
+      context 'parsing error' do
+        before {  stub_slack_request.to_return(body: '<html></html>') }
+
+        it 'raises ParsingError' do
+          expect { request }.to raise_error(Slack::Web::Api::Errors::ParsingError).with_message('parsing_error')
+          expect(exception.response.body).to eq('<html></html>')
+          expect(exception.cause).to be_a(Faraday::ParsingError)
+          expect(exception.cause.cause.cause).to be_a(JSON::ParserError)
+        end
+      end
+
+      context 'timeout' do
+        context 'open timeout' do
+          before { stub_slack_request.to_timeout }
+
+          it 'raises TimoutError' do
+            expect { request }.to raise_error(Slack::Web::Api::Errors::TimeoutError).with_message('timeout_error')
+            expect(exception.cause).to be_a(Faraday::ConnectionFailed)
+            expect(exception.cause.cause).to be_a(Net::OpenTimeout)
+          end
+        end
+
+        context 'read timeout' do
+          before { stub_slack_request.to_raise(Net::ReadTimeout) }
+
+          it 'raises TimeoutError' do
+            expect { request }.to raise_error(Slack::Web::Api::Errors::TimeoutError).with_message('timeout_error')
+            expect(exception.cause).to be_a(Faraday::TimeoutError)
+            expect(exception.cause.cause).to be_a(Net::ReadTimeout)
+          end
+        end
+      end
+
+      context '5xx response' do
+        before { stub_slack_request.to_return(status: 500, body: '{}') }
+
+        it 'raises UnavailableError' do
+          expect { request }.to raise_error(Slack::Web::Api::Errors::UnavailableError).with_message('unavailable_error')
+          expect(exception.cause).to be_a(Faraday::ServerError)
+          expect(exception.response.status).to eq(500)
+        end
+      end
+    end
   end
 end
diff --git a/spec/slack/web/faraday/response/raise_error_spec.rb b/spec/slack/web/faraday/response/raise_error_spec.rb
@@ -2,14 +2,15 @@
 require 'spec_helper'
 
 RSpec.describe Slack::Web::Faraday::Response::RaiseError do
-  subject(:raise_error_obj) { described_class.new }
+  subject(:raise_error_obj) { described_class.new(app) }
 
-  describe '#on_complete' do
-    let(:status) { 200 }
-    let(:response) { nil }
-    let(:body) { {} }
-    let(:env) { double status: status, response: response, body: body }
+  let(:app) { proc {} }
+  let(:status) { 200 }
+  let(:response) { nil }
+  let(:body) { {} }
+  let(:env) { double status: status, response: response, body: body, success?: true }
 
+  describe '#on_complete' do
     context 'with status of 429' do
       let(:status) { 429 }
 
