Skip to content
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

Nested groups/mounts not working as expected #369

Closed
Neil-Aframe opened this issue Mar 14, 2013 · 15 comments · Fixed by #416
Closed

Nested groups/mounts not working as expected #369

Neil-Aframe opened this issue Mar 14, 2013 · 15 comments · Fixed by #416

Comments

@Neil-Aframe
Copy link

My API has some resources addressable in multiple ways. I was hoping to factor this DRY-style by having two groups with before blocks that resolve params to the internal object, and then mount the same resource management code for viewing/updating etc.

I am blocked on that approach - it seems to be that the most obvious nested mounting approach does not work. The paths in the inner mount are apparently not being registered with rack, and I get a 404 from the rack server (the Grape service is not even getting called). I cannot find any examples or documentation in Grape that would point me at an alternative approach or missing syntax.

Example code that demonstrates my problem, stripped down to a minimal version here: https://gist.github.com/Neil-Aframe/5161270

@dblock
Copy link
Member

dblock commented Mar 14, 2013

Looks like a bug to me. The routes generated from your example are:

version=, method=GET, path=/by_id/by_name(.:format)
version=, method=GET, path=/by_id/by_name(.:format)
version=v2, method=GET, path=/api/:version/things(.:format)

But if we make 2 things, Thing1 and Thing2 and mount them separately, we get the following, which looks correct to me.

version=, method=GET, path=/by_name(.:format)
version=, method=GET, path=/by_id(.:format)
version=v2, method=GET, path=/api/:version/things(.:format)

If you have time, write a spec for this inside Grape and I can (help you) fix it.

@Neil-Aframe
Copy link
Author

Actually from my example, I would expect the following routes to be defined by Grape if nested mounting works the way I think it should:

version=v2, method=GET, path=/api/:version/things/by_id(.:format)
version=v2, method=GET, path=/api/:version/things/by_name(.:format)
version=v2, method=GET, path=/api/:version/things(.:format)

Please correct me if I am missing something?

I will make time to commit a spec over the next few days.

Many thanks,

Neil

@dblock
Copy link
Member

dblock commented Mar 15, 2013

Yes, you're correct. That route description could also be wrong while the actual response path be correct, but yes.

@Neil-Aframe
Copy link
Author

There is a spec with some clues already in the file spec/grape/api_spec.rb file

~line 1644, starting

it 'mounts on a nested path' do

In comments this test notes a required order for mounts to work. The main issue seems to be in order to support this sequence of outside-in loading, I would need application code that added the calls to mount after the route-defining classes were themselves were mounted, which is possible, but somewhat spoils the nice declarative style of syntax. So it would be DRY-er, but also uglier.

I added a similar spec for nesting group / mount / group / mount as per my example. This passes with Grape 0.4.0 as-is

Would you like me to add a spec asserting that reverse order of mounting should work? I suspect this is not trivial to address, but I personally would appreciate not having to care as much about order that files are loaded when building my API.

Neil

On 15 Mar 2013, at 14:29, Daniel Doubrovkine (dB.) notifications@github.com wrote:

Yes, you're correct. That route description could also be wrong while the actual response path be correct, but yes.


Reply to this email directly or view it on GitHub.

@dblock
Copy link
Member

dblock commented Mar 19, 2013

I think any spec that clearly demonstrates any problem is one step forward. I am not saying it will be easy to fix, but there're enough people around here who sometimes like a challenge :)

@ppadron
Copy link
Contributor

ppadron commented Mar 28, 2013

I just got bit by this issue, and I think this (failing) spec demonstrates the problem:

it 'mounts on a nested path from inside-out' do
  app2 = Class.new(Grape::API)
  app2.get do
    'This is App2'
  end

  app1 = Class.new(Grape::API) do
    get do
      'This is App1'
    end
    mount app2 => '/app2'
  end

  subject.mount app1 => '/app1'

  get '/app1'
  last_response.status.should == 200
  last_response.body.should == 'This is App1'

  get '/app1/app2'
  last_response.status.should == 200
  last_response.body.should == 'This is App2'
end

I'm still far from a solution, though.

I would guess that in the Grape::API.mount method we would need to iterate through the mounted app's routes and prefix them with the current namespace, but I'm still grasping the whole routing thing.

If someone could enlighten me about possible next steps I would be very happy to work on this issue.

@neilslater
Copy link

ppadron, that is pretty much the same code that I have, so I agree that is the test that would need to pass IMO - sorry I didn't push it up already!

When I look at def mount in lib/grape/api.rb it is fairly clear I don't understand enough of Grape and rack's internals to figure out the best solution though . . .

@ejholmes
Copy link

Trying to wrap my head around this one as well. I'll see if I can't fix it.

This also tests the issue:

it 'mounts on a nested path from inside-out' do
  subject.namespace :foo do
    app1 = Class.new(Grape::API)
    app1.get do
      'This is App1'
    end
    mount app1 => '/api'
  end

  get '/foo/api'
  last_response.status.should == 200
  last_response.body.should == 'This is App1'
end

@winston
Copy link

winston commented Dec 19, 2013

Got into the same issue as well today.

Is there a recommended way for keeping the resources DRY? Or is the only workaround to duplicate the resource and do something like #369 (comment), where we rename the resource?

Thank you!

@USvER
Copy link

USvER commented Feb 4, 2014

Have the same issue #570
Any solution?

@stevehill1981
Copy link

Is this still an issue?

@dblock
Copy link
Member

dblock commented Nov 26, 2014

I think this can be closed. Please reopen if someone thinks otherwise.

@dblock dblock closed this as completed Nov 26, 2014
@ekampp
Copy link
Contributor

ekampp commented Apr 27, 2017

I just ran into this problem today. Some of the instance_eval solutions solved it for me, but it seems like a hack, and something that grape should be able to handle without having to loop around the regular mounting.

Here is my code:

# https://github.com/ruby-grape/grape/issues/570#issuecomment-57456193
#
# All shared mount-points must inherit from this class, and must be mounted like
# so:
#
#   Class A < MountableApi
#   end
#
#   Class B < Grape::API
#     mount A.anonymous_class
#   end
#
#   Class C < Grape::API
#     mount A.anonymous_class
#   end
class MountableApi
  def self.anonymous_class
    Class.new(Grape::API).tap do |klass|
      klass.instance_eval(&@proc)
    end
  end

  def self.mounted(&block)
    @proc = block
  end
end

And then I use that like this on a resource that needs to be mounted in a nested fashion in multiple, other endpoints:

class List < MountableApi
  mounted do
    include Grape::Kaminari

    namespace :case_files do
      desc "List the things"
      paginate per_page: 10, max_per_page: 20
      get do
        paginate Collection.all
      end
    end
  end
end

And then in the different endpoints that needs a the List:

class Users < Grape::API
  mount List.anonymous_class
end

class Files < Grape::API
  mount List.anonymous_class
end

@dblock
Copy link
Member

dblock commented Apr 28, 2017

@ekampp Add your details?

@ekampp
Copy link
Contributor

ekampp commented Apr 29, 2017

@dblock updated my comment above to include my code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants