Split routes by verb during compilation

CHANGELOG.md

@@ -1,6 +1,15 @@
 # Changelog for v1.9
 
-Nothing, so far.
+## v1.9.0-dev
+
+### Potential breaking changes
+
+  * [Phoenix.Router] Phoenix will now group your routes per verb during compilation. This improves compilation times for large routers with no performance cost at runtime. However, this change implies that wildcard routes are always matched last, which changes the semantics of dead routes. If you had this code:
+
+      match :*, "/foo"
+      get "/foo"
+
+  The second route would never be matched but it will now be as part of this change. However, note that `get "/foo"` should not exist in the first place: it should be either removed or moved first. We recommend checking for any `match :*` in your router and moving them to the end of the file.
 
 ## v1.8
 

lib/phoenix/router.ex

@@ -457,7 +457,7 @@ defmodule Phoenix.Router do
         %{method: method, path_info: path_info, host: host} = conn = prepare(conn)
         decoded = Enum.map(path_info, &URI.decode/1)
 
-        case __match_route__(decoded, method, host) do
+        case __match_route__(method, decoded, host) do
           {metadata, prepare, pipeline, plug_opts} ->
             Phoenix.Router.__call__(conn, metadata, prepare, pipeline, plug_opts)
 
@@ -494,18 +494,24 @@ defmodule Phoenix.Router do
         Helpers.define(env, routes_with_exprs)
       end
 
+    # Group routes by verb making sure the ones that match all are handled last
     {matches, {pipelines, _}} =
-      Enum.map_reduce(routes_with_exprs, {[], %{}}, &build_match/2)
+      routes_with_exprs
+      |> Enum.group_by(&elem(&1, 0).verb)
+      |> Map.pop(:*, [])
+      |> then(fn {match_routes_exprs, map} ->
+        Map.to_list(map) ++ [{:*, match_routes_exprs}]
+      end)
+      |> Enum.map_reduce({[], %{}}, &build_match_verb/2)
 
     routes_per_path =
-      routes_with_exprs
-      |> Enum.group_by(&elem(&1, 1).path, &elem(&1, 0))
+      Enum.group_by(routes_with_exprs, &elem(&1, 1).path, &elem(&1, 0))
 
     verifies =
       routes_with_exprs
       |> Enum.map(&elem(&1, 1).path)
       |> Enum.uniq()
-      |> Enum.map(&build_verify(&1, routes_per_path))
+      |> Enum.map(&build_verify(&1, routes_per_path, env))
 
     verify_catch_all =
       quote generated: true do
@@ -515,14 +521,6 @@ defmodule Phoenix.Router do
         end
       end
 
-    match_catch_all =
-      quote generated: true do
-        @doc false
-        def __match_route__(_path_info, _verb, _host) do
-          :error
-        end
-      end
-
     forward_catch_all =
       quote generated: true do
         @doc false
@@ -560,16 +558,15 @@ defmodule Phoenix.Router do
       unquote(verifies)
       unquote(verify_catch_all)
       unquote(matches)
-      unquote(match_catch_all)
       unquote(forward_catch_all)
     end
   end
 
-  defp build_verify(path, routes_per_path) do
+  defp build_verify(path, routes_per_path, env) do
     routes = Map.get(routes_per_path, path)
     warn_on_verify? = Enum.all?(routes, & &1.warn_on_verify?)
 
-    case Enum.find(routes, &(&1.kind == :forward)) do
+    case find_forward_and_check_status(routes, nil, nil, env) do
       %{metadata: %{forward: forward}, plug: plug, plug_opts: plug_opts} ->
         quote generated: true do
           def __forward__(unquote(plug)) do
@@ -591,13 +588,92 @@ defmodule Phoenix.Router do
     end
   end
 
