bploetz / versionist

A plugin for versioning Rails based RESTful APIs.
MIT License
971 stars 51 forks source link

api_version with both header and parameter versioning strategies #88

Closed jessicb closed 5 years ago

jessicb commented 5 years ago

My routes file looks like this, with identical controllers under each api_version

scope path: :api, as :api, module: :api do
  api_version module: "v1", 
    header: { name: 'version', value: '1' }, 
    parameter: { name: 'v', value: '1' } do
      scope controller: :some do
        get 'index'
      end
      # additional controllers...
    end
  end

  api_version module: "v0", 
    header: { name: 'version', value: '0' }, 
    parameter: { name: 'v', value: '0' } do
      scope controller: :some do
        get 'index'
      end
      # additional controllers with same names as in v1...
    end
  end
end

My api controllers have this structure in the app:

app/controllers/api/v0/some_controller.rb app/controllers/api/v1/some_controller.rb

With this setup, an api consumer hitting any endpoint and passing ?v=0 to try to hit the v0 controller is instead routed to the v1 controller with the same name. The resolution to be able to hit v0 endpoints was to remove the header option from v1, leaving:

api_version module "v1", 
    parameter: { name: 'v', value: '1' } do
    # some routez

I would like to be able to have a v1 api consumer use a header as well, is there an alternate syntax that will allow this?

bploetz commented 5 years ago

@jessicb can you show me what those two controllers look like (not the whole file, but just the module and class declaration)?

jessicb commented 5 years ago

Sure! They are identical: class Api::V0::BaseController < ActionController::Base class Api::V1::BaseController < ActionController::Base

bploetz commented 5 years ago

@jessicb and what versions of Rails and versionist are you using?

jessicb commented 5 years ago

I'm on rails 5.1.6 and versionist 1.5.0.

bploetz commented 5 years ago

@jessicb let me replicate this setup locally and see what's going on. Stay tuned......

bploetz commented 5 years ago

@jessicb Hmmm, are you sure you copied those routes correctly? I copied the above routes verbatim, and I'm getting a syntax error:

> ruby -v
ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-darwin15]

> rails -v
Rails 5.1.6

> cat config/routes.rb 
Rails.application.routes.draw do
  # issue #88

  scope path: :api, as :api, module: :api do
    api_version module: "v1", 
      header: { name: 'version', value: '1' }, 
      parameter: { name: 'v', value: '1' } do
        scope controller: :some do
          get 'index'
        end
        # additional controllers...
      end
    end

    api_version module: "v0", 
      header: { name: 'version', value: '0' }, 
      parameter: { name: 'v', value: '0' } do
        scope controller: :some do
          get 'index'
        end
        # additional controllers with same names as in v1...
      end
    end
  end
end
bundle exec rake routes
rake aborted!
SyntaxError: /Users/bploetz/workspace/versionist-test-rails51/config/routes.rb:4: syntax error, unexpected tSYMBEG, expecting keyword_do or '{' or '('
  scope path: :api, as :api, module: :api do
                        ^
/Users/bploetz/workspace/versionist-test-rails51/config/routes.rb:23: syntax error, unexpected keyword_end, expecting end-of-input
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/dependencies.rb:286:in `load'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/dependencies.rb:286:in `block in load'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/dependencies.rb:258:in `load_dependency'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/dependencies.rb:286:in `load'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:55:in `block in load_paths'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:55:in `each'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:55:in `load_paths'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:18:in `reload!'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:41:in `block in updater'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/file_update_checker.rb:81:in `call'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/file_update_checker.rb:81:in `execute'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:42:in `updater'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:31:in `execute_if_updated'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/finisher.rb:128:in `block in <module:Finisher>'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/initializable.rb:30:in `instance_exec'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/initializable.rb:30:in `run'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/initializable.rb:59:in `block in run_initializers'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/initializable.rb:58:in `run_initializers'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application.rb:353:in `initialize!'
/Users/bploetz/workspace/versionist-test-rails51/config/environment.rb:5:in `<top (required)>'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application.rb:329:in `require'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application.rb:329:in `require_environment!'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application.rb:445:in `block in run_tasks_blocks'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/rake-12.3.2/exe/rake:27:in `<top (required)>'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/bin/ruby_executable_hooks:15:in `eval'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/bin/ruby_executable_hooks:15:in `<main>'
Tasks: TOP => routes => environment
(See full trace by running task with --trace)

