Allow use of either rest-client or faraday by armilam · Pull Request #307 · jeremytregunna/ruby-trello

armilam · 2022-05-13T19:51:56Z

What does this PR do?

This adds a configuration option to allow apps to choose between rest-client and faraday for the http client.

Trello.configure do |config|
  config.http_client = 'faraday'
  # or
  config.http_client = 'rest-client'
end

Highlights

It's written in a way to allow for supporting more http clients in the future.
If config.http_client is not set, the gem tries to use faraday, then rest-client. If neither is present, it raises and error.
It changes the return value of Trello::Client#get, #post, #put, and #delete for consistency. It returns the Trello::Response object every time, and it stores the response body as a string on the Trello::Response#body so that the caller can access the response's status code, body, and headers reliably regardless of the http client used.

Why are we doing this? Any context or related work?

The rest-client gem is no longer actively maintained and its dependencies are starting to fall behind. See this PR on the rest-client repo for context.

Faraday is fairly commonly used, actively maintained, and has few dependencies, making it a good candidate for a replacement http library.

In order to continue supporting those using rest-client without forcing them to change, we allow either as an option, with the option to support more in the future.

Where should a reviewer start?

Start by looking at:

lib/trello.rb for the configuration setup
lib/trello/net.rb
- The client-specific code in here was moved into lib/trello/net/faraday.rb and lib/trello/net/rest_client.rb
lib/trello/json_utils.rb

The rest are just updating the tests.

Manual testing steps?

Since this impacts http calls, I would check that successful responses, error responses, and network-level errors (like timeouts) are still handled correctly, for each of faraday and rest-client.

rest-client is no longer actively maintained. faraday is popular, still active, and the code changes to make the switch should be minimal.

armilam · 2022-05-13T19:54:41Z

+      def parse_json(data, encoding = 'UTF-8')
+        case data
+        when Faraday::Response
+          JSON.parse(data.body.force_encoding(encoding))
+        else
+          JSON.parse(data.force_encoding(encoding))
+        end


I realize this might not be the most ideal place to parse out a Faraday response body, but it's the place that had the least impact on the rest of the code. I'm open to moving this if we'd rather it not be here.

armilam · 2022-05-13T20:27:09Z

I tested this in my own rails app and it works without any changes in the app, beyond updating the ruby-trello gem.

hoppergee · 2022-05-14T05:55:24Z

Thank your @armilam for this PR. Any thoughts on this, @runlevel5 ?

Should we support faraday and rest-client at the same time for a while in case it breaks those existing apps? Maybe we can let the user choose which HTTP client to use?

armilam · 2022-05-16T14:16:31Z

If you'd prefer to support both in a minor version update, I'd be happy to update this PR to do that. Personally I think it's worth a major version update to drop rest-client, but I understand if you'd want to reduct the impact of the change.

hoppergee · 2022-05-17T00:51:05Z

Actually, I would like to make multiple HTTP clients support as a feature in the long term. And I think the faraday is good one for the default. But we still have to make rest-client as the default for a while. How do you think?

armilam · 2022-05-17T01:09:57Z

That does make sense. I'll take a shot at adding the ability to configure which client to use, with Faraday as the default.

hoppergee · 2022-05-17T02:25:25Z

Look forward to your good news!

And move rest-client and faraday dependencies to dev_dependencies

This raises an error if the proper gem is not present

armilam · 2022-05-17T17:21:50Z

Well, I have an implementation that I think is good. I'm just trying to figure out how to run all the tests with each of the two http clients without having to go into each spec file and iterate over the clients.

I've pushed my updates without an attempt to test with both clients. Working on trying to do it with matrixeval.

armilam · 2022-05-18T13:15:38Z

+        begin
+          require 'faraday'
+        rescue LoadError
+        end


We rescue this in case the project does not have faraday installed and is using rest-client instead. We do the same in the rest-client version of this class in case the opposite is true.

armilam · 2022-05-18T13:16:31Z

-  s.add_dependency 'rest-client', '>= 1.8.0'
+
+  s.add_development_dependency 'rest-client', '>= 1.8.0'
+  s.add_development_dependency 'faraday', '>= 2.0.0'


We don't want to require both of these gems in everyone's projects, so we make them dev dependencies. We'll need to update documentation to say that one of these is required.

armilam · 2022-05-18T13:19:08Z

+  Trello::HTTP_CLIENTS.each do |key, _client|
+    context "using #{key}" do
+      include_context "using #{key}" do


This is the only reliable way I've been able to get the tests to run with each of the http clients. I have not copied this pattern to the other test files yet in case someone has a better idea.

Ideally, we'd run all tests with Trello.http_client = 'faraday' and again with Trello.http_client = 'rest-client', except for a few that specifically test the configuration.

If this is the best pattern, I can copy it to the other test files, too.

armilam · 2022-05-18T13:24:09Z

I'm also having a bit of trouble getting matrixeval to run this - I could use some help there, please.

hoppergee · 2022-05-18T17:37:16Z

I'm also having a bit of trouble getting matrixeval to run this - I could use some help there, please.

Just fetched your code, the MatrixEval is working on my side. Any details about the issue you are meeting?

BTW, I haven't checked the code details, I will find time to review it this week.

armilam · 2022-05-18T17:55:06Z

It works when I run it on the master branch, but on this branch every variant fails with this error:

Bundler::GemNotFound: Could not find gem 'rest-client (>= 1.8.0)' in locally installed gems.

