Merge pull request #15443 from tgxworld/preload_head_routes

Map HEAD requests to GET routes instead of duplicating GET routes.

5 files changed, 80 insertions, 24 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 30986d0a6a..bdb14b69e5 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,11 @@
+*   Avoid duplicating routes for HEAD requests.
+
+    Instead of duplicating the routes, we will first match the HEAD request to
+    HEAD routes. If no match is found, we will then map the HEAD request to
+    GET routes.
+
+    *Guo Xiang Tan*, *Andrew White*
+
 *   Requests that hit `ActionDispatch::Static` can now take advantage
     of gzipped assets on disk. By default a gzip asset will be served if
     the client supports gzip and a compressed file is on disk.
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index f35289253b..c441f7f95b 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -105,6 +105,12 @@ module ActionDispatch
       @request_method ||= check_method(env["REQUEST_METHOD"])
     end
 
+    def request_method=(request_method) #:nodoc:
+      if check_method(request_method)
+        @request_method = env["REQUEST_METHOD"] = request_method
+      end
+    end
+
     # Returns a symbol form of the #request_method
     def request_method_symbol
       HTTP_METHOD_LOOKUP[request_method]
diff --git a/actionpack/lib/action_dispatch/journey/router.rb b/actionpack/lib/action_dispatch/journey/router.rb
index 21817b374c..9131b65380 100644
--- a/actionpack/lib/action_dispatch/journey/router.rb
+++ b/actionpack/lib/action_dispatch/journey/router.rb
@@ -101,11 +101,13 @@ module ActionDispatch
             r.path.match(req.path_info)
           }
 
-          if req.env["REQUEST_METHOD"] === "HEAD"
-            routes.concat get_routes_as_head(routes)
-          end
+          routes =
+            if req.request_method == "HEAD"
+              match_head_routes(routes, req)
+            else
+              match_routes(routes, req)
+            end
 
-          routes.select! { |r| r.matches?(req) }
           routes.sort_by!(&:precedence)
 
           routes.map! { |r|
@@ -118,19 +120,23 @@ module ActionDispatch
           }
         end
 
-        def get_routes_as_head(routes)
-          precedence = (routes.map(&:precedence).max || 0) + 1
-          routes.select { |r|
-            r.verb === "GET" && !(r.verb === "HEAD")
-          }.map! { |r|
-            Route.new(r.name,
-                      r.app,
-                      r.path,
-                      r.conditions.merge(request_method: "HEAD"),
-                      r.defaults).tap do |route|
-                        route.precedence = r.precedence + precedence
-                      end
-          }
+        def match_head_routes(routes, req)
+          head_routes = match_routes(routes, req)
+
+          if head_routes.empty?
+            begin
+              req.request_method = "GET"
+              match_routes(routes, req)
+            ensure
+              req.request_method = "HEAD"
+            end
+          else
+            head_routes
+          end
+        end
+
+        def match_routes(routes, req)
+          routes.select { |r| r.matches?(req) }
         end
     end
   end
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
index 84bd392fd9..00b80c7357 100644
--- a/actionpack/test/dispatch/request_test.rb
+++ b/actionpack/test/dispatch/request_test.rb
@@ -650,6 +650,18 @@ class RequestMethod < BaseRequestTest
     end
   end
 
+  test "allow request method hacking" do
+    request = stub_request('REQUEST_METHOD' => 'POST')
+
+    assert_equal 'POST', request.request_method
+    assert_equal 'POST', request.env["REQUEST_METHOD"]
+
+    request.request_method = 'GET'
+
+    assert_equal 'GET', request.request_method
+    assert_equal 'GET', request.env["REQUEST_METHOD"]
+  end
+
   test "invalid http method raises exception" do
     assert_raise(ActionController::UnknownHttpMethod) do
       stub_request('REQUEST_METHOD' => 'RANDOM_METHOD').request_method
diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb
index 2e7e8e1bea..8e90d4100f 100644
--- a/actionpack/test/journey/router_test.rb
+++ b/actionpack/test/journey/router_test.rb
@@ -503,6 +503,25 @@ module ActionDispatch
         assert called
       end
 
+      def test_recognize_head_route
+        path   = Path::Pattern.from_string "/books(/:action(.:format))"
+        app    = Object.new
+        conditions = { request_method: 'HEAD' }
+        @router.routes.add_route(app, path, conditions, {})
+
+        env = rails_env(
+          'PATH_INFO' => '/books/list.rss',
+          'REQUEST_METHOD' => 'HEAD'
+        )
+
+        called = false
+        @router.recognize(env) do |r, params|
+          called = true
+        end
+
+        assert called
+      end
+
       def test_recognize_head_request_as_get_route
         path   = Path::Pattern.from_string "/books(/:action(.:format))"
         app    = Object.new
@@ -525,19 +544,24 @@ module ActionDispatch
       def test_recognize_cares_about_verbs
         path   = Path::Pattern.from_string "/books(/:action(.:format))"
         app    = Object.new
-        conditions = {
-          :request_method => 'GET'
-        }
+        conditions = { request_method: 'GET' }
         @router.routes.add_route(app, path, conditions, {})
 
+        env = rails_env 'PATH_INFO' => '/books/list.rss',
+                        "REQUEST_METHOD" => "POST"
+
+        called = false
+        @router.recognize(env) do |r, params|
+          called = true
+        end
+
+        assert_not called
+
         conditions = conditions.dup
         conditions[:request_method] = 'POST'
 
         post = @router.routes.add_route(app, path, conditions, {})
 
-        env = rails_env 'PATH_INFO' => '/books/list.rss',
-                        "REQUEST_METHOD"    => "POST"
-
         called = false
         @router.recognize(env) do |r, params|
           assert_equal post, r
@@ -561,7 +585,7 @@ module ActionDispatch
       end
 
       def rails_env env, klass = ActionDispatch::Request
-        klass.new env
+        klass.new(rack_env(env))
       end
 
       def rack_env env