-  defp build_match({route, expr}, {acc_pipes, known_pipes}) do
+  defp find_forward_and_check_status(
+         [%{kind: kind, verb: verb} = route | routes],
+         forward,
+         star_route,
+         env
+       ) do
+    forward =
+      if kind == :forward and is_nil(forward) do
+        route
+      else
+        forward
+      end
+
+    star_route =
+      cond do
+        star_route ->
+          IO.warn(
+            "found route matching on #{inspect(route.path)} after match(:*, #{inspect(star_route.path)})",
+            Macro.Env.stacktrace(%{env | line: route.line})
+          )
+
+          star_route
+
+        verb == :* ->
+          route
+
+        true ->
+          nil
+      end
+
+    find_forward_and_check_status(routes, forward, star_route, env)
+  end
+
+  defp find_forward_and_check_status([], forward, _star_route, _env) do
+    forward
+  end
+
+  defp build_match_verb({:*, routes_exprs}, acc) do
+    name = :__match_route_catch_all__
+    {clauses, acc} = Enum.map_reduce(routes_exprs, acc, &build_match_path(name, &1, &2))
+
+    dispatch =
+      quote generated: true do
+        unquote({:__block__, [], clauses})
+
+        defp __match_route_catch_all__(_path, _host) do
+          :error
+        end
+
+        @doc false
+        def __match_route__(_, path, host) do
+          __match_route_catch_all__(path, host)
+        end
+      end
+
+    {dispatch, acc}
+  end
+
+  defp build_match_verb({verb, routes_exprs}, acc) do
+    pattern = verb |> to_string() |> String.upcase()
+    name = :"__match_route_#{verb}__"
+
+    {clauses, acc} = Enum.map_reduce(routes_exprs, acc, &build_match_path(name, &1, &2))
+
+    dispatch =
+      quote generated: true do
+        unquote({:__block__, [], clauses})
+
+        defp unquote(name)(path, host) do
+          __match_route_catch_all__(path, host)
+        end
+
+        def __match_route__(unquote(pattern), path, host) do
+          unquote(name)(path, host)
+        end
+      end
+
+    {dispatch, acc}
+  end
+
+  defp build_match_path(name, {route, expr}, {acc_pipes, known_pipes}) do
     {pipe_name, acc_pipes, known_pipes} = build_match_pipes(route, acc_pipes, known_pipes)
 
     %{
       prepare: prepare,
       dispatch: dispatch,
-      verb_match: verb_match,
       path_params: path_params,
       hosts: hosts,
       path: path
@@ -606,7 +682,7 @@ defmodule Phoenix.Router do
     clauses =
       for host <- hosts do
         quote line: route.line do
-          def __match_route__(unquote(path), unquote(verb_match), unquote(host)) do
+          defp unquote(name)(unquote(path), unquote(host)) do
             {unquote(build_metadata(route, path_params)),
              fn var!(conn, :conn), %{path_params: var!(path_params, :conn)} ->
                unquote(prepare)
@@ -676,28 +752,37 @@ defmodule Phoenix.Router do
   Useful for defining routes not included in the built-in macros.
 
   The catch-all verb, `:*`, may also be used to match all HTTP methods.
+  However, keep in mind that routes with the catch-all verb are always
+  matched last. Therefore it is recommended that all `match :*` routes
+  are defined at the end of the file.
 
   ## Options
 
     * `:as` - configures the named helper. If `nil`, does not generate
       a helper. Has no effect when using verified routes exclusively
+
     * `:alias` - configure if the scope alias should be applied to the route.
-      Defaults to true, disables scoping if false.
+      Defaults to true, disables scoping if false
+
     * `:log` - the level to log the route dispatching under, may be set to false. Defaults to
       `:debug`. Route dispatching contains information about how the route is handled (which controller
       action is called, what parameters are available and which pipelines are used) and is separate from
       the plug level logging. To alter the plug log level, please see
-      https://phoenix.hexdocs.pm/Phoenix.Logger.html#module-dynamic-log-level.
+      https://phoenix.hexdocs.pm/Phoenix.Logger.html#module-dynamic-log-level
+
     * `:private` - a map of private data to merge into the connection
       when a route matches
+
     * `:assigns` - a map of data to merge into the connection when a route matches
+
     * `:metadata` - a map of metadata used by the telemetry events and returned by
       `route_info/4`. The `:mfa` field is used by telemetry to print logs and by the
-      router to emit compile time checks. Custom fields may be added.
-    * `:warn_on_verify` - the boolean for whether matches to this route trigger
-      an unmatched route warning for `Phoenix.VerifiedRoutes`. It is useful to ignore
-      an otherwise catch-all route definition from being matched when verifying routes.
-      Defaults `false`.
+      router to emit compile time checks. Custom fields may be added
+
+    * `:warn_on_verify` - the boolean for whether matches to this route in verified
+      routes should emit a warning, rather than being accepted as verified. It is useful
+      to ignore an otherwise catch-all route definition from being matched when verifying
+      routes. Defaults `false`
 
   ## Examples
 
@@ -1266,7 +1351,7 @@ defmodule Phoenix.Router do
 
   def route_info(router, method, split_path, host) when is_list(split_path) do
     with {metadata, _prepare, _pipeline, {_plug, _opts}} <-
-           router.__match_route__(split_path, method, host) do
+           router.__match_route__(method, split_path, host) do
       Map.delete(metadata, :conn)
     end
   end

lib/phoenix/router/route.ex

@@ -127,8 +127,7 @@ defmodule Phoenix.Router.Route do
       dispatch: build_dispatch(route),
       hosts: build_host_match(route.hosts),
       path_params: build_path_params(binding),
-      prepare: build_prepare(route),
-      verb_match: verb_match(route.verb)
+      prepare: build_prepare(route)
     }
   end
 
@@ -138,9 +137,6 @@ defmodule Phoenix.Router.Route do
     for host <- hosts, do: Plug.Router.Utils.build_host_match(host)
   end
 
-  defp verb_match(:*), do: Macro.var(:_verb, nil)
-  defp verb_match(verb), do: verb |> to_string() |> String.upcase()
-
   defp build_path_params(binding), do: {:%{}, [], binding}
 
   defp build_path_and_binding(%Route{path: path} = route) do

test/phoenix/router/route_test.exs

@@ -64,7 +64,6 @@ defmodule Phoenix.Router.RouteTest do
       )
       |> exprs()
 
