Change behaviors of rescue_from to not depend on mount order

[wowinter13]

Hi, found an interesting case when rescue_from :all should override outer rescue_from :error_class

class PublicApi < Grape::API
  rescue_from ActiveRecord::RecordNotFound do
    binding.pry
    error_response(status: 403)
  end
  mount AuthController
end
class AuthController < Grape::API
  rescue_from :all do 
    error_response(status: 403)
  end

  post :request do
    User.find_by!(phone: phone)
  end
end

15:  rescue_from ActiveRecord::RecordNotFound do
15:     binding.pry
16:     error_response(status: 403)
17:  end

For some reason, it seems to not working correctly. I suppose AuthController rescue_from :all to be called first.
But if we mount AuthController above rescue_from ActiveRecord::RecordNotFound it would work correctly.
So, does it look like a bug or additional info to documentation?

[dblock]

Can you turn it into a spec? Looks incorrect to me.

[wowinter13]

@dblock Should I create a PR with a spec? Or gist in the discussion would be enough?

[dblock]

@dblock Should I create a PR with a spec? Or gist in the discussion would be enough?

Yes. And don't hesitate to try a fix!

[jcagarcia]

Hey guys! 👋

I've been examining this issue and I'm uncertain if it qualifies as a bug. As far as I can tell, Grape seems to be functioning as intended.

In your PublicApi < Grape::API class, you've specified a rescue_from for the specific class ActiveRecord::RecordNotFound. Following this, you're mounting the AuthController < Grape::API class, which has a declared rescue_from :all.

What's happening here is that upon mounting, you essentially "merge" the AuthController into the PublicApi context, inheriting all the defined rescue_from methods. Nevertheless, your primary class remains PublicApi.

Consequently, when an error occurs, the first error handlers considered are those within the class. If none apply, it then resorts to the :all handlers.

grape/lib/grape/middleware/error.rb

Lines 43 to 46 in 3901bf4

	rescue_handler_for_base_only_class(e.class) ||
	rescue_handler_for_class_or_its_ancestor(e.class) ||
	rescue_handler_for_grape_exception(e.class) ||
	rescue_handler_for_any_class(e.class) ||

In your scenario, the initial entry point is rescue_from ActiveRecord::RecordNotFound because even though you're invoking an endpoint defined in the AuthController class, the context remains within the PublicApi class. Hence, the initially considered handler is rescue_from ActiveRecord::RecordNotFound.

By altering the order of mounting before the rescue_from ActiveRecord::RecordNotFound definition, the rescue handler isn't in place when those endpoints are mounted. Consequently, as @wowinter13 correctly said, the :all rescue is directly invoked. This is why the sequence impacts the handler chosen.

With that being said, I still maintain the view that this behavior is not a bug; rather, it aligns with the expected functionality.

What do you think? Make sense?

[dblock]

Thanks for the deep dive! If you compare this behavior with version, then it looks different: a version declaration in AuthController would take precedence over the declaration in PublicApi, won't it? In that case logically at least we'd want locality to be consistent? WDYT?

[jcagarcia]

Hi @dblock , thanks for the answer!

I believe the functionality of the version method differs in this context. It's essential to consider that the mount method involves reusing endpoints within another class's context. Removing the mount from the equation, the example above can be translated as follows in terms of the rescue_from methods:

class PublicApi < Grape::API
  rescue_from ActiveRecord::RecordNotFound do
    binding.pry
    error_response(status: 403)
  end
  
  rescue_from :all do 
    error_response(status: 403)
  end

  post :request do
    User.find_by!(phone: phone)
  end
end

In this scenario, there are two rescue_from declarations within the PublicApi context. The handling priority is given to the specific class one, which seems reasonable to me.

Regarding the version method, replacing rescue_from with version in the previous example results in:

class PublicApi < Grape::API
  version 'v1'

  version 'v2'

  post :request do
    User.find_by!(phone: phone)
  end
end

Here, all endpoints after version 'v2' become available under /v2. As there are no defined endpoints under version 'v1', none are accessible under /v1.

A comparison between the version and rescue_from methods can be made by declaring both as rescue_from ActiveRecord::RecordNotFound in separate classes when using mount:

