fix(#1922): Allow to use instance variables defined in the endpoints inside rescue_from

[jcagarcia]

Hey 👋

I was taking a look at #1922 and after some checks I can see that we are in a different instance when we are inside the rescue_from method.

require 'grape'

class MyAPI < Grape::API
  class MyError < StandardError; end

  before do
    puts "self => #{self.class}"
    @my_var = 328
  end

  rescue_from MyError do |e|
    puts "self => #{self.class}"
    puts "my var value => #{@my_var}"
    error!({ my_var: @my_var }, 401)
  end

  get '/wadus' do
    puts "my var value => #{@my_var}"
    puts "self => #{self.class}"
    raise MyError.new
  end
end

The current output of the previous code is:

self =>#<Class:0x0000000107c79d88>
my var value => 328
self => #<Class:0x0000000107c79d88>
self => ##<Class:0x0000000104c77950>
my var value =>

This means that we cannot have access to the same instance variables declared in the before because we are not in the same instance.

For solving it, I'm adding the scope of the endpoint when performing the handler. So now I'm performing endpoint.instance_exec(&handler) instead of instance_exec(&handler) where the endpoint is the instance obtained from @env["api.endpoint"] here.

Doing it in that way, we are executing the handler under the endpoint instance scope and we have the variables accessible.

self => #<Class:0x000000010448a148>
my var value => 328
self => #<Class:0x000000010448a148>
self => #<Class:0x000000010448a148>
my var value => 328

However, there are some trade-offs that we need to fix in this issue in order to pass all the tests.

As now we are performing the handler under the endpoint instance, the methods called inside the rescue_from should be defined in the inside_route.rb file. I've experienced problems with two of those methods:

• The error! method. Is defined both times in inside_routes.rb and in the error.rb. However, the one defined in the inside_routes.rb is throwing an error and the one defined in the error.rb is just returning a rack response.
  • Proposed Solution: Catch the thrown error when performing the handler and if an error appears because someone calls the error! method, call the error! method from the error.rb again for returning the rack response. Check this
• The rack_response method is used in several tests inside the rescue_from. However, this method is not defined in the inside_route.rb file, what causes that a NoMethodError is raised.
  • Proposed Solution: Define the rack_response method in the inside_route.rb file, so now endpoints can also perform this method for building a rack response.

[dblock]

This looks very good! No other specs are failing, which means we're on the right track to defining this behavior.

1. What happens if I do @var = 1 in the first call, does it exist in the second call to the same API? What's the scope of @var across multiple API methods? All these need tests.
2. Document this behavior in README.
3. The meaning of @var has changed in rescue_from, it will need an UPGRADING explanation. No specs were broken, but is it a breaking change?

[jcagarcia]

Hi @dblock 👋 thanks!

For the first point:

If you do @var = 1 in the first call, @var should not be available in a second call. This is because a new instance is created for each of the different endpoints. Here an example:

class MyAPI < Grape::API
  class MyError < StandardError; end

  rescue_from MyError do |e|
    puts "rescue_from_self => #{self.class}"
    puts "rescue_from_my_var => #{@my_var}"
    error!({ my_var: @my_var }, 401)
  end

  get '/first' do
    @my_var = 1
    puts "my_var => #{@my_var}"
    puts "self => #{self.class}"
    raise MyError.new
  end

  get '/second' do
    puts "my_var => #{@my_var}"
    puts "self => #{self.class}"
    raise MyError.new
  end
end

And here the output after performing /first, /second and /first again.

my_var => 1
self => #<Class:0x0000000105ffa518>
rescue_from_self => #<Class:0x0000000105ffa518>
rescue_from_my_var => 1
::1 - - [22/Nov/2023:16:34:44 +0100] "GET /first HTTP/1.1" 401 12 0.0334
my_var =>
self => #<Class:0x0000000105ff9578>
rescue_from_self => #<Class:0x0000000105ff9578>
rescue_from_my_var =>
::1 - - [22/Nov/2023:16:34:47 +0100] "GET /second HTTP/1.1" 401 15 0.0012
my_var => 1
self => #<Class:0x0000000105ffa518>
rescue_from_self => #<Class:0x0000000105ffa518>
rescue_from_my_var => 1
::1 - - [22/Nov/2023:16:34:51 +0100] "GET /first HTTP/1.1" 401 12 0.0014

As you can see, one instance per endpoint is created (when calling /first again, the same instance is reused), so variables are not accessible across multiple API methods.

Do you think we should add several tests for checking that this behavior does not change?

For point number 2:

Totally agree on document this behavior in the README 👍 let me add a commit to this PR

For point number 3:

Yes. Before these changes, the meaning of @var inside the rescue_from was totally different, so let me add an explanation to the UPGRADING document 👍

Thank you again for the review!!

[dblock]

If you do @var = 1 in the first call, @var should not be available in a second call. This is because a new instance is created for each of the different endpoints.

Great. Let's write a spec like this.

Thanks again!

[jcagarcia]

Hey @dblock 👋

I've just updated the specs for including the cases we discussed above. Also, I've updated the UPGRADING and README files for explaining the behavior of the instance variables.

Let me know if anything else is needed 😄

[dblock]

Looks good. Can I ask for a quick example in UPGRADING and also say "This behavior was undefined until 2.1.0." so that it doesn't sound like a breaking change.

[dblock]

Merged, thank you.

[numbata]

Just FYI. Because the context for calling a rescue_from block has changed, the error! method that is called from inside the block has also changed along with the signature. The old one was in Grape::Middleware::Error:

def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil)

and now the error! is defined by the Grape::DSL::InsideRoute:

def error!(message, status = nil, additional_headers = nil)

So if for some reason the error! was called with the additional backtrace and/or original_exception arguments, it will now break. Potentially.

[dblock]

@numbata So should these method be aligned? Either way, it would be helpful to write a test that demonstrates this, and/or add it to UGPRADING.md.

[numbata]

@dblock let's align them! Will do Done here: #2468