bploetz / versionist

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

:default => true errors out with rails 4.0.2 #59

Open millisami opened 10 years ago

millisami commented 10 years ago

Its working with the rails 4.0.2 app except :default => true options.

In the README, there is an expected behaviour Policy that says the routes get output twice when running rake. In 4.0.2 rails, it errors out with:

/Users/millisami/asset-management/.bundle/gems/actionpack-4.0.2/lib/action_dispatch/routing/route_set.rb:434:in `add_route': Invalid route name, already in use: 'api_gps_positions'  (ArgumentError)
You may have defined two routes with the same name using the `:as` option, or you may be overriding a route already defined by a resource with the same naming. For the latter, you can restrict the routes created with `resources` as explained here:
http://guides.rubyonrails.org/routing.html#restricting-the-routes-created
  from /Users/millisami/asset-management/.bundle/gems/actionpack-4.0.2/lib/action_dispatch/routing/mapper.rb:1445:in `add_route'

Is there any patch to make the default option true?

bploetz commented 10 years ago

This sounds like a duplicate of https://github.com/bploetz/versionist/issues/46, which I have been unable to reproduce locally. Can you send me the following info so I can try to reproduce?

Thanks.

millisami commented 10 years ago

@bploetz Here is the info: ruby 2.0.0p247 (2013-06-27 revision 41674) [x86_64-darwin13.0.0]

And routes and Gemfile at https://gist.github.com/millisami/8463fc0c10cc6edde276

bploetz commented 10 years ago

Thanks for this @millisami. I've created a test Rails app which mimics this structure and am still not able to reproduce the error. You can find the app here: https://github.com/bploetz/versionist-issue59

Running rake routes yields no errors:

>  bundle exec rake routes
               Prefix Verb   URI Pattern                                   Controller#Action
          jasminerice        /jasmine                                      Jasminerice::Engine
              apitome        /api/docs                                     Apitome::Engine
api_machine_telematic POST   /api/machines/:machine_id/telematic(.:format) api/v1/telematics#create {:format=>:json}
                      PATCH  /api/machines/:machine_id/telematic(.:format) api/v1/telematics#update {:format=>:json}
                      PUT    /api/machines/:machine_id/telematic(.:format) api/v1/telematics#update {:format=>:json}
         api_machines GET    /api/machines(.:format)                       api/v1/machines#index {:format=>:json}
                      POST   /api/machines(.:format)                       api/v1/machines#create {:format=>:json}
      new_api_machine GET    /api/machines/new(.:format)                   api/v1/machines#new {:format=>:json}
     edit_api_machine GET    /api/machines/:id/edit(.:format)              api/v1/machines#edit {:format=>:json}
          api_machine GET    /api/machines/:id(.:format)                   api/v1/machines#show {:format=>:json}
                      PATCH  /api/machines/:id(.:format)                   api/v1/machines#update {:format=>:json}
                      PUT    /api/machines/:id(.:format)                   api/v1/machines#update {:format=>:json}
                      DELETE /api/machines/:id(.:format)                   api/v1/machines#destroy {:format=>:json}
                      POST   /api/machines/:machine_id/telematic(.:format) api/v1/telematics#create {:format=>:json}
                      PATCH  /api/machines/:machine_id/telematic(.:format) api/v1/telematics#update {:format=>:json}
                      PUT    /api/machines/:machine_id/telematic(.:format) api/v1/telematics#update {:format=>:json}
                      GET    /api/machines(.:format)                       api/v1/machines#index {:format=>:json}
                      POST   /api/machines(.:format)                       api/v1/machines#create {:format=>:json}
                      GET    /api/machines/new(.:format)                   api/v1/machines#new {:format=>:json}
                      GET    /api/machines/:id/edit(.:format)              api/v1/machines#edit {:format=>:json}
                      GET    /api/machines/:id(.:format)                   api/v1/machines#show {:format=>:json}
                      PATCH  /api/machines/:id(.:format)                   api/v1/machines#update {:format=>:json}
                      PUT    /api/machines/:id(.:format)                   api/v1/machines#update {:format=>:json}
                      DELETE /api/machines/:id(.:format)                   api/v1/machines#destroy {:format=>:json}

