Remountable APIs, allows to re-mounting all APIs #1803

myxoh · 2018-10-19T13:26:15Z

Addresses this feature request: #570

A lot of times (specially when using versioning) we want to re-mount an endpoint in a different namespace, perhaps with some tiny bit of different configuration each time.

On our project, for example, we have a massive API using Grape, and while we want to introduce versioning we don't want to:
1- Have to copy & paste the whole project (nor re-write the whole project at once),
2- Have to re-write our project to use one of the proposed meta-programming approaches.

Creates a new module that can replace a Grape::API in most cases which allows APIs to be re-mounted

Fixes all remaining issues

dblock

Yes! This looks great. Minor changes in text and what not please?

@namusyaka Can I have a second pair of eyes when you have a chance?

dblock · 2018-10-19T14:28:25Z

README.md

+You can mount the same endpoints in two different locations
+
+```ruby
+class Voting::API < Grape::GrapeAPI


Grape::API

dblock · 2018-10-19T14:28:58Z

README.md

+end
+
+class Post::API < Grape::API
+  mount Voting::API, with: {votable: 'posts'}


Rubocop, add spaces between { votable: 'posts' }.

dblock · 2018-10-19T14:29:53Z

spec/grape/remountable_api_spec.rb

+    root_api
+  end
+
+  describe 'mounted RemountableAPI' do


We no longer have RemountableAPI.

dblock · 2018-10-19T14:32:59Z

Other than our own fear, uncertainty and doubt, what are the downsides?

Specs pass. We trust the specs.

I will do a thorough code review. In the meantime do consider if any parts of the remountable implementation can be simplified by moving them into the api instance class itself.

dblock · 2018-10-19T14:36:40Z

lib/grape/api_instance.rb

+module Grape
+  # The API Instance class, is the engine behind Grape::API. Each class that inherits
+  # from this will represent a different API
+  class APIInstance


Would this be better called Grape::API::Instance? Or Grape::SomethingSomething::API (to match how we have Grape::DSL::API)? Just thinking out loud, no strong feelings.

The naming could be important because if someone is having trouble with the new Grape::API they could just inherit from Grape::API::Instance, making the diff prettier ;)

I'm happy with that naming. Regarding the solution if someone's having trouble: Should I add an entry that says something among those lines in updating.md ?

dblock · 2018-10-19T14:37:20Z

lib/grape/api_instance.rb

+require 'grape/router'
+
+module Grape
+  # The API Instance class, is the engine behind Grape::API. Each class that inherits


.. represent a different API instance?

dblock · 2018-10-19T14:38:50Z

lib/grape/dsl/routing.rb

@@ -77,9 +77,12 @@ def do_not_route_options!
          namespace_inheritable(:do_not_route_options, true)
        end

-        def mount(mounts)
+        def mount(mounts, with: {})


Do we use named parameters elsewhere. I know we no longer support older Rubies, but if we don't maybe make this consistent as opts = {} and opts[:with] || {}?

dblock · 2018-10-19T14:41:21Z

lib/grape/dsl/routing.rb

          mounts = { mounts => '/' } unless mounts.respond_to?(:each_pair)
          mounts.each_pair do |app, path|
+            if app.respond_to?(:mount_instance)
+              return mount(app.mount_instance(configuration: with) => path)


This return seems odd, can you explain? What if we have more mounts? I think it should be a if ... else for whatever follows?

So the point of this return is not to execute the rest of the code as part of a Grape::API, only as part of a Grape::API::Instance. However, I do agree that return is the wrong approach, I just need to call next.
I'm surprised no test complained. Perhaps we don't have test coverage for mounting multiple apps, path on a single call?
Can you help me write a failing spec to make sure I better understand what the use case of this each_pair?

Do we support something like:

mount(API1 => '/api_1', API2 => '/api_2'})

?

We're definitely missing tests.

I didn't know that we support this kind of mounting, but it looks like! Thanks for looking into it.

dblock · 2018-10-19T14:42:18Z

spec/grape/remountable_api_spec.rb

@@ -0,0 +1,85 @@
+require 'spec_helper'


More naming, this is just about remounting, so maybe api_remount_spec.rb? NBD

myxoh · 2018-10-19T14:57:02Z

No good reason, it's mainly because this is my first contribution to Grape, and due to the potential impact I wanted to avoid modifying the core behaviour so drastically. However, if you are happy with giving it a large look, I'm happy with implementing and supporting this changes.
I'll address the naming in a little moment. Should we close the other PR?

myxoh · 2018-10-19T16:09:22Z

lib/grape/dsl/routing.rb

          mounts = { mounts => '/' } unless mounts.respond_to?(:each_pair)
          mounts.each_pair do |app, path|
+            if app.respond_to?(:mount_instance)
+              return mount(app.mount_instance(configuration: opts[:with] || {}) => path)


The return issue is still not addressed, I still need a failing spec to see whether next is appropiate, or whether this is better handled by a refactor

myxoh · 2018-10-19T16:12:58Z

Also waiting on what you think we should do on the updating (as technically no change is required, but might be helpful to have information explaining how to fallback)

dblock · 2018-10-19T20:58:18Z

No good reason, it's mainly because this is my first contribution to Grape, and due to the potential impact I wanted to avoid modifying the core behaviour so drastically. However, if you are happy with giving it a large look, I'm happy with implementing and supporting this changes.

What could go wrong :)

I'll address the naming in a little moment. Should we close the other PR?

Yes, go for it.

dblock · 2018-10-19T20:58:51Z

Also waiting on what you think we should do on the updating (as technically no change is required, but might be helpful to have information explaining how to fallback)

You should write a paragraph in UPGRADING.

Fixes routing to get specs working

myxoh · 2018-10-20T16:48:17Z

Also waiting on what you think we should do on the updating (as technically no change is required, but might be helpful to have information explaining how to fallback)

You should write a paragraph in UPGRADING.

Done. The only point is that I've added the upgrading to upgrading to >= 1.1.1, not sure whether this PR should go in a point release. Of course that's completely your decision 👍 .

Let me know anything else you need to get this over the line 😄

myxoh · 2018-10-22T12:32:14Z

By trying it out on a big project, I've faced a bunch of breaking issues with no easy solution

Grape swagger seemed completely broken
Referencing a constant without the namespace is broken.

I'm back to the drawing board, and adding specs to represent these issues later today

myxoh · 2018-10-22T18:03:33Z

So. After a lot of investigation, issues were rooted in 2 main causes.
1- Autoloading in rails is ffed up. I would probably put a warning in the upgrading.md, but there's no easy solution. There was not a problem with the namespaces as I feared.
2- Grape-Swagger is opening and closing Grape::API but it should be opening and closing Grape::API::Instance (And also modifying the reference in GrapeSwagger#create_documentation_class to Grape::API::Instance)

After fixing these 2 things, I've got my project back working.

I'm waiting on your thoughts and what's next then

dblock

I think you should open an issue in Grape::Swagger related to this, we'll want a proper fix there before we release Grape 1.2 with this change since that gem is very popular. Maybe you can attempt it too? ;)

dblock · 2018-10-22T19:21:56Z

README.md

@@ -366,6 +368,59 @@ class Twitter::API < Grape::API
 end
 ```

