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

Dependency on faraday_middleware prevents update to faraday 2.0 #399

Closed
brandonrdn opened this issue Feb 23, 2022 · 11 comments
Closed

Dependency on faraday_middleware prevents update to faraday 2.0 #399

brandonrdn opened this issue Feb 23, 2022 · 11 comments

Comments

@brandonrdn
Copy link

brandonrdn commented Feb 23, 2022

faraday_middleware has been deprecated, with json and instrumentation middleware being bundled in faraday 2.0, and other middleware are being re-released as independent gems. Since faraday_middleware is never being updated to support faraday >= 2.0, the current slack-ruby-client can never use it either. Is there any chance we can remove the faraday_middleware dependency and bump the minimum version of faraday to 2.0?

@dblock
Copy link
Collaborator

dblock commented Feb 26, 2022

We sure can, please contribute @brandonrdn !

@schinery
Copy link
Contributor

I tried to have a go at this myself but the chain of gems that actually needs updating is pretty troublesome...

  • This gem needs slack-ruby-danger updated to support Faraday 2.0
  • slack-ruby-danger needs danger updated to support Faraday 2.0
  • danger needs this issue closed and a release made to support Faraday 2.0

So, am wondering is it more important to be able to use danger in this repo than users to be able to use Faraday 2.0, which as @pboling commented "is far more consequential for the users of oauth2, so it takes precedence", at least until danger does support Faraday 2.0?

@dblock
Copy link
Collaborator

dblock commented Apr 27, 2022

The easiest workaround is to move Danger into a separate danger/Gemfile and separate the task in GHA.

@schinery
Copy link
Contributor

Ignoring danger for the moment, this is as far as I've got locally trying to get Faraday updated.

Code changes:

I've commented out the code that needs replacing rather than removing for the moment. Also ignore the debugger, was trying to figure out why the parse error wasn't being thrown.

diff --git i/Gemfile w/Gemfile
index 4faec06..adf42a9 100644
--- i/Gemfile
+++ w/Gemfile
@@ -14,14 +14,16 @@ end
 
 group :test do
   gem 'activesupport'
+  gem 'byebug'
   gem 'erubis'
+  gem 'faraday-typhoeus'
   gem 'json-schema'
   gem 'rake', '~> 13'
   gem 'rspec'
   gem 'rubocop', '~> 1.26.0'
   gem 'rubocop-performance'
   gem 'rubocop-rspec'
-  gem 'slack-ruby-danger', '~> 0.2.0', require: false
+  # gem 'slack-ruby-danger', github: "schinery/danger" # '~> 0.2.0', require: false
   gem 'timecop'
   if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.0.0')
     # https://github.com/vcr/vcr/pull/907

diff --git i/lib/slack-ruby-client.rb w/lib/slack-ruby-client.rb
index 54c351d..ee96afd 100644
--- i/lib/slack-ruby-client.rb
+++ w/lib/slack-ruby-client.rb
@@ -10,7 +10,9 @@ require_relative 'slack/messages/formatting'
 
 # Web API
 require 'faraday'
-require 'faraday_middleware'
+# require 'faraday_middleware'
+require 'faraday/mashify'
+require 'faraday/multipart'
 require 'json'
 require 'logger'
 begin

diff --git i/lib/slack/web/faraday/connection.rb w/lib/slack/web/faraday/connection.rb
index 5f461b5..d89bfac 100644
--- i/lib/slack/web/faraday/connection.rb
+++ w/lib/slack/web/faraday/connection.rb
@@ -22,11 +22,15 @@ module Slack
               options[:request] = request_options if request_options.any?
 
               ::Faraday::Connection.new(endpoint, options) do |connection|
-                connection.use ::Faraday::Request::Multipart
-                connection.use ::Faraday::Request::UrlEncoded
+                # connection.use ::Faraday::Request::Multipart
+                connection.request :multipart
+                # connection.use ::Faraday::Request::UrlEncoded
+                connection.request :url_encoded
                 connection.use ::Slack::Web::Faraday::Response::RaiseError
-                connection.use ::FaradayMiddleware::Mashify, mash_class: Slack::Messages::Message
-                connection.use ::FaradayMiddleware::ParseJson
+                # connection.use ::FaradayMiddleware::Mashify, mash_class: Slack::Messages::Message
+                connection.response :mashify, mash_class: Slack::Messages::Message
+                # connection.use ::FaradayMiddleware::ParseJson
+                connection.response :json
                 connection.use ::Slack::Web::Faraday::Response::WrapError
                 connection.response :logger, logger if logger
                 connection.adapter adapter

diff --git i/lib/slack/web/faraday/response/raise_error.rb w/lib/slack/web/faraday/response/raise_error.rb
index ce37685..8465ea2 100644
--- i/lib/slack/web/faraday/response/raise_error.rb
+++ w/lib/slack/web/faraday/response/raise_error.rb
@@ -3,7 +3,7 @@ module Slack
   module Web
     module Faraday
       module Response
