Returning an invalid response in a rescue_from block causes “NoMethodError: undefined method 'default_rescue_handler'”

[manueljacob]

If the following code path is executed, an exception “NoMethodError: undefined method 'default_rescue_handler'” is raised.

grape/lib/grape/middleware/error.rb

Line 128 in 5671969

run_rescue_handler(:default_rescue_handler, Grape::Exceptions::InvalidResponse.new, endpoint)

The reason is that the following code does not take private methods into account:

grape/lib/grape/middleware/error.rb

Lines 114 to 116 in 5671969

	raise NoMethodError, "undefined method '#{handler}'" unless respond_to?(handler)
	handler = public_method(handler)

The reason for the introduction of the error is that the code referred to above was forgotten to be changed in c6ad84a, in which default_rescue_handler was made private and other code was changed from using :default_rescue_handler to using method(:default_rescue_handler). The fix for the problem is to apply that change to the code referred to above as well.

Another problem is that the code path is seemingly untested. While preparing a pull request containing a test and a fix, I was unsure where to put the test, as there are many places where rescue_from is tested.

[dblock]

@manueljacob Thanks for reporting this. Sounds like you're working on a fix (+ tests)? Thank you.

[ericproulx]

This test is failing

context 'when rescue_from block results in an invalid response' do
  subject { last_response }

  let(:api) do
    Class.new(described_class) do
      rescue_from :all do
        :whatever
      end

      get { raise ArgumentError, 'Oops!' }
    end
  end

  let(:app) { api }

  before { get '/' }

  it { is_expected.to be_server_error }
end

[manueljacob]

@manueljacob Thanks for reporting this. Sounds like you're working on a fix (+ tests)? Thank you.

I applied the (simple) fix locally and it worked for me. I didn’t yet start writing a test because I was unsure where to put it.

[manueljacob]

It turns out that the code path is already tested, but a bug in rspec-mocks causes the private method to become public: https://github.com/ruby-grape/grape/blob/v2.1.3/spec/grape/api_spec.rb#L2200

[ericproulx]

It turns out that the code path is already tested, but a bug in rspec-mocks causes the private method to become public: https://github.com/ruby-grape/grape/blob/v2.1.3/spec/grape/api_spec.rb#L2200

Wow, nice catch!. We only have one expect_any_instance_of in our spec suite and I'm really not in favor of keeping it. Like you suggested, we can just wrap the :default_rescue_handler with method like the other cases.