Fixed construction of get parameters for arrays

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

4 files changed, 70 insertions, 53 deletions

diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index 82b12f3a7b..8356b8ce55 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -397,13 +397,7 @@ module ActionController
         merged = recall.merge(options)
         expire_on = Routing.expiry_hash(options, recall)
     
-        path, keys = generate_path(merged, options, expire_on)
-    
-        # Factor out?
-        extras = {}
-        k = nil
-        keys.each {|k| extras[k] = options[k]} 
-        [path, extras]
+        generate_path(merged, options, expire_on)
       end
       
       def generate_path(merged, options, expire_on)
@@ -590,7 +584,7 @@ module ActionController
       end
 
       def extra_keys(options, recall = {})
-        generate(options.dup, recall).last.keys
+        generate(options.dup, recall).last
       end
     end
 
diff --git a/actionpack/lib/action_controller/url_rewriter.rb b/actionpack/lib/action_controller/url_rewriter.rb
index 8ae6f3e57b..546bd13eb7 100644
--- a/actionpack/lib/action_controller/url_rewriter.rb
+++ b/actionpack/lib/action_controller/url_rewriter.rb
@@ -31,31 +31,27 @@ module ActionController
         rewritten_url
       end
 
-      def rewrite_path(options)
-        options = options.symbolize_keys
-        options.update((options[:params] || {}).symbolize_keys)
+      def rewrite_path(original_options)
+        options = original_options.symbolize_keys
+        options.update(params.symbolize_keys) if (params = options[:params])
         RESERVED_OPTIONS.each {|k| options.delete k}
-        path, extras = Routing::Routes.generate(options, @request)
+        path, extra_keys = Routing::Routes.generate(options, @request) # Warning: Routes will mutate and violate the options hash
 
-        if extras[:overwrite_params]
-          params_copy = @request.parameters.reject { |k,v| %w(controller action).include? k }
-          params_copy.update extras[:overwrite_params]
-          extras.delete(:overwrite_params)
-          extras.update(params_copy)
-        end
-
-        path <<  build_query_string(extras) unless extras.empty?
+        path << build_query_string(original_options.symbolize_keys, extra_keys) unless extra_keys.empty?
         
         path
       end
 
       # Returns a query string with escaped keys and values from the passed hash. If the passed hash contains an "id" it'll
       # be added as a path element instead of a regular parameter pair.
-      def build_query_string(hash)
+      def build_query_string(hash, only_keys = nil)
         elements = []
         query_string = ""
+
+        only_keys ||= hash.keys
         
-        hash.each do |key, value|
+        only_keys.each do |key|
+          value = hash[key] 
           key = CGI.escape key.to_s
           key <<  '[]' if value.class == Array
           value = [ value ] unless value.class == Array
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index cec1b0313d..27433624e9 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -554,13 +554,13 @@ class RouteSetTests < Test::Unit::TestCase
     
     assert_equal({:controller => ::Controllers::Admin::UserController, :action => 'show', :id => '10'}.stringify_keys, rs.recognize_path(%w(admin user show 10)))
     
-    assert_equal ['/admin/user/show/10', {}], rs.generate({:controller => 'admin/user', :action => 'show', :id => 10})
+    assert_equal ['/admin/user/show/10', []], rs.generate({:controller => 'admin/user', :action => 'show', :id => 10})
     
-    assert_equal ['/admin/user/show', {}], rs.generate({:action => 'show'}, {:controller => 'admin/user', :action => 'list', :id => '10'})
-    assert_equal ['/admin/user/list/10', {}], rs.generate({}, {:controller => 'admin/user', :action => 'list', :id => '10'})
+    assert_equal ['/admin/user/show', []], rs.generate({:action => 'show'}, {:controller => 'admin/user', :action => 'list', :id => '10'})
+    assert_equal ['/admin/user/list/10', []], rs.generate({}, {:controller => 'admin/user', :action => 'list', :id => '10'})
 
-    assert_equal ['/admin/stuff', {}], rs.generate({:controller => 'stuff'}, {:controller => 'admin/user', :action => 'list', :id => '10'})
-    assert_equal ['/stuff', {}], rs.generate({:controller => '/stuff'}, {:controller => 'admin/user', :action => 'list', :id => '10'})
+    assert_equal ['/admin/stuff', []], rs.generate({:controller => 'stuff'}, {:controller => 'admin/user', :action => 'list', :id => '10'})
+    assert_equal ['/stuff', []], rs.generate({:controller => '/stuff'}, {:controller => 'admin/user', :action => 'list', :id => '10'})
   end
 
   def test_ignores_leading_slash
