From 1a26695b367b1369b1a7970e67c081729a592b80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dieter=20Sp=C3=A4th?= Date: Fri, 1 Aug 2014 15:23:54 +0200 Subject: [PATCH 1/3] error messages can now be presented --- CHANGELOG.md | 1 + README.md | 32 ++++++++++++++++ grape.gemspec | 2 +- lib/grape/dsl/inside_route.rb | 38 +++++++++++-------- lib/grape/dsl/request_response.rb | 10 +++++ lib/grape/endpoint.rb | 3 +- lib/grape/error_formatter/base.rb | 30 +++++++++++++++ lib/grape/error_formatter/json.rb | 2 + lib/grape/error_formatter/txt.rb | 2 + lib/grape/error_formatter/xml.rb | 2 + lib/grape/middleware/error.rb | 8 +++- spec/grape/api_spec.rb | 49 +++++++++++++++++++++++++ spec/grape/dsl/request_response_spec.rb | 7 ++++ spec/grape/middleware/error_spec.rb | 34 ++++++++++++++++- spec/grape/middleware/exception_spec.rb | 13 +++++++ spec/support/endpoint_faker.rb | 23 ++++++++++++ 16 files changed, 236 insertions(+), 20 deletions(-) create mode 100644 spec/support/endpoint_faker.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index f1c093d233..d3386a42f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ * [#703](https://github.com/intridea/grape/pull/703): Removed `Grape::Middleware::Auth::OAuth2` - [@dspaeth-faber](https://github.com/dspaeth-faber). * [#719](https://github.com/intridea/grape/pull/719): Allow passing options hash to a custom validator - [@elado](https://github.com/elado). * [#716](https://github.com/intridea/grape/pull/716): Calling `content-type` will now return the current content-type - [@dblock](https://github.com/dblock). +* [#705](https://github.com/intridea/grape/pull/705): Errors can now be presented - [@dspaeth-faber](https://github.com/dspaeth-faber). * Your contribution here. 0.8.0 (7/10/2014) diff --git a/README.md b/README.md index 2dee3fcf88..8cee80cbda 100644 --- a/README.md +++ b/README.md @@ -944,6 +944,38 @@ instead of a message. error!({ error: "unexpected error", detail: "missing widget" }, 500) ``` +When you'r error message is representable, a Hash with an option `with` or your +status is documented with an `Entity` it will be presented. + +Examples: + +```ruby + + +desc 'my route' , http_code: [[408, 'Unauthorized', API::Error]], + +error!(Error.new(...), 400) +error!({ message: 'An Error', with: API::Error }, 401) +error!({ message: 'An Error' }, 408) + +``` + +Error messages can be enriched with the status code, when you use a Hash or your message respond to +`code` + +```ruby + +class API < Grape::API + add_http_code_on_error true + + get '/' do + error!({message: 'Error'}, 404) # => result as json "{"message":"Error", "code":404}" + end +end + + +``` + ### Default Error HTTP Status Code By default Grape returns a 500 status code from `error!`. You can change this with `default_error_status`. diff --git a/grape.gemspec b/grape.gemspec index 752a3681f5..29f5256016 100644 --- a/grape.gemspec +++ b/grape.gemspec @@ -24,7 +24,7 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'virtus', '>= 1.0.0' s.add_runtime_dependency 'builder' - s.add_development_dependency 'grape-entity', '>= 0.2.0' + s.add_development_dependency 'grape-entity', '>= 0.4.4' s.add_development_dependency 'rake' s.add_development_dependency 'maruku' s.add_development_dependency 'yard' diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 283f126ca6..99a52e8275 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -173,22 +173,7 @@ def present(*args) else [nil, args.first] end - entity_class = options.delete(:with) - - if entity_class.nil? - # entity class not explicitely defined, auto-detect from relation#klass or first object in the collection - object_class = if object.respond_to?(:klass) - object.klass - else - object.respond_to?(:first) ? object.first.class : object.class - end - - object_class.ancestors.each do |potential| - entity_class ||= (settings[:representations] || {})[potential] - end - - entity_class ||= object_class.const_get(:Entity) if object_class.const_defined?(:Entity) && object_class.const_get(:Entity).respond_to?(:represent) - end + entity_class = entity_class_for_obj(object, options) root = options.delete(:root) @@ -216,6 +201,27 @@ def present(*args) def route env["rack.routing_args"][:route_info] end + + def entity_class_for_obj(object, options) + entity_class = options.delete(:with) + + if entity_class.nil? + # entity class not explicitely defined, auto-detect from relation#klass or first object in the collection + object_class = if object.respond_to?(:klass) + object.klass + else + object.respond_to?(:first) ? object.first.class : object.class + end + + object_class.ancestors.each do |potential| + entity_class ||= (settings[:representations] || {})[potential] + end + + entity_class ||= object_class.const_get(:Entity) if object_class.const_defined?(:Entity) && object_class.const_get(:Entity).respond_to?(:represent) + end + + entity_class + end end end end diff --git a/lib/grape/dsl/request_response.rb b/lib/grape/dsl/request_response.rb index 6f9167dcad..8ae8e3d658 100644 --- a/lib/grape/dsl/request_response.rb +++ b/lib/grape/dsl/request_response.rb @@ -146,6 +146,16 @@ def represent(model_class, options) raise Grape::Exceptions::InvalidWithOptionForRepresent.new unless options[:with] && options[:with].is_a?(Class) imbue(:representations, model_class => options[:with]) end + + # Specify if the http code should be added to + # the error message + def add_http_code_on_error(new_value = nil) + if new_value.nil? + settings[:add_http_code_on_error] + else + set(:add_http_code_on_error, new_value) + end + end end end end diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 486b202351..c75603d140 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -238,7 +238,8 @@ def build_middleware rescue_options: settings[:rescue_options], rescue_handlers: merged_setting(:rescue_handlers), base_only_rescue_handlers: merged_setting(:base_only_rescue_handlers), - all_rescue_handler: settings[:all_rescue_handler] + all_rescue_handler: settings[:all_rescue_handler], + add_http_code_on_error: settings[:add_http_code_on_error] aggregate_setting(:middleware).each do |m| m = m.dup diff --git a/lib/grape/error_formatter/base.rb b/lib/grape/error_formatter/base.rb index 265dbca042..2d526a6f14 100644 --- a/lib/grape/error_formatter/base.rb +++ b/lib/grape/error_formatter/base.rb @@ -26,6 +26,36 @@ def formatter_for(api_format, options = {}) end end end + + module_function + + def present(message, env) + present_options = {} + present_options[:with] = message.delete(:with) if message.is_a?(Hash) + + presenter = env['api.endpoint'].entity_class_for_obj(message, present_options) + + unless presenter + begin + http_codes = env['api.endpoint'].route.route_http_codes || [] + found_code = http_codes.find do |http_code| + (http_code[0].to_i == env['api.endpoint'].status) && http_code[2].respond_to?(:represent) + end + + presenter = found_code[2] if found_code + rescue + nil # some bad specs don't define env + end + end + + if presenter + embeds = { env: env } + embeds[:version] = env['api.version'] if env['api.version'] + message = presenter.represent(message, embeds).serializable_hash + end + + message + end end end end diff --git a/lib/grape/error_formatter/json.rb b/lib/grape/error_formatter/json.rb index 3cb67d8ccf..3ba6eec02f 100644 --- a/lib/grape/error_formatter/json.rb +++ b/lib/grape/error_formatter/json.rb @@ -3,6 +3,8 @@ module ErrorFormatter module Json class << self def call(message, backtrace, options = {}, env = nil) + message = Grape::ErrorFormatter::Base.present(message, env) + result = message.is_a?(Hash) ? message : { error: message } 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 index ca94ed499f..6675f00618 100644 --- a/lib/grape/error_formatter/txt.rb +++ b/lib/grape/error_formatter/txt.rb @@ -3,6 +3,8 @@ module ErrorFormatter module Txt class << self def call(message, backtrace, options = {}, env = nil) + message = Grape::ErrorFormatter::Base.present(message, env) + result = message.is_a?(Hash) ? MultiJson.dump(message) : message if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty? result += "\r\n " diff --git a/lib/grape/error_formatter/xml.rb b/lib/grape/error_formatter/xml.rb index 0a5501365d..7ba2e53143 100644 --- a/lib/grape/error_formatter/xml.rb +++ b/lib/grape/error_formatter/xml.rb @@ -3,6 +3,8 @@ module ErrorFormatter module Xml class << self def call(message, backtrace, options = {}, env = nil) + message = Grape::ErrorFormatter::Base.present(message, env) + result = message.is_a?(Hash) ? message : { message: message } if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty? result = result.merge(backtrace: backtrace) diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index 5d468032f7..ff32611a1b 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -15,7 +15,8 @@ def default_options rescue_options: { backtrace: false }, # true to display backtrace 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 + all_rescue_handler: nil, # rescue handler block to rescue from all exceptions + add_http_code_on_error: false # add http code to error messages } end @@ -76,6 +77,11 @@ def rack_response(message, status = options[:default_status], headers = { 'Conte end def format_message(message, backtrace) + if options[:add_http_code_on_error] + message.code = env['api.endpoint'].status if message.respond_to? :code + message = message.merge(code: env['api.endpoint'].status) if message.respond_to? :merge + end + format = env['api.format'] || options[:format] formatter = Grape::ErrorFormatter::Base.formatter_for(format, options) throw :error, status: 406, message: "The requested format '#{format}' is not supported." unless formatter diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index e4e5fd820b..f2b1efa78c 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -1,5 +1,6 @@ require 'spec_helper' require 'shared/versioning_examples' +require 'grape-entity' describe Grape::API do subject { Class.new(Grape::API) } @@ -1762,6 +1763,54 @@ def self.call(object, env) end end + describe '.add_http_code_on_error' do + it 'allows adding http status code' do + subject.add_http_code_on_error true + + subject.desc 'some desc', http_codes: [[401, 'Unauthorized', Class.new(Grape::Entity)]] + + subject.get '/exception' do + error!({}, 408) + end + + get '/exception' + expect(last_response.status).to eql 408 + expect(last_response.body).to eql({ code: 408 }.to_json) + end + end + + describe 'http_codes' do + + let(:error_presenter) do + Class.new(Grape::Entity) do + expose :code + expose :static + + def static + 'some static text' + end + + end + + end + + it 'is used as presenter' do + + subject.add_http_code_on_error true + + subject.desc 'some desc', http_codes: [[401, 'Error'], [408, 'Unauthorized', error_presenter], [409, 'Error']] + + subject.get '/exception' do + error!({}, 408) + end + + get '/exception' + expect(last_response.status).to eql 408 + expect(last_response.body).to eql({ code: 408, static: 'some static text' }.to_json) + end + + end + context 'routes' do describe 'empty api structure' do it 'returns an empty array of routes' do diff --git a/spec/grape/dsl/request_response_spec.rb b/spec/grape/dsl/request_response_spec.rb index eeda5d132f..859424be06 100644 --- a/spec/grape/dsl/request_response_spec.rb +++ b/spec/grape/dsl/request_response_spec.rb @@ -118,6 +118,13 @@ def self.imbue(key, value) subject.represent :ThisClass, with: presenter end end + + describe '.add_http_code_on_error' do + it 'sets a flag if the error code should be added to the error' do + expect(subject).to receive(:set).with(:add_http_code_on_error, true) + subject.add_http_code_on_error true + end + end end end end diff --git a/spec/grape/middleware/error_spec.rb b/spec/grape/middleware/error_spec.rb index 8b1ed8e597..c80ec1ab31 100644 --- a/spec/grape/middleware/error_spec.rb +++ b/spec/grape/middleware/error_spec.rb @@ -1,6 +1,17 @@ require 'spec_helper' +require 'grape-entity' describe Grape::Middleware::Error do + module ErrorSpec + class ErrorEntity < Grape::Entity + expose :code + expose :static + + def static + 'static text' + end + end + end class ErrApp class << self attr_accessor :error @@ -13,12 +24,16 @@ def call(env) end def app + opts = options Rack::Builder.app do - use Grape::Middleware::Error, default_message: 'Aww, hamburgers.' + use Spec::Support::EndpointFaker + use Grape::Middleware::Error, opts run ErrApp end end + let(:options) { { default_message: 'Aww, hamburgers.' } } + it 'sets the status code appropriately' do ErrApp.error = { status: 410 } get '/' @@ -42,4 +57,21 @@ def app get '/' expect(last_response.body).to eq('Aww, hamburgers.') end + + context 'with http code' do + let(:options) { { default_message: 'Aww, hamburgers.', add_http_code_on_error: true } } + it 'adds the status code if wanted' do + ErrApp.error = { message: {} } + get '/' + + expect(last_response.body).to eq({ code: 200 }.to_json) + end + + it 'presents an error message' do + ErrApp.error = { message: { with: ErrorSpec::ErrorEntity } } + get '/' + + expect(last_response.body).to eq({ code: 200, static: 'static text' }.to_json) + end + end end diff --git a/spec/grape/middleware/exception_spec.rb b/spec/grape/middleware/exception_spec.rb index e747a3f601..2d5fd45247 100644 --- a/spec/grape/middleware/exception_spec.rb +++ b/spec/grape/middleware/exception_spec.rb @@ -54,6 +54,7 @@ def call(env) it 'does not trap errors by default' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error run ExceptionApp end @@ -63,6 +64,7 @@ def call(env) context 'with rescue_all set to true' do it 'sets the message appropriately' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: true run ExceptionApp end @@ -72,6 +74,7 @@ def call(env) it 'defaults to a 500 status' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: true run ExceptionApp end @@ -81,6 +84,7 @@ def call(env) it 'is possible to specify a different default status code' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: true, default_status: 500 run ExceptionApp end @@ -90,6 +94,7 @@ def call(env) it 'is possible to return errors in json format' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: true, format: :json run ExceptionApp end @@ -99,6 +104,7 @@ def call(env) it 'is possible to return hash errors in json format' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: true, format: :json run ErrorHashApp end @@ -109,6 +115,7 @@ def call(env) it 'is possible to return errors in jsonapi format' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: true, format: :jsonapi run ExceptionApp end @@ -118,6 +125,7 @@ def call(env) it 'is possible to return hash errors in jsonapi format' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: true, format: :jsonapi run ErrorHashApp end @@ -128,6 +136,7 @@ def call(env) it 'is possible to return errors in xml format' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: true, format: :xml run ExceptionApp end @@ -137,6 +146,7 @@ def call(env) it 'is possible to return hash errors in xml format' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: true, format: :xml run ErrorHashApp end @@ -147,6 +157,7 @@ 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: { @@ -162,6 +173,7 @@ def call(env) it 'does not trap regular error! codes' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error run AccessDeniedApp end @@ -171,6 +183,7 @@ def call(env) it 'responds to custom Grape exceptions appropriately' do @app ||= Rack::Builder.app do + use Spec::Support::EndpointFaker use Grape::Middleware::Error, rescue_all: false run CustomErrorApp end diff --git a/spec/support/endpoint_faker.rb b/spec/support/endpoint_faker.rb new file mode 100644 index 0000000000..2d9c82f8ad --- /dev/null +++ b/spec/support/endpoint_faker.rb @@ -0,0 +1,23 @@ +module Spec + module Support + class EndpointFaker + class FakerAPI < Grape::API + get '/' do + end + end + + def initialize(app, endpoint = FakerAPI.endpoints.first) + @app = app + @endpoint = endpoint + end + + def call(env) + @endpoint.instance_exec do + @request = Grape::Request.new(env.dup) + end + + @app.call(env.merge('api.endpoint' => @endpoint)) + end + end + end +end From 26426739460b30a20e972568566db35d9addb69c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dieter=20Sp=C3=A4th?= Date: Mon, 18 Aug 2014 14:25:48 +0200 Subject: [PATCH 2/3] removed automatic http code add on error --- README.md | 16 ---------------- lib/grape/dsl/request_response.rb | 9 --------- lib/grape/endpoint.rb | 3 +-- lib/grape/middleware/error.rb | 8 +------- spec/grape/api_spec.rb | 21 +-------------------- spec/grape/dsl/request_response_spec.rb | 7 ------- spec/grape/middleware/error_spec.rb | 8 ++++---- 7 files changed, 7 insertions(+), 65 deletions(-) diff --git a/README.md b/README.md index 8cee80cbda..e673006adb 100644 --- a/README.md +++ b/README.md @@ -958,22 +958,6 @@ error!(Error.new(...), 400) error!({ message: 'An Error', with: API::Error }, 401) error!({ message: 'An Error' }, 408) -``` - -Error messages can be enriched with the status code, when you use a Hash or your message respond to -`code` - -```ruby - -class API < Grape::API - add_http_code_on_error true - - get '/' do - error!({message: 'Error'}, 404) # => result as json "{"message":"Error", "code":404}" - end -end - - ``` ### Default Error HTTP Status Code diff --git a/lib/grape/dsl/request_response.rb b/lib/grape/dsl/request_response.rb index 8ae8e3d658..109bcd8cdd 100644 --- a/lib/grape/dsl/request_response.rb +++ b/lib/grape/dsl/request_response.rb @@ -147,15 +147,6 @@ def represent(model_class, options) imbue(:representations, model_class => options[:with]) end - # Specify if the http code should be added to - # the error message - def add_http_code_on_error(new_value = nil) - if new_value.nil? - settings[:add_http_code_on_error] - else - set(:add_http_code_on_error, new_value) - end - end end end end diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index c75603d140..486b202351 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -238,8 +238,7 @@ def build_middleware rescue_options: settings[:rescue_options], rescue_handlers: merged_setting(:rescue_handlers), base_only_rescue_handlers: merged_setting(:base_only_rescue_handlers), - all_rescue_handler: settings[:all_rescue_handler], - add_http_code_on_error: settings[:add_http_code_on_error] + all_rescue_handler: settings[:all_rescue_handler] aggregate_setting(:middleware).each do |m| m = m.dup diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index ff32611a1b..5d468032f7 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -15,8 +15,7 @@ def default_options rescue_options: { backtrace: false }, # true to display backtrace 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 - add_http_code_on_error: false # add http code to error messages + all_rescue_handler: nil # rescue handler block to rescue from all exceptions } end @@ -77,11 +76,6 @@ def rack_response(message, status = options[:default_status], headers = { 'Conte end def format_message(message, backtrace) - if options[:add_http_code_on_error] - message.code = env['api.endpoint'].status if message.respond_to? :code - message = message.merge(code: env['api.endpoint'].status) if message.respond_to? :merge - end - format = env['api.format'] || options[:format] formatter = Grape::ErrorFormatter::Base.formatter_for(format, options) throw :error, status: 406, message: "The requested format '#{format}' is not supported." unless formatter diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index f2b1efa78c..5ac9a5d5a7 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -1763,22 +1763,6 @@ def self.call(object, env) end end - describe '.add_http_code_on_error' do - it 'allows adding http status code' do - subject.add_http_code_on_error true - - subject.desc 'some desc', http_codes: [[401, 'Unauthorized', Class.new(Grape::Entity)]] - - subject.get '/exception' do - error!({}, 408) - end - - get '/exception' - expect(last_response.status).to eql 408 - expect(last_response.body).to eql({ code: 408 }.to_json) - end - end - describe 'http_codes' do let(:error_presenter) do @@ -1795,13 +1779,10 @@ def static end it 'is used as presenter' do - - subject.add_http_code_on_error true - subject.desc 'some desc', http_codes: [[401, 'Error'], [408, 'Unauthorized', error_presenter], [409, 'Error']] subject.get '/exception' do - error!({}, 408) + error!({code: 408}, 408) end get '/exception' diff --git a/spec/grape/dsl/request_response_spec.rb b/spec/grape/dsl/request_response_spec.rb index 859424be06..eeda5d132f 100644 --- a/spec/grape/dsl/request_response_spec.rb +++ b/spec/grape/dsl/request_response_spec.rb @@ -118,13 +118,6 @@ def self.imbue(key, value) subject.represent :ThisClass, with: presenter end end - - describe '.add_http_code_on_error' do - it 'sets a flag if the error code should be added to the error' do - expect(subject).to receive(:set).with(:add_http_code_on_error, true) - subject.add_http_code_on_error true - end - end end end end diff --git a/spec/grape/middleware/error_spec.rb b/spec/grape/middleware/error_spec.rb index c80ec1ab31..f3e009e0a5 100644 --- a/spec/grape/middleware/error_spec.rb +++ b/spec/grape/middleware/error_spec.rb @@ -59,19 +59,19 @@ def app end context 'with http code' do - let(:options) { { default_message: 'Aww, hamburgers.', add_http_code_on_error: true } } + let(:options) { { default_message: 'Aww, hamburgers.'} } it 'adds the status code if wanted' do - ErrApp.error = { message: {} } + ErrApp.error = { message: {code: 200} } get '/' expect(last_response.body).to eq({ code: 200 }.to_json) end it 'presents an error message' do - ErrApp.error = { message: { with: ErrorSpec::ErrorEntity } } + ErrApp.error = { message: {code: 200, with: ErrorSpec::ErrorEntity } } get '/' - expect(last_response.body).to eq({ code: 200, static: 'static text' }.to_json) + expect(last_response.body).to eq({code: 200, static: 'static text' }.to_json) end end end From b3097a81ff359e14de11752d21a0a30a4a99b42f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dieter=20Sp=C3=A4th?= Date: Mon, 18 Aug 2014 14:42:50 +0200 Subject: [PATCH 3/3] fixed an issue with env and presenting errors --- Gemfile | 3 +++ lib/grape/dsl/request_response.rb | 1 - lib/grape/error_formatter/base.rb | 18 ++++++++---------- spec/grape/api_spec.rb | 2 +- spec/grape/middleware/error_spec.rb | 8 ++++---- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Gemfile b/Gemfile index a08e2173b3..7c9e6d21af 100644 --- a/Gemfile +++ b/Gemfile @@ -14,3 +14,6 @@ platforms :rbx do gem 'rubinius-developer_tools' gem 'racc' end + +gem 'pry' +gem 'pry-byebug' diff --git a/lib/grape/dsl/request_response.rb b/lib/grape/dsl/request_response.rb index 109bcd8cdd..6f9167dcad 100644 --- a/lib/grape/dsl/request_response.rb +++ b/lib/grape/dsl/request_response.rb @@ -146,7 +146,6 @@ def represent(model_class, options) raise Grape::Exceptions::InvalidWithOptionForRepresent.new unless options[:with] && options[:with].is_a?(Class) imbue(:representations, model_class => options[:with]) end - end end end diff --git a/lib/grape/error_formatter/base.rb b/lib/grape/error_formatter/base.rb index 2d526a6f14..5c68314203 100644 --- a/lib/grape/error_formatter/base.rb +++ b/lib/grape/error_formatter/base.rb @@ -35,17 +35,15 @@ def present(message, env) presenter = env['api.endpoint'].entity_class_for_obj(message, present_options) - unless presenter - begin - http_codes = env['api.endpoint'].route.route_http_codes || [] - found_code = http_codes.find do |http_code| - (http_code[0].to_i == env['api.endpoint'].status) && http_code[2].respond_to?(:represent) - end - - presenter = found_code[2] if found_code - rescue - nil # some bad specs don't define env + unless presenter || env['rack.routing_args'].nil? + # env['api.endpoint'].route does not work when the error occurs within a middleware + # the Endpoint does not have a valid env at this moment + http_codes = env['rack.routing_args'][:route_info].route_http_codes || [] + found_code = http_codes.find do |http_code| + (http_code[0].to_i == env['api.endpoint'].status) && http_code[2].respond_to?(:represent) end + + presenter = found_code[2] if found_code end if presenter diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 5ac9a5d5a7..12c3ff2d41 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -1782,7 +1782,7 @@ def static subject.desc 'some desc', http_codes: [[401, 'Error'], [408, 'Unauthorized', error_presenter], [409, 'Error']] subject.get '/exception' do - error!({code: 408}, 408) + error!({ code: 408 }, 408) end get '/exception' diff --git a/spec/grape/middleware/error_spec.rb b/spec/grape/middleware/error_spec.rb index f3e009e0a5..ed627fbc55 100644 --- a/spec/grape/middleware/error_spec.rb +++ b/spec/grape/middleware/error_spec.rb @@ -59,19 +59,19 @@ def app end context 'with http code' do - let(:options) { { default_message: 'Aww, hamburgers.'} } + let(:options) { { default_message: 'Aww, hamburgers.' } } it 'adds the status code if wanted' do - ErrApp.error = { message: {code: 200} } + ErrApp.error = { message: { code: 200 } } get '/' expect(last_response.body).to eq({ code: 200 }.to_json) end it 'presents an error message' do - ErrApp.error = { message: {code: 200, with: ErrorSpec::ErrorEntity } } + ErrApp.error = { message: { code: 200, with: ErrorSpec::ErrorEntity } } get '/' - expect(last_response.body).to eq({code: 200, static: 'static text' }.to_json) + expect(last_response.body).to eq({ code: 200, static: 'static text' }.to_json) end end end