refactor of error handling

lib/instapaper/error.rb

@@ -4,14 +4,32 @@ module Instapaper
     # @return [Integer]
     attr_reader :code
 
+    ClientError = Class.new(self)
+    ServerError = Class.new(self)
     ServiceUnavailableError = Class.new(self)
     BookmarkError = Class.new(self)
     FolderError = Class.new(self)
     HighlightError = Class.new(self)
     OAuthError = Class.new(self)
 
-    ERRORS = {
+    CLIENT_ERRORS = {
-      401  => 'Unauthorized',
+      400 => 'Bad Request',
+      401 => 'Unauthorized',
+      402 => 'Payment Required',
+      403 => 'Forbidden',
+      404 => 'Not Found',
+      405 => 'Method Not Allowed',
+    }
+
+    SERVER_ERRORS = {
+      500 => 'Internal Server Error',
+      501 => 'Not Implemented',
+      502 => 'Bad Gateway',
+      503 => 'Service Unavailable',
+      504 => 'Gateway Timeout',
+    }
+
+    SERVICE_ERRORS = {
       1040 => 'Rate-limit exceeded',
       1041 => 'Premium account required',
       1042 => 'Application is suspended',
@@ -43,29 +61,47 @@ module Instapaper
     }
 
     CODES = [
-      ERRORS,
+      CLIENT_ERRORS,
+      SERVER_ERRORS,
+      SERVICE_ERRORS,
       BOOKMARK_ERRORS,
       FOLDER_ERRORS,
       HIGHLIGHT_ERRORS,
     ].collect(&:keys).flatten
 
+    HTTP_ERRORS = CLIENT_ERRORS.merge(SERVER_ERRORS)
+
     # Create a new error from an HTTP response
     #
     # @param response [HTTP::Response]
     # @return [Instapaper::Error]
     def self.from_response(code, path)
-      if ERRORS.keys.include?(code)
+      if (HTTP_ERRORS.keys + SERVICE_ERRORS.keys).include?(code)
-        new(ERRORS[code], code)
+        from_response_code(code)
       else
         case path
         when /highlights/ then HighlightError.new(HIGHLIGHT_ERRORS[code], code)
         when /bookmarks/ then BookmarkError.new(BOOKMARK_ERRORS[code], code)
         when /folders/ then FolderError.new(FOLDER_ERRORS[code], code)
-        else new('Unknown Error Code', code)
+        else new('Unknown Error', code)
         end
       end
     end
 
+    # Create a new error from an HTTP response code
+    #
+    # @param code [Integer]
+    # @return [Instapaper::Error]
+    def self.from_response_code(code)
+      if CLIENT_ERRORS.keys.include?(code)
+        ClientError.new(CLIENT_ERRORS[code], code)
+      elsif SERVER_ERRORS.keys.include?(code)
+        ServerError.new(SERVER_ERRORS[code], code)
+      elsif SERVICE_ERRORS.keys.include?(code)
+        new(SERVICE_ERRORS[code], code)
+      end
+    end
+
     # Initializes a new Error object
     #
     # @param message [Exception, String]

lib/instapaper/http/request.rb

@@ -3,8 +3,8 @@ require 'http'
 require 'json'
 require 'net/https'
 require 'openssl'
-require 'instapaper/error'
 require 'instapaper/http/headers'
+require 'instapaper/http/response'
 
 module Instapaper
   module HTTP
@@ -27,61 +27,17 @@ module Instapaper
 
       # @return [Array, Hash]
       def perform
-        perform_request
+        raw = @options.delete(:raw)
+        response = Instapaper::HTTP::Response.new(perform_request, path, raw)
+        response.valid? && response.body
       end
 
       private
 
       def perform_request
-        raw = @options.delete(:raw)
         @headers = Instapaper::HTTP::Headers.new(@client, @request_method, @uri, @options).request_headers
         options_key = @request_method == :get ? :params : :form
-        response = ::HTTP.headers(@headers).public_send(@request_method, @uri.to_s, options_key => @options)
+        ::HTTP.headers(@headers).public_send(@request_method, @uri.to_s, options_key => @options)
-        fail_if_error(response, raw)
-        raw ? response.to_s : parsed_response(response)
-      end
-
-      def fail_if_error(response, raw)
-        fail_if_error_unparseable_response(response) unless raw
-        fail_if_error_in_body(parsed_response(response))
-        fail_if_error_response_code(response)
-      end
-
-      def fail_if_error_response_code(response)
-        return if response.status == 200
-
-        if Instapaper::Error::CODES.include?(response.status.code)
-          fail Instapaper::Error.from_response(response.status.code, @path)
-        else
-          fail Instapaper::Error::ServiceUnavailableError
-        end
-      end
-
-      def fail_if_error_unparseable_response(response)
-        response.parse(:json)
-      rescue JSON::ParserError
-        raise Instapaper::Error::ServiceUnavailableError
-      end
-
-      def fail_if_error_in_body(response)
-        error = error(response)
-        fail(error) if error
-      end
-
-      def error(response)
-        return unless response.is_a?(Array)
-        return unless response.size > 0
-        return unless response.first['type'] == 'error'
-
-        Instapaper::Error.from_response(response.first['error_code'], @path)
-      end
-
-      def parsed_response(response)
-        @parsed_response ||= begin
-          response.parse(:json)
-        rescue
-          response.body
-        end
       end
     end
   end

lib/instapaper/http/response.rb

@@ -0,0 +1,62 @@
+require 'instapaper/error'
+
+module Instapaper
+  module HTTP
+    class Response
+      attr_reader :response, :raw_format, :path
+      def initialize(response, path, raw_format = false)
+        @response = response
+        @path = path
+        @raw_format = raw_format
+      end
+
+      def body
+        raw_format ? response.to_s : parsed
+      end
+
+      def valid?
+        !error?
+      end
+
+      def error?
+        fail_if_body_unparseable unless raw_format
+        fail_if_body_contains_error
+        fail_if_http_error
+      end
+
+      private
+
+      def parsed
+        @parsed_response ||= begin
+          response.parse(:json)
+        rescue
+          response.body
+        end
+      end
+
+      def fail_if_http_error
+        return if response.status.ok?
+
+        if Instapaper::Error::CODES.include?(response.status.code)
+          fail Instapaper::Error.from_response(response.status.code, path)
+        else
+          fail Instapaper::Error, 'Unknown Error'
+        end
+      end
+
+      def fail_if_body_unparseable
+        response.parse(:json)
+      rescue JSON::ParserError
+        raise Instapaper::Error::ServiceUnavailableError
+      end
+
+      def fail_if_body_contains_error
+        return unless parsed.is_a?(Array)
+        return unless parsed.size > 0
+        return unless parsed.first['type'] == 'error'
+
+        fail Instapaper::Error.from_response(parsed.first['error_code'], @path)
+      end
+    end
+  end
+end

spec/instapaper/error_spec.rb

@@ -19,7 +19,33 @@ describe Instapaper::Error do
     end
   end
 
-  Instapaper::Error::ERRORS.each do |status, exception|
+  Instapaper::Error::CLIENT_ERRORS.each do |status, exception|
+    context "when HTTP status is #{status}" do
+      let(:response_body) { %([{"type":"error", "error_code":#{status}, "message":"Error Message"}]) }
+      before do
+        stub_post('/api/1.1/oauth/access_token')
+          .to_return(status: status, body: response_body, headers: {content_type: 'application/json; charset=utf-8'})
+      end
+      it "raises #{exception}" do
+        expect { @client.access_token('foo', 'bar') }.to raise_error(Instapaper::Error::ClientError)
+      end
+    end
+  end
+
+  Instapaper::Error::SERVER_ERRORS.each do |status, exception|
+    context "when HTTP status is #{status}" do
+      let(:response_body) { %([{"type":"error", "error_code":#{status}, "message":"Error Message"}]) }
+      before do
+        stub_post('/api/1.1/oauth/access_token')
+          .to_return(status: status, body: response_body, headers: {content_type: 'application/json; charset=utf-8'})
+      end
+      it "raises #{exception}" do
+        expect { @client.access_token('foo', 'bar') }.to raise_error(Instapaper::Error::ServerError)
+      end
+    end
+  end
+
+  Instapaper::Error::SERVICE_ERRORS.each do |status, exception|
     context "when HTTP status is #{status}" do
       let(:response_body) { %([{"type":"error", "error_code":#{status}, "message":"Error Message"}]) }
       before do
@@ -71,13 +97,13 @@ describe Instapaper::Error do
     end
   end
 
-  context 'HTTP errors' do
+  describe '.from_response' do
-    before do
+    context 'with null path' do
-      stub_post('/api/1.1/oauth/access_token')
+      it 'raises an Instapaper::Error' do
-        .to_return(status: 401, body: 'Unauthorized', headers: {})
+        error = Instapaper::Error.from_response(5000, nil)
-    end
+        expect(error).to be_an Instapaper::Error
-    it 'raises an Instapaper::Error' do
+        expect(error.message).to eq('Unknown Error')
-      expect { @client.access_token('foo', 'bar') }.to raise_error(Instapaper::Error)
+      end
     end
   end
 end

spec/instapaper/http/request_spec.rb

@@ -9,8 +9,8 @@ describe Instapaper::HTTP::Request do
         stub_post('/api/1.1/folders/list')
           .to_return(status: 503, body: '', headers: {content_type: 'application/json; charset=utf-8'})
       end
-      it 'raises a ServiceUnavailableError' do
+      it 'raises a ServerError' do
-        expect { client.folders }.to raise_error(Instapaper::Error::ServiceUnavailableError)
+        expect { client.folders }.to raise_error(Instapaper::Error::ServerError)
       end
     end
 

spec/instapaper/http/response_spec.rb

@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+class FakeResponse
+  def initialize(body)
+    @body = body
+  end
+
+  def parse(_)
+    ::JSON.parse(@body)
+  end
+end
+
+describe Instapaper::HTTP::Response do
+  describe '#body' do
+    context 'raw response' do
+      it 'returns the response in raw text' do
+        resp = Instapaper::HTTP::Response.new('foo', '', true)
+        expect(resp.body).to eq('foo')
+      end
+    end
+
+    context 'regular response' do
+      let(:fake_response) { FakeResponse.new('{"foo":"bar"}') }
+      it 'returns the parsed response' do
+        resp = Instapaper::HTTP::Response.new(fake_response, '')
+        expect(resp.body).to be_a(Hash)
+      end
+    end
+  end
+
+  describe '#valid?' do
+    context 'when http error' do
+    end
+
+    context 'when body unparseable' do
+    end
+
+    context 'when error in body' do
+    end
+  end
+end