Update Faraday to >= 2.0 #406

schinery · 2022-05-03T05:38:05Z

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 · 2022-05-03T06:16:38Z

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 · 2022-05-03T13:30:56Z

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.

dblock

Good progress. More comments below.

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

dblock · 2022-05-03T13:31:49Z

Gemfile

@@ -15,13 +15,15 @@ end
 group :test do
  gem 'activesupport'
  gem 'erubis'
+  gem 'faraday-typhoeus'


Is this becoming a gem dependency? Does it belong in .gemspec?

This was added as testing dependency because it was required for the 'non default adapter' spec in spec/slack/web/client_spec.rb:177 and the adapters are no longer bundled in.

dblock · 2022-05-03T13:32:51Z

lib/slack/web/faraday/response/raise_error.rb

            error_class = Slack::Web::Api::Errors::ERROR_CLASSES[error_message]
            error_class ||= Slack::Web::Api::Errors::SlackError
            raise error_class.new(error_message, env.response)
+          # TODO: Do we need to still handle ::Faraday::ParsingError or will this be handled else where?
+          # rescue ::Faraday::ParsingError


rescue it the same way as we rescued JSON::ParseError before, update Slack::Web::Api::Errors::ParsingError to also include the inner exception

This parse rescue has been moved to lib/slack/web/faraday/request.rb as the Faraday::ParsingError is now triggered when processing the response.

As part of this, and to handle non JSON responses such as HTML 500s, I've currently set content_type: /\b*$/ in the :json response options. The question is do you think this should be more restrictive and only parse certain known content types?

This means content-type can be anything, doesn't it? So what's the point in having it? I think we should just try to parse any response because the Slack API says it will always send JSON, except when it's broken, and that's when we need this.

dblock · 2022-05-03T13:33:13Z

lib/slack/web/faraday/response/raise_error.rb

+          # Need to check if the body has been parsed here as it will only be parsed by the Faraday JSON middleware if
+          # the response is content_type 'application/json'. This would of been handled automatically by the
+          # ::FaradayMiddleware::ParseJson but that is no longer available.
+          def process_body(env)


Sounds like this is no longer needed because Faraday does it.

Yeah this wasn't needed after all and has been removed.

dblock · 2022-05-03T13:34:05Z

spec/slack/web/client_spec.rb

@@ -280,8 +281,7 @@
        it 'raises ParsingError' do


Modify to include the correct content-type.

Write a test without the correct content-type, too, possibly changing expectations.

Updated the spec that checked for the HTML response and added one for malformed JSON.

There is the question above regarding correct content types as the new code will currently try and parse any content type returned, which I guess these two specs cover?

schinery · 2022-05-03T13:36:22Z

@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 · 2022-05-03T13:38:22Z

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

Ah this answers my Ruby version question above 👍🏻

dblock · 2022-05-03T16:16:14Z

CHANGELOG.md

