Configurable http_timeout on API requests (#70)

This PR adds support for setting a timeout for executing API requests. By default, it will use 60 seconds (which is also the default RestClient uses) but this can be changed by setting `config.http_timeout = xxx` to the desired number of seconds. To support this feature, the code calling RestClient had to be fully refactored.

Fixes #63

Author: jylamont

.gitignore

@@ -1,4 +1,5 @@
 info
 *.gem
 Gemfile.lock
-.ruby-version
+.ruby-version
+.zed

CHANGES.md

@@ -8,6 +8,8 @@
 
 - Added Zeitwerk for improved code autoloading
 
+- Added `http_timeout` configuration option. By default it is set to 60 (seconds).
+
 ## 0.9.0
 
 - **Added support for Sidekiq**. You can now use Sidekiq to deliver API requests to Vero. To do so, just specify `config.async = :sidekiq` in your config.rb file.

README.markdown

@@ -25,6 +25,8 @@ To get started with a Ruby on Rails application, add the following initializer:
 # config/initializers/vero.rb
 Vero::App.init do |config|
   config.tracking_api_key = ENV['VERO_TRACKING_API_KEY']
+
+  config.http_timeout = 30 # default timeout per API request is set to 60 (seconds)
 end
 ```
 

lib/vero/api/workers/base_api.rb

@@ -13,7 +13,6 @@ def self.perform(domain, options)
   def initialize(domain, options)
     @domain = domain
     self.options = options
-    setup_logging
   end
 
   def perform
@@ -22,21 +21,11 @@ def perform
   end
 
   def options=(val)
-    @options = options_with_symbolized_keys(val)
+    @options = val.transform_keys(&:to_sym)
   end
 
   protected
 
-  def setup_logging
-    return unless Vero::App.logger
-
-    RestClient.log = Object.new.tap do |proxy|
-      def proxy.<<(message)
-        Vero::App.logger.info message
-      end
-    end
-  end
-
   def url
   end
 
@@ -45,20 +34,17 @@ def validate!
   end
 
   def request
-    request_headers = {content_type: :json, accept: :json}
-
-    if request_method == :get
-      RestClient.get(url, request_headers)
-    else
-      RestClient.send(request_method, url, JSON.dump(@options), request_headers)
-    end
+    http_client.do_request(request_method, url, @options.except(:_config))
   end
 
   def request_method
     raise NotImplementedError, "#{self.class.name}#request_method should be overridden"
   end
 
-  def options_with_symbolized_keys(val)
-    val.transform_keys(&:to_sym)
+  def http_client
+    Vero::HttpClient.new(
+      logger: Vero::App.logger,
+      http_timeout: @options.dig(:_config, :http_timeout)
+    )
   end
 end

lib/vero/config.rb

@@ -4,9 +4,9 @@ class Vero::Config
   attr_writer :development_mode # Deprecated field
 
   attr_writer :domain
-  attr_accessor :tracking_api_key, :async, :disabled, :logging
+  attr_accessor :tracking_api_key, :async, :disabled, :logging, :http_timeout
 
-  ACCEPTED_ATTRIBUTES = %i[tracking_api_key async disabled logging domain]
+  ACCEPTED_ATTRIBUTES = %i[tracking_api_key async disabled logging domain http_timeout]
 
   # Extracts accepted attributes from the given object. It isn't necessarily a Vero::Config instance.
   def self.extract_accepted_attrs_from(object)
@@ -24,7 +24,10 @@ def config_params
   end
 
   def request_params
-    {tracking_api_key: tracking_api_key}.compact
+    {
+      tracking_api_key: tracking_api_key,
+      _config: {http_timeout: http_timeout}
+    }.compact
   end
 
   def domain
@@ -49,6 +52,7 @@ def reset!
     self.async = true
     self.logging = false
     self.tracking_api_key = nil
+    self.http_timeout = Vero::HttpClient::DEFAULT_HTTP_TIMEOUT
   end
 
   def update_attributes(attributes = {})

lib/vero/http_client.rb

@@ -0,0 +1,40 @@
+class Vero::HttpClient
+  DEFAULT_HTTP_TIMEOUT = 60
+
+  def initialize(http_timeout:, logger: nil)
+    @http_timeout = http_timeout
+    setup_logging!(logger) if logger
+  end
+
+  def get(url, headers = {})
+    do_request(:get, url, nil, headers)
+  end
+
+  def post(url, body, headers = {})
+    do_request(:post, url, body, headers)
+  end
+
+  def put(url, body, headers = {})
+    do_request(:put, url, body, headers)
+  end
+
+  def do_request(method, url, body = nil, headers = {})
+    request_params = {method: method, url: url, headers: default_headers.merge(headers), timeout: @http_timeout}
+    request_params[:payload] = JSON.dump(body) unless method == :get
+    RestClient::Request.execute(request_params)
+  end
+
+  private
+
+  def default_headers
+    {content_type: :json, accept: :json}
+  end
+
+  def setup_logging!(logger)
+    RestClient.log = Object.new.tap do |proxy|
+      def proxy.<<(message)
+        logger.info message
+      end
+    end
+  end
+end

spec/lib/api/base_api_spec.rb

@@ -3,7 +3,11 @@
 require "spec_helper"
 
 describe Vero::Api::Workers::Events::TrackAPI do
-  subject { Vero::Api::Workers::Events::TrackAPI.new("https://api.getvero.com", {auth_token: "abcd", identity: {email: "test@test.com"}, event_name: "test_event"}) }
+  let(:payload) do
+    {auth_token: "abcd", identity: {email: "test@test.com"}, event_name: "test_event"}
+  end
+
+  subject { Vero::Api::Workers::Events::TrackAPI.new("https://api.getvero.com", payload) }
 
   it_behaves_like "a Vero wrapper" do
     let(:end_point) { "/api/v2/events/track.json" }
@@ -12,48 +16,44 @@
   context "request with properties" do
     describe :validate! do
       it "should raise an error if event_name is a blank String" do
-        options = {auth_token: "abcd", identity: {email: "test@test.com"}, event_name: nil}
-        subject.options = options
+        subject.options = payload.except(:event_name)
         expect { subject.send(:validate!) }.to raise_error(ArgumentError)
 
-        options = {auth_token: "abcd", identity: {email: "test@test.com"}, event_name: "test_event"}
-        subject.options = options
+        subject.options = payload
         expect { subject.send(:validate!) }.to_not raise_error
       end
 
       it "should raise an error if data is not either nil or a Hash" do
-        options = {auth_token: "abcd", identity: {email: "test@test.com"}, event_name: "test_event", data: []}
-        subject.options = options
+        subject.options = payload.merge(data: [])
         expect { subject.send(:validate!) }.to raise_error(ArgumentError)
 
-        options = {auth_token: "abcd", identity: {email: "test@test.com"}, event_name: "test_event", data: nil}
-        subject.options = options
+        subject.options = payload.merge(data: nil)
         expect { subject.send(:validate!) }.to_not raise_error
 
-        options = {auth_token: "abcd", identity: {email: "test@test.com"}, event_name: "test_event", data: {}}
-        subject.options = options
+        subject.options = payload.merge(data: {})
         expect { subject.send(:validate!) }.to_not raise_error
       end
 
       it "should not raise an error when the keys are Strings" do
-        options = {"auth_token" => "abcd", "identity" => {"email" => "test@test.com"}, "event_name" => "test_event", "data" => {}}
+        options = {"auth_token" => "abcd", "identity" => {"email" => "test@test.com"}, "event_name" => "test_event",
+                   "data" => {}}
         subject.options = options
         expect { subject.send(:validate!) }.to_not raise_error
       end
 
       it "should not raise an error when keys are Strings for initialization" do
-        options = {"auth_token" => "abcd", "identity" => {"email" => "test@test.com"}, "event_name" => "test_event", "data" => {}}
-        expect { Vero::Api::Workers::Events::TrackAPI.new("https://api.getvero.com", options).send(:validate!) }.to_not raise_error
+        payload.transform_keys!(&:to_s)
+
+        expect do
+          Vero::Api::Workers::Events::TrackAPI.new("https://api.getvero.com", payload).send(:validate!)
+        end.to_not raise_error
       end
     end
 
     describe "request" do
       it "should send a request to the Vero API" do
         stub = stub_request(:post, "https://api.getvero.com/api/v2/events/track.json")
-          .with(
-            body: {auth_token: "abcd", identity: {email: "test@test.com"}, event_name: "test_event"}.to_json,
-            headers: {"Content-Type" => "application/json", "Accept" => "application/json"}
-          )
+          .with(body: payload.to_json)
           .to_return(status: 200)
 
         subject.send(:request)
@@ -65,9 +65,12 @@
 
   describe "integration test" do
     it "should not raise any errors" do
-      obj = Vero::Api::Workers::Events::TrackAPI.new("https://api.getvero.com", {auth_token: "abcd", identity: {email: "test@test.com"}, event_name: "test_event"})
+      obj = Vero::Api::Workers::Events::TrackAPI.new("https://api.getvero.com", payload)
+
+      stub_request(:post, "https://api.getvero.com/api/v2/events/track.json")
+        .with(body: payload.to_json)
+        .to_return(status: 200)
 
-      allow(RestClient).to receive(:post).and_return(200)
       expect { obj.perform }.to_not raise_error
     end
   end

spec/lib/api/events/track_api_spec.rb

@@ -4,31 +4,44 @@
 
 describe Vero::Api::Events do
   let(:subject) { Vero::Api::Events }
+  let(:mock_context) { Vero::Context.new }
 
-  describe :track! do
-    it "should call the TrackAPI object via the configured sender" do
-      input = {event_name: "test_event", identity: {email: "james@getvero.com"}, data: {test: "test"}}
-      expected = input.merge(tracking_api_key: "abc123")
+  let(:input) { {event_name: "test_event", identity: {email: "james@getvero.com"}, data: {test: "test"}} }
+  let(:expected) { input.merge(tracking_api_key: "abc123", _config: {http_timeout: 60}) }
 
-      mock_context = Vero::Context.new
-      allow(mock_context.config).to receive(:configured?).and_return(true)
-      allow(mock_context.config).to receive(:tracking_api_key).and_return("abc123")
+  before do
+    allow(mock_context.config).to receive(:configured?).and_return(true)
+    allow(mock_context.config).to receive(:tracking_api_key).and_return("abc123")
+    allow(Vero::App).to receive(:default_context).and_return(mock_context)
+  end
 
-      allow(Vero::App).to receive(:default_context).and_return(mock_context)
+  it "should pass http_timeout to API requests" do
+    allow(mock_context.config).to receive(:http_timeout).and_return(30)
 
-      expect(Vero::Sender).to receive(:call).with(Vero::Api::Workers::Events::TrackAPI, true, "https://api.getvero.com", expected)
+    expect(Vero::Sender).to(
+      receive(:call).with(
+        Vero::Api::Workers::Events::TrackAPI, true, "https://api.getvero.com", expected.merge(_config: {http_timeout: 30})
+      )
+    )
+    subject.track!(input)
+  end
 
-      subject.track!(input)
+  describe :track! do
+    context "should call the TrackAPI object via the configured sender" do
+      specify do
+        expect(Vero::Sender).to receive(:call).with(Vero::Api::Workers::Events::TrackAPI, true, "https://api.getvero.com", expected)
+        subject.track!(input)
+      end
     end
   end
 end
 
 describe Vero::Api::Users do
   let(:subject) { Vero::Api::Users }
   let(:mock_context) { Vero::Context.new }
-  let(:expected) { input.merge(tracking_api_key: "abc123") }
+  let(:expected) { input.merge(tracking_api_key: "abc123", _config: {http_timeout: 60}) }
 
-  before :each do
+  before do
     allow(mock_context.config).to receive(:configured?).and_return(true)
     allow(mock_context.config).to receive(:tracking_api_key).and_return("abc123")
     allow(Vero::App).to receive(:default_context).and_return(mock_context)

spec/lib/api_spec.rb

@@ -31,12 +31,12 @@
   end
 
   describe :request_params do
-    it "should return a hash of tracking_api_key and development_mode if they are set" do
+    it "should return a hash containing tracking_api_key if set" do
       config.tracking_api_key = nil
-      expect(config.request_params).to eq({})
+      expect(config.request_params.key?(:tracking_api_key)).to be_falsey
 
       config.tracking_api_key = "abcd1234"
-      expect(config.request_params).to eq({tracking_api_key: "abcd1234"})
+      expect(config.request_params).to include(tracking_api_key: "abcd1234")
     end
   end