ruby-grape / grape

An opinionated framework for creating REST-like APIs in Ruby.
http://www.ruby-grape.org
MIT License
9.88k stars 1.22k forks source link

variable scope in Grape versioned-controller #1627

Open ghost opened 7 years ago

ghost commented 7 years ago

Hello, I have been using Grape to build an API within an already-existing Rails project. I have been dealing with an error that I do not understand and I wonder if somebody could draw some light on this as it could be a bug but I am not sure.

My colleague and I were tasked with creating 4 end-points for this API:

This is what my colleague wrote (which works perfectly!):

module API
  class V1::Package < Grape::API
    get '/packages/:id' do
      @package = ::Package.find_by(id: params[:id])
      render rabl: 'v1/packages/show'
    end

    get '/packages' do
      @packages = ::Package.all
      render rabl: 'v1/packages/index'
    end
  end
end

And this is what I wrote (basically the exact same, but for options):

module API
  class V1::Option < Grape::API
    get '/options/:id' do
      @option = ::Option.find_by(id: params[:id])
      render rabl: 'v1/options/show'
    end

    get '/options' do
      @options = ::Option.all
      render rabl: 'v1/options/index'
    end
  end
end

The first end-point (/options/:id) works but the second one throws an error.

  API::V1::Option GET /api/v1/options/ returns all options
     Failure/Error: get '/api/v1/options'

     TypeError:
       no implicit conversion of Symbol into Integer

This is because basically @options is expected to be a Hash and not an Array (or ActiveRecord::Relation). Further debugging made me discover that using a different variable (something other than @options) fixed the issue!

But why is this?

In the first example (my colleague's code), @packages is set to nil before being set to whatever we set it to. But in my case @options was assigned to:

{
                   :method => [
        [0] "GET"
    ],
                     :path => [
        [0] "/options"
    ],
                      :for => API::V1::Option < Grape::API,
            :route_options => {
        :params => {}
    },
    :options_route_enabled => true
}

and (re-)assigning it to an array, a collection or an ActiveRecord::Relation broke the code. For now, I'll just re-name my variable to something else but I'd love to hear somebody else's opinion on this as I don't think this behaviour is expected.

dblock commented 7 years ago

What's the stack for that error? I think you're overwriting an @options that is set somewhere in Grape. The endpoints should be isolated and anything you do inside of them shouldn't affect anything else obviously, so lets call it a bug. It would be helpful if you could write a test for it.

To fix your issue, don't set @options, but a local var options. The only reason to set instance variables like @options is to carry these across functions, which is impossible with a singleton approach like a Grape API anyway.

ghost commented 7 years ago

@dblock thank you! We'll remove all instance variables from the Grape controllers then! I am at work now, but I'd be happy to write a test for it later this weekend. Yes.. looks like a 'leaked' variable.

ghost commented 7 years ago

@dblock mmm... it appears that I need to keep the @ as my .rabl views are not able to access any local variable from some other method (as I should have expected)

dblock commented 7 years ago

You should be passing the data into the rabl formatter in some other way.