Skip to content

Potential bug in rails routing with singular and plural resources #8814

Nbblrr opened this Issue Jan 8, 2013 · 5 comments

2 participants

Nbblrr commented Jan 8, 2013

I may have found a weird bug in rails routing.

Basically, I've an application with my own implementation of users and access rights. The Users Controller is mapped through single and plural resource like this (code is a bit long but usefull after):

    resource :user, :as => :current_user, :type => :current_user do
        collection do
            post 'one_request'
            post 'other_request'
    resources :users
    resources :sessions, :only => [:new, :create, :destroy] do
        collection do
            post 'request_new_password'

    match "/signin" => 'sessions#new', :as => :signin
    match '/signout', :to => 'sessions#destroy'

    match '/foo' => redirect('/bar')

This code is tested with rspec with code like this one :

    describe "GET 'show'" do
        describe "Singular path" do
            it "should redirect without authentication (HTML)" do
                get 'show'
                response.should redirect_to(signin_path)

The weird problem is that if I remove the useless redirection in my config/routes.rb, I've the following error on all spec related to singular resource testing :

  1) UsersController GET 'show' Singular path should redirect without authentication (HTML)
     Failure/Error: get 'show', :type => :current_user
       No route matches {:controller=>"users", :action=>"show"}
     # ./spec/controllers/users_controller_spec.rb:138:in `block (4 levels) in <top (required)>'

To my mind, it seems to be an error in the way rails handle routes other wise I can't see why a useless redirection would change my route in another controller. This bugs seems to be limited only to singular resources and may be created by other parameters in my code.

What do you think ?

Nbblrr commented Jan 9, 2013

I forgot to mention another weird thing : this route (/user for example) does work in the browser.

Ruby on Rails member

What's happening is that the controller spec is similar to a functional test and the test runner tries to create a url for the mock request from the controller, action and any params passed to get. Because you've added the :type => :current_user to the route definition the url generation code needs that to select the correct route.

When running in the browser the :type parameter will generally be picked up from either the recall parameters or supplied directly via the named route helper and everything works fine. However in the test/spec there's no recall params and the route is generated using url_for so it isn't supplied automatically. If you pass the :type param to the get call it should generate the route correctly.

The reason that it works with the redirect route is that it provides a fallback for the url_for - it will actually build an incorrect url along the lines of /foo?controller=users&action=show. Note that this only applies to named routes - if you passed an :as => nil to the redirect route definition then it would generate an error in the test/spec again.

This is something that we plan to address for the Rails 4 release by relaxing the requirement for the extra params to be supplied only when they are either in the route's options or in requirements, e.g:

# These will require :bar when using url_for
get '/foo', to: 'foo', bar: true
get '/foo', to: 'foo', requirements: { bar: true }

# This will not require :bar when using url_for
get '/foo', to: 'foo', defaults: { bar: true }

TL;DR: add :type => :current_user to your get call in your spec.

@pixeltrix pixeltrix was assigned Jan 9, 2013
Nbblrr commented Jan 9, 2013

There is no change when I use :type => :current_user like this :

    get 'show', :type => :current_user

Error message :

  1) UsersController GET 'show' Sin gular path should redirect without authentication (HTML)
     Failure/Error: get 'show', :type => :current_user
       No route matches {:type=>"current_user", :controller=>"users", :action=>"show"}
     # ./spec/controllers/users_controller_spec.rb:138:in `block (4 levels) in <top (required)>'

However you are right, the bug comes from the :type element and there is no problem when I remove it (it was not mandatory in my case).

Is the first behavior a potential bug ? Do you want me to investigate on it ?
Otherwise I won't use and the :type element and I can close the ticket.

Ruby on Rails member

There is no change when I use :type => :current_user like this

Hmm, looking at the code suggests that it should - I'll investigate as part of the changes I mentioned as it needs to work. Please leave the ticket open thanks.

Nbblrr commented Jan 9, 2013

Ok, let me know if you need some help to reproduce the bug.
Thanks !

@pixeltrix pixeltrix added a commit that closed this issue Jan 15, 2013
@pixeltrix pixeltrix Change the behavior of route defaults
This commit changes route defaults so that explicit defaults are no
longer required where the key is not part of the path. For example:

  resources :posts, bucket_type: 'posts'

will be required whenever constructing the url from a hash such as a
functional test or using url_for directly. However using the explicit
form alters the behavior so it's not required:

  resources :projects, defaults: { bucket_type: 'projects' }

This changes existing behavior slightly in that any routes which
only differ in their defaults will match the first route rather
than the closest match.

Closes #8814
@pixeltrix pixeltrix closed this in f1d8f2a Jan 15, 2013
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.