Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Attempt to namespace a controller #40

Closed
romanbsd opened this Issue · 19 comments

7 participants

@romanbsd

I have a view in admin namespace served by Admin::DashboardController#show. This view has url_for(:controller => 'stats', :action => 'show')

In journey 1.0.3 this works as expected. In journey 1.0.4 it fails, because it tries to namespace the controller:

ActionController::RoutingError (No route matches {:controller=>"admin/stats", :action=>"show"})
@jefmathiot

We also encountered this issue with a similar namespace setup. We had to downgrade to 1.0.3.

@ccocchi

Same here with API namespace. Worked fine in journey 1.0.3.

Test looks like

describe Api::V1::UsersController do
  describe "GET show" do
    it "should not throw an error"
      get :show, id: '1'
    end
  end

And the error

No route matches {:controller=>"api/v1/users", :action=>"show"}

The route is correctly handle by rake routes

GET      /users/:id(.:format)                                api/v1/users#show
@schneems
Collaborator

Are you able to use url_for and pass in any parameters that work? Such as :controller => '/stats'. To me specifying a controller should be explicit and this would not be expected. The journey team is quite busy, any chance someone could fork journey, add a test to the suite that reproduces this error?

That would likely help if this regression is unwanted. If you do so, can you reference and close this PR?

@romanbsd

It's a pity that Rails 3.2.7 depends on journey 1.0.4, it makes it impossible to upgrade to 3.2.6

@tenderlove
Owner

Can someone post any kind of test? Even an entire rails repo with a failing test would be a great help. Thanks.

@schneems
Collaborator

I made a super simple rails app that demonstrates the behavior https://github.com/heroku/journey-namespace-issue you can run

$ rake

you should receive a failure:

test_should_get_index(Admin::BarsControllerTest):
ActionView::Template::Error: No route matches {:controller=>"admin/foos"}
    /Users/schneems/.rvm/gems/ruby-1.9.3-p194/gems/actionpack-3.2.7/lib/action_dispatch/routing/route_set.rb:532:in `raise_routing_error'
    /Users/schneems/.rvm/gems/ruby-1.9.3-p194/gems/actionpack-3.2.7/lib/action_dispatch/routing/route_set.rb:528:in `rescue in generate'
    /Users/schneems/.rvm/gems/ruby-1.9.3-p194/gems/actionpack-3.2.7/lib/action_dispatch/routing/route_set.rb:520:in `generate'
    /Users/schneems/.rvm/gems/ruby-1.9.3-p194/gems/actionpack-3.2.7/lib/action_dispatch/routing/route_set.rb:561:in `generate'
    /Users/schneems/.rvm/gems/ruby-1.9.3-p194/gems/actionpack-3.2.7/lib/action_dispatch/routing/route_set.rb:586:in `url_for'
    /Users/schneems/.rvm/gems/ruby-1.9.3-p194/gems/actionpack-3.2.7/lib/action_dispatch/routing/url_for.rb:148:in `url_for'
    /Users/schneems/.rvm/gems/ruby-1.9.3-p194/gems/actionpack-3.2.7/lib/action_view/helpers/url_helper.rb:107:in `url_for'

Hope this helps, let me know if you want something changed.

@pixeltrix
Owner

@schneems is the double declaration of resources :foos necessary to generate the error?

@pixeltrix
Owner

Are you sure it generated the correct url before? The only change between 1.0.3 and 1.0.4 with respect to url generation is c70e548 and if I undo that the url generated is /assets?controller=admin%2Ffoos.

@pixeltrix
Owner

Okay, I've confirmed that even in Rails-3.2.0/Journey-1.0.0 the url generated was /assets?controller=admin%2Ffoos so the question is this a regression from Rails-3.1 and Rack::Mount.

@pixeltrix
Owner

Just checked and this how it's meant to be - Rails 3.1 and Rack::Mount generate a routing error when used with @schneems example app. If we look at the relevant code in Rails it's obviously intentional - the initial commit was 2 years ago and @kennyj made a fix that allowed using '/' to specify an absolute controller 6 months ago.

@pixeltrix pixeltrix closed this
@schneems
Collaborator

Great sleuthing, you think this behavior warrants documentation in url_for ?

@romanbsd

I second @schneems

This is a unexpected and breaking change.

@schneems
Collaborator

@romanbsd I don't disagree with the change, though expected behaviors should be documented. I try to never use url_for directly.

@pixeltrix
Owner

@romanbsd it isn't really a change - it's just stopped generating an incorrect url. At least this way you'll now know about the error. Like @schneems I try to use named routes wherever possible - much quicker than having Journey find the best match.

@schneems yes, some documentation about the use of relative controller paths would be useful - especially about the use of a leading slash to specify an absolute path.

@romanbsd

@pixeltrix the code in question is a code of a helper, which is shared among the controllers. With journey 1.0.3 it was generating the correct (in my opinion) urls when used from a regular controllers, as well as from namespaced controllers. The change in journey 1.0.4 takes into account the current namespacing of the controller, which I consider a breaking change. Regarding the named routes - this is impossible in my case, as the url is generated automagically by the helper.

@pixeltrix
Owner

@romanbsd you'll have to post some code that generates a correct url on Rails-3.2.6/Journey-1.0.3 then, because this is what I get:

Testapp::Application.routes.draw do
  namespace :admin do
    get '/dashboard', :to => 'dashboard#show', :as => :dashboard
  end

  get '/stats', :to => 'stats#show', :as => :stats
end
$ rails c
Loading development environment (Rails 3.2.6)
>> app.get '/admin/dashboard'
=> 200
>> app.url_for :controller => 'stats', :action => 'show'
=> "http://www.example.com/assets?action=show&controller=admin%2Fstats"
>> app.url_for :controller => '/stats', :action => 'show'
=> "http://www.example.com/stats"
$ rails c
Loading development environment (Rails 3.2.7)
>> app.get '/admin/dashboard'
=> 200
>> app.url_for :controller => 'stats', :action => 'show'
ActionController::RoutingError: No route matches {:controller=>"admin/stats", :action=>"show"}
>> app.url_for :controller => '/stats', :action => 'show'
=> "http://www.example.com/stats"
@keo
keo commented

If I have a namespaced route:

  scope '/account' do
    resources :email_accounts
  end

then in a view I do:

if current_page? controller: "email_accounts"

and it says: No route matches {:controller=>"devise/email_accounts"}

if I prepend a slash to it it works:

if current_page? controller: "/email_accounts"

This looks a bit odd because it is namespaced with /account in the URL.

Shouldn't it be doing the following: look for the controller in the current scope, then if not found, look for the same controller in the root scope?

@pixeltrix
Owner

Shouldn't it be doing the following: look for the controller in the current scope, then if not found, look for the same controller in the root scope?

That would often generate incorrect urls which would be likely to go unnoticed - this way it's brought to the developers attention. You can use named route helpers with current_page? if that would be better, e.g:

if current_page? email_accounts_url

This looks a bit odd because it is namespaced with /account in the URL.

But your controller isn't namespaced - if you namespace the controller it will work because the namespace depth of the current controller and the new controller are the same, e.g:

scope '/account', :module => 'account' do
  resources :email_accounts
end

this will now work:

if current_page? controller: 'account/email_accounts'
@schneems
Collaborator

Added docs for this functionality: rails/docrails@e463351

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.