From 939244bd3e931ac88cdfa7cfa0c58f864a79ee6b Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Tue, 1 Apr 2025 08:41:49 -0400 Subject: [PATCH] Use faraday retry, move retry logic to concrete provider level (#2042) --- Gemfile.lock | 4 +- app/models/provider.rb | 20 +--------- app/models/provider/synth.rb | 21 +++++----- lib/retryable.rb | 19 --------- test/models/provider_test.rb | 76 +++++++++++++++++------------------- 5 files changed, 50 insertions(+), 90 deletions(-) delete mode 100644 lib/retryable.rb diff --git a/Gemfile.lock b/Gemfile.lock index c3e5ceaf..75bef73e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -169,7 +169,7 @@ GEM multipart-post (~> 2.0) faraday-net_http (3.4.0) net-http (>= 0.5.0) - faraday-retry (2.2.1) + faraday-retry (2.3.0) faraday (~> 2.0) ffi (1.17.1-aarch64-linux-gnu) ffi (1.17.1-aarch64-linux-musl) @@ -239,7 +239,7 @@ GEM listen (3.9.0) rb-fsevent (~> 0.10, >= 0.10.3) rb-inotify (~> 0.9, >= 0.9.10) - logger (1.6.6) + logger (1.7.0) logtail (0.1.17) msgpack (~> 1.0) logtail-rack (0.2.6) diff --git a/app/models/provider.rb b/app/models/provider.rb index 98d9fff1..c90866e7 100644 --- a/app/models/provider.rb +++ b/app/models/provider.rb @@ -1,6 +1,4 @@ class Provider - include Retryable - Response = Data.define(:success?, :data, :error) class Error < StandardError @@ -23,17 +21,8 @@ class Provider PaginatedData = Data.define(:paginated, :first_page, :total_pages) UsageData = Data.define(:used, :limit, :utilization, :plan) - # Subclasses can specify errors that can be retried - def retryable_errors - [] - end - - def with_provider_response(retries: default_retries, error_transformer: nil, &block) - data = if retries > 0 - retrying(retryable_errors, max_retries: retries) { yield } - else - yield - end + def with_provider_response(error_transformer: nil, &block) + data = yield Response.new( success?: true, @@ -67,9 +56,4 @@ class Provider self.class::Error.new(error.message) end end - - # Override to set class-level number of retries for methods using `with_provider_response` - def default_retries - 0 - end end diff --git a/app/models/provider/synth.rb b/app/models/provider/synth.rb index 87036f88..e095d2be 100644 --- a/app/models/provider/synth.rb +++ b/app/models/provider/synth.rb @@ -39,7 +39,7 @@ class Provider::Synth < Provider # ================================ def fetch_exchange_rate(from:, to:, date:) - with_provider_response retries: 2 do + with_provider_response do response = client.get("#{base_url}/rates/historical") do |req| req.params["date"] = date.to_s req.params["from"] = from @@ -53,7 +53,7 @@ class Provider::Synth < Provider end def fetch_exchange_rates(from:, to:, start_date:, end_date:) - with_provider_response retries: 1 do + with_provider_response do data = paginate( "#{base_url}/rates/historical-range", from: from, @@ -128,7 +128,7 @@ class Provider::Synth < Provider end def fetch_security_prices(security, start_date:, end_date:) - with_provider_response retries: 1 do + with_provider_response do params = { start_date: start_date, end_date: end_date @@ -191,14 +191,6 @@ class Provider::Synth < Provider TransactionEnrichmentData = Data.define(:name, :icon_url, :category) - def retryable_errors - [ - Faraday::TimeoutError, - Faraday::ConnectionFailed, - Faraday::SSLError - ] - end - def base_url ENV["SYNTH_URL"] || "https://api.synthfinance.com" end @@ -213,6 +205,13 @@ class Provider::Synth < Provider def client @client ||= Faraday.new(url: base_url) do |faraday| + faraday.request(:retry, { + max: 2, + interval: 0.05, + interval_randomness: 0.5, + backoff_factor: 2 + }) + faraday.response :raise_error faraday.headers["Authorization"] = "Bearer #{api_key}" faraday.headers["X-Source"] = app_name diff --git a/lib/retryable.rb b/lib/retryable.rb deleted file mode 100644 index 2f1bd002..00000000 --- a/lib/retryable.rb +++ /dev/null @@ -1,19 +0,0 @@ -module Retryable - def retrying(retryable_errors = [], max_retries: 3) - attempts = 0 - - begin - on_last_attempt = attempts == max_retries - 1 - - yield on_last_attempt - rescue *retryable_errors => e - attempts += 1 - - if attempts < max_retries - retry - else - raise e - end - end - end -end diff --git a/test/models/provider_test.rb b/test/models/provider_test.rb index 5b9a9287..6d7fe13b 100644 --- a/test/models/provider_test.rb +++ b/test/models/provider_test.rb @@ -2,60 +2,56 @@ require "test_helper" require "ostruct" class TestProvider < Provider + TestError = Class.new(StandardError) + + def initialize(client) + @client = client + end + def fetch_data - with_provider_response(retries: 3) do - client.get("/test") + with_provider_response do + @client.get("/test") end end - private - def client - @client ||= Faraday.new - end - - def retryable_errors - [ Faraday::TimeoutError ] + def fetch_data_with_error_transformer + with_provider_response(error_transformer: ->(error) { TestError.new(error.message) }) do + @client.get("/test") end + end end class ProviderTest < ActiveSupport::TestCase setup do - @provider = TestProvider.new + @client = mock + @provider = TestProvider.new(@client) end - test "retries then provides failed response" do - client = mock - Faraday.stubs(:new).returns(client) - - client.expects(:get) - .with("/test") - .raises(Faraday::TimeoutError) - .times(3) - - response = @provider.fetch_data - - assert_not response.success? - assert_match "timeout", response.error.message - end - - test "fail, retry, succeed" do - client = mock - Faraday.stubs(:new).returns(client) - - sequence = sequence("retry_sequence") - - client.expects(:get) - .with("/test") - .raises(Faraday::TimeoutError) - .in_sequence(sequence) - - client.expects(:get) - .with("/test") - .returns(Provider::Response.new(success?: true, data: "success", error: nil)) - .in_sequence(sequence) + test "returns success response with data" do + @client.expects(:get).with("/test").returns({ some: "data" }) response = @provider.fetch_data assert response.success? + assert_equal({ some: "data" }, response.data) + end + + test "returns failed response with error" do + @client.expects(:get).with("/test").raises(StandardError.new("some error")) + + response = @provider.fetch_data + + assert_not response.success? + assert_equal("some error", response.error.message) + end + + test "provider can transform error" do + @client.expects(:get).with("/test").raises(StandardError.new("some error")) + + response = @provider.fetch_data_with_error_transformer + + assert_not response.success? + assert_equal("some error", response.error.message) + assert_instance_of TestProvider::TestError, response.error end end