[bugfix] handle unexpected response bodies (intercom#492)

* [refactor] add "m" for easier testing

* [refactor] address minitest deprecations in request_spec.rb

* [refactor] remove unnecessary raise on failure

If the JSON parsing errors, `parsed_body` is `nil` and the following
line raises the error.

* [refactor] DRY code formatting in if/else

* [refactor] allow access to pry in tests

* [refactor] clarify public methods in Request

This will help us better reason about which methods should have tests,
and which methods other objects are depending on specific behavior from.

* [refactor] clean up request specs

Done as a step toward fixing a bug in `#parse_body`

- Moves client test to client_spec.rb
- Replaces parse_body tests with higher level tests
- Makes parse_body private
- Organizes and cleans up the request_spec file generally

* [refactor] simplify response body parsing

`parse_body` was doing three things:
  - checking for response errors
  - handling parse errors
  - actually parsing the response body

We get some cleaner code by separating the parsing from the response
state checking.

* [bugfix] raise an error on unexpected response

Some responses do not have an expected error code and have non-JSON
bodies that fail to parse. The unexpected `nil` caused runtime errors.
This fix raises an error on those responses with more information that
will allow developers to find a path forward, either by more fixes to
this gem or by handling the new error in their own code.

I'm giving this version bump a patch level bump because it looks like
all existing functionality is preserved. We will now simply get better
information instead of `undefined method '[]' for nil:NilClass`.

Addresses intercom#491

* [refactor] inline private methods in Request.

Author: adsteel

README.md

@@ -560,3 +560,24 @@ intercom = Intercom::Client.new(token: ENV['AT'], handle_rate_limit: true)
 - **Send coherent history**. Make sure each individual commit in your pull
   request is meaningful. If you had to make multiple intermediate commits while
   developing, please squash them before sending them to us.
+
+### Development
+
+#### Running tests
+
+```bash
+# all tests
+bundle exec spec
+
+# unit tests
+bundle exec spec:unit
+
+# integration tests
+bundle exec spec:integration
+
+# single test file
+bundle exec m spec/unit/intercom/job_spec.rb
+
+# single test
+bundle exec m spec/unit/intercom/job_spec.rb:49
+```

intercom.gemspec

@@ -19,6 +19,7 @@ Gem::Specification.new do |spec|
   spec.require_paths = ["lib"]
 
   spec.add_development_dependency 'minitest', '~> 5.4'
+  spec.add_development_dependency "m", "~> 1.5.0"
   spec.add_development_dependency 'rake', '~> 10.3'
   spec.add_development_dependency 'mocha', '~> 1.0'
   spec.add_development_dependency "fakeweb", ["~> 1.3"]

lib/intercom/errors.rb

@@ -63,6 +63,9 @@ class BadRequestError < IntercomError; end
   # Raised when you have exceeded the API rate limit
   class RateLimitExceeded < IntercomError; end
 
+  # Raised when some attribute of the response cannot be handled
+  class UnexpectedResponseError < IntercomError; end
+
   # Raised when the request throws an error not accounted for
   class UnexpectedError < IntercomError; end
 

lib/intercom/request.rb

@@ -3,64 +3,48 @@
 
 module Intercom
   class Request
-    attr_accessor :path, :net_http_method, :rate_limit_details, :handle_rate_limit
-
-    def initialize(path, net_http_method)
-      self.path = path
-      self.net_http_method = net_http_method
-      self.handle_rate_limit = false
-    end
-
-    def set_common_headers(method, base_uri)
-      method.add_field('AcceptEncoding', 'gzip, deflate')
-    end
-
-    def set_basic_auth(method, username, secret)
-      method.basic_auth(CGI.unescape(username), CGI.unescape(secret))
-    end
+    class << self
+      def get(path, params)
+        new(path, Net::HTTP::Get.new(append_query_string_to_url(path, params), default_headers))
+      end
 
-    def set_api_version(method, api_version)
-      method.add_field('Intercom-Version', api_version)
-    end
+      def post(path, form_data)
+        new(path, method_with_body(Net::HTTP::Post, path, form_data))
+      end
 
-    def self.get(path, params)
-      new(path, Net::HTTP::Get.new(append_query_string_to_url(path, params), default_headers))
-    end
+      def delete(path, params)
+        new(path, method_with_body(Net::HTTP::Delete, path, params))
+      end
 
-    def self.post(path, form_data)
-      new(path, method_with_body(Net::HTTP::Post, path, form_data))
-    end
+      def put(path, form_data)
+        new(path, method_with_body(Net::HTTP::Put, path, form_data))
+      end
 
-    def self.delete(path, params)
-      new(path, method_with_body(Net::HTTP::Delete, path, params))
-    end
+      private def method_with_body(http_method, path, params)
+        request = http_method.send(:new, path, default_headers)
+        request.body = params.to_json
+        request["Content-Type"] = "application/json"
+        request
+      end
 
-    def self.put(path, form_data)
-      new(path, method_with_body(Net::HTTP::Put, path, form_data))
-    end
+      private def default_headers
+        {'Accept-Encoding' => 'gzip, deflate', 'Accept' => 'application/vnd.intercom.3+json', 'User-Agent' => "Intercom-Ruby/#{Intercom::VERSION}"}
+      end
 
-    def self.method_with_body(http_method, path, params)
-      request = http_method.send(:new, path, default_headers)
-      request.body = params.to_json
-      request["Content-Type"] = "application/json"
-      request
+      private def append_query_string_to_url(url, params)
+        return url if params.empty?
+        query_string = params.map { |k, v| "#{k.to_s}=#{CGI::escape(v.to_s)}" }.join('&')
+        url + "?#{query_string}"
+      end
     end
 
-    def self.default_headers
-      {'Accept-Encoding' => 'gzip, deflate', 'Accept' => 'application/vnd.intercom.3+json', 'User-Agent' => "Intercom-Ruby/#{Intercom::VERSION}"}
+    def initialize(path, net_http_method)
+      self.path = path
+      self.net_http_method = net_http_method
+      self.handle_rate_limit = false
     end
 
-    def client(uri, read_timeout:, open_timeout:)
-      net = Net::HTTP.new(uri.host, uri.port)
-      if uri.is_a?(URI::HTTPS)
-        net.use_ssl = true
-        net.verify_mode = OpenSSL::SSL::VERIFY_PEER
-        net.ca_file = File.join(File.dirname(__FILE__), '../data/cacert.pem')
-      end
-      net.read_timeout = read_timeout
-      net.open_timeout = open_timeout
-      net
-    end
+    attr_accessor :handle_rate_limit
 
     def execute(target_base_url=nil, username:, secret: nil, read_timeout: 90, open_timeout: 30, api_version: nil)
       retries = 3
@@ -72,10 +56,16 @@ def execute(target_base_url=nil, username:, secret: nil, read_timeout: 90, open_
         client(base_uri, read_timeout: read_timeout, open_timeout: open_timeout).start do |http|
           begin
             response = http.request(net_http_method)
+
             set_rate_limit_details(response)
-            decoded_body = decode_body(response)
-            parsed_body = parse_body(decoded_body, response)
             raise_errors_on_failure(response)
+
+            parsed_body = extract_response_body(response)
+
+            return nil if parsed_body.nil?
+
+            raise_application_errors_on_failure(parsed_body, response.code.to_i) if parsed_body['type'] == 'error.list'
+
             parsed_body
           rescue Intercom::RateLimitExceeded => e
             if @handle_rate_limit
@@ -98,55 +88,91 @@ def execute(target_base_url=nil, username:, secret: nil, read_timeout: 90, open_
       end
     end
 
-    def decode_body(response)
-      decode(response['content-encoding'], response.body)
-    end
+    attr_accessor :path,
+                  :net_http_method,
+                  :rate_limit_details
 
-    def parse_body(decoded_body, response)
-      parsed_body = nil
-      return parsed_body if decoded_body.nil? || decoded_body.strip.empty?
-      begin
-        parsed_body = JSON.parse(decoded_body)
-      rescue JSON::ParserError => _
-        raise_errors_on_failure(response)
+    private :path,
+            :net_http_method,
+            :rate_limit_details
+
+    private def client(uri, read_timeout:, open_timeout:)
+      net = Net::HTTP.new(uri.host, uri.port)
+      if uri.is_a?(URI::HTTPS)
+        net.use_ssl = true
+        net.verify_mode = OpenSSL::SSL::VERIFY_PEER
+        net.ca_file = File.join(File.dirname(__FILE__), '../data/cacert.pem')
       end
-      raise_errors_on_failure(response) if parsed_body.nil?
-      raise_application_errors_on_failure(parsed_body, response.code.to_i) if parsed_body['type'] == 'error.list'
-      parsed_body
+      net.read_timeout = read_timeout
+      net.open_timeout = open_timeout
+      net
     end
 
-    def set_rate_limit_details(response)
+    private def extract_response_body(response)
+      decoded_body = decode(response['content-encoding'], response.body)
+
+      json_parse_response(decoded_body, response.code)
+    end
+
+    private def decode(content_encoding, body)
+      return body if (!body) || body.empty? || content_encoding != 'gzip'
+      Zlib::GzipReader.new(StringIO.new(body)).read.force_encoding("utf-8")
+    end
+
+    private def json_parse_response(str, code)
+      return nil if str.to_s.empty?
+
+      JSON.parse(str)
+    rescue JSON::ParserError
+      msg = <<~MSG.gsub(/[[:space:]]+/, " ").strip # #squish from ActiveSuppor
+        Expected a JSON response body. Instead got '#{str}'
+        with status code '#{code}'.
+      MSG
+
+      raise UnexpectedResponseError, msg
+    end
+
+    private def set_rate_limit_details(response)
       rate_limit_details = {}
       rate_limit_details[:limit] = response['X-RateLimit-Limit'].to_i if response['X-RateLimit-Limit']
       rate_limit_details[:remaining] = response['X-RateLimit-Remaining'].to_i if response['X-RateLimit-Remaining']
       rate_limit_details[:reset_at] = Time.at(response['X-RateLimit-Reset'].to_i) if response['X-RateLimit-Reset']
       @rate_limit_details = rate_limit_details
     end
 
-    def decode(content_encoding, body)
-      return body if (!body) || body.empty? || content_encoding != 'gzip'
-      Zlib::GzipReader.new(StringIO.new(body)).read.force_encoding("utf-8")
+    private def set_common_headers(method, base_uri)
+      method.add_field('AcceptEncoding', 'gzip, deflate')
     end
 
-    def raise_errors_on_failure(res)
-      if res.code.to_i.eql?(404)
+    private def set_basic_auth(method, username, secret)
+      method.basic_auth(CGI.unescape(username), CGI.unescape(secret))
+    end
+
+    private def set_api_version(method, api_version)
+      method.add_field('Intercom-Version', api_version)
+    end
+
+    private def raise_errors_on_failure(res)
+      code = res.code.to_i
+
+      if code == 404
         raise Intercom::ResourceNotFound.new('Resource Not Found')
-      elsif res.code.to_i.eql?(401)
+      elsif code == 401
         raise Intercom::AuthenticationError.new('Unauthorized')
-      elsif res.code.to_i.eql?(403)
+      elsif code == 403
         raise Intercom::AuthenticationError.new('Forbidden')
-      elsif res.code.to_i.eql?(429)
+      elsif code == 429
         raise Intercom::RateLimitExceeded.new('Rate Limit Exceeded')
-      elsif res.code.to_i.eql?(500)
+      elsif code == 500
         raise Intercom::ServerError.new('Server Error')
-      elsif res.code.to_i.eql?(502)
+      elsif code == 502
         raise Intercom::BadGatewayError.new('Bad Gateway Error')
-      elsif res.code.to_i.eql?(503)
+      elsif code == 503
         raise Intercom::ServiceUnavailableError.new('Service Unavailable')
       end
     end
 
-    def raise_application_errors_on_failure(error_list_details, http_code)
+    private def raise_application_errors_on_failure(error_list_details, http_code)
       # Currently, we don't support multiple errors
       error_details = error_list_details['errors'].first
       error_code = error_details['type'] || error_details['code']
@@ -198,18 +224,12 @@ def raise_application_errors_on_failure(error_list_details, http_code)
       end
     end
 
-    def message_for_unexpected_error_with_type(error_details, parsed_http_code)
+    private def message_for_unexpected_error_with_type(error_details, parsed_http_code)
       "The error of type '#{error_details['type']}' is not recognized. It occurred with the message: #{error_details['message']} and http_code: '#{parsed_http_code}'. Please contact Intercom with these details."
     end
 
-    def message_for_unexpected_error_without_type(error_details, parsed_http_code)
+    private def message_for_unexpected_error_without_type(error_details, parsed_http_code)
       "An unexpected error occured. It occurred with the message: #{error_details['message']} and http_code: '#{parsed_http_code}'. Please contact Intercom with these details."
     end
-
-    def self.append_query_string_to_url(url, params)
-      return url if params.empty?
-      query_string = params.map { |k, v| "#{k.to_s}=#{CGI::escape(v.to_s)}" }.join('&')
-      url + "?#{query_string}"
-    end
   end
 end

lib/intercom/version.rb

@@ -1,3 +1,3 @@
 module Intercom #:nodoc:
-  VERSION = "3.9.0"
+  VERSION = "3.9.1"
 end

spec/spec_helper.rb

@@ -3,6 +3,7 @@
 require 'mocha/setup'
 require 'webmock'
 require 'time'
+require 'pry'
 include WebMock::API
 
 def test_customer(email="bob@example.com")

spec/unit/intercom/client_spec.rb

@@ -4,12 +4,22 @@ module Intercom
   describe Client do
     let(:app_id) { 'myappid' }
     let(:api_key) { 'myapikey' }
-    let(:client) { Client.new(app_id: app_id, api_key: api_key) }
+    let(:client) do
+      Client.new(
+        app_id: app_id,
+        api_key: api_key,
+        handle_rate_limit: true
+      )
+    end
 
     it 'should set the base url' do
       client.base_url.must_equal('https://api.intercom.io')
     end
 
+    it 'should have handle_rate_limit set' do
+      _(client.handle_rate_limit).must_equal(true)
+    end
+
     it 'should be able to change the base url' do
       prev = client.options(Intercom::Client.set_base_url('https://mymockintercom.io'))
       client.base_url.must_equal('https://mymockintercom.io')

spec/unit/intercom/request_spec.rb

@@ -3,107 +3,146 @@
 
 WebMock.enable!
 
-describe 'Intercom::Request' do
-  it 'raises an error when a html error page rendered' do
-    response = OpenStruct.new(:code => 500)
-    req = Intercom::Request.new('path/', 'GET')
-    proc {req.parse_body('<html>something</html>', response)}.must_raise(Intercom::ServerError)
+describe 'Intercom::Request', '#execute' do
+  let(:uri) {"https://api.intercom.io/users"}
+  let(:req) { Intercom::Request.get(uri, {}) }
+  let(:default_body) { { data: "test" }.to_json }
+
+  def execute!
+    req.execute(uri, username: 'ted', secret: '')
   end
 
-  it 'raises a RateLimitExceeded error when the response code is 429' do
-    response = OpenStruct.new(:code => 429)
-    req = Intercom::Request.new('path/', 'GET')
-    proc {req.parse_body('<html>something</html>', response)}.must_raise(Intercom::RateLimitExceeded)
+  it 'should call sleep for rate limit error three times and raise a rate limit error otherwise' do
+    stub_request(:any, uri).to_return(
+      status: [429, "Too Many Requests"],
+      headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s },
+      body: default_body
+    )
+
+    req.handle_rate_limit=true
+
+    req.expects(:sleep).times(3).with(any_parameters)
+
+    expect { execute! }.must_raise(Intercom::RateLimitExceeded)
   end
 
-  it 'parse_body raises an error if the decoded_body is "null"' do
-    response = OpenStruct.new(:code => 500)
-    req = Intercom::Request.new('path/', 'GET')
-    proc { req.parse_body('null', response)}.must_raise(Intercom::ServerError)
+  it 'should not call sleep for rate limit error' do
+    stub_request(:any, uri).to_return(
+      status: [200, "OK"],
+      headers: { 'X-RateLimit-Reset' => Time.now.utc + 10 },
+      body: default_body
+    )
+
+    req.handle_rate_limit=true
+    req.expects(:sleep).never.with(any_parameters)
+
+    execute!
   end
 
-  describe 'Intercom::Client' do
-    let(:client) { Intercom::Client.new(token: 'foo', handle_rate_limit: true) }
-    let(:uri) {"https://api.intercom.io/users"}
+  it 'should call sleep for rate limit error just once' do
+    stub_request(:any, uri).to_return(
+      status: [429, "Too Many Requests"],
+      headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s },
+    ).then.to_return(status: [200, "OK"], body: default_body)
 
-    it 'should have handle_rate_limit set' do
-       client.handle_rate_limit.must_equal(true)
-    end
+    req.handle_rate_limit=true
+    req.expects(:sleep).with(any_parameters)
+
+    execute!
+  end
+
+  it 'should not sleep if rate limit reset time has passed' do
+    stub_request(:any, uri).to_return(
+      status: [429, "Too Many Requests"],
+      headers: { 'X-RateLimit-Reset' => Time.parse("February 25 2010").utc.to_i.to_s },
+      body: default_body
+    ).then.to_return(status: [200, "OK"], body: default_body)
+
+    req.handle_rate_limit=true
+    req.expects(:sleep).never.with(any_parameters)
+
+    execute!
+  end
+
+  it 'handles an empty body gracefully' do
+    stub_request(:any, uri).to_return(
+      status: 200,
+      body: nil
+    )
 
-    it 'should call sleep for rate limit error three times and raise a rate limit error otherwise' do
-      expect {
-        stub_request(:any, uri).\
-        to_return(status: [429, "Too Many Requests"], headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s })
-        req = Intercom::Request.get(uri, "")
-        req.handle_rate_limit=true
-        req.expects(:sleep).times(3).with(any_parameters)
-        req.execute(uri, username: "ted", secret: "")
-      }.must_raise(Intercom::RateLimitExceeded)
+    assert_nil(execute!)
+  end
+
+  describe 'HTTP error handling' do
+    it 'raises an error when the response is successful but the body is not JSON' do
+      stub_request(:any, uri).to_return(
+        status: 200,
+        body: '<html>something</html>'
+      )
+
+      expect { execute! }.must_raise(Intercom::UnexpectedResponseError)
     end
 
