diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 41b50f3ee..04940c0fe 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2017-06-17 22:15:11 +0300 using RuboCop version 0.47.0. +# on 2017-07-11 00:59:31 +0100 using RuboCop version 0.47.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -44,6 +44,11 @@ Metrics/ModuleLength: Metrics/PerceivedComplexity: Max: 14 +# Offense count: 2 +Style/BlockDelimiters: + Exclude: + - 'spec/grape/middleware/formatter_spec.rb' + # Offense count: 2 Style/IdenticalConditionalBranches: Exclude: diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cbb0ffa5..4af97c7fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,10 +3,11 @@ #### Features * Your contribution here. +* [#1652](https://github.com/ruby-grape/grape/pull/1652): Add the original exception to the error_formatter the original exception - [@dcsg](https://github.com/dcsg). #### Fixes -* Your contribution here. +* [#1652](https://github.com/ruby-grape/grape/pull/1652): Fix missing backtrace that was not being bubble up to the error_formatter - [@dcsg](https://github.com/dcsg). ### 1.0.0 (7/3/2017) diff --git a/lib/grape/error_formatter/json.rb b/lib/grape/error_formatter/json.rb index e11210495..d3ebff765 100644 --- a/lib/grape/error_formatter/json.rb +++ b/lib/grape/error_formatter/json.rb @@ -4,12 +4,16 @@ module Json extend Base class << self - def call(message, backtrace, options = {}, env = nil) + def call(message, backtrace, options = {}, env = nil, original_exception = nil) result = wrap_message(present(message, env)) - if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty? + rescue_options = options[:rescue_options] || {} + if rescue_options[:backtrace] && backtrace && !backtrace.empty? result = result.merge(backtrace: backtrace) end + if rescue_options[:original_exception] && original_exception + result = result.merge(original_exception: original_exception.inspect) + end ::Grape::Json.dump(result) end diff --git a/lib/grape/error_formatter/txt.rb b/lib/grape/error_formatter/txt.rb index db8762f95..b58e03152 100644 --- a/lib/grape/error_formatter/txt.rb +++ b/lib/grape/error_formatter/txt.rb @@ -4,14 +4,19 @@ module Txt extend Base class << self - def call(message, backtrace, options = {}, env = nil) + def call(message, backtrace, options = {}, env = nil, original_exception = nil) message = present(message, env) result = message.is_a?(Hash) ? ::Grape::Json.dump(message) : message - if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty? - result += "\r\n " + rescue_options = options[:rescue_options] || {} + if rescue_options[:backtrace] && backtrace && !backtrace.empty? + result += "\r\n backtrace:" result += backtrace.join("\r\n ") end + if rescue_options[:original_exception] && original_exception + result += "\r\n original exception:" + result += "\r\n #{original_exception.inspect}" + end result end end diff --git a/lib/grape/error_formatter/xml.rb b/lib/grape/error_formatter/xml.rb index ea2eb3951..73ebf09c8 100644 --- a/lib/grape/error_formatter/xml.rb +++ b/lib/grape/error_formatter/xml.rb @@ -4,13 +4,17 @@ module Xml extend Base class << self - def call(message, backtrace, options = {}, env = nil) + def call(message, backtrace, options = {}, env = nil, original_exception = nil) message = present(message, env) result = message.is_a?(Hash) ? message : { message: message } - if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty? + rescue_options = options[:rescue_options] || {} + if rescue_options[:backtrace] && backtrace && !backtrace.empty? result = result.merge(backtrace: backtrace) end + if rescue_options[:original_exception] && original_exception + result = result.merge(original_exception: original_exception.inspect) + end result.respond_to?(:to_xml) ? result.to_xml(root: :error) : result.to_s end end diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index dcf6e0144..3c00786bf 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -14,7 +14,10 @@ def default_options rescue_all: false, # true to rescue all exceptions rescue_grape_exceptions: false, rescue_subclasses: true, # rescue subclasses of exceptions listed - rescue_options: { backtrace: false }, # true to display backtrace, true to let Grape handle Grape::Exceptions + rescue_options: { + backtrace: false, # true to display backtrace, true to let Grape handle Grape::Exceptions + original_exception: false, # true to display exception + }, rescue_handlers: {}, # rescue handler blocks base_only_rescue_handlers: {}, # rescue handler blocks rescuing only the base class all_rescue_handler: nil # rescue handler block to rescue from all exceptions @@ -77,13 +80,13 @@ def exec_handler(e, &handler) end end - def error!(message, status = options[:default_status], headers = {}, backtrace = []) + def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil) headers = headers.reverse_merge(Grape::Http::Headers::CONTENT_TYPE => content_type) - rack_response(format_message(message, backtrace), status, headers) + rack_response(format_message(message, backtrace, original_exception), status, headers) end def handle_error(e) - error_response(message: e.message, backtrace: e.backtrace) + error_response(message: e.message, backtrace: e.backtrace, original_exception: e) end # TODO: This method is deprecated. Refactor out. @@ -92,19 +95,24 @@ def error_response(error = {}) message = error[:message] || options[:default_message] headers = { Grape::Http::Headers::CONTENT_TYPE => content_type } headers.merge!(error[:headers]) if error[:headers].is_a?(Hash) - backtrace = error[:backtrace] || [] - rack_response(format_message(message, backtrace), status, headers) + backtrace = error[:backtrace] || error[:original_exception] && 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 def rack_response(message, status = options[:default_status], headers = { Grape::Http::Headers::CONTENT_TYPE => content_type }) Rack::Response.new([message], status, headers).finish end - def format_message(message, backtrace) + def format_message(message, backtrace, original_exception = nil) format = env[Grape::Env::API_FORMAT] || options[:format] 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) + 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) end private diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index 1e21619b0..5f8caa50f 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -45,7 +45,7 @@ def build_formatted_response(status, headers, bodies) Rack::Response.new(bodymap, status, headers) end rescue Grape::Exceptions::InvalidFormatter => e - throw :error, status: 500, message: e.message + throw :error, status: 500, message: e.message, backtrace: e.backtrace, original_exception: e end def fetch_formatter(headers, options) @@ -110,7 +110,7 @@ def read_rack_input(body) rescue Grape::Exceptions::Base => e raise e rescue StandardError => e - throw :error, status: 400, message: e.message + throw :error, status: 400, message: e.message, backtrace: e.backtrace, original_exception: e end else env[Grape::Env::API_REQUEST_BODY] = body diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 779893f8e..9b5db5a47 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -1996,7 +1996,7 @@ class ChildError < ParentError; end before :each do module ApiSpec class CustomErrorFormatter - def self.call(message, _backtrace, _options, _env) + def self.call(message, _backtrace, _options, _env, _original_exception) "message: #{message} @backtrace" end end @@ -2018,7 +2018,7 @@ def self.call(message, _backtrace, _options, _env) before :each do module ApiSpec class CustomErrorFormatter - def self.call(message, _backtrace, _option, _env) + def self.call(message, _backtrace, _option, _env, _original_exception) "message: #{message} @backtrace" end end diff --git a/spec/grape/middleware/exception_spec.rb b/spec/grape/middleware/exception_spec.rb index 362cc3eba..78494a031 100644 --- a/spec/grape/middleware/exception_spec.rb +++ b/spec/grape/middleware/exception_spec.rb @@ -158,13 +158,14 @@ def call(_env) it 'is possible to specify a custom formatter' do @app ||= Rack::Builder.app do use Spec::Support::EndpointFaker - use Grape::Middleware::Error, rescue_all: true, - format: :custom, - error_formatters: { - custom: lambda do |message, _backtrace, _options, _env| - { custom_formatter: message }.inspect - end - } + use Grape::Middleware::Error, + rescue_all: true, + format: :custom, + error_formatters: { + custom: lambda do |message, _backtrace, _options, _env, _original_exception| + { custom_formatter: message }.inspect + end + } run ExceptionSpec::ExceptionApp end get '/' @@ -192,5 +193,46 @@ def call(_env) expect(last_response.status).to eq(400) expect(last_response.body).to eq('failed validation') end + + context 'with rescue_options :backtrace and :exception set to true' do + it 'is possible to return the backtrace and the original exception in json format' do + @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker + use Grape::Middleware::Error, + rescue_all: true, + format: :json, + rescue_options: { backtrace: true, original_exception: true } + run ExceptionSpec::ExceptionApp + end + get '/' + expect(last_response.body).to include('error', 'rain!', 'backtrace', 'original_exception', 'RuntimeError') + end + + it 'is possible to return the backtrace and the original exception in xml format' do + @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker + use Grape::Middleware::Error, + rescue_all: true, + format: :xml, + rescue_options: { backtrace: true, original_exception: true } + run ExceptionSpec::ExceptionApp + end + get '/' + expect(last_response.body).to include('error', 'rain!', 'backtrace', 'original-exception', 'RuntimeError') + end + + it 'is possible to return the backtrace and the original exception in txt format' do + @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker + use Grape::Middleware::Error, + rescue_all: true, + format: :txt, + rescue_options: { backtrace: true, original_exception: true } + run ExceptionSpec::ExceptionApp + end + get '/' + expect(last_response.body).to include('error', 'rain!', 'backtrace', 'original exception', 'RuntimeError') + end + end end end diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb index c90f6c1f9..aeab1c4e1 100644 --- a/spec/grape/middleware/formatter_spec.rb +++ b/spec/grape/middleware/formatter_spec.rb @@ -324,4 +324,28 @@ def self.call(_, _) expect(bodies.body.first).to eq({ message: 'invalid' }.to_json) end end + + context 'custom parser raises exception and rescue options are enabled for backtrace and original_exception' do + it 'adds the backtrace and original_exception to the error output' do + subject = Grape::Middleware::Formatter.new( + app, + rescue_options: { backtrace: true, original_exception: true }, + parsers: { json: ->(_object, _env) { raise StandardError, 'fail' } } + ) + io = StringIO.new('{invalid}') + error = catch(:error) { + subject.call( + 'PATH_INFO' => '/info', + 'REQUEST_METHOD' => 'POST', + 'CONTENT_TYPE' => 'application/json', + 'rack.input' => io, + 'CONTENT_LENGTH' => io.length + ) + } + + expect(error[:message]).to eq 'fail' + expect(error[:backtrace].size).to be >= 1 + expect(error[:original_exception].class).to eq StandardError + end + end end