If it's working for you, maybe I'll try adding a variant for http_client_gem.

armilam · 2022-05-18T18:04:31Z

Okay, I added a matrixeval variant for the client. Let me know if it runs correctly. If it does, then it should cover both faraday and rest-client for all the tests that don't explicitly set the client.

armilam · 2022-05-18T18:11:42Z

I got matrixeval working, and everything passes with ruby 2.6 and up. I think this is ready for review now, with the only work left being updating documentation. I'd be happy to take a shot at that if you're good with the code changes.

hoppergee · 2022-05-27T08:24:38Z

Hi @armilam, thanks for your work. I just check your code. All looks good. But we still get something on hand:

Update the .github/workflows/main.yml to include faraday and reset-client
Can you help to pass the tests under Ruby 2.5 with something like this in Gemfile.

# Gemfile
if RUBY_VERSION  < '2.6.0'
  gem 'faraday', '~> 1.0'
else
  gem 'faraday', '~> 2.0'
end

You can run all tests with meval -a bundle install and meval -a rspec. Or run test for a specific circumstance like this meval --ruby 2.5 --active_model 6.0 --http_client_gem faraday rspec

Can you help to bring the rest-client back as the default in both codebase and gemspec? I want to release this update with a minor version. Then set faraday as the default in next major version.
Update the README about this new feature.

Thanks again @armilam.

armilam · 2022-05-27T14:05:09Z

Absolutely. Thanks for the feedback. I'll make those changes.

- Update CI to run the tests against each http client - Use older Faraday for older ruby versions - Update the README to talk about the new http_client configuration

armilam · 2022-05-27T15:12:15Z

@hoppergee I've made the updates you requested. Please let me know if there's anything else to do.

hoppergee · 2022-05-28T03:35:52Z

Thank you for developing so quickly! I will do a last check today and may release it in two days.

armilam · 2022-05-31T19:17:40Z

Thank you!

armilam added 3 commits May 13, 2022 15:41

Switch from rest-client to faraday in gemspec

28c53cb

rest-client is no longer actively maintained. faraday is popular, still active, and the code changes to make the switch should be minimal.

Change code to use Faraday rather than RestClient

2a365df

Update tests for switch to faraday

531f6cf

armilam commented May 13, 2022

View reviewed changes

armilam force-pushed the switch-to-faraday branch from 96a4d61 to bb55f43 Compare May 13, 2022 20:05

Use :raise_error to simplify error handling

9cd1be4

armilam force-pushed the switch-to-faraday branch from bb55f43 to 9cd1be4 Compare May 13, 2022 20:07

Add http_client configuration with :faraday as the default

ea7877d

armilam added 7 commits May 17, 2022 10:15

Use modules to separate out http client implementations

c69448b

Auto-choose http client in a more functional way

44f555a

Move client-specific JSON-parsing to client-specific modules

d10e13a

Re-add rest-client specific tests

7a9c828

And move rest-client and faraday dependencies to dev_dependencies

Require the http client on configuration

3c8b14b

This raises an error if the proper gem is not present

Standardize response structure independent of the active http client

60cf9ac

Use the correct http client gem versions

e42cb3b

Update a spec to test with both http clients

12326ba

armilam commented May 18, 2022

View reviewed changes

Use matrixeval to run tests with each supported http client

1ecb48d

Add a default http_client_gem

4ee173f

armilam changed the title ~~Switch from RestClient to Faraday~~ Allow use of either rest-client or faraday May 19, 2022

Use rest-client by default

3461d93

- Update CI to run the tests against each http client - Use older Faraday for older ruby versions - Update the README to talk about the new http_client configuration

hoppergee approved these changes May 30, 2022

View reviewed changes

hoppergee merged commit 9312d24 into jeremytregunna:master May 30, 2022

armilam deleted the switch-to-faraday branch May 31, 2022 19:17

armilam mentioned this pull request May 31, 2022

Remove rest-client dependency in the gemspec and default to faraday #308

Merged

fastjames mentioned this pull request Dec 19, 2023

rest-client gem dependency is abandoned pusher/push-notifications-ruby#34

Closed

Conversation

armilam commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why are we doing this? Any context or related work?

Where should a reviewer start?

Manual testing steps?

Uh oh!

armilam May 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

armilam commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoppergee commented May 14, 2022

Uh oh!

armilam commented May 16, 2022

Uh oh!

hoppergee commented May 17, 2022

Uh oh!

armilam commented May 17, 2022

Uh oh!

hoppergee commented May 17, 2022

Uh oh!

armilam commented May 17, 2022

Uh oh!

armilam May 18, 2022

Choose a reason for hiding this comment

Uh oh!

armilam May 18, 2022

Choose a reason for hiding this comment

Uh oh!

armilam May 18, 2022

Choose a reason for hiding this comment

Uh oh!

armilam commented May 18, 2022

Uh oh!

hoppergee commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

armilam commented May 18, 2022

Uh oh!

armilam commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

armilam commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoppergee commented May 27, 2022

Uh oh!

armilam commented May 27, 2022

Uh oh!

armilam commented May 27, 2022

Uh oh!

hoppergee commented May 28, 2022

Uh oh!

armilam commented May 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

armilam commented May 13, 2022 •

edited

Loading

armilam commented May 13, 2022 •

edited

Loading

hoppergee commented May 18, 2022 •

edited

Loading

armilam commented May 18, 2022 •

edited

Loading

armilam commented May 18, 2022 •

edited

Loading