From 5a61811d9d1b278bf2256c298d0d13d1af797cb1 Mon Sep 17 00:00:00 2001 From: Eric Date: Sun, 21 Jan 2024 14:14:51 +0100 Subject: [PATCH 1/8] Only error! is public Minor refactor --- lib/grape/middleware/error.rb | 87 +++++++++++++++++------------------ 1 file changed, 43 insertions(+), 44 deletions(-) diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index 02db9aae0d..d657b8e3c7 100644 --- 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,66 +33,66 @@ 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) + rack_response( + status, headers.reverse_merge(Rack::CONTENT_TYPE => content_type), + format_message(message, backtrace, original_exception) + ) 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) @@ -101,22 +100,22 @@ def rescue_handler_for_class_or_its_ancestor(klass) 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 +125,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) From 50f355f794c9e5ff9543ae7269eabd6165cb9de9 Mon Sep 17 00:00:00 2001 From: Eric Date: Sun, 21 Jan 2024 14:16:22 +0100 Subject: [PATCH 2/8] Remove `rack_response` from inside_route Replace `rack_response` to `error!` --- lib/grape/dsl/inside_route.rb | 17 ----- spec/grape/api_spec.rb | 62 ++++++++----------- .../exceptions/body_parse_errors_spec.rb | 6 +- .../exceptions/invalid_accept_header_spec.rb | 8 +-- .../validations/validators/values_spec.rb | 4 +- 5 files changed, 34 insertions(+), 63 deletions(-) diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 0286595b8c..7ebd6aab8f 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -167,23 +167,6 @@ def error!(message, status = nil, additional_headers = nil) throw :error, message: message, status: self.status, headers: headers end - # Creates a Rack response based on the provided message, status, and headers. - # The content type in the headers is set to the default content type unless provided. - # The message is HTML-escaped if the content type is 'text/html'. - # - # @param message [String] The content of the response. - # @param status [Integer] The HTTP status code. - # @params headers [Hash] (optional) Headers for the response - # (default: {Rack::CONTENT_TYPE => content_type}). - # - # Returns: - # 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) - end - # Redirect to a new url. # # @param url [String] The url to be redirect. diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 97c2dbff07..8164bea8dc 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -2102,7 +2102,7 @@ class CustomError < Grape::Exceptions::Base; end it 'rescues custom grape exceptions' do subject.rescue_from ApiSpec::CustomError do |e| - rack_response('New Error', e.status) + error!('New Error', e.status) end subject.get '/custom_error' do raise ApiSpec::CustomError.new(status: 400, message: 'Custom Error') @@ -2120,7 +2120,7 @@ class CustomError < Grape::Exceptions::Base; end allow(Grape::Formatter).to receive(:formatter_for) { formatter } subject.rescue_from :all do |_e| - rack_response('Formatter Error', 500) + error!('Formatter Error', 500) end subject.get('/formatter_exception') { 'Hello world' } @@ -2143,7 +2143,7 @@ class CustomError < Grape::Exceptions::Base; end describe '.rescue_from klass, block' do it 'rescues Exception' do subject.rescue_from RuntimeError do |e| - rack_response("rescued from #{e.message}", 202) + error!("rescued from #{e.message}", 202) end subject.get '/exception' do raise 'rain!' @@ -2164,7 +2164,7 @@ class CommunicationError < StandardError; end it 'rescues an error via rescue_from :all' do subject.rescue_from :all do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/exception' do raise ConnectionError @@ -2176,7 +2176,7 @@ class CommunicationError < StandardError; end it 'rescues a specific error' do subject.rescue_from ConnectionError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/exception' do raise ConnectionError @@ -2188,7 +2188,7 @@ class CommunicationError < StandardError; end it 'rescues a subclass of an error by default' do subject.rescue_from RuntimeError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/exception' do raise ConnectionError @@ -2200,10 +2200,10 @@ class CommunicationError < StandardError; end it 'rescues multiple specific errors' do subject.rescue_from ConnectionError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.rescue_from DatabaseError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/connection' do raise ConnectionError @@ -2221,7 +2221,7 @@ class CommunicationError < StandardError; end it 'does not rescue a different error' do subject.rescue_from RuntimeError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/uncaught' do raise CommunicationError @@ -2234,7 +2234,7 @@ class CommunicationError < StandardError; end describe '.rescue_from klass, lambda' do it 'rescues an error with the lambda' do subject.rescue_from ArgumentError, lambda { - rack_response('rescued with a lambda', 400) + error!('rescued with a lambda', 400) } subject.get('/rescue_lambda') { raise ArgumentError } @@ -2245,7 +2245,7 @@ class CommunicationError < StandardError; end it 'can execute the lambda with an argument' do subject.rescue_from ArgumentError, lambda { |e| - rack_response(e.message, 400) + error!(e.message, 400) } subject.get('/rescue_lambda') { raise ArgumentError, 'lambda takes an argument' } @@ -2326,7 +2326,7 @@ class ChildError < ParentError; end it 'rescues error as well as subclass errors with rescue_subclasses option set' do subject.rescue_from ApiSpec::APIErrors::ParentError, rescue_subclasses: true do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/caught_child' do raise ApiSpec::APIErrors::ChildError @@ -2347,7 +2347,7 @@ class ChildError < ParentError; end it 'sets rescue_subclasses to true by default' do subject.rescue_from ApiSpec::APIErrors::ParentError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/caught_child' do raise ApiSpec::APIErrors::ChildError @@ -2359,7 +2359,7 @@ class ChildError < ParentError; end it 'does not rescue child errors if rescue_subclasses is false' do subject.rescue_from ApiSpec::APIErrors::ParentError, rescue_subclasses: false do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/uncaught' do raise ApiSpec::APIErrors::ChildError @@ -2389,7 +2389,7 @@ class ChildError < ParentError; end it 'rescues grape exceptions with a user-defined handler' do subject.rescue_from grape_exception.class do |_error| - rack_response('Redefined Error', 403) + error!('Redefined Error', 403) end exception = grape_exception @@ -2572,11 +2572,7 @@ def self.call(message, _backtrace, _option, _env, _original_exception) end get '/excel.json' expect(last_response.status).to eq(406) - if ActiveSupport::VERSION::MAJOR == 3 - expect(last_response.body).to eq('The requested format 'txt' is not supported.') - else - expect(last_response.body).to eq('The requested format 'txt' is not supported.') - end + expect(last_response.body).to eq(Rack::Utils.escape_html("The requested format 'txt' is not supported.")) end end @@ -3375,7 +3371,7 @@ def static context 'when some rescues are defined by mounted' do it 'inherits parent rescues' do subject.rescue_from :all do |e| - rack_response("rescued from #{e.message}", 202) + error!("rescued from #{e.message}", 202) end app = Class.new(described_class) @@ -3393,14 +3389,14 @@ def static it 'prefers rescues defined by mounted if they rescue similar error class' do subject.rescue_from StandardError do - rack_response('outer rescue') + error!('outer rescue') end app = Class.new(described_class) subject.namespace :mounted do rescue_from StandardError do - rack_response('inner rescue') + error!('inner rescue') end app.get('/fail') { raise 'doh!' } mount app @@ -3412,14 +3408,14 @@ def static it 'prefers rescues defined by mounted even if outer is more specific' do subject.rescue_from ArgumentError do - rack_response('outer rescue') + error!('outer rescue') end app = Class.new(described_class) subject.namespace :mounted do rescue_from StandardError do - rack_response('inner rescue') + error!('inner rescue') end app.get('/fail') { raise ArgumentError.new } mount app @@ -3431,14 +3427,14 @@ def static it 'prefers more specific rescues defined by mounted' do subject.rescue_from StandardError do - rack_response('outer rescue') + error!('outer rescue') end app = Class.new(described_class) subject.namespace :mounted do rescue_from ArgumentError do - rack_response('inner rescue') + error!('inner rescue') end app.get('/fail') { raise ArgumentError.new } mount app @@ -4125,11 +4121,7 @@ def before end get '/something' expect(last_response.status).to eq(406) - if ActiveSupport::VERSION::MAJOR == 3 - expect(last_response.body).to eq('{"error":"The requested format 'txt' is not supported."}') - else - expect(last_response.body).to eq('{"error":"The requested format 'txt' is not supported."}') - end + expect(last_response.body).to eq(Rack::Utils.escape_html({ error: "The requested format 'txt' is not supported." }.to_json)) end end @@ -4141,11 +4133,7 @@ def before end get '/something?format=' expect(last_response.status).to eq(406) - if ActiveSupport::VERSION::MAJOR == 3 - expect(last_response.body).to eq('The requested format '<script>blah</script>' is not supported.') - else - expect(last_response.body).to eq('The requested format '<script>blah</script>' is not supported.') - end + expect(last_response.body).to eq(Rack::Utils.escape_html("The requested format '' is not supported.")) end end diff --git a/spec/grape/exceptions/body_parse_errors_spec.rb b/spec/grape/exceptions/body_parse_errors_spec.rb index 7017400f6e..06866b7b32 100644 --- a/spec/grape/exceptions/body_parse_errors_spec.rb +++ b/spec/grape/exceptions/body_parse_errors_spec.rb @@ -6,7 +6,7 @@ before do subject.rescue_from :all do |_e| - rack_response 'message was processed', 400 + error! 'message was processed', 400 end subject.params do requires :beer @@ -58,7 +58,7 @@ def app before do subject.rescue_from :all do |_e| - rack_response 'message was processed', 400 + error! 'message was processed', 400 end subject.rescue_from :grape_exceptions @@ -96,7 +96,7 @@ def app before do subject.rescue_from :grape_exceptions do |e| - rack_response "Custom Error Contents, Original Message: #{e.message}", 400 + error! "Custom Error Contents, Original Message: #{e.message}", 400 end subject.params do diff --git a/spec/grape/exceptions/invalid_accept_header_spec.rb b/spec/grape/exceptions/invalid_accept_header_spec.rb index ca4aec5ec7..42edfb5261 100644 --- a/spec/grape/exceptions/invalid_accept_header_spec.rb +++ b/spec/grape/exceptions/invalid_accept_header_spec.rb @@ -44,7 +44,7 @@ before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false subject.rescue_from :all do |e| - rack_response 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e[:headers] end subject.get '/beer' do 'beer received' @@ -114,7 +114,7 @@ def app before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false subject.rescue_from :all do |e| - rack_response 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e[:headers] end subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], @@ -194,7 +194,7 @@ def app before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true subject.rescue_from :all do |e| - rack_response 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e[:headers] end subject.get '/beer' do 'beer received' @@ -273,7 +273,7 @@ def app before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true subject.rescue_from :all do |e| - rack_response 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e[:headers] end subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], diff --git a/spec/grape/validations/validators/values_spec.rb b/spec/grape/validations/validators/values_spec.rb index 4780829a10..c66bf42e53 100644 --- a/spec/grape/validations/validators/values_spec.rb +++ b/spec/grape/validations/validators/values_spec.rb @@ -404,13 +404,13 @@ def even?(value) expect(last_response.body).to eq({ error: 'type does not have a valid value' }.to_json) end - it 'validates against values in an endless range', if: ActiveSupport::VERSION::MAJOR >= 6 do + it 'validates against values in an endless range' do get('/endless', type: 10) expect(last_response.status).to eq 200 expect(last_response.body).to eq({ type: 10 }.to_json) end - it 'does not allow an invalid value for a parameter using an endless range', if: ActiveSupport::VERSION::MAJOR >= 6 do + it 'does not allow an invalid value for a parameter using an endless range' do get('/endless', type: 0) expect(last_response.status).to eq 400 expect(last_response.body).to eq({ error: 'type does not have a valid value' }.to_json) From d419b8a114620e00629f289557cd8df0078e233a Mon Sep 17 00:00:00 2001 From: Eric Date: Sun, 21 Jan 2024 14:24:22 +0100 Subject: [PATCH 3/8] error! is now private call self.status once inside route --- lib/grape/dsl/inside_route.rb | 4 ++-- lib/grape/middleware/error.rb | 15 ++++++++------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 7ebd6aab8f..6eee05299a 100644 --- 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 # Redirect to a new url. diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index d657b8e3c7..b210b4ac68 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -38,13 +38,6 @@ def call!(env) 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) - rack_response( - status, headers.reverse_merge(Rack::CONTENT_TYPE => content_type), - format_message(message, backtrace, original_exception) - ) - end - private def rack_response(status, headers, message) @@ -138,6 +131,14 @@ 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 From be10a16ac585cbc6970982e833dedf8f79cf14d9 Mon Sep 17 00:00:00 2001 From: Eric Date: Sun, 21 Jan 2024 14:32:50 +0100 Subject: [PATCH 4/8] Fix rubocop --- lib/grape/middleware/error.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index b210b4ac68..545519a1a1 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -138,7 +138,6 @@ def error!(message, status = options[:default_status], headers = {}, backtrace = ) end - def error?(response) response.is_a?(Hash) && response[:message] && response[:status] && response[:headers] end From 5cc2265525ba7dfbb451977aec166db43de62c9b Mon Sep 17 00:00:00 2001 From: Eric Date: Sun, 21 Jan 2024 14:35:32 +0100 Subject: [PATCH 5/8] Add CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b87d4bfd7c..f007545789 100644 --- 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) From dc837a7acf6fc509e85064abce048bd66de000e6 Mon Sep 17 00:00:00 2001 From: Eric Date: Sun, 21 Jan 2024 22:02:24 +0100 Subject: [PATCH 6/8] Add UPGRADING.md entry Revert rack_response in inside_route with deprecation Add spec --- UPGRADING.md | 3 +++ lib/grape/dsl/inside_route.rb | 18 ++++++++++++++++++ spec/grape/api_spec.rb | 19 +++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/UPGRADING.md b/UPGRADING.md index f8cdb93b37..59ed26bafa 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -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. + #### 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 index 6eee05299a..6bd0a4690e 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -167,6 +167,24 @@ def error!(message, status = nil, additional_headers = nil) throw :error, message: message, status: status, headers: headers end + # Creates a Rack response based on the provided message, status, and headers. + # The content type in the headers is set to the default content type unless provided. + # The message is HTML-escaped if the content type is 'text/html'. + # + # @param message [String] The content of the response. + # @param status [Integer] The HTTP status code. + # @params headers [Hash] (optional) Headers for the response + # (default: {Rack::CONTENT_TYPE => content_type}). + # + # Returns: + # A Rack::Response object containing the specified message, status, and headers. + # + def rack_response(message, status = 200, headers = { Rack::CONTENT_TYPE => content_type }) + Grape.deprecator.warn('Use error! instead of rack_response') + 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. # # @param url [String] The url to be redirect. diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 8164bea8dc..0568bdf0d2 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -4422,4 +4422,23 @@ def uniqe_id_route end end end + + context 'rack_response deprecated' do + let(:app) do + Class.new(described_class) do + rescue_from :all do + rack_response('deprecated', 500) + end + + get 'test' do + raise ArgumentError + end + end + end + + it 'raises a deprecation' do + expect(Grape.deprecator).to receive(:warn).with('Use error! instead of rack_response') + get 'test' + end + end end From ac6c7a3365063b4d6a812a4039f613437ad6e426 Mon Sep 17 00:00:00 2001 From: Eric Date: Sun, 21 Jan 2024 22:05:06 +0100 Subject: [PATCH 7/8] Fix escape_html --- lib/grape/dsl/inside_route.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 6bd0a4690e..99e9b27620 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -181,7 +181,7 @@ def error!(message, status = nil, additional_headers = nil) # def rack_response(message, status = 200, headers = { Rack::CONTENT_TYPE => content_type }) Grape.deprecator.warn('Use error! instead of rack_response') - message = Rack::Utils::escape_html(message) if headers[Rack::CONTENT_TYPE] == 'text/html' + 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 From aaa598257afb769e35581c746a45597c6313603d Mon Sep 17 00:00:00 2001 From: Eric Date: Mon, 22 Jan 2024 10:40:53 +0100 Subject: [PATCH 8/8] Fix UPGRADING.md Change deprecation msg --- UPGRADING.md | 5 ++++- lib/grape/dsl/inside_route.rb | 2 +- spec/grape/api_spec.rb | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 59ed26bafa..900d0a340a 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -4,7 +4,10 @@ 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. + +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 diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 99e9b27620..a5ed227ef0 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -180,7 +180,7 @@ 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 }) - Grape.deprecator.warn('Use error! instead of rack_response') + 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 diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 0568bdf0d2..95b46079c8 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -4437,8 +4437,9 @@ def uniqe_id_route end it 'raises a deprecation' do - expect(Grape.deprecator).to receive(:warn).with('Use error! instead of rack_response') + expect(Grape.deprecator).to receive(:warn).with('The rack_response method has been deprecated, use error! instead.') get 'test' + expect(last_response.body).to eq('deprecated') end end end