Skip Rack applications and redirects when generating urls

When generating an unnamed url (i.e. using `url_for` with an options
hash) we should skip anything other than standard Rails routes otherwise
it will match the first mounted application or redirect and generate a
url with query parameters rather than raising an error if the options
hash doesn't match any defined routes.

Fixes #8018

actionpack/CHANGELOG.md

@@ -1,3 +1,10 @@
+*   Skip routes pointing to a redirect or mounted application when generating urls
+    using an options hash as they aren't relevant and generate incorrect urls.
+
+    Fixes #8018
+
+    *Andrew White*
+
 *   Move `MissingHelperError` out of the `ClassMethods` module.
 
     *Yves Senn*

actionpack/lib/action_dispatch/journey/formatter.rb

@@ -18,7 +18,11 @@ module ActionDispatch
 
         match_route(name, constraints) do |route|
           parameterized_parts = extract_parameterized_parts(route, options, recall, parameterize)
-          next if !name && route.requirements.empty? && route.parts.empty?
+
+          # Skip this route unless a name has been provided or it is a
+          # standard Rails route since we can't determine whether an options
+          # hash passed to url_for matches a Rack application or a redirect.
+          next unless name || route.dispatcher?
 
           missing_keys = missing_keys(route, parameterized_parts)
           next unless missing_keys.empty?

actionpack/lib/action_dispatch/journey/route.rb

@@ -16,6 +16,14 @@ module ActionDispatch
         @app         = app
         @path        = path
 
+        # Unwrap any constraints so we can see what's inside for route generation.
+        # This allows the formatter to skip over any mounted applications or redirects
+        # that shouldn't be matched when using a url_for without a route name.
+        while app.is_a?(Routing::Mapper::Constraints) do
+          app = app.app
+        end
+        @dispatcher  = app.is_a?(Routing::RouteSet::Dispatcher)
+
         @constraints = constraints
         @defaults    = defaults
         @required_defaults = nil
@@ -93,6 +101,10 @@ module ActionDispatch
         end
       end
 
+      def dispatcher?
+        @dispatcher
+      end
+
       def matches?(request)
         constraints.all? do |method, value|
           next true unless request.respond_to?(method)

actionpack/test/controller/integration_test.rb

@@ -277,7 +277,7 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest
     end
 
     def redirect
-      redirect_to :action => "get"
+      redirect_to action_url('get')
     end
   end
 
@@ -511,8 +511,8 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest
         end
 
         set.draw do
-          match ':action', :to => controller, :via => [:get, :post]
-          get 'get/:action', :to => controller
+          match ':action', :to => controller, :via => [:get, :post], :as => :action
+          get 'get/:action', :to => controller, :as => :get_action
         end
 
         self.singleton_class.send(:include, set.url_helpers)

actionpack/test/dispatch/routing_test.rb

@@ -3620,3 +3620,56 @@ class TestRouteDefaults < ActionDispatch::IntegrationTest
     assert_equal '/projects/1', url_for(controller: 'projects', action: 'show', id: 1, only_path: true)
   end
 end
+
+class TestRackAppRouteGeneration < ActionDispatch::IntegrationTest
+  stub_controllers do |routes|
+    Routes = routes
+    Routes.draw do
+      rack_app = lambda { |env| [200, { 'Content-Type' => 'text/plain' }, []] }
+      mount rack_app, at: '/account', as: 'account'
+      mount rack_app, at: '/:locale/account', as: 'localized_account'
+    end
+  end
+
+  def app
+    Routes
+  end
+
+  include Routes.url_helpers
+
+  def test_mounted_application_doesnt_match_unnamed_route
+    assert_raise(ActionController::UrlGenerationError) do
+      assert_equal '/account?controller=products', url_for(controller: 'products', action: 'index', only_path: true)
+    end
+
+    assert_raise(ActionController::UrlGenerationError) do
+      assert_equal '/de/account?controller=products', url_for(controller: 'products', action: 'index', :locale => 'de', only_path: true)
+    end
+  end
+end
+
+class TestRedirectRouteGeneration < ActionDispatch::IntegrationTest
+  stub_controllers do |routes|
+    Routes = routes
+    Routes.draw do
+      get '/account', to: redirect('/myaccount'), as: 'account'
+      get '/:locale/account', to: redirect('/%{locale}/myaccount'), as: 'localized_account'
+    end
+  end
+
+  def app
+    Routes
+  end
+
+  include Routes.url_helpers
+
+  def test_redirect_doesnt_match_unnamed_route
+    assert_raise(ActionController::UrlGenerationError) do
+      assert_equal '/account?controller=products', url_for(controller: 'products', action: 'index', only_path: true)
+    end
+
+    assert_raise(ActionController::UrlGenerationError) do
+      assert_equal '/de/account?controller=products', url_for(controller: 'products', action: 'index', :locale => 'de', only_path: true)
+    end
+  end
+end

