Normalize recall params when the route is not a standard route otherwise :controller and :action may appear in the generated url [#4326 state:resolved]

Signed-off-by: José Valim <jose.valim@gmail.com>

2 files changed, 48 insertions, 3 deletions

diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 97fe137633..7be79d3200 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -185,7 +185,7 @@ module ActionDispatch
           end
       end
 
-      attr_accessor :routes, :named_routes
+      attr_accessor :set, :routes, :named_routes
       attr_accessor :disable_clear_and_finalize, :resources_path_names
       attr_accessor :default_url_options, :request_class
 
@@ -296,6 +296,7 @@ module ActionDispatch
           @extras      = extras
 
           normalize_options!
+          normalize_recall!
           normalize_controller_action_id!
           use_relative_controller!
           controller.sub!(%r{^/}, '') if controller
@@ -336,6 +337,15 @@ module ActionDispatch
           end
         end
 
+        def normalize_recall!
+          # If the target route is not a standard route then remove controller and action
+          # from the options otherwise they will appear in the url parameters
+          if block_or_proc_route_target?
+            recall.delete(:controller) unless segment_keys.include?(:controller)
+            recall.delete(:action) unless segment_keys.include?(:action)
+          end
+        end
+
         # This pulls :controller, :action, and :id out of the recall.
         # The recall key is only used if there is no key in the options
         # or if the key in the options is identical. If any of
@@ -371,7 +381,7 @@ module ActionDispatch
 
         def generate
           error = ActionController::RoutingError.new("No route matches #{options.inspect}")
-          path, params = @set.generate(:path_info, named_route, options, recall, opts)
+          path, params = @set.set.generate(:path_info, named_route, options, recall, opts)
 
           raise error unless path
 
@@ -402,6 +412,19 @@ module ActionDispatch
           return false unless current_controller
           controller.to_param != current_controller.to_param
         end
+
+        private
+          def named_route_exists?
+            named_route && set.named_routes[named_route]
+          end
+
+          def block_or_proc_route_target?
+            named_route_exists? && !set.named_routes[named_route].app.is_a?(Dispatcher)
+          end
+
+          def segment_keys
+            named_route_exists? ? set.named_routes[named_route].segment_keys : []
+          end
       end
 
       # Generate the path indicated by the arguments, and return an array of
@@ -415,7 +438,7 @@ module ActionDispatch
       end
 
       def generate(options, recall = {}, extras = false)
-        Generator.new(options, recall, @set, extras).generate
+        Generator.new(options, recall, self, extras).generate
       end
 
       RESERVED_OPTIONS = [:anchor, :params, :only_path, :host, :protocol, :port, :trailing_slash]
diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb
index 299d6dd5bd..72d4897630 100644
--- a/actionpack/test/template/url_helper_test.rb
+++ b/actionpack/test/template/url_helper_test.rb
@@ -429,6 +429,10 @@ class UrlHelperControllerTest < ActionController::TestCase
 
       map.connect ":controller/:action/:id"
       # match "/:controller(/:action(/:id))"
+
+      match 'url_helper_controller_test/url_helper/normalize_recall_params',
+        :to => UrlHelperController.action(:normalize_recall),
+        :as => :normalize_recall_params
     end
 
     def show_url_for
@@ -447,6 +451,14 @@ class UrlHelperControllerTest < ActionController::TestCase
       render :inline => '<%= url_for(nil) %>'
     end
 
+    def normalize_recall_params
+      render :inline => '<%= normalize_recall_params_path %>'
+    end
+
+    def recall_params_not_changed
+      render :inline => '<%= url_for(:action => :show_url_for) %>'
+    end
+
     def rescue_action(e) raise e end
   end
 
@@ -488,6 +500,16 @@ class UrlHelperControllerTest < ActionController::TestCase
     get :show_named_route, :kind => 'url'
     assert_equal 'http://testtwo.host/url_helper_controller_test/url_helper/show_named_route', @response.body
   end
+
+  def test_recall_params_should_be_normalized_when_using_block_route
+    get :normalize_recall_params
+    assert_equal '/url_helper_controller_test/url_helper/normalize_recall_params', @response.body
+  end
+
+  def test_recall_params_should_not_be_changed_when_using_normal_route
+    get :recall_params_not_changed
+    assert_equal '/url_helper_controller_test/url_helper/show_url_for', @response.body
+  end
 end
 
 class TasksController < ActionController::Base