fix(request): Handle edge cases in response parsing (#29)

README.md

@@ -21,12 +21,18 @@ end
 ```
 
 **Instantiating with a block:**
+
+- **NOTE:** This pretty much works the same way as passing a block to `Faraday.new`. Any config you can use with `Faraday` directly, you can do with `HTTPigeon::Request`
+
 ```ruby
+require "faraday/retry"
+
 # @option [String] base_url the base URI (required)
-request = HTTPigeon::Request.new(base_url: 'https://dummyjson.com') do |connection|
-  # connection is an instance of Faraday::Connection
-  connection.headers['foo'] = 'barzzz'
-  connection.options['timeout'] = 15
+request = HTTPigeon::Request.new(base_url: 'https://dummyjson.com') do |config|
+  # config is an instance of Faraday::Connection
+  config.headers['foo'] = 'barzzz'
+  config.options['timeout'] = 15
+  config.request :retry, { max: 5 }
   ...
 end
 

bin/console

@@ -7,8 +7,7 @@ require "httpigeon"
 # You can add fixtures and/or initialization code here to make experimenting
 # with your gem easier. You can also use a different console, if you like.
 
-# (If you use this, don't forget to add pry to your Gemfile!)
-# require "pry"
+require "pry"
 # Pry.start
 
 require "irb"

lib/httpigeon/request.rb

@@ -32,12 +32,14 @@ def post(endpoint, payload, headers = {}, event_type = nil, log_filters = [])
 
     def initialize(base_url:, options: nil, headers: nil, adapter: nil, logger: nil, event_type: nil, log_filters: nil)
       @base_url = base_url
-      @event_type = event_type
-      @log_filters = log_filters || []
-      @logger = logger || default_logger
 
       request_headers = default_headers.merge(headers.to_h)
-      base_connection = Faraday.new(url: base_url)
+
+      base_connection = Faraday.new(url: base_url).tap do |config|
+        config.headers.deep_merge!(request_headers)
+        config.options.merge!(options.to_h)
+        config.response :httpigeon_logger, logger if logger.is_a?(HTTPigeon::Logger)
+      end
 
       @connection = if block_given?
                       yield(base_connection) && base_connection
@@ -47,7 +49,7 @@ def initialize(base_url:, options: nil, headers: nil, adapter: nil, logger: nil,
                         faraday.options.merge!(options.to_h)
                         faraday.request :url_encoded
                         faraday.adapter adapter || Faraday.default_adapter
-                        faraday.response :httpigeon_logger, @logger
+                        faraday.response :httpigeon_logger, default_logger(event_type, log_filters) unless logger.is_a?(HTTPigeon::Logger)
                       end
                     end
     end
@@ -67,15 +69,24 @@ def run(method: :get, path: '/', payload: {})
 
     private
 
-    attr_reader :path, :logger, :event_type, :log_filters
+    attr_reader :logger, :event_type, :log_filters
 
     def parse_response
-      JSON.parse(response_body).with_indifferent_access
-    rescue JSON::ParserError
+      parsed_body = response_body.is_a?(String) ? JSON.parse(response_body) : response_body
+
+      case parsed_body
+      when Hash
+        parsed_body.with_indifferent_access
+      when Array
+        parsed_body.map(&:with_indifferent_access)
+      else
+        parsed_body
+      end
+    rescue JSON::ParserError, NoMethodError
       response_body.presence
     end
 
-    def default_logger
+    def default_logger(event_type, log_filters)
       HTTPigeon::Logger.new(event_type: event_type, log_filters: log_filters)
     end
 

spec/httpigeon/request_spec.rb

@@ -42,44 +42,140 @@
   end
 
   describe '#new' do
-    it 'sets the expected default headers' do
-      allow(SecureRandom).to receive(:uuid).and_return('secure-random-uuid')
-
-      test_table = [
-        { # when auto generate request id is turned off
-          auto_generate_request_id: false,
-          request_headers: { 'Foo' => 'barzz' },
-          included_headers: { 'Accept' => 'application/json', 'Foo' => 'barzz' }
-        },
-        { # when auto generate request id is turned on
-          auto_generate_request_id: true,
-          request_headers: { 'Foo' => 'barzz' },
-          included_headers: { 'Accept' => 'application/json', 'X-Request-Id' => 'secure-random-uuid', 'Foo' => 'barzz' }
-        }
-      ]
-
-      test_table.each do |test_case|
-        allow(HTTPigeon).to receive(:auto_generate_request_id).and_return(test_case[:auto_generate_request_id])
-        request = described_class.new(base_url: 'https://www.example.com', headers: test_case[:request_headers], event_type: 'event.type', log_filters: [:super_secret])
-
-        expect(request.connection.headers.slice(*%w[Accept X-Request-Id Foo])).to eq(test_case[:included_headers])
-      end
+    let(:custom_logger_klass) { double('some-custom-logger-klass') }
+
+    before do
+      logger_instance = double('instance-of-custom-logger')
+
+      allow(logger_instance).to receive(:is_a?).with(HTTPigeon::Logger).and_return(true)
+      allow(custom_logger_klass).to receive(:new).and_return(logger_instance)
     end
 
-    it 'uses the default logger if a custom logger is not provided' do
-      allow(HTTPigeon::Logger).to receive(:new)
+    context 'when a block is given' do
+      it 'sets the expected default headers' do
+        allow(SecureRandom).to receive(:uuid).and_return('secure-random-uuid')
+
+        test_table = [
+          { # when auto generate request id is turned off
+            auto_generate_request_id: false,
+            request_headers: { 'Foo' => 'barzz' },
+            included_headers: { 'Accept' => 'application/json', 'Foo' => 'barzz' }
+          },
+          { # when auto generate request id is turned on
+            auto_generate_request_id: true,
+            request_headers: { 'Foo' => 'barzz' },
+            included_headers: { 'Accept' => 'application/json', 'X-Request-Id' => 'secure-random-uuid', 'Foo' => 'barzz' }
+          }
+        ]
+
+        test_table.each do |test_case|
+          allow(HTTPigeon).to receive(:auto_generate_request_id).and_return(test_case[:auto_generate_request_id])
+
+          request = described_class.new(base_url: 'https://www.example.com', headers: test_case[:request_headers]) do |conn|
+            conn.request :json
+            conn.response :json
+          end
+
+          expect(request.connection.headers.slice(*%w[Accept X-Request-Id Foo])).to eq(test_case[:included_headers])
+        end
+      end
+
+      it 'sets the request options if provided' do
+        request = described_class.new(base_url: 'https://www.example.com', options: { timeout: 10 }) do |conn|
+          conn.request :json
+          conn.response :json
+        end
+
+        expect(request.connection.options.timeout).to eq(10)
+      end
 
-      described_class.new(base_url: 'https://www.example.com', event_type: 'event.type', log_filters: [:super_secret])
+      it 'does not use the default logger if a custom logger is not provided' do
+        allow(HTTPigeon::Logger).to receive(:new)
+
+        described_class.new(base_url: 'https://www.example.com') do |conn|
+          conn.request :json
+          conn.response :json
+        end
+
+        expect(HTTPigeon::Logger).not_to have_received(:new)
+      end
+
+      it 'sets the custom :httpigeon_logger if provided and is a type of httpigeon logger' do
+        allow(HTTPigeon::Logger).to receive(:new)
+
+        request = described_class.new(base_url: 'https://www.example.com', logger: custom_logger_klass.new) do |conn|
+          conn.request :json
+          conn.response :json
+        end
+
+        expect(HTTPigeon::Logger).not_to have_received(:new)
+        expect(request.connection.builder.handlers).to include(HTTPigeon::Middleware::HTTPigeonLogger)
+      end
 
-      expect(HTTPigeon::Logger).to have_received(:new).with(event_type: 'event.type', log_filters: [:super_secret])
+      it 'does not set a custom :httpigeon_logger if provided and is not a type of httpigeon logger' do
+        allow(HTTPigeon::Logger).to receive(:new)
+
+        request = described_class.new(base_url: 'https://www.example.com', logger: Logger.new($stdout)) do |conn|
+          conn.request :json
+          conn.response :json
+        end
+
+        expect(HTTPigeon::Logger).not_to have_received(:new)
+        expect(request.connection.builder.handlers).not_to include(HTTPigeon::Middleware::HTTPigeonLogger)
+      end
     end
 
-    it 'uses the custom logger if one if provided' do
-      allow(HTTPigeon::Logger).to receive(:new)
+    context 'when a block is not given' do
+      it 'sets the expected default headers' do
+        allow(SecureRandom).to receive(:uuid).and_return('secure-random-uuid')
+
+        test_table = [
+          { # when auto generate request id is turned off
+            auto_generate_request_id: false,
+            request_headers: { 'Foo' => 'barzz' },
+            included_headers: { 'Accept' => 'application/json', 'Foo' => 'barzz' }
+          },
+          { # when auto generate request id is turned on
+            auto_generate_request_id: true,
+            request_headers: { 'Foo' => 'barzz' },
+            included_headers: { 'Accept' => 'application/json', 'X-Request-Id' => 'secure-random-uuid', 'Foo' => 'barzz' }
+          }
+        ]
+
+        test_table.each do |test_case|
+          allow(HTTPigeon).to receive(:auto_generate_request_id).and_return(test_case[:auto_generate_request_id])
+          request = described_class.new(base_url: 'https://www.example.com', headers: test_case[:request_headers], event_type: 'event.type', log_filters: [:super_secret])
+
+          expect(request.connection.headers.slice(*%w[Accept X-Request-Id Foo])).to eq(test_case[:included_headers])
+        end
+      end
+
+      it 'uses the default logger if a custom logger is not provided' do
+        allow(HTTPigeon::Logger).to receive(:new)
+
+        request = described_class.new(base_url: 'https://www.example.com', event_type: 'event.type', log_filters: [:super_secret])
+
+        expect(HTTPigeon::Logger).to have_received(:new).with(event_type: 'event.type', log_filters: [:super_secret])
+        expect(request.connection.builder.handlers).to include(HTTPigeon::Middleware::HTTPigeonLogger)
+      end
+
+      it 'uses the custom if provided and is a type of httpigeon logger' do
+        allow(HTTPigeon::Logger).to receive(:new)
 
-      described_class.new(base_url: 'http://www.example.com', logger: Logger.new($stdout))
+        request = described_class.new(base_url: 'http://www.example.com', logger: custom_logger_klass.new)
+
+        expect(HTTPigeon::Logger).not_to have_received(:new)
+        expect(request.connection.builder.handlers).to include(HTTPigeon::Middleware::HTTPigeonLogger)
+      end
 
-      expect(HTTPigeon::Logger).not_to have_received(:new)
+      it 'uses the default logger if a custom logger is provided but is not a type of httpigeon logger' do
+        allow(HTTPigeon::Logger).to receive(:new)
+
+        request = described_class.new(base_url: 'https://www.example.com', logger: Logger.new($stdout))
+
+        expect(HTTPigeon::Logger).to have_received(:new)
+        expect(request.connection.builder.handlers).to include(HTTPigeon::Middleware::HTTPigeonLogger)
+      end
     end
   end
 

spec/spec_helper.rb

@@ -8,6 +8,7 @@
 end
 
 require 'httpigeon'
+require 'pry'
 
 # See https://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration
 # for more details on configuration