-    it 'should not call sleep for rate limit error' do
-      # Use webmock to mock the HTTP request
-      stub_request(:any, uri).\
-      to_return(status: [200, "OK"], headers: { 'X-RateLimit-Reset' => Time.now.utc + 10 })
-      req = Intercom::Request.get(uri, "")
-      req.handle_rate_limit=true
-      req.expects(:sleep).never.with(any_parameters)
-      req.execute(uri, username: "ted", secret: "")
+    it 'raises an error when an html error page rendered' do
+      stub_request(:any, uri).to_return(
+        status: 500,
+        body: '<html>something</html>'
+      )
+
+      expect { execute! }.must_raise(Intercom::ServerError)
     end
 
-    it 'should call sleep for rate limit error just once' do
-      # Use webmock to mock the HTTP request
-      stub_request(:any, uri).\
-      to_return(status: [429, "Too Many Requests"], headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s }).\
-      then.to_return(status: [200, "OK"])
-      req = Intercom::Request.get(uri, "")
-      req.handle_rate_limit=true
-      req.expects(:sleep).with(any_parameters)
-      req.execute(uri, username: "ted", secret: "")
+    it 'raises an error if the decoded_body is "null"' do
+      stub_request(:any, uri).to_return(
+        status: 500,
+        body: 'null'
+      )
+
+      expect { execute! }.must_raise(Intercom::ServerError)
     end
 
