Permalink
Browse files

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
  • Loading branch information...
1 parent 90d2802 commit f1d8f2af72e21d41efd02488f1c2dcf829e17783 @pixeltrix pixeltrix committed Jan 15, 2013
View
@@ -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:
@@ -45,7 +45,7 @@ def segments
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 @@ def required_parts
@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
@@ -61,8 +61,8 @@ def initialize(set, scope, path, options)
normalize_path!
normalize_options!
normalize_requirements!
- normalize_defaults!
normalize_conditions!
+ normalize_defaults!
end
def to_route
@@ -186,6 +186,13 @@ def normalize_conditions!
@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?
@@ -421,7 +421,7 @@ def build_conditions(current_conditions, path_values)
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
@@ -931,3 +931,34 @@ def test_controller_name
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
@@ -3300,3 +3300,31 @@ def test_regexp_port_constraints
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
@@ -6,18 +6,18 @@ class TestRoute < ActiveSupport::TestCase
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 @@ def test_extras_are_not_included_if_optional_parameter_is_nil
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"}

4 comments on commit f1d8f2a

Owner

pixeltrix replied Jan 15, 2013

@dhh can you check this out to see if it works with your tests - thanks!

Owner

pixeltrix replied Jan 15, 2013

@tenderlove I'm adding the required defaults to an array in the constraints hash which feels like an incredible hack, however I couldn't see anyway of smuggling the information from the mapper to the route without changing the method signature - if you can see a better way please enlighten me.

Owner

dhh replied Jan 15, 2013

This is much improved! We're down from 170+ failures to 12.

But I'm still getting some issues related to it. I'll post them here one by one. The first I traced down is:

resources :projects, defaults: { bucket_type: 'project' } do
  resource :dropbox, only: :show, controller: :dropbox
end

test "show escapes project names" do
  get :show, id: projects(:bcx).id, format: "vcf"
end

ActionController::UrlGenerationError: No route matches {:id=>605816632, :format=>"vcf", :controller=>"dropbox", :action=>"show"}
    test/functional/dropbox_controller_test.rb:18:in `block in <class:DropboxControllerTest>'
Owner

dhh replied Jan 15, 2013

Ah, seems like most of these failures I now have are legit. There really isn't a match. Same with the one I posted here. It should have been project_id.

So 👍, I think we can close this!

Please sign in to comment.