Improve parameter expiry handling to fix sticky-id issue. Add a more informative Route#to_s method.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@4441 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

2 files changed, 64 insertions, 9 deletions

diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index 2c67df254f..a878e606ec 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -120,10 +120,16 @@ module ActionController
 # puts
         instance_eval method_decl, "generated code (#{__FILE__}:#{__LINE__})"
 
-        method_decl = "def generate(#{args})\nappend_query_string(*generate_raw(options, hash, expire_on))\nend"
+        # expire_on.keys == recall.keys; in other words, the keys in the expire_on hash
+        # are the same as the keys that were recalled from the previous request. Thus,
+        # we can use the expire_on.keys to determine which keys ought to be used to build
+        # the query string. (Never use keys from the recalled request when building the
+        # query string.)
+
+        method_decl = "def generate(#{args})\npath, hash = generate_raw(options, hash, expire_on)\nappend_query_string(path, hash, extra_keys(hash, expire_on))\nend"
         instance_eval method_decl, "generated code (#{__FILE__}:#{__LINE__})"
 
-        method_decl = "def generate_extras(#{args})\npath, hash = generate_raw(options, hash, expire_on)\n[path, hash.keys.map(&:to_sym) - significant_keys]\nend"
+        method_decl = "def generate_extras(#{args})\npath, hash = generate_raw(options, hash, expire_on)\n[path, extra_keys(hash, expire_on)]\nend"
         instance_eval method_decl, "generated code (#{__FILE__}:#{__LINE__})"
       end
   
@@ -212,10 +218,20 @@ module ActionController
 
       # Generate the query string with any extra keys in the hash and append
       # it to the given path, returning the new path.
-      def append_query_string(path, hash)
+      def append_query_string(path, hash, query_keys=nil)
         return nil unless path
-        query = hash.keys.map(&:to_sym) - significant_keys
-        "#{path}#{build_query_string(hash, query)}"
+        query_keys ||= extra_keys(hash)
+        "#{path}#{build_query_string(hash, query_keys)}"
+      end
+
+      # Determine which keys in the given hash are "extra". Extra keys are
+      # those that were not used to generate a particular route. The extra
+      # keys also do not include those recalled from the prior request, nor
+      # do they include any keys that were implied in the route (like a
+      # :controller that is required, but not explicitly used in the text of
+      # the route.)
+      def extra_keys(hash, recall={})
+        (hash || {}).keys.map { |k| k.to_sym } - (recall || {}).keys - significant_keys
       end
 
       # Build a query string from the keys of the given hash. If +only_keys+
@@ -228,7 +244,7 @@ module ActionController
         only_keys ||= hash.keys
         
         only_keys.each do |key|
-          value = hash[key] 
+          value = hash[key] or next
           key = CGI.escape key.to_s
           if value.class == Array
             key <<  '[]'
@@ -301,8 +317,11 @@ module ActionController
       end
 
       def to_s
-        @to_s ||= segments.inject("") { |str,s| str << s.to_s }
-      end
+        @to_s ||= begin
+            segs = segments.inject("") { |str,s| str << s.to_s }
+            "%-6s %-40s %s" % [(conditions[:method] || :any).to_s.upcase, segs, requirements.inspect]
+          end     
+      end     
   
     protected
   
@@ -973,7 +992,8 @@ module ActionController
           action = merged[:action]
 
           raise "Need controller and action!" unless controller && action
-          routes = routes_by_controller[controller][action][merged.keys.sort_by { |x| x.object_id }]
+          # don't use the recalled keys when determining which routes to check
+          routes = routes_by_controller[controller][action][options.keys.sort_by { |x| x.object_id }]
 
           routes.each do |route|
             results = route.send(method, options, merged, expire_on)
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index 51b27a536a..d33cf3c6b3 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -837,6 +837,10 @@ class RouteTest < Test::Unit::TestCase
     assert_equal '', @route.build_query_string({})
   end
 
+  def test_build_query_string_with_nil_value
+    assert_equal '', @route.build_query_string({:x => nil})
+  end
+
   def test_simple_build_query_string
     assert_equal '?x=1&y=2', @route.build_query_string(:x => '1', :y => '2')
   end
@@ -1337,6 +1341,37 @@ class RouteSetTest < Test::Unit::TestCase
     url = set.generate({:controller => "foo/bar", :action => "baz", :id => 7}, current)
     assert_equal "/foo/bar/baz/7", url
   end
+
+  def test_id_is_not_impossibly_sticky
+    set.draw do |map|
+      map.connect 'foo/:number', :controller => "people", :action => "index"
+      map.connect ':controller/:action/:id'
+    end
+
+    url = set.generate({:controller => "people", :action => "index", :number => 3},
+      {:controller => "people", :action => "index", :id => "21"})
+    assert_equal "/foo/3", url
+  end
+
+  def test_id_is_sticky_when_it_ought_to_be
+    set.draw do |map|
+      map.connect ':controller/:id/:action'
+    end
+
+    url = set.generate({:action => "destroy"}, {:controller => "people", :action => "show", :id => "7"})
+    assert_equal "/people/7/destroy", url
+  end
+
+  def test_use_static_path_when_possible
+    set.draw do |map|
+      map.connect 'about', :controller => "welcome", :action => "about"
+      map.connect ':controller/:action/:id'
+    end
+
+    url = set.generate({:controller => "welcome", :action => "about"},
+      {:controller => "welcome", :action => "get", :id => "7"})
+    assert_equal "/about", url
+  end
 end
 
 class RoutingTest < Test::Unit::TestCase