-    it 'should not sleep if rate limit reset time has passed' do
-      # Use webmock to mock the HTTP request
-      stub_request(:any, uri).\
-      to_return(status: [429, "Too Many Requests"], headers: { 'X-RateLimit-Reset' => Time.parse("February 25 2010").utc.to_i.to_s }).\
-      then.to_return(status: [200, "OK"])
-      req = Intercom::Request.get(uri, "")
-      req.handle_rate_limit=true
-      req.expects(:sleep).never.with(any_parameters)
-      req.execute(uri, username: "ted", secret: "")
+    it 'raises a RateLimitExceeded error when the response code is 429' do
+      stub_request(:any, uri).to_return(
+        status: 429,
+        body: 'null'
+      )
+
+      expect { execute! }.must_raise(Intercom::RateLimitExceeded)
     end
   end
 
-
-  describe "Application errors on failure" do
+  describe "application error handling" do
     let(:uri) {"https://api.intercom.io/conversations/reply"}
+    let(:req) { Intercom::Request.put(uri, {}) }
+
     it 'should raise ResourceNotUniqueError error on resource_conflict code' do
-      # Use webmock to mock the HTTP request
-      stub_request(:put, uri).\
-      to_return(status: [409, "Resource Already Exists"], headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s }, body: {type: "error.list", errors: [ code: "resource_conflict" ]}.to_json)
-      req = Intercom::Request.put(uri, "")
-      expect { req.execute(uri, username: "ted", secret: "") }.must_raise(Intercom::ResourceNotUniqueError)
+      stub_request(:put, uri).to_return(
+        status: [409, "Resource Already Exists"],
+        headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s },
+        body: { type: "error.list", errors: [ code: "resource_conflict" ] }.to_json
+      )
+
+      expect { execute! }.must_raise(Intercom::ResourceNotUniqueError)
     end
 
     it 'should raise ApiVersionInvalid error on intercom_version_invalid code' do
