Fixed finding handler in error middleware. Fixes #1364

[ktimothy]

No description provided.

[dblock]

Need CHANGELOG and a test, please.

[ktimothy]

Sure, will do that tomorrow.

[ktimothy]

Looks like the issue is a little bit deeper. Last call to rescue_from using with: overwrites previously set with: value.

For now option 'with:' is stored now in rescue_options in middleware/error, which makes it common for all exceptions.

I think the option 'with:' should be stored for each exception type separately.

[dblock]

You probably want to dup options first, since you're modifying the original hash or continue just doing options[:with].

[dblock]

See minor-ish comments, otherwise good to merge?

[ktimothy]

Yes, just made small fixes.

[dblock]

That's not what I meant. This particular one actually returns a new hash every time according to the documentation, so a dup is just doing extra unnecessary work.

[dblock]

So what's wrong with just checking options[:with] below? Why does it need to delete that value?

[ktimothy]

This particular one actually returns a new hash every time

Oh, shame to me =)

Ok, we should have discussed it a little bit before I made my changes =). The option :with was used in middleware/error. I removed that usage, and stored its value in rescue_handlers hash.

So I decided to delete :with from options hash just to clean it from unused key. And that's why I had to fix request_response_spec.rb.

[dblock]

Sorry to be a pest, I have more questions ;)

[ktimothy]

No problem ;)

[dblock]

@ktimothy I am so sorry to be thick and slow here (typing this from an island in the Caribbean). Amend the code to what you want it to be, and re-explain the whole thing to me like a two year old. Really appreciate your patience.

[ktimothy]

@dblock I hope you have a good time there =)

Ok, here is the whole thing.

At first I handled different exceptions using block syntax:

rescue_from ArgumentError do |e|
  rack_response({error: 'argument_error'}.to_json)
end

rescue_from :all do |e|
  rack_response({error: 'internal_error'}.to_json)
end

Then I wanted to move exception handlers from api class to helpers module. So I tried to use options :with:

rescue_from ArgumentError, with: :rescue_argument_error
rescue_from :all, with: :all_errors_handler

which made my api code much better.

But, unfortunately, grape stopped handling ArgumentError, calling :all_errors_handler instead. I started to dig into the problem, and found out, that :with is stored in :rescue_options hash, which is common for all exception types. Because of that :with option is overwritten every time you call rescue_from: ..., with: .... This behavior is definitely wrong, as we save last with: value in options.

As a solution I suggest to store with: in rescue_handlers and base_only_rescue_handlers, where block handlers are now stored. So values in these two hashes can be either a block or a symbol.

Error middleware now does not check rescue_options[:with], it tries to take handler from rescue_handlers`` andbase_only_rescue_handlers, then triesall_rescue_handler```. If handler is a symbol, we know that we should use helper method with corresponding name.

[dblock]

Merged via 044dad7, thanks for the clear explanation!

[ktimothy]

Awesome, thanks!