class PublicApi < Grape::API
  rescue_from ActiveRecord::RecordNotFound do
    binding.pry
    error_response(status: 403)
  end
  mount AuthController
end

class AuthController < Grape::API
  rescue_from ActiveRecord::RecordNotFound do 
    error_response(status: 403)
  end

  post :request do
    User.find_by!(phone: phone)
  end
end

This translates to:

class PublicApi < Grape::API
  rescue_from ActiveRecord::RecordNotFound do
    binding.pry
    error_response(status: 403)
  end
  
  rescue_from ActiveRecord::RecordNotFound do 
    error_response(status: 403)
  end

  post :request do
    User.find_by!(phone: phone)
  end
end

In this case, the last error handler defined takes precedence. This behavior aligns with the functionality of the version method. The difference lies in the fact that the version method doesn’t have a specified execution order, whereas the defined rescue_from handlers possess a defined sequence.

Does it make sense?

UPDATE:

What I'm start thinking is that the real bug is the "working scenario" described by @wowinter13 about changing the order of the mount. If you mount the AuthController before defining the rescue_from ActiveRecord::RecordNotFound handler like:

class PublicApi < Grape::API
  mount AuthController
  rescue_from ActiveRecord::RecordNotFound do
    binding.pry
    error_response(status: 403)
  end
end

class AuthController < Grape::API
  rescue_from :all do 
    error_response(status: 403)
  end

  post :request do
    User.find_by!(phone: phone)
  end
end

It should be translated to:

class PublicApi < Grape::API
  rescue_from :all do 
    error_response(status: 403)
  end

  post :request do
    User.find_by!(phone: phone)
  end

  rescue_from ActiveRecord::RecordNotFound do
    binding.pry
    error_response(status: 403)
  end
end

And it should follow the strategy for selecting the error handler and enter first in the rescue_from ActiveRecord::RecordNotFound. However, it's entering the rescue_from :all. Is like when performing the mount is taking into account ONLY the elements defined before mounting.

[dblock]

I think you're completely correct on all the points above.

So, is the problem in this issue correctly formulated or do we need to edit it? What's the tl;dr of the ask? What do we intend to change?

Either way, before changing anything, we have to start by ensuring that we have specs covering all current behavior and documentation that expresses it correctly. Appreciate any PRs!

[jcagarcia]

Hey @dblock ,

I think that the issue should be edited from the original title "An incorrect behavior of :rescue_from" to something like "Different behaviors of rescue_from method when changing mount order"

The tl;dr of the ask is something like "Depending on the order of the mount, the selected rescue_from handler differs. Should the rescue_from selection precedence behave always in the same way when using or not using mount?"

If you agree, let me work on this :) First trying to cover in specs the current behavior and documenting it and then, trying to solve the mount issue.

[jcagarcia]

Hi again @dblock ,

I was working on documenting the current behavior and to add some tests for these scenarios and I found that there is already an explanation about the inherited rescue_from handlers and the mount order.

grape/README.md

Lines 411 to 422 in 3901bf4

	Keep in mind such declarations as `before/after/rescue_from` must be placed before `mount` in a case where they should be inherited.
	```ruby
	class Twitter::API < Grape::API
	before do
	header 'X-Base-Header', 'will be defined for all APIs that are mounted below'
	end
	mount Twitter::Users
	mount Twitter::Search
	end
	```

In fact, this was already managed in #646 some years ago.

So my question now is... do we want to change how the inheritance works when using mount?

[dblock]

I renamed this to "Change behaviors of rescue_from to not depend on mount order", lmk if this can be improved.

So my question now is... do we want to change how the inheritance works when using mount?

Well, no. Personally the order is predictable and logical (once you understand it). But the OP, @wowinter13 was visibly confused, so it tells me that I could be wrong. I guess I'd be looking for some strong opinions from various people and code contributions that change the behavior! Happy to be the arbiter.

[jcagarcia]

Hey @dblock ,

After taking a deep look to this, I think I have a possible solution that seems to be working.

Take a look to the explanation of the PR #2384

[jcagarcia]

This has been solved at #2384 :)

[dblock]

Maybe @wowinter13 wants to try it out from HEAD?