slack-ruby / slack-ruby-client

A Ruby and command-line client for the Slack Web, Real Time Messaging and Event APIs.
MIT License
1.21k stars 214 forks source link

Update Faraday to >= 2.0 #406

Closed schinery closed 2 years ago

schinery commented 2 years ago

First attempt at updating Faraday to use >= 2.0, which for the most part was fairly straightforward with the following exceptions:

1: Updating lib/slack/web/faraday/response/raise_error.rb

The spec in spec/slack/web/client_spec.rb:281 was failing with the following error:

1) Slack::Web::Client global config server failures parsing error raises ParsingError
     Failure/Error: expect { request }.to raise_error(Slack::Web::Api::Errors::ParsingError).with_message('parsing_error')

       expected Slack::Web::Api::Errors::ParsingError with "parsing_error", got #<NoMethodError: undefined method `map' for nil:NilClass

                     body['error'] || body['errors'].map { |message| message['error'] }.join(',')
                                                    ^^^^> with backtrace:
         # ./lib/slack/web/faraday/response/raise_error.rb:18:in `on_complete'
         # ./lib/slack/web/faraday/response/raise_error.rb:26:in `call'
         # ./lib/slack/web/faraday/request.rb:25:in `request'
         # ./lib/slack/web/faraday/request.rb:11:in `post'
         # ./lib/slack/web/api/endpoints/api.rb:17:in `api_test'
         # ./spec/slack/web/client_spec.rb:269:in `block (4 levels) in <top (required)>'
         # ./spec/slack/web/client_spec.rb:282:in `block (6 levels) in <top (required)>'
         # ./spec/slack/web/client_spec.rb:282:in `block (5 levels) in <top (required)>'
     # ./spec/slack/web/client_spec.rb:282:in `block (5 levels) in <top (required)>'

Finished in 10.54 seconds (files took 1.93 seconds to load)
461 examples, 1 failure, 8 pending

Failed examples:

rspec ./spec/slack/web/client_spec.rb:281 # Slack::Web::Client global config server failures parsing error raises ParsingError

I believe this is caused by there being no middleware equivalent to ::FaradayMiddleware::ParseJson, and the new :json request/response middleware appears to only parse (and throw ::Faraday::ParsingError) if the content type is application/json.

So, I've added some code to check if the env.body has been converted into Hash or not, and attempt to convert if it isn't.

It does raise the question does ::Faraday::ParsingError need to rescued somewhere else?

2: Not being able to use slack-ruby-danger

The danger gem currently doesn't support Faraday 2.0 so for the moment I've commented it out. It has been mentioned about pulling it into its own Gemfile.

schinery commented 2 years ago

Worth noting, I tried running the specs with the CONCURRENCY and SLACK_API_TOKEN set but was having issues with the Slack token. The rest of the specs ran successfully, so fingers crossed these will run ok with the correct setup.

dblock commented 2 years ago

I believe this is caused by there being no middleware equivalent to ::FaradayMiddleware::ParseJson, and the new :json request/response middleware appears to only parse (and throw ::Faraday::ParsingError) if the content type is application/json.

That makes a lot of sense. Change the spec to supply application/json.

So, I've added some code to check if the env.body has been converted into Hash or not, and attempt to convert if it isn't.

It's probably good to be defensive and not reach into the hash unless it's a hash, but don't attempt to convert it. Add a separate spec with the correct content-type and with a different one.

It does raise the question does ::Faraday::ParsingError need to rescued somewhere else?

Probably yes, and raise Slack::Web::Api::Errors::ParsingError that would wrap that one?

2: Not being able to use slack-ruby-danger

The danger gem currently doesn't support Faraday 2.0 so for the moment I've commented it out. It has been mentioned about pulling it into its own Gemfile.

Yep, I did it in https://github.com/slack-ruby/slack-ruby-bot-server/pull/142/files#diff-9ed6d3c8f52cf7b6862dd9628741eda3dbcd3683b1485977c483b6571e989904 if you just want to copy-paste.

schinery commented 2 years ago

@dblock I'll look at updating based on your above comments when I get a moment 👍🏻

Another thing, I see that the linting failed because newly added faraday-typhoeus gem only supports Ruby >= 2.7.0. As Ruby 2.5 has reached it's EOL and Ruby 2.6 does this month, should the new version of this Slack client be >= 2.7?

schinery commented 2 years ago

Also update CHANGELOG, README, and remove incompatible versions of Ruby from CI.

Ah this answers my Ruby version question above 👍🏻

schinery commented 2 years ago

@dblock I think this is ready to be re-reviewed as I've...

One thing, I couldn't see what might need updating in the README except maybe updating the Stable Release section. Let me know if this is what you meant.

dblock commented 2 years ago

Btw, if you are bored, I have a few more clients that could use some love of upgrading faraday, https://github.com/dblock/strava-ruby-client, https://github.com/dblock/iex-ruby-client, and https://github.com/dblock/open-weather-ruby-client.