diff --git a/.rubocop.yml b/.rubocop.yml index 030e01f5..f97227e9 100644 --- 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 index 8a22476a..b1921b5c 100644 --- 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 index 2c3bf6f3..2d0aac55 100644 --- 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 index bcc0e03a..7cf2e7fe 100644 --- 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 index 81848117..e5c3cc67 100644 --- 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 index 75ed9c0e..5defde52 100644 --- 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 index 0258598a..515b8aa7 100644 --- 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 index d8a43119..b9db8c38 100644 --- 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 new file mode 100644 index 00000000..2c35817e --- /dev/null +++ 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 index 6202b3a4..c091c881 100644 --- 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 deleted file mode 100644 index 518c2d33..00000000 --- a/spec/slack/web/api/errors/service_unavailable_spec.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe Slack::Web::Client do - let(:client) { described_class.new } - - it 'raises a Faraday::ClientError when Slack is unavailable', - vcr: { cassette_name: 'web/503_error' } do - begin - client.auth_test - raise 'Expected to receive Faraday::ServerError.' - rescue Faraday::ServerError => e - expect(e.response).not_to be_nil - expect(e.response[:status]).to eq 503 - end - end -end diff --git a/spec/slack/web/client_spec.rb b/spec/slack/web/client_spec.rb index fba50b74..6edc3d1e 100644 --- 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: '') } + + it 'raises ParsingError' do + expect { request }.to raise_error(Slack::Web::Api::Errors::ParsingError).with_message('parsing_error') + expect(exception.response.body).to eq('') + 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 index ee352a94..9a99cbe1 100644 --- 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 }