Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Add :only/:except options to map.resources

This allows people with huge numbers of resource routes to cut down on the memory consumption caused by the generated code.

Signed-off-by: Michael Koziarski <michael@koziarski.com>
[#1215 state:committed]
  • Loading branch information...
commit 44a3009ff068bf080de6764a8c884fbf0ceb920e 1 parent c65075f
@tomstuart tomstuart authored NZKoz committed
View
122 actionpack/lib/action_controller/resources.rb
@@ -42,7 +42,11 @@ module ActionController
#
# Read more about REST at http://en.wikipedia.org/wiki/Representational_State_Transfer
module Resources
+ INHERITABLE_OPTIONS = :namespace, :shallow, :only, :except
+
class Resource #:nodoc:
+ DEFAULT_ACTIONS = :index, :create, :new, :edit, :show, :update, :destroy
+
attr_reader :collection_methods, :member_methods, :new_methods
attr_reader :path_prefix, :name_prefix, :path_segment
attr_reader :plural, :singular
@@ -57,6 +61,7 @@ def initialize(entities, options)
arrange_actions
add_default_actions
+ set_allowed_actions
set_prefixes
end
@@ -113,6 +118,10 @@ def uncountable?
@singular.to_s == @plural.to_s
end
+ def has_action?(action)
+ !DEFAULT_ACTIONS.include?(action) || action_allowed?(action)
+ end
+
protected
def arrange_actions
@collection_methods = arrange_actions_by_methods(options.delete(:collection))
@@ -125,6 +134,30 @@ def add_default_actions
add_default_action(new_methods, :get, :new)
end
+ def set_allowed_actions
+ only, except = @options.values_at(:only, :except)
+ @allowed_actions ||= {}
+
+ if only == :all || except == :none
+ only = nil
+ except = []
+ elsif only == :none || except == :all
+ only = []
+ except = nil
+ end
+
+ if only
+ @allowed_actions[:only] = Array(only).map(&:to_sym)
+ elsif except
+ @allowed_actions[:except] = Array(except).map(&:to_sym)
+ end
+ end
+
+ def action_allowed?(action)
+ only, except = @allowed_actions.values_at(:only, :except)
+ (!only || only.include?(action)) && (!except || !except.include?(action))
+ end
+
def set_prefixes
@path_prefix = options.delete(:path_prefix)
@name_prefix = options.delete(:name_prefix)
@@ -353,6 +386,25 @@ def initialize(entity, options)
#
# map.resources :users, :has_many => { :posts => :comments }, :shallow => true
#
+ # * <tt>:only</tt> and <tt>:except</tt> - Specify which of the seven default actions should be routed to.
+ #
+ # <tt>:only</tt> and <tt>:except</tt> may be set to <tt>:all</tt>, <tt>:none</tt>, an action name or a
+ # list of action names. By default, routes are generated for all seven actions.
+ #
+ # For example:
+ #
+ # map.resources :posts, :only => [:index, :show] do |post|
+ # post.resources :comments, :except => [:update, :destroy]
+ # end
+ # # --> GET /posts (maps to the PostsController#index action)
+ # # --> POST /posts (fails)
+ # # --> GET /posts/1 (maps to the PostsController#show action)
+ # # --> DELETE /posts/1 (fails)
+ # # --> POST /posts/1/comments (maps to the CommentsController#create action)
+ # # --> PUT /posts/1/comments/1 (fails)
+ #
+ # The <tt>:only</tt> and <tt>:except</tt> options are inherited by any nested resource(s).
+ #
# If <tt>map.resources</tt> is called with multiple resources, they all get the same options applied.
#
# Examples:
@@ -478,7 +530,7 @@ def map_resource(entities, options = {}, &block)
map_associations(resource, options)
if block_given?
- with_options(:path_prefix => resource.nesting_path_prefix, :name_prefix => resource.nesting_name_prefix, :namespace => options[:namespace], :shallow => options[:shallow], &block)
+ with_options(options.slice(*INHERITABLE_OPTIONS).merge(:path_prefix => resource.nesting_path_prefix, :name_prefix => resource.nesting_name_prefix), &block)
end
end
end
@@ -495,7 +547,7 @@ def map_singleton_resource(entities, options = {}, &block)
map_associations(resource, options)
if block_given?
- with_options(:path_prefix => resource.nesting_path_prefix, :name_prefix => resource.nesting_name_prefix, :namespace => options[:namespace], :shallow => options[:shallow], &block)
+ with_options(options.slice(*INHERITABLE_OPTIONS).merge(:path_prefix => resource.nesting_path_prefix, :name_prefix => resource.nesting_name_prefix), &block)
end
end
end
@@ -507,7 +559,7 @@ def map_associations(resource, options)
name_prefix = "#{options.delete(:name_prefix)}#{resource.nesting_name_prefix}"
Array(options[:has_one]).each do |association|
- resource(association, :path_prefix => path_prefix, :name_prefix => name_prefix, :namespace => options[:namespace], :shallow => options[:shallow])
+ resource(association, options.slice(*INHERITABLE_OPTIONS).merge(:path_prefix => path_prefix, :name_prefix => name_prefix))
end
end
@@ -522,7 +574,7 @@ def map_has_many_associations(resource, associations, options)
map_has_many_associations(resource, association, options)
end
when Symbol, String
- resources(associations, :path_prefix => resource.nesting_path_prefix, :name_prefix => resource.nesting_name_prefix, :namespace => options[:namespace], :shallow => options[:shallow], :has_many => options[:has_many])
+ resources(associations, options.slice(*INHERITABLE_OPTIONS).merge(:path_prefix => resource.nesting_path_prefix, :name_prefix => resource.nesting_name_prefix, :has_many => options[:has_many]))
else
end
end
@@ -531,41 +583,39 @@ def map_collection_actions(map, resource)
resource.collection_methods.each do |method, actions|
actions.each do |action|
[method].flatten.each do |m|
- action_options = action_options_for(action, resource, m)
- map_named_routes(map, "#{action}_#{resource.name_prefix}#{resource.plural}", "#{resource.path}#{resource.action_separator}#{action}", action_options)
+ map_resource_routes(map, resource, action, "#{resource.path}#{resource.action_separator}#{action}", "#{action}_#{resource.name_prefix}#{resource.plural}", m)
end
end
end
end
def map_default_collection_actions(map, resource)
- index_action_options = action_options_for("index", resource)
index_route_name = "#{resource.name_prefix}#{resource.plural}"
if resource.uncountable?
index_route_name << "_index"
end
- map_named_routes(map, index_route_name, resource.path, index_action_options)
-
- create_action_options = action_options_for("create", resource)
- map_unnamed_routes(map, resource.path, create_action_options)
+ map_resource_routes(map, resource, :index, resource.path, index_route_name)
+ map_resource_routes(map, resource, :create, resource.path)
end
def map_default_singleton_actions(map, resource)
- create_action_options = action_options_for("create", resource)
- map_unnamed_routes(map, resource.path, create_action_options)
+ map_resource_routes(map, resource, :create, resource.path)
end
def map_new_actions(map, resource)
resource.new_methods.each do |method, actions|
actions.each do |action|
- action_options = action_options_for(action, resource, method)
- if action == :new
- map_named_routes(map, "new_#{resource.name_prefix}#{resource.singular}", resource.new_path, action_options)
- else
- map_named_routes(map, "#{action}_new_#{resource.name_prefix}#{resource.singular}", "#{resource.new_path}#{resource.action_separator}#{action}", action_options)
+ route_path = resource.new_path
+ route_name = "new_#{resource.name_prefix}#{resource.singular}"
+
+ unless action == :new
+ route_path = "#{route_path}#{resource.action_separator}#{action}"
+ route_name = "#{action}_#{route_name}"
end
+
+ map_resource_routes(map, resource, action, route_path, route_name, method)
end
end
end
@@ -574,34 +624,32 @@ def map_member_actions(map, resource)
resource.member_methods.each do |method, actions|
actions.each do |action|
[method].flatten.each do |m|
- action_options = action_options_for(action, resource, m)
-
action_path = resource.options[:path_names][action] if resource.options[:path_names].is_a?(Hash)
action_path ||= Base.resources_path_names[action] || action
- map_named_routes(map, "#{action}_#{resource.shallow_name_prefix}#{resource.singular}", "#{resource.member_path}#{resource.action_separator}#{action_path}", action_options)
+ map_resource_routes(map, resource, action, "#{resource.member_path}#{resource.action_separator}#{action_path}", "#{action}_#{resource.shallow_name_prefix}#{resource.singular}", m)
end
end
end
- show_action_options = action_options_for("show", resource)
- map_named_routes(map, "#{resource.shallow_name_prefix}#{resource.singular}", resource.member_path, show_action_options)
-
- update_action_options = action_options_for("update", resource)
- map_unnamed_routes(map, resource.member_path, update_action_options)
-
- destroy_action_options = action_options_for("destroy", resource)
- map_unnamed_routes(map, resource.member_path, destroy_action_options)
+ map_resource_routes(map, resource, :show, resource.member_path, "#{resource.shallow_name_prefix}#{resource.singular}")
+ map_resource_routes(map, resource, :update, resource.member_path)
+ map_resource_routes(map, resource, :destroy, resource.member_path)
end
- def map_unnamed_routes(map, path_without_format, options)
- map.connect(path_without_format, options)
- map.connect("#{path_without_format}.:format", options)
- end
-
- def map_named_routes(map, name, path_without_format, options)
- map.named_route(name, path_without_format, options)
- map.named_route("formatted_#{name}", "#{path_without_format}.:format", options)
+ def map_resource_routes(map, resource, action, route_path, route_name = nil, method = nil)
+ if resource.has_action?(action)
+ action_options = action_options_for(action, resource, method)
+ formatted_route_path = "#{route_path}.:format"
+
+ if route_name
+ map.named_route(route_name, route_path, action_options)
+ map.named_route("formatted_#{route_name}", formatted_route_path, action_options)
+ else
+ map.connect(route_path, action_options)
+ map.connect(formatted_route_path, action_options)
+ end
+ end
end
def add_conditions_for(conditions, method)
View
162 actionpack/test/controller/resources_test.rb
@@ -14,6 +14,8 @@ class LogosController < ResourcesController; end
class AccountsController < ResourcesController; end
class AdminController < ResourcesController; end
+class ProductsController < ResourcesController; end
+class ImagesController < ResourcesController; end
module Backoffice
class ProductsController < ResourcesController; end
@@ -776,6 +778,121 @@ def test_with_path_segment_path_prefix_requirements
end
end
+ def test_resource_has_only_show_action
+ with_routing do |set|
+ set.draw do |map|
+ map.resources :products, :only => :show
+ end
+
+ assert_resource_allowed_routes('products', {}, { :id => '1' }, :show, [:index, :new, :create, :edit, :update, :destroy])
+ assert_resource_allowed_routes('products', { :format => 'xml' }, { :id => '1' }, :show, [:index, :new, :create, :edit, :update, :destroy])
+ end
+ end
+
+ def test_singleton_resource_has_only_show_action
+ with_routing do |set|
+ set.draw do |map|
+ map.resource :account, :only => :show
+ end
+
+ assert_singleton_resource_allowed_routes('accounts', {}, :show, [:index, :new, :create, :edit, :update, :destroy])
+ assert_singleton_resource_allowed_routes('accounts', { :format => 'xml' }, :show, [:index, :new, :create, :edit, :update, :destroy])
+ end
+ end
+
+ def test_resource_does_not_have_destroy_action
+ with_routing do |set|
+ set.draw do |map|
+ map.resources :products, :except => :destroy
+ end
+
+ assert_resource_allowed_routes('products', {}, { :id => '1' }, [:index, :new, :create, :show, :edit, :update], :destroy)
+ assert_resource_allowed_routes('products', { :format => 'xml' }, { :id => '1' }, [:index, :new, :create, :show, :edit, :update], :destroy)
+ end
+ end
+
+ def test_singleton_resource_does_not_have_destroy_action
+ with_routing do |set|
+ set.draw do |map|
+ map.resource :account, :except => :destroy
+ end
+
+ assert_singleton_resource_allowed_routes('accounts', {}, [:new, :create, :show, :edit, :update], :destroy)
+ assert_singleton_resource_allowed_routes('accounts', { :format => 'xml' }, [:new, :create, :show, :edit, :update], :destroy)
+ end
+ end
+
+ def test_resource_has_only_collection_action
+ with_routing do |set|
+ set.draw do |map|
+ map.resources :products, :except => :all, :collection => { :sale => :get }
+ end
+
+ assert_resource_allowed_routes('products', {}, { :id => '1' }, [], [:index, :new, :create, :show, :edit, :update, :destroy])
+ assert_resource_allowed_routes('products', { :format => 'xml' }, { :id => '1' }, [], [:index, :new, :create, :show, :edit, :update, :destroy])
+
+ assert_recognizes({ :controller => 'products', :action => 'sale' }, :path => 'products/sale', :method => :get)
+ assert_recognizes({ :controller => 'products', :action => 'sale', :format => 'xml' }, :path => 'products/sale.xml', :method => :get)
+ end
+ end
+
+ def test_resource_has_only_member_action
+ with_routing do |set|
+ set.draw do |map|
+ map.resources :products, :except => :all, :member => { :preview => :get }
+ end
+
+ assert_resource_allowed_routes('products', {}, { :id => '1' }, [], [:index, :new, :create, :show, :edit, :update, :destroy])
+ assert_resource_allowed_routes('products', { :format => 'xml' }, { :id => '1' }, [], [:index, :new, :create, :show, :edit, :update, :destroy])
+
+ assert_recognizes({ :controller => 'products', :action => 'preview', :id => '1' }, :path => 'products/1/preview', :method => :get)
+ assert_recognizes({ :controller => 'products', :action => 'preview', :id => '1', :format => 'xml' }, :path => 'products/1/preview.xml', :method => :get)
+ end
+ end
+
+ def test_singleton_resource_has_only_member_action
+ with_routing do |set|
+ set.draw do |map|
+ map.resource :account, :except => :all, :member => { :signup => :get }
+ end
+
+ assert_singleton_resource_allowed_routes('accounts', {}, [], [:new, :create, :show, :edit, :update, :destroy])
+ assert_singleton_resource_allowed_routes('accounts', { :format => 'xml' }, [], [:new, :create, :show, :edit, :update, :destroy])
+
+ assert_recognizes({ :controller => 'accounts', :action => 'signup' }, :path => 'account/signup', :method => :get)
+ assert_recognizes({ :controller => 'accounts', :action => 'signup', :format => 'xml' }, :path => 'account/signup.xml', :method => :get)
+ end
+ end
+
+ def test_nested_resource_inherits_only_show_action
+ with_routing do |set|
+ set.draw do |map|
+ map.resources :products, :only => :show do |product|
+ product.resources :images
+ end
+ end
+
+ assert_resource_allowed_routes('images', { :product_id => '1' }, { :id => '2' }, :show, [:index, :new, :create, :edit, :update, :destroy], 'products/1/images')
+ assert_resource_allowed_routes('images', { :product_id => '1', :format => 'xml' }, { :id => '2' }, :show, [:index, :new, :create, :edit, :update, :destroy], 'products/1/images')
+ end
+ end
+
+ def test_nested_resource_has_only_show_and_member_action
+ with_routing do |set|
+ set.draw do |map|
+ map.resources :products, :only => [:index, :show] do |product|
+ product.resources :images, :member => { :thumbnail => :get }, :only => :show
+ end
+ end
+
+ assert_resource_allowed_routes('images', { :product_id => '1' }, { :id => '2' }, :show, [:index, :new, :create, :edit, :update, :destroy], 'products/1/images')
+ assert_resource_allowed_routes('images', { :product_id => '1', :format => 'xml' }, { :id => '2' }, :show, [:index, :new, :create, :edit, :update, :destroy], 'products/1/images')
+
+ assert_recognizes({ :controller => 'images', :action => 'thumbnail', :product_id => '1', :id => '2' }, :path => 'products/1/images/2/thumbnail', :method => :get)
+ assert_recognizes({ :controller => 'images', :action => 'thumbnail', :product_id => '1', :id => '2', :format => 'jpg' }, :path => 'products/1/images/2/thumbnail.jpg', :method => :get)
+ end
+ end
+
protected
def with_restful_routing(*args)
with_routing do |set|
@@ -979,6 +1096,51 @@ def assert_resource_methods(expected, resource, action_method, method)
end
end
+ def assert_resource_allowed_routes(controller, options, shallow_options, allowed, not_allowed, path = controller)
+ shallow_path = "#{path}/#{shallow_options[:id]}"
+ format = options[:format] && ".#{options[:format]}"
+ options.merge!(:controller => controller)
+ shallow_options.merge!(options)
+
+ assert_whether_allowed(allowed, not_allowed, options, 'index', "#{path}#{format}", :get)
+ assert_whether_allowed(allowed, not_allowed, options, 'new', "#{path}/new#{format}", :get)
+ assert_whether_allowed(allowed, not_allowed, options, 'create', "#{path}#{format}", :post)
+ assert_whether_allowed(allowed, not_allowed, shallow_options, 'show', "#{shallow_path}#{format}", :get)
+ assert_whether_allowed(allowed, not_allowed, shallow_options, 'edit', "#{shallow_path}/edit#{format}", :get)
+ assert_whether_allowed(allowed, not_allowed, shallow_options, 'update', "#{shallow_path}#{format}", :put)
+ assert_whether_allowed(allowed, not_allowed, shallow_options, 'destroy', "#{shallow_path}#{format}", :delete)
+ end
+
+ def assert_singleton_resource_allowed_routes(controller, options, allowed, not_allowed, path = controller.singularize)
+ format = options[:format] && ".#{options[:format]}"
+ options.merge!(:controller => controller)
+
+ assert_whether_allowed(allowed, not_allowed, options, 'new', "#{path}/new#{format}", :get)
+ assert_whether_allowed(allowed, not_allowed, options, 'create', "#{path}#{format}", :post)
+ assert_whether_allowed(allowed, not_allowed, options, 'show', "#{path}#{format}", :get)
+ assert_whether_allowed(allowed, not_allowed, options, 'edit', "#{path}/edit#{format}", :get)
+ assert_whether_allowed(allowed, not_allowed, options, 'update', "#{path}#{format}", :put)
+ assert_whether_allowed(allowed, not_allowed, options, 'destroy', "#{path}#{format}", :delete)
+ end
+
+ def assert_whether_allowed(allowed, not_allowed, options, action, path, method)
+ action = action.to_sym
+ options = options.merge(:action => action.to_s)
+ path_options = { :path => path, :method => method }
+
+ if Array(allowed).include?(action)
+ assert_recognizes options, path_options
+ elsif Array(not_allowed).include?(action)
+ assert_not_recognizes options, path_options
+ end
+ end
+
+ def assert_not_recognizes(expected_options, path)
+ assert_raise ActionController::RoutingError, ActionController::MethodNotAllowed, Test::Unit::AssertionFailedError do
+ assert_recognizes(expected_options, path)
+ end
+ end
+
def distinct_routes? (r1, r2)
if r1.conditions == r2.conditions and r1.requirements == r2.requirements then
if r1.segments.collect(&:to_s) == r2.segments.collect(&:to_s) then

16 comments on commit 44a3009

@adkron

woot. I was working on this, but now I can stop.

@samgranieri

This is fantastic! I hope this makes it into 2.2

@joshuaclayton

I would love to see this included in 2.2. Totally awesome.

@adkron

+1 for 2.2 inclusion

@sob

+1 for 2.2 inclusion as well. This is sweet.

@jonnii

+1 for 2.2 inclusion.

@NZKoz
Owner

Guys, this is still the 2.2 branch, it’s going in. ;)

