Fix #recognize inconsistency with #serve

When routing a request, `#find_routes` will iterate through potential
routes for the request and yield them to the passed block. `#serve` and
`#recognize` both used `#find_routes`, but ended up handling things
slightly differently. Both methods would modify the `script_name` and
`path_info` of the request if the current route is not anchored.
However, only `#serve` used `#chomp` and would restore the request's
original `script_name` and `path_info` after each iteration. This
inconsistency can cause issues as `#recognize` can end up not matching
them even though `#serve` would.

This commit fixes the issue by unifying more of the implementations of
`#serve` and `#recognize`. Now, `#find_routes` implements the
mutation/restoration of request params for both `#serve` and
`#recognize` so that their behavior is consistent and request mutation
isn't leaked between iterations.

One test had to be changed because it depended on the request mutation
leaking outside of `#recognize`, so it was updated to make assertions
during `#recognize` instead.

Author: skipkayhil

actionpack/lib/action_dispatch/journey/router.rb

@@ -27,45 +27,20 @@ def eager_load!
       end
 
       def serve(req)
-        find_routes(req) do |match, parameters, route|
-          set_params  = req.path_parameters
-          path_info   = req.path_info
-          script_name = req.script_name
-
-          unless route.path.anchored
-            req.script_name = (script_name.to_s + match.to_s).chomp("/")
-            req.path_info = match.post_match
-            req.path_info = "/" + req.path_info unless req.path_info.start_with? "/"
-          end
-
+        find_routes(req) do |route, parameters|
           req.path_parameters = parameters
           req.route = route
 
           _, headers, _ = response = route.app.serve(req)
 
-          if headers[Constants::X_CASCADE] == "pass"
-            req.script_name     = script_name
-            req.path_info       = path_info
-            req.path_parameters = set_params
-            next
-          end
-
-          return response
+          return response unless headers[Constants::X_CASCADE] == "pass"
         end
 
         [404, { Constants::X_CASCADE => "pass" }, ["Not Found"]]
       end
 
-      def recognize(rails_req)
-        find_routes(rails_req) do |match, parameters, route|
-          unless route.path.anchored
-            rails_req.script_name = match.to_s
-            rails_req.path_info   = match.post_match
-            rails_req.path_info   = "/" + rails_req.path_info unless rails_req.path_info.start_with? "/"
-          end
-
-          yield(route, parameters)
-        end
+      def recognize(rails_req, &block)
+        find_routes(rails_req, &block)
       end
 
       def visualizer
@@ -100,7 +75,10 @@ def filter_routes(path)
         end
 
         def find_routes(req)
-          path_info = req.path_info
+          req_params  = req.path_parameters
+          path_info   = req.path_info
+          script_name = req.script_name
+
           routes = filter_routes(path_info)
 
           custom_routes.each { |r|
@@ -122,7 +100,7 @@ def find_routes(req)
           routes.each do |r|
             match_data = r.path.match(path_info)
 
-            path_parameters = req.path_parameters.merge r.defaults
+            path_parameters = req_params.merge r.defaults
 
             index = 1
             match_data.names.each do |name|
@@ -137,7 +115,21 @@ def find_routes(req)
               end
               index += 1
             end
-            yield [match_data, path_parameters, r]
+
+            if r.path.anchored
+              yield(r, path_parameters)
+            else
+              req.script_name = (script_name.to_s + match_data.to_s).chomp("/")
+              req.path_info = match_data.post_match
+              req.path_info = "/" + req.path_info unless req.path_info.start_with? "/"
+
+              yield(r, path_parameters)
+
+              req.script_name     = script_name
+              req.path_info       = path_info
+            end
+
+            req.path_parameters = req_params
           end
         end
 

actionpack/lib/action_dispatch/routing/route_set.rb

@@ -932,7 +932,6 @@ def recognize_path_with_request(req, path, extras, raise_on_missing: true)
               params[key] = URI::RFC2396_PARSER.unescape(value)
             end
           end
-          original_path_parameters = req.path_parameters
           req.path_parameters = params
           app = route.app
           if app.matches?(req) && app.dispatcher?
@@ -946,8 +945,6 @@ def recognize_path_with_request(req, path, extras, raise_on_missing: true)
           elsif app.matches?(req) && app.engine?
             path_parameters = app.rack_app.routes.recognize_path_with_request(req, path, extras, raise_on_missing: false)
             return path_parameters if path_parameters
-          else
-            req.path_parameters = original_path_parameters
           end
         end
 

actionpack/test/dispatch/routing_assertions_test.rb

@@ -71,6 +71,10 @@ def self.name
       mount root_engine => "/"
 
       get "/shelf/foo", controller: "query_articles", action: "index"
+
+      get "*path", to: "symbols#index", constraints: ->(request) do
+        request.fullpath == "/request_mutated"
+      end
     end
   end
 
@@ -181,6 +185,10 @@ def test_assert_recognizes_continue_to_recognize_after_it_tried_engines
     assert_recognizes({ controller: "query_articles", action: "index" }, "/shelf/foo")
   end
 
+  def test_assert_recognizes_doesnt_mutate_request
+    assert_recognizes({ controller: "symbols", action: "index", path: "request_mutated" }, "/request_mutated")
+  end
+
   def test_assert_routing
     assert_routing("/articles", controller: "articles", action: "index")
   end

actionpack/test/journey/router_test.rb

@@ -147,10 +147,16 @@ def test_recognize_with_unbound_regexp
 
         env = rails_env "PATH_INFO" => "/foo/bar"
 
-        router.recognize(env) { |*_| }
+        recognized = false
+
+        router.recognize(env) do |*_|
+          assert_equal "/foo", env.env["SCRIPT_NAME"]
+          assert_equal "/bar", env.env["PATH_INFO"]
+
+          recognized = true
+        end
 
-        assert_equal "/foo", env.env["SCRIPT_NAME"]
-        assert_equal "/bar", env.env["PATH_INFO"]
+        assert recognized
       end
 
       def test_bound_regexp_keeps_path_info