From c6843e23373c626ae49ad9fa253d4f7538d3434f Mon Sep 17 00:00:00 2001 From: Andrew White Date: Sun, 4 Jul 2010 06:52:52 +0100 Subject: Refactor resource options and scoping. Resource classes are now only responsible for controlling how they are named. All other options passed to resources are pushed out to the scope. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- actionpack/lib/action_dispatch/routing/mapper.rb | 163 +++++++++-------------- actionpack/test/dispatch/routing_test.rb | 83 +++++++++++- 2 files changed, 145 insertions(+), 101 deletions(-) diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 64e12f960d..f44c10f533 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -444,7 +444,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] - MERGE_FROM_SCOPE_OPTIONS = [:shallow, :constraints] + RESOURCE_OPTIONS = [:as, :controller, :path] class Resource #:nodoc: DEFAULT_ACTIONS = [:index, :create, :new, :show, :update, :destroy, :edit] @@ -463,16 +463,6 @@ 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 @@ -489,68 +479,31 @@ module ActionDispatch singular end - alias_method :nested_name, :member_name - # Checks for uncountable plurals, and appends "_index" if they're. def collection_name singular == plural ? "#{plural}_index" : plural end - def shallow? - options[:shallow] ? true : false - end - - def constraints - options[:constraints] || {} - end - - def id_constraint? - options[:id] && options[:id].is_a?(Regexp) || constraints[:id] && constraints[:id].is_a?(Regexp) - end - - def id_constraint - options[:id] || constraints[:id] - end - - def collection_options - (options || {}).dup.tap do |opts| - opts.delete(:id) - opts[:constraints] = options[:constraints].dup if options[:constraints] - opts[:constraints].delete(:id) if options[:constraints].is_a?(Hash) - end - end - - def nested_path - "#{path}/:#{singular}_id" - end - - def nested_options - {}.tap do |opts| - opts[:as] = member_name - opts["#{singular}_id".to_sym] = id_constraint if id_constraint? - opts[:options] = { :shallow => shallow? } unless options[:shallow].nil? - end - end - def resource_scope - [{ :controller => controller }] + { :controller => controller } end def collection_scope - [path, collection_options] + path end def member_scope - ["#{path}/:id", options] + "#{path}/:id" end def new_scope(new_path) - ["#{path}/#{new_path}"] + "#{path}/#{new_path}" end def nested_scope - [nested_path, nested_options] + "#{path}/:#{singular}_id" end + end class SingletonResource < Resource #:nodoc: @@ -559,7 +512,7 @@ module ActionDispatch def initialize(entities, options) @name = entities.to_s @path = options.delete(:path) || @name - @controller = (options.delete(:controller) || @name.to_s.pluralize).to_s + @controller = (options.delete(:controller) || plural).to_s @as = options.delete(:as) @options = options end @@ -569,24 +522,10 @@ module ActionDispatch end alias :collection_name :member_name - def nested_path - path - end - - def nested_options - {}.tap do |opts| - opts[:as] = member_name - opts[:options] = { :shallow => shallow? } unless @options[:shallow].nil? - end - end - - def shallow? - false - end - def member_scope - [path, options] + path end + alias :nested_scope :member_scope end def initialize(*args) #:nodoc: @@ -610,17 +549,17 @@ module ActionDispatch collection_scope do post :create - end if parent_resource.actions.include?(:create) + end if resource_actions.include?(:create) new_scope do get :new - end if parent_resource.actions.include?(:new) + end if resource_actions.include?(:new) member_scope do - 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) + 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) end end @@ -638,19 +577,19 @@ module ActionDispatch yield if block_given? collection_scope do - get :index if parent_resource.actions.include?(:index) - post :create if parent_resource.actions.include?(:create) + get :index if resource_actions.include?(:index) + post :create if resource_actions.include?(:create) end new_scope do get :new - end if parent_resource.actions.include?(:new) + end if resource_actions.include?(:new) member_scope do - 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) + 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) end end @@ -693,18 +632,18 @@ module ActionDispatch end with_scope_level(:nested) do - if parent_resource.shallow? + if shallow? with_exclusive_scope do if @scope[:shallow_path].blank? - scope(*parent_resource.nested_scope) { yield } + scope(parent_resource.nested_scope, nested_options) { yield } else scope(@scope[:shallow_path], :as => @scope[:shallow_prefix]) do - scope(*parent_resource.nested_scope) { yield } + scope(parent_resource.nested_scope, nested_options) { yield } end end end else - scope(*parent_resource.nested_scope) { yield } + scope(parent_resource.nested_scope, nested_options) { yield } end end end @@ -723,6 +662,10 @@ module ActionDispatch end end + def shallow? + parent_resource.instance_of?(Resource) && @scope[:shallow] + end + def match(*args) options = args.extract_options!.dup options[:anchor] = true unless options.key?(:anchor) @@ -794,23 +737,30 @@ 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) } return true end - if path_names = options.delete(:path_names) - scope(:path_names => path_names) do + scope_options = options.slice!(*RESOURCE_OPTIONS) + unless scope_options.empty? + scope(scope_options) do send(method, resources.pop, options, &block) end return true end - scope_options = @scope.slice(*MERGE_FROM_SCOPE_OPTIONS).delete_if{ |k,v| v.blank? } - options.reverse_merge!(scope_options) unless scope_options.empty? - options.reverse_merge!(@scope[:options]) unless @scope[:options].blank? - if resource_scope? nested do send(method, resources.pop, options, &block) @@ -853,7 +803,7 @@ module ActionDispatch def resource_scope(resource) with_scope_level(resource.is_a?(SingletonResource) ? :resource : :resources, resource) do - scope(*parent_resource.resource_scope) do + scope(parent_resource.resource_scope) do yield end end @@ -861,7 +811,7 @@ module ActionDispatch def new_scope with_scope_level(:new) do - scope(*parent_resource.new_scope(action_path(:new))) do + scope(parent_resource.new_scope(action_path(:new))) do yield end end @@ -869,7 +819,7 @@ module ActionDispatch def collection_scope with_scope_level(:collection) do - scope(*parent_resource.collection_scope) do + scope(parent_resource.collection_scope) do yield end end @@ -877,18 +827,33 @@ module ActionDispatch def member_scope with_scope_level(:member) do - scope(*parent_resource.member_scope) do + scope(parent_resource.member_scope) do yield end end end + def nested_options + {}.tap do |options| + options[:as] = parent_resource.member_name + options[:constraints] = { "#{parent_resource.singular}_id".to_sym => id_constraint } if id_constraint? + end + end + + def id_constraint? + @scope[:id].is_a?(Regexp) || (@scope[:constraints] && @scope[:constraints][:id].is_a?(Regexp)) + end + + def id_constraint + @scope[:id] || @scope[:constraints][:id] + end + def canonical_action?(action, flag) flag && CANONICAL_ACTIONS.include?(action) end def shallow_scoping? - parent_resource && parent_resource.shallow? && @scope[:scope_level] == :member + shallow? && @scope[:scope_level] == :member end def path_for_action(action, path) diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 1bfc92aa3d..d56612d6ec 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -298,8 +298,8 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest resource :dashboard, :constraints => { :ip => /192\.168\.1\.\d{1,3}/ } + resource :token, :module => :api scope :module => :api do - resource :token resources :errors, :shallow => true do resources :notices end @@ -349,6 +349,22 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest match ":controller(/:action(/:id))" end + scope :only => :index do + resources :clubs do + resources :players, :only => [:show] + resource :chairman, :only => [:show] + end + end + + scope :except => [:new, :create, :edit, :update] do + resources :sectors do + resources :companies, :except => :destroy do + resources :divisions, :only => :index + end + resource :leader, :except => :destroy + end + end + match '/:locale/*file.:format', :to => 'files#show', :file => /path\/to\/existing\/file/ end end @@ -1254,7 +1270,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest assert_equal 'pass', @response.headers['X-Cascade'] get '/products/0001/images' assert_equal 'images#index', @response.body - get '/products/0001/images/1' + get '/products/0001/images/0001' assert_equal 'images#show', @response.body get '/dashboard', {}, {'REMOTE_ADDR' => '10.0.0.100'} @@ -1640,6 +1656,69 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest assert_equal 'Not Found', @response.body assert_raises(ActionController::RoutingError){ movie_trailer_path(:movie_id => '00001') } end + + def test_only_option_should_be_overwritten + get '/clubs' + assert_equal 'clubs#index', @response.body + assert_equal '/clubs', clubs_path + + get '/clubs/1' + assert_equal 'Not Found', @response.body + assert_raise(NameError) { club_path(:id => '1') } + + get '/clubs/1/players' + assert_equal 'Not Found', @response.body + assert_raise(NameError) { club_players_path(:club_id => '1') } + + 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 '/clubs/1/chairman/new' + assert_equal 'Not Found', @response.body + assert_raise(NameError) { new_club_chairman_path(:club_id => '1') } + + get '/clubs/1/chairman' + assert_equal 'chairmen#show', @response.body + assert_equal '/clubs/1/chairman', club_chairman_path(:club_id => '1') + end + + def test_except_option_should_be_overwritten + get '/sectors' + assert_equal 'sectors#index', @response.body + assert_equal '/sectors', sectors_path + + get '/sectors/new' + assert_equal 'Not Found', @response.body + assert_raise(NameError) { new_sector_path } + + delete '/sectors/1' + assert_equal 'sectors#destroy', @response.body + + get '/sectors/1/companies/new' + assert_equal 'companies#new', @response.body + assert_equal '/sectors/1/companies/new', new_sector_company_path + + delete '/sectors/1/companies/1' + assert_equal 'Not Found', @response.body + + get '/sectors/1/leader/new' + assert_equal 'leaders#new', @response.body + assert_equal '/sectors/1/leader/new', new_sector_leader_path + + delete '/sectors/1/leader' + assert_equal 'Not Found', @response.body + end + + def test_only_option_should_overwrite_except_option + get '/sectors/1/companies/2/divisions' + assert_equal 'divisions#index', @response.body + assert_equal '/sectors/1/companies/2/divisions', sector_company_divisions_path + + get '/sectors/1/companies/2/divisions/3' + assert_equal 'Not Found', @response.body + assert_raise(NameError) { sector_company_division_path(:sector_id => '1', :company_id => '2', :id => '3') } + end end private -- cgit v1.2.3