@zargony

nice :)

@joshsusser

I’m all for the feature (been wanting it for ages), but I’d favor holding it to 2.2.1. Maintaining a feature freeze during a release cycle is important for the quality and stability of a release, and relaxing the discipline of a freeze for a shiny object is going to get you in trouble eventually.

@andrzejsliwa

+1 for 2.2 inclusion, great job

@alloy

I have to agree with joshsusser on this.

@NZKoz
Owner

We want it in for 2.2 final, if you think we should do another RC with it included, then we can do that. However it seems relatively harmless.

Pipe up on the ticket if you feel strongly about it. There’s an enhancement to come it seems

@joshsusser

There have probably been enough changes since RC1 to merit an RC2, especially since there have been some feature additions. And I notice there was an issue with removing index and show helpers, which is a good indication this change needs some time to bake.

@sbfaulkner

lol… apparently I was ahead of my time, I was shot down for suggesting this feature – which lead to me adding “shallow” nesting instead… oh well, now we have both

@NZKoz
Owner

@sbfaulkner: heh, yeah. Specific numbers on the memory usage made me change my mind.

@joshsusser: Indeed, this particular changeset is only intended for people looking to cut down on memory usage with thousands of routes. I just tested with my app and it was a tiny tiny difference.

We’ll do an RC2 and keep it ‘frozen’ barring surprises.

@lifo
Collaborator

Adding a blank space.

Please sign in to comment.
Something went wrong with that request. Please try again.