-    assert exprs.verb_match == "GET"
     assert exprs.path == ["foo", {:arg0, [], Phoenix.Router.Route}]
     assert exprs.binding == [{"bar", {:arg0, [], Phoenix.Router.Route}}]
     assert Macro.to_string(exprs.hosts) == "[_]"
@@ -154,7 +153,6 @@ defmodule Phoenix.Router.RouteTest do
 
     assert route.verb == :*
     assert route.kind == :match
-    assert exprs(route).verb_match == {:_verb, [], nil}
   end
 
   test "builds a catch-all verb_match for forwarded routes" do
@@ -178,7 +176,6 @@ defmodule Phoenix.Router.RouteTest do
 
     assert route.verb == :*
     assert route.kind == :forward
-    assert exprs(route).verb_match == {:_verb, [], nil}
   end
 
   test "as a plug, it forwards and sets path_info and script_name for target, then resumes" do

test/phoenix/router/routing_test.exs

@@ -39,6 +39,7 @@ defmodule Phoenix.Router.RoutingTest do
 
   defmodule Router do
     use Phoenix.Router
+    import ExUnit.Assertions, except: [trace: 3]
 
     get "/", UserController, :index, as: :users
     get "/users/top", UserController, :top, as: :top
@@ -52,12 +53,10 @@ defmodule Phoenix.Router.RoutingTest do
     get "/static/images/icons/*image", UserController, :image
     get "/exit", UserController, :exit
     get "/halt-controller", UserController, :halt
-
     trace("/trace", UserController, :trace)
     options "/options", UserController, :options
     connect "/connect", UserController, :connect
     match :move, "/move", UserController, :move
-    match :*, "/any", UserController, :any
 
     scope log: :info do
       pipe_through :noop
@@ -224,18 +223,6 @@ defmodule Phoenix.Router.RoutingTest do
     assert conn.resp_body == "users move"
   end
 
-  test "any verb matches" do
-    conn = call(Router, :get, "/any")
-    assert conn.method == "GET"
-    assert conn.status == 200
-    assert conn.resp_body == "users any"
-
-    conn = call(Router, :put, "/any")
-    assert conn.method == "PUT"
-    assert conn.status == 200
-    assert conn.resp_body == "users any"
-  end
-
   test "different verbs with similar paths" do
     conn = call(Router, :post, "/users/fallback")
     assert conn.status == 200
@@ -248,6 +235,23 @@ defmodule Phoenix.Router.RoutingTest do
     assert conn.path_params["id"] == "123"
   end
 
+  describe "warnings" do
+    test "warns on duplicate route after :*" do
+      assert ExUnit.CaptureIO.capture_io(:stderr, fn ->
+               defmodule MatchOverlap do
+                 use Phoenix.Router
+                 import ExUnit.Assertions, except: [trace: 3]
+
+                 match :*, "/foo/:bar", UserController, :index
+                 get "/foo/:baz", UserController, :show
+               end
+
+               conn = call(MatchOverlap, :get, "foo/example")
+               assert conn.resp_body == "users show"
+             end) =~ "found route matching on \"/foo/:baz\" after match(:*, \"/foo/:bar\")"
+    end
+  end
+
   describe "logging" do
     setup do
       Logger.delete_process_level(self())