-        class RaiseError < ::Faraday::Response::Middleware
+        class RaiseError < ::Faraday::Middleware
           def on_complete(env)
             raise Slack::Web::Api::Errors::TooManyRequestsError, env.response if env.status == 429
 
@@ -13,6 +13,7 @@ module Slack
             return unless body
             return if body['ok']
 
+            # debugger
             error_message =
               body['error'] || body['errors'].map { |message| message['error'] }.join(',')
 

diff --git i/lib/slack/web/faraday/response/wrap_error.rb w/lib/slack/web/faraday/response/wrap_error.rb
index 4082c14..b593cd6 100644
--- i/lib/slack/web/faraday/response/wrap_error.rb
+++ w/lib/slack/web/faraday/response/wrap_error.rb
@@ -3,7 +3,7 @@ module Slack
   module Web
     module Faraday
       module Response
-        class WrapError < ::Faraday::Response::Middleware
+        class WrapError < ::Faraday::Middleware
           UNAVAILABLE_ERROR_STATUSES = (500..599).freeze
 
           def on_complete(env)

diff --git i/slack-ruby-client.gemspec w/slack-ruby-client.gemspec
index b775ef3..f695835 100644
--- i/slack-ruby-client.gemspec
+++ w/slack-ruby-client.gemspec
@@ -10,14 +10,17 @@ Gem::Specification.new do |s|
   s.authors = ['Daniel Doubrovkine']
   s.email = '[email protected]'
   s.platform = Gem::Platform::RUBY
+  s.required_ruby_version = '>= 2.5'
   s.required_rubygems_version = '>= 1.3.6'
   s.files = `git ls-files`.split("\n")
   s.require_paths = ['lib']
   s.homepage = 'http://github.com/slack-ruby/slack-ruby-client'
   s.licenses = ['MIT']
   s.summary = 'Slack Web and RealTime API client.'
-  s.add_dependency 'faraday', '>= 1.0'
-  s.add_dependency 'faraday_middleware'
+  s.add_dependency 'faraday', '>= 2.0'
+  # s.add_dependency 'faraday_middleware'
+  s.add_dependency 'faraday-mashify'
+  s.add_dependency 'faraday-multipart'
   s.add_dependency 'gli'
   s.add_dependency 'hashie'
   s.add_dependency 'websocket-driver'

diff --git i/spec/slack/web/client_spec.rb w/spec/slack/web/client_spec.rb
index 3068e41..b2edec0 100644
--- i/spec/slack/web/client_spec.rb
+++ w/spec/slack/web/client_spec.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 require 'spec_helper'
+require 'faraday/typhoeus'
 
 RSpec.describe Slack::Web::Client do
   before do

diff --git i/spec/spec_helper.rb w/spec/spec_helper.rb
index f57133b..c8c5db9 100644
--- i/spec/spec_helper.rb
+++ w/spec/spec_helper.rb
@@ -3,6 +3,7 @@ $LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..'))
 
 require 'rubygems'
 require 'rspec'
+require 'byebug'
 require 'timecop'
 
 require 'slack_ruby_client'

Specs:

With the above changes I currently have 1 failing spec and a number of pending ones.

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) integration test gets hello
     # missing SLACK_API_TOKEN and/or CONCURRENCY
     # ./spec/integration/integration_spec.rb:113

  2) integration test gets close, followed by closed
     # missing SLACK_API_TOKEN and/or CONCURRENCY
     # ./spec/integration/integration_spec.rb:189

  3) integration test client connected responds to message
     # missing SLACK_API_TOKEN and/or CONCURRENCY
     # ./spec/integration/integration_spec.rb:84

  4) integration test client connected sends message
     # missing SLACK_API_TOKEN and/or CONCURRENCY
     # ./spec/integration/integration_spec.rb:103

  5) integration test with websocket_ping set sends pings
     # missing SLACK_API_TOKEN and/or CONCURRENCY
     # ./spec/integration/integration_spec.rb:128

  6) integration test with websocket_ping set rebuilds the websocket connection when dropped
     # missing SLACK_API_TOKEN and/or CONCURRENCY
     # ./spec/integration/integration_spec.rb:142

  7) integration test with websocket_ping not set does not send pings
     # missing SLACK_API_TOKEN and/or CONCURRENCY
     # ./spec/integration/integration_spec.rb:171

  8) with CONCURRENCY detects concurrency
     # missing CONCURRENCY
     # ./spec/slack/real_time/concurrency/with_concurrency_spec.rb:7

Failures:

  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

Also, this error was thrown while running the specs. Don't know if it is because I have something missing or what...

