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
This commit is contained in:
parent
d3bb2e5236
commit
e768dc694d
@ -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
|
||||
|
Loading…
Reference in New Issue
Block a user