diff options
| -rw-r--r-- | actionpack/CHANGELOG.md | 16 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/journey/route.rb | 11 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/mapper.rb | 9 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/route_set.rb | 2 | ||||
| -rw-r--r-- | actionpack/test/controller/test_case_test.rb | 31 | ||||
| -rw-r--r-- | actionpack/test/dispatch/routing_test.rb | 28 | ||||
| -rw-r--r-- | actionpack/test/journey/route_test.rb | 13 |
7 files changed, 99 insertions, 11 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 6785e7ffef..4b15c5cb41 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,5 +1,21 @@ ## Rails 4.0.0 (unreleased) ## +* Change the behavior of 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. + + *Andrew White* + * Add support for routing constraints other than Regexp and String. For example this now allows the use of arrays like this: diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb index b08ac4b4eb..063302e0f2 100644 --- a/actionpack/lib/action_dispatch/journey/route.rb +++ b/actionpack/lib/action_dispatch/journey/route.rb @@ -45,7 +45,7 @@ module ActionDispatch end def required_keys - path.required_names.map { |x| x.to_sym } + required_defaults.keys + required_parts + required_defaults.keys end def score(constraints) @@ -79,10 +79,13 @@ module ActionDispatch @required_parts ||= path.required_names.map { |n| n.to_sym } end + def required_default?(key) + (constraints[:required_defaults] || []).include?(key) + end + def required_defaults - @required_defaults ||= begin - matches = parts - @defaults.dup.delete_if { |k,_| matches.include?(k) } + @required_defaults ||= @defaults.dup.delete_if do |k,_| + parts.include?(k) || !required_default?(k) end end diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 43d87e9c0b..6d93f609a6 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -61,8 +61,8 @@ module ActionDispatch normalize_path! normalize_options! normalize_requirements! - normalize_defaults! normalize_conditions! + normalize_defaults! end def to_route @@ -186,6 +186,13 @@ module ActionDispatch @conditions[key] = condition end + @conditions[:required_defaults] = [] + options.each do |key, required_default| + next if segment_keys.include?(key) || IGNORE_OPTIONS.include?(key) + next if Regexp === required_default + @conditions[:required_defaults] << key + end + via_all = options.delete(:via) if options[:via] == :all if !via_all && options[:via].blank? diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 931ad372a8..c72310cca3 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -421,7 +421,7 @@ module ActionDispatch end conditions.keep_if do |k, _| - k == :action || k == :controller || + k == :action || k == :controller || k == :required_defaults || @request_class.public_method_defined?(k) || path_values.include?(k) end end diff --git a/actionpack/test/controller/test_case_test.rb b/actionpack/test/controller/test_case_test.rb index e4d78d58b9..df31338f09 100644 --- a/actionpack/test/controller/test_case_test.rb +++ b/actionpack/test/controller/test_case_test.rb @@ -931,3 +931,34 @@ class AnonymousControllerTest < ActionController::TestCase assert_equal 'anonymous', @response.body end end + +class RoutingDefaultsTest < ActionController::TestCase + def setup + @controller = Class.new(ActionController::Base) do + def post + render :text => request.fullpath + end + + def project + render :text => request.fullpath + end + end.new + + @routes = ActionDispatch::Routing::RouteSet.new.tap do |r| + r.draw do + get '/posts/:id', :to => 'anonymous#post', :bucket_type => 'post' + get '/projects/:id', :to => 'anonymous#project', :defaults => { :bucket_type => 'project' } + end + end + end + + def test_route_option_can_be_passed_via_process + get :post, :id => 1, :bucket_type => 'post' + assert_equal '/posts/1', @response.body + end + + def test_route_default_is_not_required_for_building_request_uri + get :project, :id => 2 + assert_equal '/projects/2', @response.body + end +end diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 2d34a0f6b1..da7474e73c 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3300,3 +3300,31 @@ class TestPortConstraints < ActionDispatch::IntegrationTest assert_response :success end end + +class TestRouteDefaults < ActionDispatch::IntegrationTest + stub_controllers do |routes| + Routes = routes + Routes.draw do + resources :posts, bucket_type: 'post' + resources :projects, defaults: { bucket_type: 'project' } + end + end + + def app + Routes + end + + include Routes.url_helpers + + def test_route_options_are_required_for_url_for + assert_raises(ActionController::UrlGenerationError) do + assert_equal '/posts/1', url_for(controller: 'posts', action: 'show', id: 1, only_path: true) + end + + assert_equal '/posts/1', url_for(controller: 'posts', action: 'show', id: 1, bucket_type: 'post', only_path: true) + end + + def test_route_defaults_are_not_required_for_url_for + assert_equal '/projects/1', url_for(controller: 'projects', action: 'show', id: 1, only_path: true) + end +end diff --git a/actionpack/test/journey/route_test.rb b/actionpack/test/journey/route_test.rb index 78608a5c6b..cbe6284714 100644 --- a/actionpack/test/journey/route_test.rb +++ b/actionpack/test/journey/route_test.rb @@ -6,18 +6,18 @@ module ActionDispatch def test_initialize app = Object.new path = Path::Pattern.new '/:controller(/:action(/:id(.:format)))' - defaults = Object.new + defaults = {} route = Route.new("name", app, path, {}, defaults) assert_equal app, route.app assert_equal path, route.path - assert_equal defaults, route.defaults + assert_same defaults, route.defaults end def test_route_adds_itself_as_memo app = Object.new path = Path::Pattern.new '/:controller(/:action(/:id(.:format)))' - defaults = Object.new + defaults = {} route = Route.new("name", app, path, {}, defaults) route.ast.grep(Nodes::Terminal).each do |node| @@ -82,11 +82,14 @@ module ActionDispatch end def test_score + constraints = {:required_defaults => [:controller, :action]} + defaults = {:controller=>"pages", :action=>"show"} + path = Path::Pattern.new "/page/:id(/:action)(.:format)" - specific = Route.new "name", nil, path, {}, {:controller=>"pages", :action=>"show"} + specific = Route.new "name", nil, path, constraints, defaults path = Path::Pattern.new "/:controller(/:action(/:id))(.:format)" - generic = Route.new "name", nil, path, {} + generic = Route.new "name", nil, path, constraints knowledge = {:id=>20, :controller=>"pages", :action=>"show"} |