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

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"}