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.

Sign up for GitHub

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

Jump to bottom

Slack 500 page error handling #306

Closed
jcheatham opened this issue Dec 6, 2019 · 5 comments
Closed

Slack 500 page error handling #306

jcheatham opened this issue Dec 6, 2019 · 5 comments
Labels
confirmed bug new feature

Comments

@jcheatham
Copy link

Hi! In the process of interacting with Slack's API, it sometimes has the occasion to return an html 500 status page which then causes a Faraday::ParsingError with something like

unexpected token at '<!DOCTYPE html> <html lang="en"> <head> <meta charset="utf-8"> <title>Server Error | Slack</title> <meta name="author" content="Slack"> etc.

Since the gem is speccing out its own error namespace, it feels like it'd be more consistent to handle and re-raise something in the Slack::Web::Api::Error domain instead. Thanks!
@dblock
Copy link
Collaborator

dblock commented Dec 6, 2019

Yes, indeed. On a 500 we should be reporting something other than a parsing error. Please contribute?

@Deconstrained
Copy link

@dblock I've been looking into where in the stack/middleware to implement a rescue/raise for this, and so far the most straightforward solution is in Slack::Web::Faraday::Request.request.

Do you agree with this approach?

@dblock
Copy link
Collaborator

dblock commented Dec 7, 2019

Usually these things are handled by middleware, eg. https://github.com/slack-ruby/slack-ruby-client/blob/master/lib/slack/web/faraday/response/raise_error.rb

Start by writing a test and solving the problem in any way you can. We can improve upon it.

@Deconstrained
Copy link

I had been thinking along those lines as well, but it has been challenging to grok the order of middleware execution.

So, in this PR I built a test and explored a non-rescue method of detecting an invalid response and raising. Please let me know what you think:

#307

@ojab ojab mentioned this issue Oct 12, 2020
Merged
@dblock
Copy link
Collaborator

dblock commented Nov 1, 2020

Implemented in #350

@dblock dblock closed this as completed Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed bug new feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dblock @jcheatham @Deconstrained