Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NoMethodError when Slack is unavailable #188

Merged
merged 1 commit into from
Dec 19, 2017

Conversation

sonicdoe
Copy link
Contributor

When Slack is experiencing connectivity issues and returns a 503 Service Unavailable, slack-ruby-client should throw a Faraday::ClientError. However, due to

raise Slack::Web::Api::Errors::SlackError.new(body['error'], env.response)
a NoMethodError: undefined method `[]' for nil:NilClass is raised (since body is nil). The same error was also reported in #32.

I haven’t looked into an actual fix yet but I figured submitting a failing test would help 🙂

@dblock
Copy link
Collaborator

dblock commented Nov 30, 2017

Good call! We generally need to handle 50x error codes differently from 40x error codes that contain a JSON body.

@sonicdoe
Copy link
Contributor Author

sonicdoe commented Dec 5, 2017

By the way, that’s also on the roadmap for Faraday v1.0 (see lostisland/faraday#735).

@sonicdoe sonicdoe changed the title Add failing test for 503 Service Unavailable Fix NoMethodError when Slack is unavailable Dec 6, 2017
@sonicdoe
Copy link
Contributor Author

sonicdoe commented Dec 6, 2017

I’ve now fixed the NoMethodError by changing the logic in raise_error.rb. It now raises a SlackError only if body['ok'] is false. If body is nil, the response will fall through to ::Faraday::Response::RaiseError which will then raise a Faraday::ClientError.

@dblock
Copy link
Collaborator

dblock commented Dec 7, 2017

Perfect, care to rebase this please and I'll merge. Maybe describe error handling in README if you think it can be helpful to others, too.

Copy link
Collaborator

@jmanian jmanian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump

@dblock dblock merged commit 78f8a4a into slack-ruby:master Dec 19, 2017
@dblock
Copy link
Collaborator

dblock commented Dec 19, 2017

Thanks, sorry didn't get a notification on rebase.

@sonicdoe
Copy link
Contributor Author

Maybe describe error handling in README if you think it can be helpful to others, too.

I’ve now opened #190 which documents all of slack-ruby-client’s error handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants