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

Adds specific errors for connection timeout and failure #483

Closed
wants to merge 2 commits into from
+54 −9
Closed

Adds specific errors for connection timeout and failure #483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9a2a78
Adds specific errors for connection timeout and failure
Maimer Jan 16, 2024
5286788
Ensure `api_error` is loaded in `api_client`
Maimer Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/plaid/api_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,13 @@
require 'tempfile'
require 'faraday'
require 'faraday/multipart'
require_relative 'api_error'

module Plaid
class ApiClient
class ApiTimeoutError < ApiError; end
class ApiConnectionFailedError < ApiError; end
Comment on lines +27 to +28
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re #483 (comment), I believe it's a namespace issue

Suggested change
class ApiTimeoutError < ApiError; end
class ApiConnectionFailedError < ApiError; end
class ApiTimeoutError < Plaid::ApiError; end
class ApiConnectionFailedError < Plaid::ApiError; end

Or may need to be ::Plaid

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's a loading issue, which is only present in the tests. The namespace should work without needing to be explicit.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try changing the namespace as suggested here but now get the error:

/usr/src/app/lib/plaid/api_client.rb:26:in `<class:ApiClient>': uninitialized constant Plaid::ApiError (NameError)

If you can figure out the solution that would be great. If not, things have been a little hectic this week (performance review season, family emergency) but I can also ask some people who actually know Ruby to take a look sometime next week.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try changing the namespace as suggested here but now get the error:

/usr/src/app/lib/plaid/api_client.rb:26:in `<class:ApiClient>': uninitialized constant Plaid::ApiError (NameError)

If you can figure out the solution that would be great. If not, things have been a little hectic this week (performance review season, family emergency) but I can also ask some people who actually know Ruby to take a look sometime next week.

@phoenixy1 Apologies as I had meant to take a look at this sooner. I have pushed what I believe to be a working solution to the NameError. I believe that the test suite could be cleaned up a bit more and be made more conventional. However, I have not setup the test suite to run entirely on my machine. I did get the specific test I wrote to pass when run on its own. It would be nice if this test suite were either easier to run locally, or had a public facing CI run that I could use to verify my work. If that were the case I could spend some time standardizing the way this all works and make it easier to make changes going forward. How are the tests currently run and changes verified internally? All I see is the build step waiting in CircleCI. Would it be possibe to make that build public?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I tried this and the tests pass with these changes; I've submitted a fix for upstream.

I don't have a ton of insight into CI for this repo -- honestly, I don't have access to CircleCI either and typically run the tests locally using our internal makefile, which builds the libraries using the Docker images provided by OpenApiTools and then runs the tests. Anyway, I can pass the request along to the team that does run it!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I tried this and the tests pass with these changes; I've submitted a fix for upstream.

I don't have a ton of insight into CI for this repo -- honestly, I don't have access to CircleCI either and typically run the tests locally using our internal makefile, which builds the libraries using the Docker images provided by OpenApiTools and then runs the tests. Anyway, I can pass the request along to the team that does run it!

Awesome, glad it worked out and will be in the next release!

Thanks for passing along the CI request. At my company (AppFolio) we also use CircleCI and all of our open source repos have publicly available CI.


# The Configuration object holding settings to be used in the API client.
attr_accessor :config

Expand Down Expand Up @@ -99,9 +103,9 @@ def call_api(http_method, path, opts = {})
end
end
rescue Faraday::TimeoutError
fail ApiError.new('Connection timed out')
fail ApiTimeoutError.new('Connection timed out')
rescue Faraday::ConnectionFailed
fail ApiError.new('Connection failed')
fail ApiConnectionFailedError.new('Connection failed')
end

if opts[:return_type]
Expand Down
5 changes: 5 additions & 0 deletions templates/ruby/api_client.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ require 'faraday/multipart'

module {{moduleName}}
class ApiClient
{{#isFaraday}}
class ApiTimeoutError < ApiError; end
class ApiConnectionFailedError < ApiError; end

{{/isFaraday}}
# The Configuration object holding settings to be used in the API client.
attr_accessor :config

Expand Down
4 changes: 2 additions & 2 deletions templates/ruby/api_client_faraday_partial.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
end
end
rescue Faraday::TimeoutError
fail ApiError.new('Connection timed out')
fail ApiTimeoutError.new('Connection timed out')
rescue Faraday::ConnectionFailed
fail ApiError.new('Connection failed')
fail ApiConnectionFailedError.new('Connection failed')
end

if opts[:return_type]
Expand Down
31 changes: 29 additions & 2 deletions test/test_api_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
require 'faraday'
require 'faraday/multipart'


require_relative "test_helper"

# Internal: The test for API Client
class APIClientTest < PlaidTest
class ApiClientTest < PlaidTest
def test_api_client_exposes_farday_connection
configuration = Plaid::Configuration.new
configuration.server_index = Plaid::Configuration::Environment["sandbox"]
Expand All @@ -20,4 +19,32 @@ def test_api_client_exposes_farday_connection

assert_kind_of(Faraday::Connection, api_client.connection)
end

def test_timeout_error
api_client = Plaid::ApiClient.new
exception = -> { raise Faraday::TimeoutError.new }
options = { header_params: {} }

error = assert_raises Plaid::ApiClient::ApiTimeoutError do
api_client.connection.stub :get, exception do
api_client.call_api(:get, 'dummy_path', options)
end
end

assert_equal 'Connection timed out', error.message
end

def test_connection_error
api_client = Plaid::ApiClient.new
exception = -> { raise Faraday::ConnectionFailed.new }
options = { header_params: {} }

error = assert_raises Plaid::ApiClient::ApiConnectionFailedError do
api_client.connection.stub :get, exception do
api_client.call_api(:get, 'dummy_path', options)
end
end

assert_equal 'Connection failed', error.message
end
end
15 changes: 12 additions & 3 deletions test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
require 'bundler'

begin
Bundler.setup(:default, :development)
rescue Bundler::BundlerError => e
warn e.message
warn 'Run `bundle install` to install missing gems'
exit e.status_code
end

require "plaid"
require "minitest/autorun"
require "minitest/around/unit"
require "json"

require_relative "../lib/plaid"

# Internal: Default read timeout for HTTP calls in seconds.
NETWORK_TIMEOUT = 600

class PlaidTest < MiniTest::Test
class PlaidTest < Minitest::Test
attr_reader :client, :item, :access_token

def create_client
Expand Down