Merge pull request ruby-grape#1298 from totothink/master

clean error_formatter
przbadu · Mar 1, 2016 · 674e7e4 · 674e7e4
2 parents d78422d + 93e6998
commit 674e7e4
Show file tree

Hide file tree

Showing 15 changed files with 110 additions and 100 deletions.
diff --git a/lib/grape.rb b/lib/grape.rb
@@ -41,6 +41,9 @@ module Grape
 
     autoload :Cookies
     autoload :Validations
+    autoload :ErrorFormatter
+    autoload :Formatter
+    autoload :Parser
     autoload :Request
     autoload :Env, 'grape/util/env'
   end
@@ -85,7 +88,6 @@ module ErrorFormatter
 
   module Formatter
     extend ActiveSupport::Autoload
-    autoload :Base
     autoload :Json
     autoload :SerializableHash
     autoload :Txt
@@ -94,7 +96,6 @@ module Formatter
 
   module Parser
     extend ActiveSupport::Autoload
-    autoload :Base
     autoload :Json
     autoload :Xml
   end

diff --git a/lib/grape/dsl/request_response.rb b/lib/grape/dsl/request_response.rb
@@ -20,7 +20,7 @@ def format(new_format = nil)
           if new_format
             namespace_inheritable(:format, new_format.to_sym)
             # define the default error formatters
-            namespace_inheritable(:default_error_formatter, Grape::ErrorFormatter::Base.formatter_for(new_format, {}))
+            namespace_inheritable(:default_error_formatter, Grape::ErrorFormatter.formatter_for(new_format, {}))
             # define a single mime type
             mime_type = content_types[new_format.to_sym]
             fail Grape::Exceptions::MissingMimeType.new(new_format) unless mime_type
@@ -43,7 +43,7 @@ def parser(content_type, new_parser)
         # Specify a default error formatter.
         def default_error_formatter(new_formatter_name = nil)
           if new_formatter_name
-            new_formatter = Grape::ErrorFormatter::Base.formatter_for(new_formatter_name, {})
+            new_formatter = Grape::ErrorFormatter.formatter_for(new_formatter_name, {})
             namespace_inheritable(:default_error_formatter, new_formatter)
           else
             namespace_inheritable(:default_error_formatter)

diff --git a/lib/grape/error_formatter.rb b/lib/grape/error_formatter.rb
@@ -0,0 +1,31 @@
+module Grape
+  module ErrorFormatter
+    class << self
+      def builtin_formatters
+        {
+          serializable_hash: Grape::ErrorFormatter::Json,
+          json: Grape::ErrorFormatter::Json,
+          jsonapi: Grape::ErrorFormatter::Json,
+          txt: Grape::ErrorFormatter::Txt,
+          xml: Grape::ErrorFormatter::Xml
+        }
+      end
+
+      def formatters(options)
+        builtin_formatters.merge(options[:error_formatters] || {})
+      end
+
+      def formatter_for(api_format, options = {})
+        spec = formatters(options)[api_format]
+        case spec
+        when nil
+          options[:default_error_formatter] || Grape::ErrorFormatter::Txt
+        when Symbol
+          method(spec)
+        else
+          spec
+        end
+      end
+    end
+  end
+end
diff --git a/lib/grape/error_formatter/base.rb b/lib/grape/error_formatter/base.rb
@@ -1,34 +1,6 @@
 module Grape
   module ErrorFormatter
     module Base
-      class << self
-        FORMATTERS = {
-          serializable_hash: Grape::ErrorFormatter::Json,
-          json: Grape::ErrorFormatter::Json,
-          jsonapi: Grape::ErrorFormatter::Json,
-          txt: Grape::ErrorFormatter::Txt,
-          xml: Grape::ErrorFormatter::Xml
-        }
-
-        def formatters(options)
-          FORMATTERS.merge(options[:error_formatters] || {})
-        end
-
-        def formatter_for(api_format, options = {})
-          spec = formatters(options)[api_format]
-          case spec
-          when nil
-            options[:default_error_formatter] || Grape::ErrorFormatter::Txt
-          when Symbol
-            method(spec)
-          else
-            spec
-          end
-        end
-      end
-
-      module_function
-
       def present(message, env)
         present_options = {}
         present_options[:with] = message.delete(:with) if message.is_a?(Hash)

diff --git a/lib/grape/error_formatter/json.rb b/lib/grape/error_formatter/json.rb
@@ -1,9 +1,11 @@
 module Grape
   module ErrorFormatter
     module Json
+      extend Base
+
       class << self
         def call(message, backtrace, options = {}, env = nil)
-          result = wrap_message(Grape::ErrorFormatter::Base.present(message, env))
+          result = wrap_message(present(message, env))
 
           if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty?
             result = result.merge(backtrace: backtrace)

diff --git a/lib/grape/error_formatter/txt.rb b/lib/grape/error_formatter/txt.rb
@@ -1,9 +1,11 @@
 module Grape
   module ErrorFormatter
     module Txt
