Was it intentional to hide the http response status from the error formatter?

[drewnichols]

I'm curious if this was discussed before and done intentionally.

For some reason, the status value is not passed into the format_message method here in error!:

grape/lib/grape/middleware/error.rb

Lines 132 to 137 in 5ce44de

	def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil)
	rack_response(
	status, headers.reverse_merge(Rack::CONTENT_TYPE => content_type),
	format_message(message, backtrace, original_exception)
	)
	end

Passing it would make including it in the response body JSON easier. This is the way it's included in the JSON API error response bodies:

# HTTP/1.1 422 Unprocessable Entity
# Content-Type: application/vnd.api+json

{
  "errors": [
    {
      "status": "422",
      "source": { "pointer": "/data/attributes/firstName" },
      "title":  "Invalid Attribute",
      "detail": "First name must contain at least two characters."
    }
  ]
}

[dblock]

I don't think this was intentional at all, please feel free to PR a change.

[drewnichols]

I will make it so

[ericproulx]

Ain't the most intuitive way but you can access the status from the env variable which is Rack's env.

You can access endpoint's status code like this env[Grape::Env::API_ENDPOINT].status

describe 'error formatter env' do
  subject { last_response.body }

  let(:app) do
    Class.new(described_class) do
      format :json

      error_formatter :json, ->(message, _backtrace, _options, env, _original_exception) {
        "#{message}:#{env[Grape::Env::API_ENDPOINT].status}"
      }

      rescue_from :all do
        error!("I'm a teapot!", 418)
      end

      get { raise StandardError }
    end
  end

  before do
    get '/'
  end

  it { is_expected.to eq("I'm a teapot!:418") }
end

@dblock To be more intuitive, I would suggest to revisit the signature to also include headers. It would be a breaking change obviously but for the better good I think.

[dblock]

@drewnichols I think I agree with @ericproulx

1. Does the workaround of env[Grape::Env::API_ENDPOINT].status work for you?
2. Maybe we can add a locally scoped status in the error_formatter context?

[drewnichols]

The env[Grape::Env::API_ENDPOINT].status seems to be set in many cases but not all. I found that it works when I have custom rescue_from blocks like the following.

  rescue_from NotAuthenticatedError do |e|
    error!(e.message, 401, {}, e.backtrace, e)
  end

Ironically for grape errors rescued with the following

    rescue_from :grape_exceptions

we get

Grape::Env::API_ENDPOINT].status == 201

I'm sure there is a fix for this but it feels like an indirect long-term solution that could be broken in the future.

[dblock]

So any ideas about making this change backwards compatible?

[drewnichols]

I need another week or so to work on it. Been busy with real job and it's snowing this weekend.

[drewnichols]

I see the issue now. Anyone (like me) who created a formatter as a module like the following will get ArgumentError: wrong number of arguments (given 6, expected 5).

module CustomFormatter
  def self.call(message, backtrace, options, env, original_exception)
    { message: message, backtrace: backtrace }
  end
end

I'm guessing this is not encouraged but we could get around this by inspecting the CustomFormatter call method and checking the argument count. Something like the following in Grape::Middleware::Error.format_message.

if formatter.method(:call).arity == 6
  return formatter.call(message, backtrace, options, env, original_exception, status)
else
  return formatter.call(message, backtrace, options, env, original_exception)
end

[dblock]

I dislike the arity check a lot, but I am not seeing another option either.

My bigger concern is that this change is not future proof as we might find more things we want to pass along into custom formatters. Am I wrong to think this is a bad interface?

We could step back a little. Some ideas.

1. Hold this exact change back till a major version increment (all problems above).
2. Redesign this interface to take options only.
3. Change the calling context for custom formatters to have access to self.error, self.env, etc.

WDYT?

[ericproulx]

I think we should aim on a function with a signature that looks like this

def call(**args)
 ...
end

It gives a lot of flexibility and more future proof. Grape's internals can pass any parameters without breaking the signature.

Plus, anyone can redefine with explicit keyword args. For instance

def call(message:, status:, backtrace:, **_opts)
...
end

[ericproulx]

The env[Grape::Env::API_ENDPOINT].status seems to be set in many cases but not all. I found that it works when I have custom rescue_from blocks like the following.

rescue_from NotAuthenticatedError do |e|
error!(e.message, 401, {}, e.backtrace, e)
end

Ironically for grape errors rescued with the following

rescue_from :grape_exceptions

we get

Grape::Env::API_ENDPOINT].status == 201

I'm sure there is a fix for this but it feels like an indirect long-term solution that could be broken in the future.

I will check that. It supposed to return the appropriate status code

[ericproulx]

@drewnichols I've a fix coming when using rescue_from without block.

[ericproulx]

@drewnichols #2530 should fix the issue regarding the endpoint's status when using rescue_from without a block.

[drewnichols]

#2530 worked I can now access the HTTP status via env[Grape::Env::API_ENDPOINT].status