Use faraday retry, move retry logic to concrete provider level (#2042)

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)

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

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

lib/retryable.rb

@@ -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

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
+  def fetch_data_with_error_transformer
+    with_provider_response(error_transformer: ->(error) { TestError.new(error.message) }) do
+      @client.get("/test")
     end
-
-    def retryable_errors
-      [ Faraday::TimeoutError ]
   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