+      extend Base
+
       class << self
         def call(message, backtrace, options = {}, env = nil)
-          message = Grape::ErrorFormatter::Base.present(message, env)
+          message = present(message, env)
 
           result = message.is_a?(Hash) ? MultiJson.dump(message) : message
           if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty?

diff --git a/lib/grape/error_formatter/xml.rb b/lib/grape/error_formatter/xml.rb
@@ -1,9 +1,11 @@
 module Grape
   module ErrorFormatter
     module Xml
+      extend Base
+
       class << self
         def call(message, backtrace, options = {}, env = nil)
-          message = Grape::ErrorFormatter::Base.present(message, env)
+          message = present(message, env)
 
           result = message.is_a?(Hash) ? message : { message: message }
           if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty?

diff --git a/lib/grape/formatter.rb b/lib/grape/formatter.rb
@@ -0,0 +1,31 @@
+module Grape
+  module Formatter
+    class << self
+      def builtin_formmaters
+        {
+          json: Grape::Formatter::Json,
+          jsonapi: Grape::Formatter::Json,
+          serializable_hash: Grape::Formatter::SerializableHash,
+          txt: Grape::Formatter::Txt,
+          xml: Grape::Formatter::Xml
+        }
+      end
+
+      def formatters(options)
+        builtin_formmaters.merge(options[:formatters] || {})
+      end
+
+      def formatter_for(api_format, options = {})
+        spec = formatters(options)[api_format]
+        case spec
+        when nil
+          ->(obj, _env) { obj }
+        when Symbol
+          method(spec)
+        else
+          spec
+        end
+      end
+    end
+  end
+end
diff --git a/lib/grape/formatter/base.rb b/lib/grape/formatter/base.rb
diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb
@@ -107,7 +107,7 @@ def rack_response(message, status = options[:default_status], headers = { Grape:
 
       def format_message(message, backtrace)
         format = env[Grape::Env::API_FORMAT] || options[:format]
-        formatter = Grape::ErrorFormatter::Base.formatter_for(format, options)
+        formatter = Grape::ErrorFormatter.formatter_for(format, options)
         throw :error, status: 406, message: "The requested format '#{format}' is not supported." unless formatter
         formatter.call(message, backtrace, options, env)
       end

diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb
@@ -50,7 +50,7 @@ def build_formatted_response(status, headers, bodies)
 
       def fetch_formatter(headers, options)
         api_format = mime_types[headers[Grape::Http::Headers::CONTENT_TYPE]] || env[Grape::Env::API_FORMAT]
-        Grape::Formatter::Base.formatter_for(api_format, options)
+        Grape::Formatter.formatter_for(api_format, options)
       end
 
       # Set the content type header for the API format if it is not already present.
@@ -93,7 +93,7 @@ def read_rack_input(body)
         fmt = mime_types[request.media_type] if request.media_type
         fmt ||= options[:default_format]
         if content_type_for(fmt)
-          parser = Grape::Parser::Base.parser_for fmt, options
+          parser = Grape::Parser.parser_for fmt, options
           if parser
             begin
               body = (env[Grape::Env::API_REQUEST_BODY] = parser.call(body, env))

diff --git a/lib/grape/parser.rb b/lib/grape/parser.rb
@@ -0,0 +1,29 @@
+module Grape
+  module Parser
+    class << self
+      def builtin_parsers
+        {
+          json: Grape::Parser::Json,
+          jsonapi: Grape::Parser::Json,
+          xml: Grape::Parser::Xml
+        }
+      end
+
+      def parsers(options)
+        builtin_parsers.merge(options[:parsers] || {})
+      end
+
+      def parser_for(api_format, options = {})
+        spec = parsers(options)[api_format]
+        case spec
+        when nil
+          nil
+        when Symbol
+          method(spec)
+        else
+          spec
+        end
+      end
+    end
+  end
+end
diff --git a/lib/grape/parser/base.rb b/lib/grape/parser/base.rb
diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb
@@ -1465,7 +1465,7 @@ class CustomError < Grape::Exceptions::Base; end
     it 'can rescue exceptions raised in the formatter' do
       formatter = double(:formatter)
       allow(formatter).to receive(:call) { fail StandardError }
-      allow(Grape::Formatter::Base).to receive(:formatter_for) { formatter }
+      allow(Grape::Formatter).to receive(:formatter_for) { formatter }
 
       subject.rescue_from :all do |_e|
         rack_response('Formatter Error', 500)

diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb
@@ -50,7 +50,7 @@ def to_xml
   context 'error handling' do
     let(:formatter) { double(:formatter) }
     before do
-      allow(Grape::Formatter::Base).to receive(:formatter_for) { formatter }
+      allow(Grape::Formatter).to receive(:formatter_for) { formatter }
     end
 
     it 'rescues formatter-specific exceptions' do