Let me know......

jessicb commented 5 years ago

Sorry, that should be as: :api

bploetz commented 5 years ago

@jessicb made that change, still no bueno:

> cat config/routes.rb 
Rails.application.routes.draw do
  # issue #88

  scope path: :api, as: :api, module: :api do
    api_version module: "v1", 
      header: { name: 'version', value: '1' }, 
      parameter: { name: 'v', value: '1' } do
        scope controller: :some do
          get 'index'
        end
        # additional controllers...
      end
    end

    api_version module: "v0", 
      header: { name: 'version', value: '0' }, 
      parameter: { name: 'v', value: '0' } do
        scope controller: :some do
          get 'index'
        end
        # additional controllers with same names as in v1...
      end
    end
  end
end
> bundle exec rake routes
rake aborted!
SyntaxError: /Users/bploetz/workspace/versionist-test-rails51/config/routes.rb:24: syntax error, unexpected keyword_end, expecting end-of-input
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/dependencies.rb:286:in `load'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/dependencies.rb:286:in `block in load'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/dependencies.rb:258:in `load_dependency'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/dependencies.rb:286:in `load'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:55:in `block in load_paths'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:55:in `each'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:55:in `load_paths'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:18:in `reload!'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:41:in `block in updater'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/file_update_checker.rb:81:in `call'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/activesupport-5.1.6/lib/active_support/file_update_checker.rb:81:in `execute'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:42:in `updater'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/routes_reloader.rb:31:in `execute_if_updated'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application/finisher.rb:128:in `block in <module:Finisher>'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/initializable.rb:30:in `instance_exec'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/initializable.rb:30:in `run'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/initializable.rb:59:in `block in run_initializers'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/initializable.rb:58:in `run_initializers'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application.rb:353:in `initialize!'
/Users/bploetz/workspace/versionist-test-rails51/config/environment.rb:5:in `<top (required)>'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application.rb:329:in `require'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application.rb:329:in `require_environment!'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/railties-5.1.6/lib/rails/application.rb:445:in `block in run_tasks_blocks'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/gems/rake-12.3.2/exe/rake:27:in `<top (required)>'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/bin/ruby_executable_hooks:15:in `eval'
/Users/bploetz/.rvm/gems/ruby-2.2.5@versionist-test-rails51/bin/ruby_executable_hooks:15:in `<main>'
Tasks: TOP => routes => environment
(See full trace by running task with --trace)
jessicb commented 5 years ago

Apologies for the extra end there, this should work:

Rails.application.routes.draw do
  scope path: :api, as: :api, module: :api do
    api_version module: "v1",
      header: { name: 'version', value: '1' },
      parameter: { name: 'v', value: '1' } do
        scope controller: :base do
          get 'index'
        end
      end

    api_version module: "v0",
      header: { name: 'version', value: '0' },
      parameter: { name: 'v', value: '0' } do
      scope controller: :base do
        get 'index'
      end
    end
  end
end
bploetz commented 5 years ago

@jessicb better! And just to confirm, you said above that

app/controllers/api/v0/some_controller.rb

contains

class Api::V0::BaseController < ActionController::Base

But that can't be right (rails complains about this). Can you show me what your base controller looks like for v0, and what some_controller.rb looks like as well?

jessicb commented 5 years ago

Sorry! I'm providing skeletal examples here, because the actual files are large...

These controllers should contain the class definition provided above. app/controllers/api/v0/base_controller.rb app/controllers/api/v1/base_controller.rb

Then the some_controller files should contain:

class Api::V0::SomeController < Api::V0::BaseController
  def index
  end