-      # Use webmock to mock the HTTP request
-      stub_request(:put, uri).\
-      to_return(status: [400, "Bad Request"], headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s }, body: {type: "error.list", errors: [ code: "intercom_version_invalid" ]}.to_json)
-      req = Intercom::Request.put(uri, "")
-      expect { req.execute(uri, username: "ted", secret: "") }.must_raise(Intercom::ApiVersionInvalid)
+      stub_request(:put, uri).to_return(
+        status: [400, "Bad Request"],
+        headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s },
+        body: { type: "error.list", errors: [ code: "intercom_version_invalid" ] }.to_json
+      )
+
+      expect { execute! }.must_raise(Intercom::ApiVersionInvalid)
     end
 
     it 'should raise ResourceNotFound error on company_not_found code' do
-      stub_request(:put, uri).\
-      to_return(status: [404, "Not Found"], headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s }, body: {type: "error.list", errors: [ code: "company_not_found" ]}.to_json)
-      req = Intercom::Request.put(uri, "")
-      expect { req.execute(uri, username: "ted", secret: "") }.must_raise(Intercom::ResourceNotFound)
-    end
-  end
+      stub_request(:put, uri).to_return(
+        status: [404, "Not Found"],
+        headers: { 'X-RateLimit-Reset' => (Time.now.utc + 10).to_i.to_s },
+        body: { type: "error.list", errors: [ code: "company_not_found" ] }.to_json
+      )
 
-  it 'parse_body returns nil if decoded_body is nil' do
-    response = OpenStruct.new(:code => 500)
-    req = Intercom::Request.new('path/', 'GET')
-    assert_nil(req.parse_body(nil, response))
+      expect { execute! }.must_raise(Intercom::ResourceNotFound)
+    end
   end
 end