@@ -3,7 +3,9 @@
 * [#400](https://github.com/slack-ruby/slack-ruby-client/pull/400): Replace Travis-CI with GitHub Actions - [@dblock](https://github.com/dblock).
 * [#401](https://github.com/slack-ruby/slack-ruby-client/pull/401): Upgraded RuboCop to 1.26.0 - [@dblock](https://github.com/dblock).
 * [#401](https://github.com/slack-ruby/slack-ruby-client/pull/401): Added support for Ruby 3.1.1 - [@dblock](https://github.com/dblock).
-* Your contribution here.
+* [#406](https://github.com/slack-ruby/slack-ruby-client/pull/406): Update Faraday to `>= 2.0` - [@schinery](https://github.com/slack-ruby/slack-ruby-client/pull/406).


Let's bump the version to 1.1 (version.rb, etc.)

schinery · 2022-05-04T08:27:04Z

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

Resolved the JSON parsing stuff previously discussed. There is this question which based on the answer might need some additional changes
Created Gemfile.danger and set the GitHub action to use that so Faraday 1.0 doesn't affect the main bundle install
Updated the CHANGELOG, although let me know if I've gone a bit overboard here

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

I think we're almost there! Made a bunch of minor comments and try to address the content-type one below.

dblock · 2022-05-04T17:30:58Z

.github/workflows/test.yml

-          - { ruby: 2.7.1, concurrency: async-websocket }
-          - { ruby: 3.0.2 }
-          - { ruby: 3.1.1 }
+          - { ruby: 2.7 }


Minor: quote all rubies so we don't make the 3.0 being a float mistake

dblock · 2022-05-04T17:31:28Z

CHANGELOG.md

+* [#406](https://github.com/slack-ruby/slack-ruby-client/pull/406): Upgraded RuboCop to 1.26.1 and lock to version - [@schinery](https://github.com/slack-ruby/slack-ruby-client/pull/406).
+* [#406](https://github.com/slack-ruby/slack-ruby-client/pull/406): Updated Danger PR Linter GitHub action to use separate `Gemfile`  - [@schinery](https://github.com/slack-ruby/slack-ruby-client/pull/406).
+* Your contribution here.
+
 ### 1.0.1 (Next)


This hasn't shipped, merge with the above.

dblock · 2022-05-04T17:31:44Z

CHANGELOG.md

 ### 1.0.1 (Next)

 * [#400](https://github.com/slack-ruby/slack-ruby-client/pull/400): Replace Travis-CI with GitHub Actions - [@dblock](https://github.com/dblock).
 * [#401](https://github.com/slack-ruby/slack-ruby-client/pull/401): Upgraded RuboCop to 1.26.0 - [@dblock](https://github.com/dblock).
 * [#401](https://github.com/slack-ruby/slack-ruby-client/pull/401): Added support for Ruby 3.1.1 - [@dblock](https://github.com/dblock).
 * [#405](https://github.com/slack-ruby/slack-ruby-client/pull/405): Added `admin_apps_requests_cancel`, `admin_users_unsupportedVersions_export`, `bookmarks_add`, `bookmarks_edit`, `bookmarks_list`, `bookmarks_remove` - [@dblock](https://github.com/dblock).
 * [#404](https://github.com/slack-ruby/slack-ruby-client/pull/404): Update `chat_update` method not to raise error with parameter which contains only `reply_broadcast` - [@colorbox](https://github.com/colorbox).
-* Your contribution here.


Put this one back.

dblock · 2022-05-04T17:32:26Z

lib/slack/web/faraday/connection.rb

+                # Match any content type for JSON responses meaning Faraday::ParsingError will be raised for non-JSON
+                # responses such as HTML 500 errors.
+                # TODO: Should this be more restrictive?
+                connection.response :json, content_type: /\b*$/


So this is any content type, isn't it? Do any specs fail if you remove content_type here? I think not, and it's the old behavior that basically says "try to parse anything, which now raises ::Faraday::ParsingError, which we handle properly".

Remove comment and TODO above.

So this is any content type, isn't it? Do any specs fail if you remove content_type here? I think not, and it's the old behavior that basically says "try to parse anything, which now raises ::Faraday::ParsingError, which we handle properly".

If we remove this content_type: option then the <html></html> response specs will fail because the default for the JSON middleware parsing is /\bjson$/, so we wouldn't attempt to parse if a 500 HTML response was returned and we end up with the undefined method 'map' for nil:NilClass mention in the initial PR description.

I guess there are two options:

Leave as is it now meaning that it will try and parse any response Slack returns, which I think as you say is the old behaviour handled by the deprecated ::FaradayMiddleware::ParseJson middleware

Make the content types stricter and set it to only attempt to parse JSON or HTML content types (or whatever we know is returned) as we can set an array of content types

Remove comment and TODO above.

Removed 👍🏻

dblock

This looks good! Merging. Let's see what falls out in integration tests run on master with a token. My local run with CONCURRENCY=async-websocket SLACK_API_TOKEN=... bundle exec rake runs integration tests successfully.

dblock · 2022-05-07T14:57:29Z

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.

schinery force-pushed the update-faraday branch 2 times, most recently from 80e18b9 to cb80a2e Compare May 3, 2022 05:50

schinery marked this pull request as ready for review May 3, 2022 06:16

schinery mentioned this pull request May 3, 2022

Dependency on faraday_middleware prevents update to faraday 2.0 #399

Closed

dblock requested changes May 3, 2022

View reviewed changes

schinery force-pushed the update-faraday branch from cb80a2e to da69c1f Compare May 3, 2022 15:58

dblock reviewed May 3, 2022

View reviewed changes

schinery force-pushed the update-faraday branch 11 times, most recently from 27783da to 25bf1dd Compare May 4, 2022 08:08

Changes to update Faraday to >= 2.0

b056e0e

schinery force-pushed the update-faraday branch from 91a30d7 to b056e0e Compare May 4, 2022 08:14

Merge branch 'master' into update-faraday

719117a

schinery requested a review from dblock May 4, 2022 08:27

dblock requested changes May 4, 2022

View reviewed changes

schinery added 2 commits May 5, 2022 06:38

Updated CHANGELOG

8a3efdd

Quote rubies in test GitHub action

9297ca4

Removed comment and TODO

b051c48

schinery requested a review from dblock May 5, 2022 06:30

dblock approved these changes May 7, 2022

View reviewed changes

dblock merged commit bfd493e into slack-ruby:master May 7, 2022

schinery deleted the update-faraday branch May 7, 2022 16:09

schinery mentioned this pull request May 28, 2022

Upgrade Faraday to >= 2.0 dblock/strava-ruby-client#58

Merged

dblock mentioned this pull request Jul 1, 2023

Upgrade faraday to 2.x dblock/open-weather-ruby-client#33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Faraday to >= 2.0 #406

Update Faraday to >= 2.0 #406

schinery commented May 3, 2022 •

edited

Loading

schinery commented May 3, 2022

dblock commented May 3, 2022

dblock left a comment

dblock May 3, 2022

schinery May 3, 2022

dblock May 3, 2022

schinery May 4, 2022

dblock May 4, 2022

dblock May 3, 2022

schinery May 4, 2022

dblock May 3, 2022

schinery May 4, 2022

schinery commented May 3, 2022

schinery commented May 3, 2022

dblock May 3, 2022

schinery commented May 4, 2022

dblock left a comment

dblock May 4, 2022

dblock May 4, 2022

dblock May 4, 2022

dblock May 4, 2022 •

edited

Loading

schinery May 5, 2022 •

edited

Loading

schinery May 5, 2022

dblock left a comment •

edited

Loading

dblock commented May 7, 2022

Update Faraday to >= 2.0 #406

Update Faraday to >= 2.0 #406

Conversation

schinery commented May 3, 2022 • edited Loading

schinery commented May 3, 2022

dblock commented May 3, 2022

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schinery commented May 3, 2022

schinery commented May 3, 2022

Choose a reason for hiding this comment

schinery commented May 4, 2022

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock May 4, 2022 • edited Loading

Choose a reason for hiding this comment

schinery May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment • edited Loading

Choose a reason for hiding this comment

dblock commented May 7, 2022

schinery commented May 3, 2022 •

edited

Loading

dblock May 4, 2022 •

edited

Loading

schinery May 5, 2022 •

edited

Loading

dblock left a comment •

edited

Loading