Merge pull request rails#54550 from skipkayhil/hm-fix-recognize-mutating-request

Fix `#recognize` inconsistency with `#serve`

Author: byroot

actionpack/lib/action_dispatch/journey/router.rb

@@ -27,44 +27,74 @@ 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
-
+        recognize(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? "/"
+      def recognize(req, &block)
+        req_params  = req.path_parameters
+        path_info   = req.path_info
+        script_name = req.script_name
+
+        routes = filter_routes(path_info)
+
+        custom_routes.each { |r|
+          routes << r if r.path.match?(path_info)
+        }
+
+        if req.head?
+          routes = match_head_routes(routes, req)
+        else
+          routes.select! { |r| r.matches?(req) }
+        end
+
+        if routes.size > 1
+          routes.sort! do |a, b|
+            a.precedence <=> b.precedence
+          end
+        end
+
+        routes.each do |r|
+          match_data = r.path.match(path_info)
+
+          path_parameters = req_params.merge r.defaults
+
+          index = 1
+          match_data.names.each do |name|
+            if val = match_data[index]
+              val = if val.include?("%")
+                CGI.unescapeURIComponent(val)
+              else
+                val
+              end
+              val.force_encoding(::Encoding::UTF_8)
+              path_parameters[name.to_sym] = val
+            end
+            index += 1
+          end
+
+          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
 
-          yield(route, parameters)
+          req.path_parameters = req_params
         end
       end
 
@@ -99,48 +129,6 @@ def filter_routes(path)
           simulator.memos(path) { [] }
         end
 
-        def find_routes(req)
-          path_info = req.path_info
-          routes = filter_routes(path_info)
-
-          custom_routes.each { |r|
-            routes << r if r.path.match?(path_info)
-          }
-
-          if req.head?
-            routes = match_head_routes(routes, req)
-          else
-            routes.select! { |r| r.matches?(req) }
-          end
-
-          if routes.size > 1
-            routes.sort! do |a, b|
-              a.precedence <=> b.precedence
-            end
-          end
-
-          routes.each do |r|
-            match_data = r.path.match(path_info)
-
-            path_parameters = req.path_parameters.merge r.defaults
-
-            index = 1
-            match_data.names.each do |name|
-              if val = match_data[index]
-                val = if val.include?("%")
-                  CGI.unescapeURIComponent(val)
-                else
-                  val
-                end
-                val.force_encoding(::Encoding::UTF_8)
-                path_parameters[name.to_sym] = val
-              end
-              index += 1
-            end
-            yield [match_data, path_parameters, r]
-          end
-        end
-
         def match_head_routes(routes, req)
           head_routes = routes.select { |r| r.requires_matching_verb? && r.matches?(req) }
           return head_routes unless head_routes.empty?

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