Should error! and error_response have the same method signature?

[sunnyrjuneja]

I was trying to use error! inside of a rescue_from block and got a NoMethodError. I later found error_response and noticed that you don't get the same level of customization that you do from error!.

I'm proposing that error_response and error! have the same method signature. Their current implementations are as follows:

def error_response(error = {})
  status = error[:status] || options[:default_status]
  message = error[:message] || options[:default_message]
  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)
end

def error!(message, status = nil, headers = nil)
  self.status(status || namespace_inheritable(:default_error_status))
  throw :error, message: message, status: self.status, headers: headers
end

My use case was to use error_response like I'm using error! below:

error!({id: 'not_found', message: 'Not Found.'}, 404)

Instead, I'm skipping using error_response and returning a Rack::Response object like this:

Rack::Response.new({
  id: 'interal_error',
  message: 'Interal server error. Check logs.' 
}.to_json, 500, { 'Content-type' => 'text/error' }).finish

Apologies if I'm misunderstanding the API and there's a simple way to do the same thing.

[dblock]

There's a lot of legacy around this, so most of the reasons for it to be this way are historical and not practical. Try making a PR that changes things the way you describe, I think it's definitely a worthy endeavor.

[sunnyrjuneja]

@dblock I just got a chance to work on this and I was a bit confused. One big difference between the two method signatures is that error_response accepts a backtrace and error! doesn't.

When I run this example, I don't see a backtrace from /error. I do see one for /standard_error. It isn't clear to me if rack or grape is providing the backtrace for /standard_error.

module Example
  class API < Grape::API
    format :json

    get :standard_error do
      raise StandardError.new("hello")
    end

    get :error do
      raise Grape::Exceptions::Base
    end
  end
end

From what I can tell, the formatter shouldn't be swallowing the backtrace:
https://github.com/intridea/grape/blob/master/lib/grape/error_formatter/json.rb

Is rack providing the backtrace or grape?

Is is it the default behavior for formatters to swallow backtraces? In that case, it makes sense for the backtrace to be dropped as an argument.

[dblock]

I think the backtrace of where the exception was raised is (should be) useful in both scenarios and the error formatter or whoever returns something should have access to it and be able to render it.

[adamgotterer]

I don't believe error! can be thrown in an exception context (like from a rescue) so I'd imagine you don't actually have an exception backtrace even available.

What I think you're looking for is https://github.com/intridea/grape/blob/master/lib/grape/middleware/error.rb line 70. You'll see the backtrace comes from the exception and then is passed to format_message.

[sunnyrjuneja]

I just gave my first go at here.

I tried my best to change as little as code as possible while matching the method signature of error! but I'm not sure how I should resolve the test cases that are currently failing.

Most of the failures stem from the fact that handle_error returns a hash whose structure I'm respecting:

      def handle_error(e)
        error_response(message: e.message, backtrace: e.backtrace)
      end

The tests expect that I return whatever's in the key :message but the point of the PR was to respect whatever was passed to error_response.

Here's a sample of what I'm talking about:

  1) Grape::API custom middleware .use mounts behind error middleware
     Failure/Error: expect(last_response.body).to eq('Caught in the Net')

       expected: "Caught in the Net"
            got: "{\"message\":\"Caught in the Net\"}"

       (compared using ==)
     # ./spec/grape/api_spec.rb:981:in `block (4 levels) in <top (required)>'

Any thoughts on how to proceed? I know it may not be clear what I'm saying, so feel free to ask me to elaborate.

[dblock]

It's possible that you need to change the tests. Obviously you want to be able to render both a String ("Caught in the Net") and a JSON (message: ...), so as long as the tests cover both you're good. The important thing is that for those that used to just raise or error! in the API don't have to change a thing and get the same result.

The next thing to think about is to explain in UPGRADING what users who called error_response directly are supposed to do.

[sunnyrjuneja]

@dblock I'll give it some thought. I'll work on the tests this week and hopefully have something in mind for UPGRADING.

[sunnyrjuneja]

Hey @dblock, I apologize for the delay on this. However, I believe I've found a better solution. I've created a method called error! that wraps error_response.

My recommendation is to encourage the use of error! over rack_response. By doing so, we can do the following:

format :json
subject.rescue_from Grape::Exceptions::ValidationErrors do |e|
  rack_response e.to_json, 400
end

# Becomes
format :json
subject.rescue_from Grape::Exceptions::ValidationErrors do |e|
  error! e, 400
end

People using error_response need not to change. error_response be a quick way to send an error while error! can maintain api compatibility with other error! and allow more fine-grained control.

If someone decides to use format :xml, it should update no problem without needing to change the code. I can update the README and tests if you want. I've included a commit that shows the implementation.

However, this does pose one problem: although I had updated all the tests, there was one that I couldn't get to pass. Would you mind taking a look?

[sunnyrjuneja]

Here is the failing test: sunnyrjuneja@844b5c9

Here is the branch: https://github.com/whatasunnyday/grape/tree/error-new

Once again, I'm sorry for the extreme delay on this PR. I hope adding this is still interesting to you.