error messages can now be presented

[dspaeth-faber]

This PR is a proposal (and misses specs & co) to present error messages like started to discuss here.
It will be possible to do some thing like this:

module  API
  module Entities
    class Error < Grape::Entity
      expose :error, documentation: { type: 'string', required: true }
      expose :code, documentation: { type: 'integer', required: true }
      expose :detail, documentation: { type: 'string', required: true }
      expose :href, documentation: { type: 'url', required: true }
      expose :rel, documentation: { type: 'string', required: true }
      expose :time, documentation: { type: 'datetime', required: true }

      def rel
        'self'
      end

      def href
        options[:env]['api.endpoint'].request.url
      end

      def time
        Time.now.utc
      end
    end
  end
end

error!({ error: 'Not Found',
            detail: "Some more information",
            with: API::Entities::Error
           }, 404)

This PR requires the upcomming version of grape-entity .

[dblock]

You don't want dup here, merge already copies the hash and returns another one.

2.0.0-p353 :001 > h = { foo: 'bar' }
 => {:foo=>"bar"} 
2.0.0-p353 :002 > h.merge(bar: 'foo')
 => {:foo=>"bar", :bar=>"foo"} 
2.0.0-p353 :003 > h
 => {:foo=>"bar"} 
2.0.0-p353 :004 > 

[dblock]

I think this is totally fine.

[dspaeth-faber]

@dblock Ok, added spec's and refactored it a little bit.

[mbleigh]

I appreciate the efforts to clean up error presentation, it's definitely a weak point of the framework. Personally I'd prefer a more comprehensive approach as I outlined in #620 but unfortunately have not yet had the time to implement.

I'm going to try to find some time this month to implement my proposal which I think covers this use case and more...do you agree that this PR would be unnecessary if #620 were implemented?

[dspaeth-faber]

The DSL cleanup is very nice. To have a class for descriptions is also very cool.

But is this implementation compatible with grape-swagger?

With your current implementation Errors are still not presentable with custom presenters. Don't force the user to use you'r Grape::ErrorEntity. grape-swagger generetes response models out of the http_codes. You can ensure this API-Contract best when you respresent you'r errors with the documented Entities. So use the Entities please.

In the long run you'r implementation is of course better (except the enforced error format).

But done is better than perfect.

[dblock]

your

[dblock]

Let me sit on this PR for a bit and consider the proposals in #620. In the meantime, can you please make sure this build is passing clean? Thanks.

[dspaeth-faber]

I did make the changes. I can't make the build pass. This PR needs a new grape-entity version.

[dblock]

Really? Looking at the spec output I don't see why you couldn't make this backward compatible. What am I missing?

[dspaeth-faber]

undefined method 'code' for {:code=>408}:Hash that is the part which show's that the new entity gem is needed. With the current one you can't present a Hash . Hash presentation was introduced with https://guthub.com/intridea/grape-entity/pull/85

[dblock]

I see. I'll do my best to release a grape-entity this week and will update/merge this.

[dspaeth-faber]

Thank you! What's about @mbleigh work? I realy like his cleaner way, but it need's a lot of work I think.

[dblock]

I've released grape-entity, so this can be wrapped up. I wouldn't wait on @mbleigh's proposal because it's not close to being finished and this does move us forward, albeit not all the way.

[dspaeth-faber]

Nice. I did update dependencies and pushed new version.

[dblock]

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.

[dspaeth-faber]

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.

[dblock]

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

[dspaeth-faber]

Ok, I did remove the error code presentation and fixed an issue when errors occour within a middleware.

[dspaeth-faber]

What's about the URI::InvalidURIError with 2.2.head? Do you know why this fails?

[dblock]

I rewrote the README part of this and committed squashed in 37362d8. Lets deal with whatever fails from here.

[dspaeth-faber]

I did open an ruby issue.
This does not work with ruby 2.2-head but all previous rubies:

require 'uri'

URI('//example.org/nested_optional_group?items[][key]=foo')
URI('//example.org/nested_optional_group?items[key]=foo')
URI('//example.org/nested_optional_group?items[]=foo')

[dblock]

Yeah, I am not to worried about HEADs ;)

[dspaeth-faber]

So head should maybe removed from .travis.yml?

[dblock]

It's allowed to fail exactly for this kind of stuff. It's good that we catch Ruby bugs early and report them (like you did).

[dspaeth-faber]

I won't create a PR for adding status codes to error responses. I will solve this within my presenter.