end
class Api::V1::SomeController < Api::V1::BaseController
  def index
  end
end
bploetz commented 5 years ago

@jessicb no worries, it's just important that I'm comparing apples to apples when trying to reproduce this issue. :) Stay tuned......

bploetz commented 5 years ago

@jessicb this bit:

scope controller: :base do
  get 'index'
end

What exactly is that doing, or what are you trying to have that do? I've never seen scope: :controller before. And what's weird is if I don't have an index action in class Api::V1::BaseController rails complains.

Started GET "/api/index" for 127.0.0.1 at 2019-05-03 16:23:14 -0400

AbstractController::ActionNotFound (The action 'index' could not be found for Api::V1::BaseController):
jessicb commented 5 years ago

Try this instead of scope controller: :base do. scope controller: :some, as: :some, path: :some do

And then the endpoints would be /api/some/index?v=0 vs /api/some/index?v=1.

bploetz commented 5 years ago

@jessicb Ok, I did something similar (not exactly) and I can reproduce the bug. Stay tuned........

bploetz commented 5 years ago

@jessicb I'm still trying to find the bug that's causing this, but in the mean time I've found that, at least for me locally, if I change the order of the api_version blocks in the routes.rb file so that 0 comes first and then 1, everything works as expected. So you can give that a try and see if it works for you too.

Still working on it though.....

bploetz commented 5 years ago

@jessicb ok, figured it out. So it turns out that it's not the order of api_version blocks in routes.rb that was the culprit, it's actually that it was picking up your header v1 strategy all the time. Why?

The way the header strategy works is that it looks for a header on the request with the name you've configured in your header strategy, grabs that header's value, and sees if the configured header value matches the header's value on the request. If not, it moves on to the other configured versioning strategies looking for a match.

You unfortunately chose "version" as your header name. Rack comes with a bunch of built in headers, one of which is "HTTPVERSION", which represents the version of the HTTP protocol the client is using (note: the `HTTP` prefix is added by rack for all incoming request headers). For example:

["HTTP_VERSION", "HTTP/1.1"]

That HTTP_ prefix is removed by Rails and the uppercased/underscored string is returned to it's normal format before being stored as the key for the header in the ActionDispatch::Http::Headers object (see the comment at the top of https://api.rubyonrails.org/classes/ActionDispatch/Http/Headers.html for an example). So the "HTTP_VERSION" header just becomes "version", which is the versioning header name you configured in api_version. And since HTTP headers can contain multiple values and quality qualifiers, versionist doesn't look for an exact match of the header value, just that your configured header value is within the header value string. See this issue for the gory details as to why. So in this case, your configured header version of "1" is found within "HTTP/1.1." and thus the false positive match.

Ugh.

I've pushed up a change which will raise an exception if you choose "version" for your header name, as this clashes with the builtin HTTP_VERSION header as noted above. So you will need to choose a different header name (API-VERSION, X-VERSION, etc) to use for your header versioning strategy configuration. Once I changed the header name in my example app, everything worked as expected.

If you could update your Gemfile to point at this change

gem 'versionist', :git => 'https://github.com/bploetz/versionist.git', :branch => 'fix/issue_88'

And then run bundle update versionist, you should see it complain about your routes configuration on startup. Change your header name to something other than "version" and you should be good to go.

I will merge this to master and release a new version of versionist with this change when you confirm this works for you.

Any questions let me know.

jessicb commented 5 years ago

@bploetz Thanks for digging into this, it's an interesting bug. I'm surprised that no one else has run into this using version as a header name...

I will test and report back, thanks again.

bploetz commented 5 years ago

@jessicb yeah I had the same reaction: how has no one run into this already?! :)

jessicb commented 5 years ago

@bploetz This is looking good after further testing!

bploetz commented 5 years ago

@jessicb great, thanks for testing. Will release a new version with this change.

bploetz commented 5 years ago

versionist 2.0.0 has been released with this change.

https://rubygems.org/gems/versionist