Slack::RealTime::Client
E, [2019-01-19T21:25:48.000000 #33521] ERROR -- id=T04KB5WQH, name=dblock, domain=dblockdotorg#run_handlers: ArgumentError (ArgumentError)
/Users/stu/Workspace/github/slack-ruby-client/spec/slack/real_time/event_handlers/event_handlers_spec.rb:13:in `block (3 levels) in <top (required)>'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-mocks-3.11.1/lib/rspec/mocks/message_expectation.rb:763:in `block in call'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-mocks-3.11.1/lib/rspec/mocks/message_expectation.rb:762:in `map'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-mocks-3.11.1/lib/rspec/mocks/message_expectation.rb:762:in `call'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-mocks-3.11.1/lib/rspec/mocks/message_expectation.rb:622:in `invoke_incrementing_actual_calls_by'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-mocks-3.11.1/lib/rspec/mocks/message_expectation.rb:475:in `invoke'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-mocks-3.11.1/lib/rspec/mocks/proxy.rb:217:in `message_received'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-mocks-3.11.1/lib/rspec/mocks/proxy.rb:366:in `message_received'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-mocks-3.11.1/lib/rspec/mocks/method_double.rb:80:in `proxy_method_invoked'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-mocks-3.11.1/lib/rspec/mocks/method_double.rb:64:in `block (2 levels) in define_proxy_method'
/Users/stu/Workspace/github/slack-ruby-client/lib/slack/real_time/stores/store.rb:429:in `block in <class:Store>'
/Users/stu/Workspace/github/slack-ruby-client/lib/slack/real_time/client.rb:256:in `instance_exec'
/Users/stu/Workspace/github/slack-ruby-client/lib/slack/real_time/client.rb:256:in `block in run_handlers'
/Users/stu/Workspace/github/slack-ruby-client/lib/slack/real_time/client.rb:255:in `each'
/Users/stu/Workspace/github/slack-ruby-client/lib/slack/real_time/client.rb:255:in `run_handlers'
/Users/stu/Workspace/github/slack-ruby-client/lib/slack/real_time/client.rb:244:in `dispatch'
/Users/stu/Workspace/github/slack-ruby-client/spec/slack/real_time/event_handlers/event_handlers_spec.rb:14:in `block (3 levels) in <top (required)>'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-expectations-3.11.0/lib/rspec/matchers/built_in/raise_error.rb:59:in `matches?'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-expectations-3.11.0/lib/rspec/matchers/built_in/raise_error.rb:81:in `does_not_match?'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-expectations-3.11.0/lib/rspec/expectations/handler.rb:90:in `does_not_match?'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-expectations-3.11.0/lib/rspec/expectations/handler.rb:79:in `block in handle_matcher'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-expectations-3.11.0/lib/rspec/expectations/handler.rb:27:in `with_matcher'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-expectations-3.11.0/lib/rspec/expectations/handler.rb:76:in `handle_matcher'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-expectations-3.11.0/lib/rspec/expectations/expectation_target.rb:78:in `not_to'
/Users/stu/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/rspec-expectations-3.11.0/lib/rspec/expectations/expectation_target.rb:144:in `not_to'
/Users/stu/Workspace/github/slack-ruby-client/spec/slack/real_time/event_handlers/event_handlers_spec.rb:14:in `block (2 levels) in <top (required)>'
...
  is not fatal

Does this look like I'm on the right track with this and is there something I should/could be doing to get fix the error and to get the pending tests to run?

@dblock
Copy link
Collaborator

dblock commented Apr 28, 2022

It's on the right track! Set export CONCURRENCY=async-websocket and some of these will run. The rest are integration tests, and you need a Slack app (an API token). If you can PR a green passing version I can confirm that those work before merging.

@schinery
Copy link
Contributor

schinery commented May 3, 2022

@dblock I've put this PR up for you to have a look at. Definitely would be good to get some thoughts on the lib/slack/web/faraday/response/raise_error.rb changes.

Also as mentioned in the PR, 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 though so 🤞🏻

@dblock
Copy link
Collaborator

dblock commented May 7, 2022

Thanks @schinery for doing the work of upgrading Faraday! Closing via #406. If ya'll could try HEAD before we release that'd be grand.

@dblock dblock closed this as completed May 7, 2022
@armilam
Copy link

armilam commented May 9, 2022

For what it's worth, I tested it in my own application with Faraday 2 and found that the slack client is working for us.

@schinery
Copy link
Contributor

schinery commented May 27, 2022

Sorry @dblock I've only got around to testing this myself as been busy on other stuff.

Can also confirm that the Faraday upgrade works for me in one of the apps I've tested it in, which I tested using the following:

Gemfile:

gem "slack-ruby-client", github: "slack-ruby/slack-ruby-client", branch: "master" # "~> 1.0"

Code:

client = Slack::Web::Client.new
client.chat_postMessage(channel: "#qa", text: "Testing Slack client with Faraday 2", as_user: true)

Slack message:

slack-message

So like @armilam, for me it is working. There were also no problems installing it with other gems that use Faraday such as Octokit.

Does this help with getting this released soon?

@dblock
Copy link
Collaborator

dblock commented Jun 5, 2022

I released 1.1.0.

@dblock
Copy link
Collaborator

dblock commented Jun 5, 2022

If someone could take care of #409 that'd be very helpful.

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

No branches or pull requests

4 participants