Fix response headers from lint (#2414)

* Only error! is public Minor refactor * Remove `rack_response` from inside_route Replace `rack_response` to `error!` * error! is now private call self.status once inside route * Fix rubocop * Add CHANGELOG.md * Add UPGRADING.md entry Revert rack_response in inside_route with deprecation Add spec * Fix escape_html * Fix UPGRADING.md Change deprecation msg
ruby-grape · Jan 22, 2024 · c6ad84a · c6ad84a
1 parent e9aa45b
commit c6ad84a
Show file tree

Hide file tree

Showing 8 changed files with 112 additions and 97 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,6 +27,7 @@
 * [#2382](https://github.com/ruby-grape/grape/pull/2382): Fix values validator for params wrapped in `with` block - [@numbata](https://github.com/numbata).
 * [#2387](https://github.com/ruby-grape/grape/pull/2387): Fix rubygems version within workflows - [@ericproulx](https://github.com/ericproulx).
 * [#2405](https://github.com/ruby-grape/grape/pull/2405): Fix edge workflow - [@ericproulx](https://github.com/ericproulx).
+* [#2414](https://github.com/ruby-grape/grape/pull/2414): Fix Rack::Lint missing content-type - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 ### 2.0.0 (2023/11/11)

diff --git a/UPGRADING.md b/UPGRADING.md
@@ -3,6 +3,12 @@ Upgrading Grape
 
 ### Upgrading to >= 2.1.0
 
+#### Changes in rescue_from
+
+The `rack_response` method has been deprecated and the `error_response` method has been removed. Use `error!` instead.
+
+See [#2414](https://github.com/ruby-grape/grape/pull/2414) for more information.
+
 #### Grape::Router::Route.route_xxx methods have been removed
 
 - `route_method` is accessible through `request_method`

diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb
@@ -162,9 +162,9 @@ def configuration
       # @param status [Integer] the HTTP Status Code. Defaults to default_error_status, 500 if not set.
       # @param additional_headers [Hash] Addtional headers for the response.
       def error!(message, status = nil, additional_headers = nil)
-        self.status(status || namespace_inheritable(:default_error_status))
+        status = self.status(status || namespace_inheritable(:default_error_status))
         headers = additional_headers.present? ? header.merge(additional_headers) : header
-        throw :error, message: message, status: self.status, headers: headers
+        throw :error, message: message, status: status, headers: headers
       end
 
       # Creates a Rack response based on the provided message, status, and headers.
@@ -180,8 +180,9 @@ def error!(message, status = nil, additional_headers = nil)
       # A Rack::Response object containing the specified message, status, and headers.
       #
       def rack_response(message, status = 200, headers = { Rack::CONTENT_TYPE => content_type })
-        message = ERB::Util.html_escape(message) if headers[Rack::CONTENT_TYPE] == 'text/html'
-        Rack::Response.new([message], Rack::Utils.status_code(status), headers)
+        Grape.deprecator.warn('The rack_response method has been deprecated, use error! instead.')
+        message = Rack::Utils.escape_html(message) if headers[Rack::CONTENT_TYPE] == 'text/html'
+        Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), headers)
       end
 
       # Redirect to a new url.

diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb
@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 require 'grape/middleware/base'
-require 'active_support/core_ext/string/output_safety'
 
 module Grape
   module Middleware
@@ -34,89 +33,82 @@ def initialize(app, *options)
 
       def call!(env)
         @env = env
-        begin
-          error_response(catch(:error) do
-            return @app.call(@env)
-          end)
-        rescue Exception => e # rubocop:disable Lint/RescueException
-          handler =
-            rescue_handler_for_base_only_class(e.class) ||
-            rescue_handler_for_class_or_its_ancestor(e.class) ||
-            rescue_handler_for_grape_exception(e.class) ||
-            rescue_handler_for_any_class(e.class) ||
-            raise
-
-          run_rescue_handler(handler, e, @env[Grape::Env::API_ENDPOINT])
-        end
+        error_response(catch(:error) { return @app.call(@env) })
+      rescue Exception => e # rubocop:disable Lint/RescueException
+        run_rescue_handler(find_handler(e.class), e, @env[Grape::Env::API_ENDPOINT])
       end
 
-      def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil)
-        headers = headers.reverse_merge(Rack::CONTENT_TYPE => content_type)
-        rack_response(format_message(message, backtrace, original_exception), status, headers)
-      end
-
-      def default_rescue_handler(e)
-        error_response(message: e.message, backtrace: e.backtrace, original_exception: e)
-      end
-
-      # TODO: This method is deprecated. Refactor out.
-      def error_response(error = {})
-        status = error[:status] || options[:default_status]
-        message = error[:message] || options[:default_message]
-        headers = { Rack::CONTENT_TYPE => content_type }
-        headers.merge!(error[:headers]) if error[:headers].is_a?(Hash)
-        backtrace = error[:backtrace] || error[:original_exception]&.backtrace || []
-        original_exception = error.is_a?(Exception) ? error : error[:original_exception] || nil
-        rack_response(format_message(message, backtrace, original_exception), status, headers)
-      end
+      private
 
-      def rack_response(message, status = options[:default_status], headers = { Rack::CONTENT_TYPE => content_type })
-        message = ERB::Util.html_escape(message) if headers[Rack::CONTENT_TYPE] == TEXT_HTML
-        Rack::Response.new([message], Rack::Utils.status_code(status), headers)
+      def rack_response(status, headers, message)
+        message = Rack::Utils.escape_html(message) if headers[Rack::CONTENT_TYPE] == TEXT_HTML
+        Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), headers)
       end
 
       def format_message(message, backtrace, original_exception = nil)
         format = env[Grape::Env::API_FORMAT] || options[:format]
         formatter = Grape::ErrorFormatter.formatter_for(format, **options)
+        return formatter.call(message, backtrace, options, env, original_exception) if formatter
+
         throw :error,
               status: 406,
               message: "The requested format '#{format}' is not supported.",
               backtrace: backtrace,
-              original_exception: original_exception unless formatter
-        formatter.call(message, backtrace, options, env, original_exception)
+              original_exception: original_exception
       end
 
-      private
+      def find_handler(klass)
+        rescue_handler_for_base_only_class(klass) ||
+          rescue_handler_for_class_or_its_ancestor(klass) ||
+          rescue_handler_for_grape_exception(klass) ||
+          rescue_handler_for_any_class(klass) ||
+          raise
+      end
+
+      def error_response(error = {})
+        status = error[:status] || options[:default_status]
+        message = error[:message] || options[:default_message]
+        headers = { Rack::CONTENT_TYPE => content_type }.tap do |h|
+          h.merge!(error[:headers]) if error[:headers].is_a?(Hash)
+        end
+        backtrace = error[:backtrace] || error[:original_exception]&.backtrace || []
+        original_exception = error.is_a?(Exception) ? error : error[:original_exception] || nil
+        rack_response(status, headers, format_message(message, backtrace, original_exception))
+      end
+
+      def default_rescue_handler(e)
+        error_response(message: e.message, backtrace: e.backtrace, original_exception: e)
+      end
 
       def rescue_handler_for_base_only_class(klass)
         error, handler = options[:base_only_rescue_handlers].find { |err, _handler| klass == err }
 
         return unless error
 
-        handler || :default_rescue_handler
+        handler || method(:default_rescue_handler)
       end
 
       def rescue_handler_for_class_or_its_ancestor(klass)
         error, handler = options[:rescue_handlers].find { |err, _handler| klass <= err }
 
         return unless error
 
-        handler || :default_rescue_handler
+        handler || method(:default_rescue_handler)
       end
 
       def rescue_handler_for_grape_exception(klass)
         return unless klass <= Grape::Exceptions::Base
-        return :error_response if klass == Grape::Exceptions::InvalidVersionHeader
+        return method(:error_response) if klass == Grape::Exceptions::InvalidVersionHeader
         return unless options[:rescue_grape_exceptions] || !options[:rescue_all]
 
-        options[:grape_exceptions_rescue_handler] || :error_response
+        options[:grape_exceptions_rescue_handler] || method(:error_response)
       end
 
       def rescue_handler_for_any_class(klass)
         return unless klass <= StandardError
         return unless options[:rescue_all] || options[:rescue_grape_exceptions]
 
-        options[:all_rescue_handler] || :default_rescue_handler
+        options[:all_rescue_handler] || method(:default_rescue_handler)
       end
 
       def run_rescue_handler(handler, error, endpoint)
@@ -126,9 +118,9 @@ def run_rescue_handler(handler, error, endpoint)
           handler = public_method(handler)
         end
 
-        response = (catch(:error) do
+        response = catch(:error) do
           handler.arity.zero? ? endpoint.instance_exec(&handler) : endpoint.instance_exec(error, &handler)
-        end)
+        end
 
         response = error!(response[:message], response[:status], response[:headers]) if error?(response)
 
@@ -139,6 +131,13 @@ def run_rescue_handler(handler, error, endpoint)
         end
       end
 
+      def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil)
+        rack_response(
+          status, headers.reverse_merge(Rack::CONTENT_TYPE => content_type),
+          format_message(message, backtrace, original_exception)
+        )
+      end
+
       def error?(response)
         response.is_a?(Hash) && response[:message] && response[:status] && response[:headers]
       end