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

error messages can now be presented #705

Closed
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -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)
Expand Down
32 changes: 32 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

I find this option either very confusing or bad practice.

It populates the result with an HTTP error code, but you already get the HTTP error code from ... HTTP. This creates an opportunity for a different value here vs. the HTTP response, for example if you have some middleware upstream that rewrites things (not uncommon).

I suggest removing it from this PR, and opening an issue for discussion. Generally there should be a way to augment errors with things like the HTTP status code or, say, a value of a header or something else without magic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not every client can evaluate an HTTP client error. For instance when you have a batch script which uses your API with curl and a pipe then it is easier to evaluate the JSON data and grep the status code from there. Furthor more when your data contains an error code you know it was triggered by your server and not because a network error.

Or think of jsonp (http://www.theguardian.com/info/developer-blog/2012/jul/16/http-status-codes-jsonp). For JSONP you need an error in your data and can't use HTTP error codes.

With the middleware you named how do you ensure that you don't break the contract for your API? HTTP error codes are documented with swagger too.

So in my point of view it is valid to duplicate the HTTP error code, but if you want I can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

I want to think about it. Can you remove it for now and split it into a separate issue? Sorry for the hassle.


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`.
Expand Down
2 changes: 1 addition & 1 deletion grape.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
38 changes: 22 additions & 16 deletions lib/grape/dsl/inside_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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
10 changes: 10 additions & 0 deletions lib/grape/dsl/request_response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion lib/grape/endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 30 additions & 0 deletions lib/grape/error_formatter/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

This is swallowing a useful exception, and seems to only serve specs. In some conditions you're going to get unexpected results and will have no way of knowing why. Specs should be fixed instead of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be 37 spec's to fix :(

Copy link
Member

Choose a reason for hiding this comment

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

Thankfully we have before blocks :)

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
2 changes: 2 additions & 0 deletions lib/grape/error_formatter/json.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions lib/grape/error_formatter/txt.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down
2 changes: 2 additions & 0 deletions lib/grape/error_formatter/xml.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 7 additions & 1 deletion lib/grape/middleware/error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
49 changes: 49 additions & 0 deletions spec/grape/api_spec.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require 'spec_helper'
require 'shared/versioning_examples'
require 'grape-entity'

describe Grape::API do
subject { Class.new(Grape::API) }
Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions spec/grape/dsl/request_response_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
34 changes: 33 additions & 1 deletion spec/grape/middleware/error_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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 '/'
Expand All @@ -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
Loading