Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

myxoh
Copy link
Member

@myxoh myxoh commented Oct 17, 2018

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.

As such, this solution allows you to replace Grape::API for Grape::RemountableAPI on inheritance, and with that alone the API will now be mountable in several places.

This is a first pass, but I'd appreciate feedback and comments

@dblock
Copy link
Member

dblock commented Oct 18, 2018

This is amazing, well done 👏

Dumb question first, why don't we make RemountableAPI into Grape::API?

@myxoh
Copy link
Member Author

myxoh commented Oct 18, 2018

Hi. Thanks @dblock . It is a very fair question.
The main reason is that the implementation details are fundamentally different, and hence the two don't behave 100% the same in 100% the cases.
The other reason is that Grape::RemountableAPI uses Grape::API internally (but this could be solved by just renaming the prior class if it weren't for the problem above)
Currently, if you replace on the specs all instances of Class.new(Grape::API) and << Grape::API with RemountableAPI
./spec/grape/api/patch_method_helpers_spec.rb
and
./spec/grape/api/inherited_helpers_spec.rb
Fail to load. Even deleting these produces 37 failures (out of the 1807 specs)

Perhaps I should be clearer that, while RemountableAPI will handle most cases, it won't handle all of them.

Failed examples:

rspec ./spec/grape/api/shared_helpers_exactly_one_of_spec.rb:40 # Grape::API::Helpers defines parameters
rspec ./spec/grape/api/shared_helpers_spec.rb:31 # Grape::API::Helpers defines parameters
rspec ./spec/grape/api_spec.rb:3112 # Grape::API.mount mounting an API applies format to a mounted API with nested resources
rspec ./spec/grape/api_spec.rb:3127 # Grape::API.mount mounting an API applies auth to a mounted API with nested resources
rspec ./spec/grape/api_spec.rb:3224 # Grape::API.compile sets the instance
rspec ./spec/grape/dsl/helpers_spec.rb:85 # Grape::DSL::Helpers.helpers in child classes is available
rspec ./spec/grape/integration/global_namespace_function_spec.rb:24 # Grape::API with a global namespace function works
rspec ./spec/grape/integration/rack_sendfile_spec.rb:28 # Rack::Sendfile  contains Sendfile headers
rspec ./spec/grape/integration/rack_sendfile_spec.rb:39 # Rack::Sendfile  not contains Sendfile headers
rspec ./spec/grape/integration/rack_spec.rb:4 # Rack correctly populates params from a Tempfile
rspec ./spec/grape/loading_spec.rb:39 # Grape::API execute first request in reasonable time
rspec ./spec/grape/middleware/auth/dsl_spec.rb:17 # Grape::Middleware::Auth::DSL.auth stets auth parameters
rspec ./spec/grape/middleware/auth/dsl_spec.rb:24 # Grape::Middleware::Auth::DSL.auth can be called multiple times
rspec ./spec/grape/middleware/versioner/header_spec.rb:315 # Grape::Middleware::Versioner::Header when there are multiple versions with complex vendor specified with rescue_from :all with header versioned endpoints and a rescue_all block defined responds correctly to a v1 request
rspec ./spec/grape/middleware/versioner/header_spec.rb:321 # Grape::Middleware::Versioner::Header when there are multiple versions with complex vendor specified with rescue_from :all with header versioned endpoints and a rescue_all block defined responds correctly to a v2 request
rspec ./spec/grape/middleware/versioner/param_spec.rb:101 # Grape::Middleware::Versioner::Param when there are multiple versions without a custom param responds correctly to a v1 request
rspec ./spec/grape/middleware/versioner/param_spec.rb:107 # Grape::Middleware::Versioner::Param when there are multiple versions without a custom param responds correctly to a v2 request
rspec ./spec/grape/middleware/versioner/param_spec.rb:153 # Grape::Middleware::Versioner::Param when there are multiple versions with a custom param responds correctly to a v1 request
rspec ./spec/grape/middleware/versioner/param_spec.rb:159 # Grape::Middleware::Versioner::Param when there are multiple versions with a custom param responds correctly to a v2 request
rspec ./spec/grape/remountable_api_spec.rb:37 # Grape::RemountableAPI mounted RemountableAPI with a defined route when mounting twice can access the votes in both places
rspec ./spec/grape/remountable_api_spec.rb:57 # Grape::RemountableAPI mounted RemountableAPI with a defined route when mounting on namespace can access the votes in both places
rspec ./spec/grape/remountable_api_spec.rb:77 # Grape::RemountableAPI mounted RemountableAPI with a dynamically configured route will use the dynamic configuration on all routes
rspec ./spec/grape/validations/validators/presence_spec.rb:21 # Grape::Validations::PresenceValidator without validation does not validate for any params
rspec ./spec/grape/validations/validators/presence_spec.rb:39 # Grape::Validations::PresenceValidator with a custom validation message requires when missing
rspec ./spec/grape/validations/validators/presence_spec.rb:44 # Grape::Validations::PresenceValidator with a custom validation message requires when empty
rspec ./spec/grape/validations/validators/presence_spec.rb:48 # Grape::Validations::PresenceValidator with a custom validation message valid when set
rspec ./spec/grape/validations/validators/presence_spec.rb:65 # Grape::Validations::PresenceValidator with a required regexp parameter supplied in the POST body validates id
rspec ./spec/grape/validations/validators/presence_spec.rb:91 # Grape::Validations::PresenceValidator with a required non-empty string requires when missing
rspec ./spec/grape/validations/validators/presence_spec.rb:96 # Grape::Validations::PresenceValidator with a required non-empty string requires when empty
rspec ./spec/grape/validations/validators/presence_spec.rb:101 # Grape::Validations::PresenceValidator with a required non-empty string valid when set
rspec ./spec/grape/validations/validators/presence_spec.rb:125 # Grape::Validations::PresenceValidator with multiple parameters per requires validates for all defined params
rspec ./spec/grape/validations/validators/presence_spec.rb:145 # Grape::Validations::PresenceValidator with required parameters and no type validates name, company
rspec ./spec/grape/validations/validators/presence_spec.rb:172 # Grape::Validations::PresenceValidator with nested parameters validates nested parameters
rspec ./spec/grape/validations/validators/presence_spec.rb:204 # Grape::Validations::PresenceValidator with triply nested required parameters validates triple nested parameters
rspec ./spec/grape/validations/validators/presence_spec.rb:253 # Grape::Validations::PresenceValidator with reused parameter documentation once required and once optional works with required
rspec ./spec/grape/validations/validators/presence_spec.rb:262 # Grape::Validations::PresenceValidator with reused parameter documentation once required and once optional works with optional
rspec ./spec/integration/multi_xml/xml_spec.rb:4 # ActiveSupport::XmlMini uses multi_xml

@myxoh
Copy link
Member Author

myxoh commented Oct 19, 2018

I've carried on working on it and now have a version that passes all specs. I still feel because of the core internal details changing, it's a safer idea to create a new class for this version, and then potentially we could make it the default if no problem arise.
But I'll leave that decision up to you

@myxoh myxoh mentioned this pull request Oct 19, 2018
Merged
@dblock
Copy link
Member

dblock commented Oct 19, 2018

It worries me that the re-mountable version doesn't behave like the original Grape::API. Every one of these specs that fails when you make RemountableAPI into Grape::API represents a bug in RemountableAPI as far as I can see.

I really want this feature in Grape and I really think we can make it work. I would start moving parts of RemountableAPI into Grape::API without breaking tests until the delta is very clear.

I realize I am asking for a lot. What do you think?

@dblock
Copy link
Member

dblock commented Oct 19, 2018

I wrote the above before seeing #1803. You've done the work! Lets discuss that PR first there.

@myxoh myxoh closed this Oct 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@myxoh @dblock