Refactor handling of :only and :except options. The rules are:

1. Don't inherit when specified as an option on a resource
2. Don't push into scope when specified as an option on a resource
2. Resources pull in :only or :except options from scope
3. Either :only or :except in nested scope overwrites parent scope

[#5048 state:resolved]

Signed-off-by: José Valim <jose.valim@gmail.com>

2 files changed, 236 insertions, 73 deletions

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 6529cf6f31..aab272f565 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -433,12 +433,16 @@ module ActionDispatch
           end
 
           def merge_options_scope(parent, child)
-            (parent || {}).merge(child)
+            (parent || {}).except(*override_keys(child)).merge(child)
           end
 
           def merge_shallow_scope(parent, child)
             child ? true : false
           end
+
+          def override_keys(child)
+            child.key?(:only) || child.key?(:except) ? [:only, :except] : []
+          end
       end
 
       module Resources
@@ -446,7 +450,7 @@ module ActionDispatch
         # a path appended since they fit properly in their scope level.
         VALID_ON_OPTIONS = [:new, :collection, :member]
         CANONICAL_ACTIONS = [:index, :create, :new, :show, :update, :destroy]
-        RESOURCE_OPTIONS = [:as, :controller, :path]
+        RESOURCE_OPTIONS = [:as, :controller, :path, :only, :except]
 
         class Resource #:nodoc:
           DEFAULT_ACTIONS = [:index, :create, :new, :show, :update, :destroy, :edit]
@@ -465,6 +469,16 @@ module ActionDispatch
             self.class::DEFAULT_ACTIONS
           end
 
+          def actions
+            if only = @options[:only]
+              Array(only).map(&:to_sym)
+            elsif except = @options[:except]
+              default_actions - Array(except).map(&:to_sym)
+            else
+              default_actions
+            end
+          end
+
           def name
             @as || @name
           end
@@ -551,17 +565,17 @@ module ActionDispatch
 
             collection_scope do
               post :create
-            end if resource_actions.include?(:create)
+            end if parent_resource.actions.include?(:create)
 
             new_scope do
               get :new
-            end if resource_actions.include?(:new)
+            end if parent_resource.actions.include?(:new)
 
             member_scope  do
-              get    :show if resource_actions.include?(:show)
-              put    :update if resource_actions.include?(:update)
-              delete :destroy if resource_actions.include?(:destroy)
-              get    :edit if resource_actions.include?(:edit)
+              get    :show if parent_resource.actions.include?(:show)
+              put    :update if parent_resource.actions.include?(:update)
+              delete :destroy if parent_resource.actions.include?(:destroy)
+              get    :edit if parent_resource.actions.include?(:edit)
             end
           end
 
@@ -579,19 +593,19 @@ module ActionDispatch
             yield if block_given?
 
             collection_scope do
-              get  :index if resource_actions.include?(:index)
-              post :create if resource_actions.include?(:create)
+              get  :index if parent_resource.actions.include?(:index)
+              post :create if parent_resource.actions.include?(:create)
             end
 
             new_scope do
               get :new
-            end if resource_actions.include?(:new)
+            end if parent_resource.actions.include?(:new)
 
             member_scope  do
-              get    :show if resource_actions.include?(:show)
-              put    :update if resource_actions.include?(:update)
-              delete :destroy if resource_actions.include?(:destroy)
-              get    :edit if resource_actions.include?(:edit)
+              get    :show if parent_resource.actions.include?(:show)
+              put    :update if parent_resource.actions.include?(:update)
+              delete :destroy if parent_resource.actions.include?(:destroy)
+              get    :edit if parent_resource.actions.include?(:edit)
             end
           end
 
@@ -739,16 +753,6 @@ module ActionDispatch
             @scope[:scope_level_resource]
           end
 
-          def resource_actions
-            if only = @scope[:options][:only]
-              Array(only).map(&:to_sym)
-            elsif except = @scope[:options][:except]
-              parent_resource.default_actions - Array(except).map(&:to_sym)
-            else
-              parent_resource.default_actions
-            end
-          end
-
           def apply_common_behavior_for(method, resources, options, &block)
             if resources.length > 1
               resources.each { |r| send(method, r, options, &block) }
@@ -763,6 +767,10 @@ module ActionDispatch
               return true
             end
 
+            unless action_options?(options)
+              options.merge!(scope_action_options) if scope_action_options?
+            end
+
             if resource_scope?
               nested do
                 send(method, resources.pop, options, &block)
@@ -773,6 +781,18 @@ module ActionDispatch
             false
           end
 
+          def action_options?(options)
+            options[:only] || options[:except]
+          end
+
+          def scope_action_options?
+            @scope[:options].is_a?(Hash) && (@scope[:options][:only] || @scope[:options][:except])
+          end
+
+          def scope_action_options
+            @scope[:options].slice(:only, :except)
+          end
+
           def resource_scope?
             [:resource, :resources].include?(@scope[:scope_level])
           end
@@ -899,7 +919,7 @@ module ActionDispatch
               collection_name = parent_resource.collection_name
               member_name = parent_resource.member_name
               name_prefix = "#{name_prefix}_" if name_prefix.present?
-            end 
+            end
 
             case @scope[:scope_level]
             when :collection
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 463a62cd53..90d05d4a67 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -352,19 +352,55 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
         match ":controller(/:action(/:id))"
       end
 
-      scope :only => :index do
-        resources :clubs do
-          resources :players, :only => [:show]
-          resource  :chairman, :only => [:show]
+      scope :only => [:index, :show] do
+        namespace :only do
+          resources :clubs do
+            resources :players
+            resource  :chairman
+          end
         end
       end
 
-      scope :except => [:new, :create, :edit, :update] do
-        resources :sectors do
-          resources :companies, :except => :destroy do
-            resources :divisions, :only => :index
+      scope :except => [:new, :create, :edit, :update, :destroy] do
+        namespace :except do
+          resources :clubs do
+            resources :players
+            resource  :chairman
+          end
+        end
+      end
+
+      scope :only => :show do
+        namespace :only do
+          resources :sectors, :only => :index do
+            resources :companies do
+              scope :only => :index do
+                resources :divisions
+              end
+              scope :except => [:show, :update, :destroy] do
+                resources :departments
+              end
+            end
+            resource  :leader
+            resources :managers, :except => [:show, :update, :destroy]
+          end
+        end
+      end
+
+      scope :except => :index do
+        namespace :except do
+          resources :sectors, :except => [:show, :update, :destroy] do
+            resources :companies do
+              scope :except => [:show, :update, :destroy] do
+                resources :divisions
+              end
+              scope :only => :index do
+                resources :departments
+              end
+            end
+            resource  :leader
+            resources :managers, :only => :index
           end
-          resource  :leader, :except => :destroy
         end
       end
 
@@ -1661,72 +1697,179 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     end
   end
 
-  def test_only_option_should_be_overwritten
+  def test_only_should_be_read_from_scope
     with_test_routes do
-      get '/clubs'
-      assert_equal 'clubs#index', @response.body
-      assert_equal '/clubs', clubs_path
+      get '/only/clubs'
+      assert_equal 'only/clubs#index', @response.body
+      assert_equal '/only/clubs', only_clubs_path
 
-      get '/clubs/1'
+      get '/only/clubs/1/edit'
       assert_equal 'Not Found', @response.body
-      assert_raise(NoMethodError) { club_path(:id => '1') }
+      assert_raise(NoMethodError) { edit_only_club_path(:id => '1') }
+
+      get '/only/clubs/1/players'
+      assert_equal 'only/players#index', @response.body
+      assert_equal '/only/clubs/1/players', only_club_players_path(:club_id => '1')
 
-      get '/clubs/1/players'
+      get '/only/clubs/1/players/2/edit'
       assert_equal 'Not Found', @response.body
-      assert_raise(NoMethodError) { club_players_path(:club_id => '1') }
+      assert_raise(NoMethodError) { edit_only_club_player_path(:club_id => '1', :id => '2') }
 
-      get '/clubs/1/players/2'
-      assert_equal 'players#show', @response.body
-      assert_equal '/clubs/1/players/2', club_player_path(:club_id => '1', :id => '2')
+      get '/only/clubs/1/chairman'
+      assert_equal 'only/chairmen#show', @response.body
+      assert_equal '/only/clubs/1/chairman', only_club_chairman_path(:club_id => '1')
 
-      get '/clubs/1/chairman/new'
+      get '/only/clubs/1/chairman/edit'
       assert_equal 'Not Found', @response.body
-      assert_raise(NoMethodError) { new_club_chairman_path(:club_id => '1') }
+      assert_raise(NoMethodError) { edit_only_club_chairman_path(:club_id => '1') }
+    end
+  end
 
-      get '/clubs/1/chairman'
-      assert_equal 'chairmen#show', @response.body
-      assert_equal '/clubs/1/chairman', club_chairman_path(:club_id => '1')
+  def test_except_should_be_read_from_scope
+    with_test_routes do
+      get '/except/clubs'
+      assert_equal 'except/clubs#index', @response.body
+      assert_equal '/except/clubs', except_clubs_path
+
+      get '/except/clubs/1/edit'
+      assert_equal 'Not Found', @response.body
+      assert_raise(NoMethodError) { edit_except_club_path(:id => '1') }
+
+      get '/except/clubs/1/players'
+      assert_equal 'except/players#index', @response.body
+      assert_equal '/except/clubs/1/players', except_club_players_path(:club_id => '1')
+
+      get '/except/clubs/1/players/2/edit'
+      assert_equal 'Not Found', @response.body
+      assert_raise(NoMethodError) { edit_except_club_player_path(:club_id => '1', :id => '2') }
+
+      get '/except/clubs/1/chairman'
+      assert_equal 'except/chairmen#show', @response.body
+      assert_equal '/except/clubs/1/chairman', except_club_chairman_path(:club_id => '1')
+
+      get '/except/clubs/1/chairman/edit'
+      assert_equal 'Not Found', @response.body
+      assert_raise(NoMethodError) { edit_except_club_chairman_path(:club_id => '1') }
     end
   end
 
-  def test_except_option_should_be_overwritten
+  def test_only_option_should_override_scope
     with_test_routes do
-      get '/sectors'
-      assert_equal 'sectors#index', @response.body
-      assert_equal '/sectors', sectors_path
+      get '/only/sectors'
+      assert_equal 'only/sectors#index', @response.body
+      assert_equal '/only/sectors', only_sectors_path
 
-      get '/sectors/1/edit'
+      get '/only/sectors/1'
       assert_equal 'Not Found', @response.body
-      assert_raise(NoMethodError) { edit_sector_path(:id => '1') }
+      assert_raise(NoMethodError) { only_sector_path(:id => '1') }
+    end
+  end
+
+  def test_only_option_should_not_inherit
+    with_test_routes do
+      get '/only/sectors/1/companies/2'
+      assert_equal 'only/companies#show', @response.body
+      assert_equal '/only/sectors/1/companies/2', only_sector_company_path(:sector_id => '1', :id => '2')
 
-      delete '/sectors/1'
-      assert_equal 'sectors#destroy', @response.body
+      get '/only/sectors/1/leader'
+      assert_equal 'only/leaders#show', @response.body
+      assert_equal '/only/sectors/1/leader', only_sector_leader_path(:sector_id => '1')
+    end
+  end
 
-      get '/sectors/1/companies/new'
-      assert_equal 'companies#new', @response.body
-      assert_equal '/sectors/1/companies/new', new_sector_company_path(:sector_id => '1')
+  def test_except_option_should_override_scope
+    with_test_routes do
+      get '/except/sectors'
+      assert_equal 'except/sectors#index', @response.body
+      assert_equal '/except/sectors', except_sectors_path
 
-      delete '/sectors/1/companies/1'
+      get '/except/sectors/1'
       assert_equal 'Not Found', @response.body
+      assert_raise(NoMethodError) { except_sector_path(:id => '1') }
+    end
+  end
+
+  def test_except_option_should_not_inherit
+    with_test_routes do
+      get '/except/sectors/1/companies/2'
+      assert_equal 'except/companies#show', @response.body
+      assert_equal '/except/sectors/1/companies/2', except_sector_company_path(:sector_id => '1', :id => '2')
+
+      get '/except/sectors/1/leader'
+      assert_equal 'except/leaders#show', @response.body
+      assert_equal '/except/sectors/1/leader', except_sector_leader_path(:sector_id => '1')
+    end
+  end
+
+  def test_except_option_should_override_scoped_only
+    with_test_routes do
+      get '/only/sectors/1/managers'
+      assert_equal 'only/managers#index', @response.body
+      assert_equal '/only/sectors/1/managers', only_sector_managers_path(:sector_id => '1')
+
+      get '/only/sectors/1/managers/2'
+      assert_equal 'Not Found', @response.body
+      assert_raise(NoMethodError) { only_sector_manager_path(:sector_id => '1', :id => '2') }
+    end
+  end
+
+  def test_only_option_should_override_scoped_except
+    with_test_routes do
+      get '/except/sectors/1/managers'
+      assert_equal 'except/managers#index', @response.body
+      assert_equal '/except/sectors/1/managers', except_sector_managers_path(:sector_id => '1')
+
+      get '/except/sectors/1/managers/2'
+      assert_equal 'Not Found', @response.body
+      assert_raise(NoMethodError) { except_sector_manager_path(:sector_id => '1', :id => '2') }
+    end
+  end
+
+  def test_only_scope_should_override_parent_scope
+    with_test_routes do
+      get '/only/sectors/1/companies/2/divisions'
+      assert_equal 'only/divisions#index', @response.body
+      assert_equal '/only/sectors/1/companies/2/divisions', only_sector_company_divisions_path(:sector_id => '1', :company_id => '2')
+
+      get '/only/sectors/1/companies/2/divisions/3'
+      assert_equal 'Not Found', @response.body
+      assert_raise(NoMethodError) { only_sector_company_division_path(:sector_id => '1', :company_id => '2', :id => '3') }
+    end
+  end
+
+  def test_except_scope_should_override_parent_scope
+    with_test_routes do
+      get '/except/sectors/1/companies/2/divisions'
+      assert_equal 'except/divisions#index', @response.body
+      assert_equal '/except/sectors/1/companies/2/divisions', except_sector_company_divisions_path(:sector_id => '1', :company_id => '2')
+
+      get '/except/sectors/1/companies/2/divisions/3'
+      assert_equal 'Not Found', @response.body
+      assert_raise(NoMethodError) { except_sector_company_division_path(:sector_id => '1', :company_id => '2', :id => '3') }
+    end
+  end
 
-      get '/sectors/1/leader/new'
-      assert_equal 'leaders#new', @response.body
-      assert_equal '/sectors/1/leader/new', new_sector_leader_path(:sector_id => '1')
+  def test_except_scope_should_override_parent_only_scope
+    with_test_routes do
+      get '/only/sectors/1/companies/2/departments'
+      assert_equal 'only/departments#index', @response.body
+      assert_equal '/only/sectors/1/companies/2/departments', only_sector_company_departments_path(:sector_id => '1', :company_id => '2')
 
-      delete '/sectors/1/leader'
+      get '/only/sectors/1/companies/2/departments/3'
       assert_equal 'Not Found', @response.body
+      assert_raise(NoMethodError) { only_sector_company_department_path(:sector_id => '1', :company_id => '2', :id => '3') }
     end
   end
 
-  def test_only_option_should_overwrite_except_option
+  def test_only_scope_should_override_parent_except_scope
     with_test_routes do
-      get '/sectors/1/companies/2/divisions'
-      assert_equal 'divisions#index', @response.body
-      assert_equal '/sectors/1/companies/2/divisions', sector_company_divisions_path(:sector_id => '1', :company_id => '2')
+      get '/except/sectors/1/companies/2/departments'
+      assert_equal 'except/departments#index', @response.body
+      assert_equal '/except/sectors/1/companies/2/departments', except_sector_company_departments_path(:sector_id => '1', :company_id => '2')
 
-      get '/sectors/1/companies/2/divisions/3'
+      get '/except/sectors/1/companies/2/departments/3'
       assert_equal 'Not Found', @response.body
-      assert_raise(NoMethodError) { sector_company_division_path(:sector_id => '1', :company_id => '2', :id => '3') }
+      assert_raise(NoMethodError) { except_sector_company_department_path(:sector_id => '1', :company_id => '2', :id => '3') }
     end
   end