diff --git a/CHANGELOG.md b/CHANGELOG.md index 17cc96aae9..e970448f1f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/phoenix/router.ex b/lib/phoenix/router.ex index e63357bb5b..848dd44846 100644 --- a/lib/phoenix/router.ex +++ b/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 diff --git a/lib/phoenix/router/route.ex b/lib/phoenix/router/route.ex index bc2f1b7999..f7d1cddb67 100644 --- a/lib/phoenix/router/route.ex +++ b/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 diff --git a/test/phoenix/router/route_test.exs b/test/phoenix/router/route_test.exs index 4ade1cb03a..0d7f4810d2 100644 --- a/test/phoenix/router/route_test.exs +++ b/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 diff --git a/test/phoenix/router/routing_test.exs b/test/phoenix/router/routing_test.exs index 5142294df5..859371ddd6 100644 --- a/test/phoenix/router/routing_test.exs +++ b/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())