Permalink
Browse files

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
  • Loading branch information...
1 parent d3bb2e5 commit e768dc694d917cb2e1f88839a8a616fae7f7719d @jamis jamis committed Jun 6, 2006
Showing with 64 additions and 9 deletions.
  1. +29 −9 actionpack/lib/action_controller/routing.rb
  2. +35 −0 actionpack/test/controller/routing_test.rb
@@ -120,10 +120,16 @@ def write_generation
# 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 @@ def generate_extras(options, hash, expire_on = {})
# 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 @@ def build_query_string(hash, only_keys=nil)
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 @@ def matches_controller_and_action?(controller, action)
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 @@ def generate(options, recall = {}, method=:generate)
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)
@@ -837,6 +837,10 @@ def test_build_empty_query_string
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 @@ def test_generate_changes_controller_module
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

0 comments on commit e768dc6

Please sign in to comment.