Routes for Jasminerice::Engine:
           GET /spec/:spec_id/fixtures/*filename(.:format) jasminerice/spec#fixtures
spec_index GET /spec(.:format)                             jasminerice/spec#index
           GET /fixtures/*filename(.:format)               jasminerice/spec#fixtures
           GET /(:suite)(.:format)                         jasminerice/spec#index

Routes for Apitome::Engine:
root GET /                apitome/docs#index
     GET /*path(.:format) apitome/docs#show

And running the server and requesting a resource works as well:

Client:

> curl http://localhost:3000/api/machines
machine v1

Server:

Started GET "/api/machines" for 127.0.0.1 at 2014-02-22 14:03:42 -0500
  ActiveRecord::SchemaMigration Load (0.2ms)  SELECT "schema_migrations".* FROM "schema_migrations"
Processing by Api::V1::MachinesController#index as JSON
  Rendered text template (0.0ms)
Completed 200 OK in 59ms (Views: 28.6ms | ActiveRecord: 0.0ms)

So what am I missing? What's different about your app and this app? Note I've used sqlite instead of Postgres, but I doubt that has anything to do with it.

millisami commented 10 years ago

@bploetz , I think I found the exact problem. If the as option is used, it generates the error. Try adding the following route:

post '/telematic/:modem_id/gps_positions', to: 'gps_positions#create', as: :gps_positions

And isn't there any fix yet for the duplicate route printed?

bploetz commented 10 years ago

@millisami Thank you, I can FINALLY reproduce this issue with the :as option.

As for the "duplicate" routes, they're not really duplicates. Separate and distinct routes are created for the default version than the specific version it points to. You'll notice that the names are different

# v1
api_machine_telematic POST   /api/machines/:machine_id/telematic(.:format) api/v1/telematics#create {:format=>:json}

# Default
                     POST   /api/machines/:machine_id/telematic(.:format) api/v1/telematics#create {:format=>:json}

You'll notice that default routes have no names. In fact, I think that is going to be the fix for this issue, to append something like "default" to the route name for default routes, such that when you run rake routes you'll be able to tell the default routes from the specific version routes, and if we append that "default" prefix to any :as options as well, the names won't collide anymore and you won't get this error.

Stay tuned.....

bploetz commented 10 years ago

@millisami I've pushed up a branch which fixes this issue with :default => true for me locally. Can you update your Gemfile to point at this branch and see if it fixes the problem for you as well?

Gemfile:

gem 'versionist', :github => 'bploetz/versionist', :branch => 'issue59'

Then bundle update versionist.

Let me know. Thanks.

bploetz commented 10 years ago

Oh, and here's what the new output of rake routes looks like for me with this fix. Note the addition of "default" in the default route names.

> rake routes
                           Prefix Verb   URI Pattern                                                           Controller#Action
                      jasminerice        /jasmine                                                              Jasminerice::Engine
                          apitome        /api/docs                                                             Apitome::Engine
            api_machine_telematic POST   /api/machines/:machine_id/telematic(.:format)                         api/v1/telematics#create {:format=>:json}
                                  PATCH  /api/machines/:machine_id/telematic(.:format)                         api/v1/telematics#update {:format=>:json}
                                  PUT    /api/machines/:machine_id/telematic(.:format)                         api/v1/telematics#update {:format=>:json}
        api_machine_gps_positions POST   /api/machines/:machine_id/telematic/:modem_id/gps_positions(.:format) api/v1/gps_positions#create {:format=>:json}
                     api_machines GET    /api/machines(.:format)                                               api/v1/machines#index {:format=>:json}
                                  POST   /api/machines(.:format)                                               api/v1/machines#create {:format=>:json}
                  new_api_machine GET    /api/machines/new(.:format)                                           api/v1/machines#new {:format=>:json}
                 edit_api_machine GET    /api/machines/:id/edit(.:format)                                      api/v1/machines#edit {:format=>:json}
                      api_machine GET    /api/machines/:id(.:format)                                           api/v1/machines#show {:format=>:json}
                                  PATCH  /api/machines/:id(.:format)                                           api/v1/machines#update {:format=>:json}
                                  PUT    /api/machines/:id(.:format)                                           api/v1/machines#update {:format=>:json}
                                  DELETE /api/machines/:id(.:format)                                           api/v1/machines#destroy {:format=>:json}
    api_default_machine_telematic POST   /api/machines/:machine_id/telematic(.:format)                         api/v1/telematics#create {:format=>:json}
                                  PATCH  /api/machines/:machine_id/telematic(.:format)                         api/v1/telematics#update {:format=>:json}
                                  PUT    /api/machines/:machine_id/telematic(.:format)                         api/v1/telematics#update {:format=>:json}
api_default_machine_gps_positions POST   /api/machines/:machine_id/telematic/:modem_id/gps_positions(.:format) api/v1/gps_positions#create {:format=>:json}
             api_default_machines GET    /api/machines(.:format)                                               api/v1/machines#index {:format=>:json}
                                  POST   /api/machines(.:format)                                               api/v1/machines#create {:format=>:json}
          new_api_default_machine GET    /api/machines/new(.:format)                                           api/v1/machines#new {:format=>:json}
         edit_api_default_machine GET    /api/machines/:id/edit(.:format)                                      api/v1/machines#edit {:format=>:json}
              api_default_machine GET    /api/machines/:id(.:format)                                           api/v1/machines#show {:format=>:json}
                                  PATCH  /api/machines/:id(.:format)                                           api/v1/machines#update {:format=>:json}
                                  PUT    /api/machines/:id(.:format)                                           api/v1/machines#update {:format=>:json}
                                  DELETE /api/machines/:id(.:format)                                           api/v1/machines#destroy {:format=>:json}
millisami commented 10 years ago

@bploetz Alright, let me update it and see.

millisami commented 10 years ago

@bploetz Its the fix. Can u merge n publish over rubygems?

bploetz commented 10 years ago

versionist 1.3.0 has been pushed to rubygems.org with this fix.

lukasnagl commented 10 years ago

I'm actually still running into this problem of duplicate routes when naming a route with as:. What did I do wrong?

routes.rb:

api_version(:module => "V1", :header => {:name => "Accept", :value => "application/vnd.qonnect.com; version=1"}, :parameter => {:name => "version", :value => "1"}, :defaults => {:format => :json}, :default => true) do
  resources :providers do
    collection do
      get  'uuid/:uuid', to: 'providers#show_uuid', as: 'uuid'
    end
  end
end

ruby --version: ruby 2.1.0p0 (2013-12-25 revision 44422) [x86_64-darwin12.0]

Gemfile entry: gem 'versionist', '~> 1.3.0'

Error Message:

Invalid route name, already in use: 'uuid_providers'
You may have defined two routes with the same name using the `:as` option, or you may be overriding a route already defined by a resource with the same naming. For the latter, you can restrict the routes created with `resources` as explained here:
bploetz commented 10 years ago

Thanks for your report @j4zz Using your routes.rb example, I see the same error locally. Looking into it....

bploetz commented 10 years ago

OK, I finally see the problem now. The fix I pushed out in versionist 1.3.0 didn't really fix the fundamental problem. :-/

So versionist treats each versioning strategy (and :default) as essentially a unique collection of routes (ultimately calling namespace and/or scope, depending on the versioning strategy/strategies used). What this means is that if you have a route that uses the :as option and you use multiple versioning strategies, that :as name will no longer be unique, and thus the error that you see.

Versionist could tack on something to the :as name to make each one unique (i.e. v1_header_uuid, v1_parameter_uuid, etc) but I think this would be surprising to the developer, since they explicitly said "name the route this".

I'm honestly not sure how to deal with this. Will get back to this when I'm not so jet-lagged.

lukasnagl commented 10 years ago

Thanks for looking into this, @bploetz. As for tacking something to the :as name, I agree that it would have some disadvantages. Personally, I would consider the problem that you would need to rename/replace all use of the route path and url helpers if you introduce a new version the bigger one, though, since it leaves quite some room for errors. If that wouldn't be (or isn't?) the case, the surprising renaming would be acceptable to me.

Just in case someone stumbles over this while searching for the error: as workaround, support only one versioning strategy in the meantime if possible

bploetz commented 10 years ago

The other workaround is to not use the :as option on routes inside an api_version.