Refactor and add test coverage for HTTP Client

app/services/ct_gov/api_client/base.rb

@@ -1,6 +1,6 @@
 module CTGov
   class ApiClient::Base
-    include HttpService
+    include HttpClient
 
     BASE_URL = "https://clinicaltrials.gov/api".freeze
 

app/services/ct_gov/api_client/v2.rb

@@ -25,27 +25,25 @@ def fetch_studies(range: nil, nct_ids: nil, page_size: nil)
 
       loop do
         params[:pageToken] = page_token
-        Rails.logger.debug("Fetching studies with params: #{params}")
 
         result = get("studies", params.compact)
 
-        if result.nil? || result["studies"].nil?
-          Rails.logger.warn("Received nil response or empty studies from API")
-          break
+        if result["studies"].nil?
+          raise "CTGov API returned invalid response after fetching #{total_fetched} studies"
         end
 
         studies = result["studies"]
 
         # Capture total count on the first run
-        total_count = result["totalCount"] if result["totalCount"]
+        total_count = result["totalCount"] if result["totalCount"] # on first page only
         total_fetched += studies.size
-        Rails.logger.info("Dowloaded #{total_fetched} from #{total_count} studies")
+        # TODO: refactor out logging functionality
         puts "Dowloaded #{total_fetched} from #{total_count} studies"
-        # Yield each page of studies to the caller
+
         yield studies
 
         page_token = result["nextPageToken"]
-        break if page_token.nil? || total_fetched >= total_count
+        break if page_token.nil?
       end
     end
 

app/services/ct_gov/study_sync_service.rb

@@ -9,6 +9,7 @@ def initialize(api_client = CTGov::ApiClient::V2.new)
       @api_client = api_client
     end
 
+    # TODO: review error handling - currently all propagated to the Updater
     def sync_recent_studies_from_api
       start_date = get_sync_start_date
       @api_client.get_studies_in_date_range(start_date: start_date, page_size: 500) do |studies|
@@ -19,7 +20,7 @@ def sync_recent_studies_from_api
     def refresh_studies_from_db
       list = Study.order(updated_at: :asc).limit(500).pluck(:nct_id)
       raise "No Studies found to sync" if list.empty?
-      @api_client.get_studies_by_nct_ids(list: list, page_size: 500) do |studies|
+      @api_client.get_studies_by_nct_ids(list: list, page_size: 1000) do |studies|
         persist(studies)
 
         # removing study logic

app/services/http_client.rb

@@ -0,0 +1,35 @@
+module HttpClient
+
+  def setup_connection(base_url, timeout: 10, open_timeout: 5, retries: 3)
+    retry_options = {
+      max: retries,
+      interval: 0.05,
+      interval_randomness: 0.5,
+      backoff_factor: 2,
+      methods: %i[get],
+      # exceptions to trigger a retry
+      exceptions: [
+        Faraday::TimeoutError, 
+        Faraday::ConnectionFailed,
+        Faraday::ServerError
+      ]
+    }
+
+    @connection = Faraday.new(url: base_url) do |f|
+      f.request :json # for post requests - not currently used
+      f.request :retry, retry_options # faraday-retry gem
+      # using Rails.logger for now - review options
+      f.response :logger, Rails.logger, { headers: false, bodies: false, errors: true }
+      f.response :raise_error # 40x, 50x errors -> Faraday::ClientError, Faraday::ServerError
+      # convert JSON string to Ruby hash inside body if header is application/json
+      f.response :json # fails with Faraday::ParsingError
+      f.adapter Faraday.default_adapter # keep for clarity
+      f.options.timeout = timeout # max timeout for request
+      f.options.open_timeout = open_timeout # for connection to open
+    end
+  end
+
+  def get(endpoint, params = {})
+    @connection.get(endpoint, params).body
+  end
+end

spec/services/http_client_spec.rb

@@ -0,0 +1,110 @@
+require "rails_helper"
+require_relative "../../app/services/http_client"
+
+describe HttpClient do
+  let(:base_url) { "https://example.com/" }
+  let(:endpoint) { "api/endpoint" }
+  let(:url) { "#{base_url}#{endpoint}" }
+  let(:params) { { param1: "value1", param2: "value2" } }
+  let(:response_body) { { "key" => "value" } }
+  let(:retries) { 2 }
+
+  # Define the webmock stub for GET requests
+  let(:get_stub_request) { stub_request(:get, url).with(query: params) }
+
+  # dynamically create a class for testing
+  let(:api_client_class) do
+    Class.new do
+      include HttpClient
+    end
+  end
+
+  let(:api_client) { api_client_class.new }
+
+  before do
+    api_client.setup_connection(base_url, retries: retries)
+  end
+
+  # including faraday configuration tests since business logic relies on it
+  # raising 4xx and 5xx errors, parsing JSON responses, and retrying requests
+  describe "#setup_connection" do
+    let(:connection) { api_client.instance_variable_get(:@connection) }
+    let(:handlers) { connection.builder.handlers }
+
+    it "sets up the connection as a Faraday connection" do
+      expect(connection).to be_a(Faraday::Connection)
+    end
+
+    it "includes the JSON response middleware" do
+      expect(handlers).to include(Faraday::Response::Json)
+    end
+
+    it "includes the JSON request middleware" do
+      expect(handlers).to include(Faraday::Request::Json)
+    end
+
+    it "configures the retry middleware" do
+      expect(handlers).to include(Faraday::Retry::Middleware)
+    end
+
+    it "includes the raise_error middleware" do
+      expect(handlers).to include(Faraday::Response::RaiseError)
+    end
+  end
+
+  describe "#get" do
+    context "successful request" do
+      it "parses and returns JSON response" do
+        get_stub_request.to_return(
+          status: 200,
+          body: response_body.to_json, # simulate a JSON response
+          headers: { "Content-Type": "application/json" } # ensure JSON parsing
+        )
+
+        response = api_client.get(endpoint, params)
+        expect(response).to eq(response_body)
+      end
+    end
+
+    context "handling errors" do
+      it "raises a Faraday::ClientError after no retries for 4xx errors" do
+        get_stub_request.to_return(status: 404, body: 'Not Found')
+
+        expect {
+          api_client.get(endpoint, params)
+        }.to raise_error(Faraday::ClientError)
+
+        # no retries for client errors
+        expect(WebMock).to have_requested(:get, url)
+                          .with(query: params)
+                          .times(1) # initial request only
+      end
+
+      it "raises a Faraday::ServerError after retries for 5xx errors" do
+        get_stub_request.to_return(status: 500, body: 'Internal Server Error')
+
+        expect {
+          api_client.get(endpoint, params)
+        }.to raise_error(Faraday::ServerError)
+
+        expect(WebMock).to have_requested(:get, url)
+                          .with(query: params)
+                          .times(1 + retries) # initial request + retries
+      end
+    end
+
+    context "when a request times out" do
+      it "retries the request and raises Faraday::TimeoutError after retries" do
+        get_stub_request.to_timeout
+
+        expect {
+          api_client.get(endpoint, params)
+        }.to raise_error(Faraday::ConnectionFailed) # final error after retries
+
+        expect(WebMock).to have_requested(:get, url)
+                          .with(query: params)
+                          .times(1 + retries) # initial request + retries
+      end
+    end
+  end
+end