Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix response headers from lint #2414

Merged
merged 8 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ Upgrading Grape

### Upgrading to >= 2.1.0

#### Changes in rescue_from
`rack_response` has been deprecated and `error_response` has been removed. Use `error!` instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rack_response method ... (just to make it a sentence)

Link to the PR here and a couple missing below.

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`
Expand Down
9 changes: 5 additions & 4 deletions lib/grape/dsl/inside_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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('Use error! instead of rack_response')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this reads cleaner?

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.
Expand Down
93 changes: 46 additions & 47 deletions lib/grape/middleware/error.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# frozen_string_literal: true

require 'grape/middleware/base'
require 'active_support/core_ext/string/output_safety'

module Grape
module Middleware
Expand Down Expand Up @@ -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)
Expand All @@ -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)

Expand All @@ -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
Expand Down
Loading