actionpack/test/journey/router_test.rb

@@ -4,9 +4,13 @@ require 'abstract_unit'
 module ActionDispatch
   module Journey
     class TestRouter < ActiveSupport::TestCase
+      # TODO : clean up routing tests so we don't need this hack
+      class StubDispatcher < Routing::RouteSet::Dispatcher; end
+
       attr_reader :routes
 
       def setup
+        @app       = StubDispatcher.new
         @routes    = Routes.new
         @router    = Router.new(@routes, {})
         @formatter = Formatter.new(@routes)
@@ -306,7 +310,7 @@ module ActionDispatch
 
       def test_nil_path_parts_are_ignored
         path  = Path::Pattern.new "/:controller(/:action(.:format))"
-        @router.routes.add_route nil, path, {}, {}, {}
+        @router.routes.add_route @app, path, {}, {}, {}
 
         params = { :controller => "tasks", :format => nil }
         extras = { :action => 'lol' }
@@ -321,7 +325,7 @@ module ActionDispatch
         str = Router::Strexp.new("/", Hash[params], ['/', '.', '?'], true)
         path  = Path::Pattern.new str
 
-        @router.routes.add_route nil, path, {}, {}, {}
+        @router.routes.add_route @app, path, {}, {}, {}
 
         path, _ = @formatter.generate(:path_info, nil, Hash[params], {})
         assert_equal '/', path
@@ -329,7 +333,7 @@ module ActionDispatch
 
       def test_generate_calls_param_proc
         path  = Path::Pattern.new '/:controller(/:action)'
-        @router.routes.add_route nil, path, {}, {}, {}
+        @router.routes.add_route @app, path, {}, {}, {}
 
         parameterized = []
         params = [ [:controller, "tasks"],
@@ -347,7 +351,7 @@ module ActionDispatch
 
       def test_generate_id
         path  = Path::Pattern.new '/:controller(/:action)'
-        @router.routes.add_route nil, path, {}, {}, {}
+        @router.routes.add_route @app, path, {}, {}, {}
 
         path, params = @formatter.generate(
           :path_info, nil, {:id=>1, :controller=>"tasks", :action=>"show"}, {})
@@ -357,7 +361,7 @@ module ActionDispatch
 
       def test_generate_escapes
         path  = Path::Pattern.new '/:controller(/:action)'
-        @router.routes.add_route nil, path, {}, {}, {}
+        @router.routes.add_route @app, path, {}, {}, {}
 
         path, _ = @formatter.generate(:path_info,
           nil, { :controller        => "tasks",
@@ -368,7 +372,7 @@ module ActionDispatch
 
       def test_generate_extra_params
         path  = Path::Pattern.new '/:controller(/:action)'
-        @router.routes.add_route nil, path, {}, {}, {}
+        @router.routes.add_route @app, path, {}, {}, {}
 
         path, params = @formatter.generate(:path_info,
           nil, { :id                => 1,
@@ -382,7 +386,7 @@ module ActionDispatch
 
       def test_generate_uses_recall_if_needed
         path  = Path::Pattern.new '/:controller(/:action(/:id))'
-        @router.routes.add_route nil, path, {}, {}, {}
+        @router.routes.add_route @app, path, {}, {}, {}
 
         path, params = @formatter.generate(:path_info,
           nil,
@@ -394,7 +398,7 @@ module ActionDispatch
 
       def test_generate_with_name
         path  = Path::Pattern.new '/:controller(/:action)'
-        @router.routes.add_route nil, path, {}, {}, {}
+        @router.routes.add_route @app, path, {}, {}, {}
 
         path, params = @formatter.generate(:path_info,
           "tasks",
@@ -541,7 +545,7 @@ module ActionDispatch
       def add_routes router, paths
         paths.each do |path|
           path  = Path::Pattern.new path
-          router.routes.add_route nil, path, {}, {}, {}
+          router.routes.add_route @app, path, {}, {}, {}
         end
       end