+## Remounting
+
+You can mount the same endpoints in two different locations


Nitpick, we have periods at the end of sentences ;)

dblock · 2018-10-22T19:22:05Z

README.md

+end
+```
+
+Assuming that the post and comment endpoints are mounted in `/posts` and `/comments`,


Same as above.

dblock · 2018-10-22T19:22:33Z

UPGRADING.md

@@ -3,6 +3,15 @@ Upgrading Grape

 ### Upgrading to >= 1.1.1


Probably 1.2, lets increment this everywhere as part of this PR?

dblock · 2018-10-22T19:26:40Z

To summarize where we are:

Amazing work by @myxoh, thanks for hanging in here till we get this in.
Lets increment the version to 1.2 as part of this PR.
Lets open an issue in grape-swagger to make it compatible with Grape 1.2 automagically.
Waiting for another CR from another maintainer for this before merging since it's a big deal.

myxoh · 2018-10-23T14:53:10Z

Bumped up the version, and opened this PR on grape-swagger: ruby-grape/grape-swagger#717 that had green specs locally (when changing the gemfile to point to the right branch)

myxoh · 2018-10-25T11:26:13Z

I've managed to address the auto-loading issues, currently on the project I'm working on upgrading to this branch + upgrading swagger was done seamlessly with no failing specs, nor code-changes required.

I've updated the grape-swagger PR addressing the requested changes. I'm still waiting for the second reviewer to take a look.

Let me know if there's anything else I can do

dblock · 2018-10-25T15:54:41Z

LGTM, I'll wait on someone else to comment (@namusyaka if you have a moment?) otherwise will merge in a few.

namusyaka · 2018-10-25T15:59:47Z

Sorry, I overlooked this PR.
This feature looks really promising, thanks so much @myxoh !

I'll review the code & design this weekend.

namusyaka

Looks good, nice work!

dblock · 2018-10-27T13:24:17Z

I merged this.

@myxoh would you like to join the co-maintainers group and get this to release? If yes what's your rubygems email? (feel free to send it over to dblock[at]dblock[dot]org if you prefer)

myxoh · 2018-10-29T11:01:27Z

Hi @dblock I've wrote you an e-mail from myxoh10[at]gmail[dot]com

myxoh added 7 commits October 17, 2018 19:57

Remountable APIs, allows to re-mount APIs that inherit from this

ae879cb

Creates a new module that can replace a Grape::API in most cases which allows APIs to be re-mounted

Adds Readme.md to remountable APIs

091a331

Adds Changelog.md

ccec543

Adds remounting section to the table of contents

641959c

Makes Remountable API a valid drop-in replacement to Grape::API

37cce04

Replaces APIs with RemountableAPI

cf14fc7

Fixes all remaining issues

Updates documentation

5fcc07b

myxoh force-pushed the replacing-api branch from 2ad22d4 to 5fcc07b Compare October 19, 2018 13:47

dblock approved these changes Oct 19, 2018

View reviewed changes

dblock mentioned this pull request Oct 19, 2018

Remountable APIs, allows to re-mount APIs that inherit from this class #1802

Closed

dblock reviewed Oct 19, 2018

View reviewed changes

myxoh added 3 commits October 19, 2018 16:27

Changes APIInstance to API::Instance

691eca8

Fixes README an specs descriptions

ffe147b

Replaces keyword args with opts

1c6a9dc

myxoh commented Oct 19, 2018

View reviewed changes

Refactors and comments

1421517

myxoh changed the title ~~Remountable APIs, allows to re-mounting all APIs - Alternative approach~~ Remountable APIs, allows to re-mounting all APIs Oct 19, 2018

myxoh added 2 commits October 20, 2018 17:43

Adds test for mounting multiple apps as one

330a6b9

Fixes routing to get specs working

Adds paragraph to upgrading explaining how to rollback behaviour

24295fd

dblock approved these changes Oct 22, 2018

View reviewed changes

Bumps up version and upgrades description

d4eee5b

myxoh mentioned this pull request Oct 23, 2018

Adds support for grape >= 1.2 ruby-grape/grape-swagger#717

Merged

Adds missing hash to UPGRADING.md

f8c3fee

myxoh added 2 commits October 23, 2018 16:25

Address problems with autoloading by searching for constants in instance

9416023

Better handles constants

ce4966d

namusyaka self-requested a review October 25, 2018 15:59

namusyaka approved these changes Oct 27, 2018

View reviewed changes

dblock merged commit ca80feb into ruby-grape:master Oct 27, 2018

dblock mentioned this pull request Nov 9, 2018

Autoloading issues after the introduction of Remountable API? #1815

Closed

fotos mentioned this pull request Dec 7, 2018

Add compatibility with Grape >= 1.2.0 davidcelis/api-pagination#117

Merged

dblock mentioned this pull request Jan 17, 2019

Running the server and specs fails for ActiveRecord based sample app slack-ruby/slack-ruby-bot-server#88

Closed

ec mentioned this pull request Feb 21, 2019

How to get an instance inside Grape::API class #1866

Closed

dblock mentioned this pull request Apr 19, 2019

Allow multiple mounts of the same API class #570

Closed

dblock mentioned this pull request Dec 22, 2019

Add failing spec for #570 #686

Closed

jsteinberg mentioned this pull request Jun 11, 2020

Memory Leak since 1.2.0 #2071

Open

dblock mentioned this pull request Sep 14, 2020

Fix retaining setup blocks when remounting APIs #2102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remountable APIs, allows to re-mounting all APIs #1803

Remountable APIs, allows to re-mounting all APIs #1803

myxoh commented Oct 19, 2018 •

edited

Loading

dblock left a comment

dblock Oct 19, 2018

dblock Oct 19, 2018

dblock Oct 19, 2018

dblock commented Oct 19, 2018

dblock Oct 19, 2018

myxoh Oct 19, 2018

dblock Oct 19, 2018

dblock Oct 19, 2018

dblock Oct 19, 2018

myxoh Oct 19, 2018

dblock Oct 19, 2018

dblock Oct 19, 2018

myxoh commented Oct 19, 2018

myxoh Oct 19, 2018

myxoh commented Oct 19, 2018

dblock commented Oct 19, 2018

dblock commented Oct 19, 2018

myxoh commented Oct 20, 2018

myxoh commented Oct 22, 2018

myxoh commented Oct 22, 2018

dblock left a comment

dblock Oct 22, 2018

dblock Oct 22, 2018

dblock Oct 22, 2018

dblock commented Oct 22, 2018

myxoh commented Oct 23, 2018

myxoh commented Oct 25, 2018

dblock commented Oct 25, 2018

namusyaka commented Oct 25, 2018

namusyaka left a comment

dblock commented Oct 27, 2018

myxoh commented Oct 29, 2018

Remountable APIs, allows to re-mounting all APIs #1803

Remountable APIs, allows to re-mounting all APIs #1803

Conversation

myxoh commented Oct 19, 2018 • edited Loading

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myxoh commented Oct 19, 2018

Choose a reason for hiding this comment

myxoh commented Oct 19, 2018

dblock commented Oct 19, 2018

dblock commented Oct 19, 2018

myxoh commented Oct 20, 2018

myxoh commented Oct 22, 2018

myxoh commented Oct 22, 2018

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Oct 22, 2018

myxoh commented Oct 23, 2018

myxoh commented Oct 25, 2018

dblock commented Oct 25, 2018

namusyaka commented Oct 25, 2018

namusyaka left a comment

Choose a reason for hiding this comment

dblock commented Oct 27, 2018

myxoh commented Oct 29, 2018

myxoh commented Oct 19, 2018 •

edited

Loading