New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Grape::Endpoint.setup for pre-run endpoint config. #397

Merged
merged 1 commit into from Apr 12, 2014

Conversation

Projects
None yet
5 participants
@mbleigh
Contributor

mbleigh commented Apr 29, 2013

This pull request addresses #396 by providing a Grape::Endpoint.setup class method which can be used to manipulate an Endpoint just before it begins executing. The use of this in a test suite would go something like this:

Let's say I have a current_user helper that I want to stub. Let's say I don't even have it implemented yet, but I don't want it to error out while I'm building my library. Simple!

describe MyAPI do
  let(:app){ MyAPI }

  context 'with a logged in user' do
    before do
      Grape::Endpoint.setup do |endpoint|
        endpoint.stub(:current_user).and_return mock(:User, name: "Bob Example")
      end
    end

    after{ Grape::Endpoint.setup nil }
  end
end

I'm entering this as a pull request for a few reasons:

  1. It adds code that will be conditionally executed every time any endpoint is called. I don't take that super lightly.
  2. Right now there is only the ability to do a single setup step which precludes nesting or multiple layers of setup. I'm not sure if this is the right way to go, but I wanted to start with a straightforward implementation.

Thoughts?

@dblock

This comment has been minimized.

Member

dblock commented Apr 29, 2013

Well, it's a workaround. But don't we really want people to be able to write something like this:

MyAPI.helpers.stub(:current_user).and_return ...

The fact is, you can just re-define a helper in a spec without a stub.

MyAPI.helpers do
  def current_user
    # return a stub
  end
end

The question is, how do we turn this into the syntax above?

@mbleigh

This comment has been minimized.

Contributor

mbleigh commented Apr 29, 2013

The problem is that Grape is context-sensitive. Where you declare your helpers matters (helpers declared before one endpoint but after another are available on the latter but not the former). So we can't simply say MyAPI.helpers because that's not an accurate picture of what's happening. What we need is to override the helpers for a given endpoint which is difficult since endpoints aren't directly retrievable from the root API.

This approach allows you to say "I don't care about context, I want this to happen when the endpoint runs" which, in my opinion, is the effect we want to achieve but may not be the most elegant way to achieve that effect.

@unorthodoxgeek

This comment has been minimized.

unorthodoxgeek commented Apr 30, 2013

I suggest a different approach

in the Endpoint class put this:

def method_missing(method_name, *args, &block)
  if helper.respond_to?(method_name)
    helper.set_context(request, env, whatever)
    helper.send(method_name, *args, &block)
  else
    super
  end
end

and thus, have a helpers object which can be easily stubbed out, by accessing MyApiClass.helpers (or helpers_object, as helpers is taken)

it makes it slightly slower as it takes a little stroll down the module tree to get to method_missing, but the endpoint really only has 5 ancestors, so it's not that big an overhead to use it.

@dblock

This comment has been minimized.

Member

dblock commented Apr 30, 2013

That would be fine. Can you please try a PR?

@unorthodoxgeek

This comment has been minimized.

unorthodoxgeek commented Apr 30, 2013

sure. will try to get a PR ready later today.

dblock added a commit to dblock/grape that referenced this pull request May 26, 2013

@dblock

This comment has been minimized.

Member

dblock commented May 26, 2013

I think the early setup mechanism here might work better towards the same goal, but I couldn't get the syntax to work. Check out https://github.com/dblock/grape/blob/spec-helpers/spec/grape/api_spec.rb#L913 for the two specs I would like to be able to pass.

@dblock

This comment has been minimized.

Member

dblock commented Jan 10, 2014

@mbleigh I want to revive this. Check out https://github.com/dblock/grape/blob/spec-helpers/spec/grape/api_spec.rb#L913 - does the setup method cover stubbing in those scenarios?

@mbleigh

This comment has been minimized.

Contributor

mbleigh commented Feb 9, 2014

@dblock sorry for being a month late to the party, not sure I follow. Are you asking if my setup method can give you functionality something like you have in your fork? I believe so.

it "allows stubbing helpers" do
  puts "declare"
  subject.helpers do
    def foo
      'foo'
    end
  end

  subject.get "/" do
    foo
  end

  get "/"
  last_response.body.should eql "foo"

  Grape::Endpoint.setup{|ep| ep.stub!(:foo).and_return("bar")}

  get "/"
  last_response.body.should eql "bar"

  Grape::Endpoint.setup nil
end
@dblock

This comment has been minimized.

Member

dblock commented Feb 10, 2014

@mbleigh I meant I'd like to see the tests in https://github.com/dblock/grape/blob/spec-helpers/spec/grape/api_spec.rb#L913 merged onto this PR and for them to pass.

@mbleigh

This comment has been minimized.

Contributor

mbleigh commented Apr 11, 2014

@dblock looking at your tests, I don't think this can ever or even necessarily should ever work. It implies a kind of global scope that simply doesn't exist in Grape. Helpers are mixed into the endpoint, and endpoints are dynamically generated based on the entire settings stack context that exists when they are defined.

The way I see it, the only potential workaround for this that comes close to what you're describing would be to provide an easy means to reach in and grab an endpoint. Something like:

endpoint = subject.at(:get, "/some/path/here")
endpoint.stub!(:dup).and_return(:endpoint)
endpoint.stub!(:helper_method).and_return("something else")

Which, coincidentally, I've been thinking about as a potentially very useful thing anyway. Other than that, I think the only serviceable option is something like this setup method.

@dblock

This comment has been minimized.

Member

dblock commented Apr 12, 2014

I am OK with merging this. It needs a section in README and an entry in CHANGELOG.

After reading this I wondered whether configure wouldn't be better than setup, btw. Don't feel strongly about it, but might be more consistent with other libraries.

@mbleigh

This comment has been minimized.

Contributor

mbleigh commented Apr 12, 2014

Do you have an example of a library that uses .configure in a similar way? I'm not opposed to changing it, but to me Grape::Endpoint.configure sounds like you're setting global configuration options (i.e. yielding a config object). Honestly I'd like a more descriptive name but nothing is coming to me. Maybe prerun or before_each?

@mbleigh

This comment has been minimized.

Contributor

mbleigh commented Apr 12, 2014

Actually, before_each seems like a better choice to me. It's descriptive and implies that it's going to happen to every instance, not some kind of class-level config. I'm changing it to that.

@dblock

This comment has been minimized.

Member

dblock commented Apr 12, 2014

You're right.

@dblock

This comment has been minimized.

Member

dblock commented Apr 12, 2014

You should squash this after all the things.

@mbleigh

This comment has been minimized.

Contributor

mbleigh commented Apr 12, 2014

OK, I think I did the squashing correctly. Let me know if there's anything off here.

@dblock

This comment has been minimized.

Member

dblock commented Apr 12, 2014

Perfect, will merge after green build.

dblock added a commit that referenced this pull request Apr 12, 2014

Merge pull request #397 from intridea/endpoint-setup
Adds Grape::Endpoint.setup for pre-run endpoint config.

@dblock dblock merged commit c706c8a into master Apr 12, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@brettfishman

This comment has been minimized.

brettfishman commented Jul 11, 2014

Just for clarity (and because the first example given above was slightly changed before this PR was merged in), here is the way to use this functionality in your specs:

describe MyAPI do

  context 'with a logged in user' do
    before do
      Grape::Endpoint.before_each do |endpoint|
        endpoint.stub(:current_user).and_return mock(:User, name: "Bob Example")
      end
    end

    after { Grape::Endpoint.before_each nil }

    it "works" do
      get "/my/protected/endpoint"
      expect(response.status).to eq 200
    end
  end
end
@fbandrey

This comment has been minimized.

fbandrey commented Nov 23, 2016

For latest versions of RSpec this is works:

...
  Grape::Endpoint.before_each do |endpoint|
    allow(endpoint).to receive(:current_user).and_return double(:user, id: 123, ...)
  end
...

yukihirop added a commit to yukihirop/onepage that referenced this pull request Feb 25, 2018

API(posts)の修正をした。
current_userをsessionで管理するようにしたのでそれに合わせた修正をした。

Grapeのhelpersブロックに定義したメソッドのモックの仕方は以下を参考にした。
ruby-grape/grape#397

Grapeでrailsで使っているsessionを使うため方法は以下を参考にした。
https://stackoverflow.com/questions/35344117/why-does-session-doesnt-work-in-grape-rails
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment