-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be 37 spec's to fix :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thankfully we have |
||
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.