@@ -689,7 +689,7 @@ class RouteSetTests < Test::Unit::TestCase
   end
 
   def test_changing_controller
-    assert_equal ['/admin/stuff/show/10', {}], rs.generate(
+    assert_equal ['/admin/stuff/show/10', []], rs.generate(
       {:controller => 'stuff', :action => 'show', :id => 10},
       {:controller => 'admin/user', :action => 'index'}
     )
@@ -730,9 +730,9 @@ class RouteSetTests < Test::Unit::TestCase
       rs.connect ':controller/:action/:id'
     end
 
-    assert_equal ['/page/20', {}], rs.generate({:id => 20}, {:controller => 'pages'})
-    assert_equal ['/page/20', {}], rs.generate(:controller => 'pages', :id => 20, :action => 'show')
-    assert_equal ['/pages/boo', {}], rs.generate(:controller => 'pages', :action => 'boo')
+    assert_equal ['/page/20', []], rs.generate({:id => 20}, {:controller => 'pages'})
+    assert_equal ['/page/20', []], rs.generate(:controller => 'pages', :id => 20, :action => 'show')
+    assert_equal ['/pages/boo', []], rs.generate(:controller => 'pages', :action => 'boo')
   end
 
   def test_route_with_fixnum_default
@@ -741,10 +741,10 @@ class RouteSetTests < Test::Unit::TestCase
       rs.connect ':controller/:action/:id'
     end
 
-    assert_equal ['/page', {}], rs.generate(:controller => 'content', :action => 'show_page')
-    assert_equal ['/page', {}], rs.generate(:controller => 'content', :action => 'show_page', :id => 1)
-    assert_equal ['/page', {}], rs.generate(:controller => 'content', :action => 'show_page', :id => '1')
-    assert_equal ['/page/10', {}], rs.generate(:controller => 'content', :action => 'show_page', :id => 10)
+    assert_equal ['/page', []], rs.generate(:controller => 'content', :action => 'show_page')
+    assert_equal ['/page', []], rs.generate(:controller => 'content', :action => 'show_page', :id => 1)
+    assert_equal ['/page', []], rs.generate(:controller => 'content', :action => 'show_page', :id => '1')
+    assert_equal ['/page/10', []], rs.generate(:controller => 'content', :action => 'show_page', :id => 10)
 
     ctrl = ::Controllers::ContentController
 
@@ -754,7 +754,7 @@ class RouteSetTests < Test::Unit::TestCase
   end
 
   def test_action_expiry
-    assert_equal ['/content', {}], rs.generate({:controller => 'content'}, {:controller => 'content', :action => 'show'})
+    assert_equal ['/content', []], rs.generate({:controller => 'content'}, {:controller => 'content', :action => 'show'})
   end
 
   def test_recognition_with_uppercase_controller_name
@@ -775,8 +775,8 @@ class RouteSetTests < Test::Unit::TestCase
       rs.connect ':controller/:action/:id'
     end
 
-    assert_equal ['/test', {}], rs.generate(:controller => 'post', :action => 'show')
-    assert_equal ['/test', {}], rs.generate(:controller => 'post', :action => 'show', :year => nil)
+    assert_equal ['/test', []], rs.generate(:controller => 'post', :action => 'show')
+    assert_equal ['/test', []], rs.generate(:controller => 'post', :action => 'show', :year => nil)
     
     x = setup_for_named_route
     assert_equal({:controller => '/post', :action => 'show'},
@@ -789,20 +789,20 @@ class RouteSetTests < Test::Unit::TestCase
       rs.connect ':controller/:action/:id'
     end
     
-    assert_equal ['/pages/2005', {}],
+    assert_equal ['/pages/2005', []],
       rs.generate(:controller => 'content', :action => 'list_pages', :year => 2005)
-    assert_equal ['/pages/2005/6', {}],
+    assert_equal ['/pages/2005/6', []],
       rs.generate(:controller => 'content', :action => 'list_pages', :year => 2005, :month => 6)
-    assert_equal ['/pages/2005/6/12', {}],
+    assert_equal ['/pages/2005/6/12', []],
       rs.generate(:controller => 'content', :action => 'list_pages', :year => 2005, :month => 6, :day => 12)
     
-    assert_equal ['/pages/2005/6/4', {}],
+    assert_equal ['/pages/2005/6/4', []],
       rs.generate({:day => 4}, {:controller => 'content', :action => 'list_pages', :year => '2005', :month => '6', :day => '12'})
 
-    assert_equal ['/pages/2005/6', {}],
+    assert_equal ['/pages/2005/6', []],
       rs.generate({:day => nil}, {:controller => 'content', :action => 'list_pages', :year => '2005', :month => '6', :day => '12'})
 
-    assert_equal ['/pages/2005', {}],
+    assert_equal ['/pages/2005', []],
       rs.generate({:day => nil, :month => nil}, {:controller => 'content', :action => 'list_pages', :year => '2005', :month => '6', :day => '12'})
   end
   
@@ -812,8 +812,8 @@ class RouteSetTests < Test::Unit::TestCase
       rs.connect ':controller/:action/:id'
     end
     
-    assert_equal ['/', {}], rs.generate(:controller => 'content', :action => 'index')
-    assert_equal ['/', {}], rs.generate(:controller => 'content')
+    assert_equal ['/', []], rs.generate(:controller => 'content', :action => 'index')
+    assert_equal ['/', []], rs.generate(:controller => 'content')
   end
 
   def test_named_url_with_no_action_specified
@@ -822,8 +822,8 @@ class RouteSetTests < Test::Unit::TestCase
       rs.connect ':controller/:action/:id'
     end
     
-    assert_equal ['/', {}], rs.generate(:controller => 'content', :action => 'index')
-    assert_equal ['/', {}], rs.generate(:controller => 'content')
+    assert_equal ['/', []], rs.generate(:controller => 'content', :action => 'index')
+    assert_equal ['/', []], rs.generate(:controller => 'content')
     
     x = setup_for_named_route
     assert_equal({:controller => '/content', :action => 'index'},
@@ -836,9 +836,9 @@ class RouteSetTests < Test::Unit::TestCase
         rs.root '', hash
         rs.connect ':controller/:action/:id'
       end
-      assert_equal ['/', {}], rs.generate({:action => nil}, {:controller => 'content', :action => 'hello'})
-      assert_equal ['/', {}], rs.generate({:controller => 'content'})
-      assert_equal ['/content/hi', {}], rs.generate({:controller => 'content', :action => 'hi'})
+      assert_equal ['/', []], rs.generate({:action => nil}, {:controller => 'content', :action => 'hello'})
+      assert_equal ['/', []], rs.generate({:controller => 'content'})
+      assert_equal ['/content/hi', []], rs.generate({:controller => 'content', :action => 'hi'})
     end
   end
   
@@ -850,8 +850,8 @@ class RouteSetTests < Test::Unit::TestCase
       rs.connect ':controller/:action/:id'
     end
 
-    assert_equal ['/categories', {}], rs.generate(:controller => 'content', :action => 'categories')
-    assert_equal ['/content/hi', {}], rs.generate({:controller => 'content', :action => 'hi'})
+    assert_equal ['/categories', []], rs.generate(:controller => 'content', :action => 'categories')
+    assert_equal ['/content/hi', []], rs.generate({:controller => 'content', :action => 'hi'})
   end
 
 end
diff --git a/actionpack/test/controller/url_rewriter_tests.rb b/actionpack/test/controller/url_rewriter_tests.rb
new file mode 100644
index 0000000000..392024f03d
--- /dev/null
+++ b/actionpack/test/controller/url_rewriter_tests.rb
@@ -0,0 +1,27 @@
+require File.dirname(__FILE__) + '/../abstract_unit'
+
+class UrlRewriterTests < Test::Unit::TestCase
+  def setup
+    @request = ActionController::TestRequest.new
+    @params = {}
+    @rewriter = ActionController::UrlRewriter.new(@request, @params)
+  end 
+  
+  def test_simple_build_query_string
+    assert_equal '?x=1&y=2', @rewriter.send(:build_query_string, :x => '1', :y => '2')
+  end
+  def test_convert_ints_build_query_string
+    assert_equal '?x=1&y=2', @rewriter.send(:build_query_string, :x => 1, :y => 2)
+  end
+  def test_escape_spaces_build_query_string
+    assert_equal '?x=hello+world&y=goodbye+world', @rewriter.send(:build_query_string, :x => 'hello world', :y => 'goodbye world')
+  end
+  def test_expand_array_build_query_string
+    assert_equal '?x[]=1&x[]=2', @rewriter.send(:build_query_string, :x => [1, 2])
+  end
+
+  def test_escape_spaces_build_query_string_selected_keys
+    assert_equal '?x=hello+world', @rewriter.send(:build_query_string, {:x => 'hello world', :y => 'goodbye world'}, [:x])
+  end
+  
+end