Allow forwarding to the same Plug more than once, as long as it is not a router

Author: josevalim

lib/phoenix/router.ex

@@ -364,7 +364,6 @@ defmodule Phoenix.Router do
   defp prelude(opts) do
     quote do
       Module.register_attribute(__MODULE__, :phoenix_routes, accumulate: true)
-      @phoenix_forwards %{}
       # TODO: Require :helpers to be explicit given
       @phoenix_helpers Keyword.get(unquote(opts), :helpers, true)
 
@@ -541,8 +540,7 @@ defmodule Phoenix.Router do
   @doc false
   defmacro __before_compile__(env) do
     routes = env.module |> Module.get_attribute(:phoenix_routes) |> Enum.reverse()
-    forwards = env.module |> Module.get_attribute(:phoenix_forwards)
-    routes_with_exprs = Enum.map(routes, &{&1, Route.exprs(&1, forwards)})
+    routes_with_exprs = Enum.map(routes, &{&1, Route.exprs(&1)})
 
     helpers =
       if Module.get_attribute(env.module, :phoenix_helpers) do
@@ -578,13 +576,6 @@ defmodule Phoenix.Router do
         end
       end
 
-    forwards =
-      for {plug, script_name} <- forwards do
-        quote do
-          def __forward__(unquote(plug)), do: unquote(script_name)
-        end
-      end
-
     forward_catch_all =
       quote generated: true do
         @doc false
@@ -623,26 +614,32 @@ defmodule Phoenix.Router do
       unquote(verify_catch_all)
       unquote(matches)
       unquote(match_catch_all)
-      unquote(forwards)
       unquote(forward_catch_all)
     end
   end
 
   defp build_verify(path, routes_per_path) do
     routes = Map.get(routes_per_path, path)
+    warn_on_verify? = Enum.all?(routes, & &1.warn_on_verify?)
 
-    forward_plug =
-      Enum.find_value(routes, fn
-        %{kind: :forward, plug: plug} -> plug
-        _ -> nil
-      end)
+    case Enum.find(routes, &(&1.kind == :forward)) do
+      %{metadata: %{forward: forward}, plug: plug} ->
+        quote generated: true do
+          def __forward__(unquote(plug)) do
+            unquote(forward)
+          end
 
-    warn_on_verify? = Enum.all?(routes, & &1.warn_on_verify?)
+          def __verify_route__(unquote(path)) do
+            {{unquote(plug), unquote(forward)}, unquote(warn_on_verify?)}
+          end
+        end
 
-    quote generated: true do
-      def __verify_route__(unquote(path)) do
-        {unquote(forward_plug), unquote(warn_on_verify?)}
-      end
+      _ ->
+        quote generated: true do
+          def __verify_route__(unquote(path)) do
+            {nil, unquote(warn_on_verify?)}
+          end
+        end
     end
   end
 
@@ -1199,16 +1196,25 @@ defmodule Phoenix.Router do
   @doc """
   Forwards a request at the given path to a plug.
 
-  All paths that match the forwarded prefix will be sent to
-  the forwarded plug. This is useful for sharing a router between
+  This is commonly used to forward all subroutes to another Plug.
+  For example:
+
+      forward "/admin", SomeLib.AdminDashboard
+
+  The above will allow `SomeLib.AdminDashboard` to handle `/admin`,
+  `/admin/foo`, `/admin/bar/baz`, and so on. Furthermore,
+  `SomeLib.AdminDashboard` does not to be aware of the prefix it
+  is mounted in. From its point of view, the routes above are simply
+  handled as `/`, `/foo`, and `/bar/baz`.
+
+  A common use case for `forward` is for sharing a router between
   applications or even breaking a big router into smaller ones.
+  However, in other for route generation to route accordingly, you
+  can only forward to a given `Phoenix.Router` once.
+
   The router pipelines will be invoked prior to forwarding the
   connection.
 
-  However, we don't advise forwarding to another endpoint.
-  The reason is that plugs defined by your app and the forwarded
-  endpoint would be invoked twice, which may lead to errors.
-
   ## Examples
 
       scope "/", MyApp do

lib/phoenix/router/route.ex

@@ -11,7 +11,7 @@ defmodule Phoenix.Router.Route do
 
     * `:verb` - the HTTP verb as an atom
     * `:line` - the line the route was defined
-    * `:kind` - the kind of route, one of `:match`, `:forward`
+    * `:kind` - the kind of route, either `:match` or `:forward`
     * `:path` - the normalized path as string
     * `:hosts` - the list of request hosts or host prefixes
     * `:plug` - the plug module
@@ -25,9 +25,22 @@ defmodule Phoenix.Router.Route do
     * `:warn_on_verify?` - whether or not to warn on route verification
   """
 
-  defstruct [:verb, :line, :kind, :path, :hosts, :plug, :plug_opts,
-             :helper, :private, :pipe_through, :assigns, :metadata,
-             :trailing_slash?, :warn_on_verify?]
+  defstruct [
+    :verb,
+    :line,
+    :kind,
+    :path,
+    :hosts,
+    :plug,
+    :plug_opts,
+    :helper,
+    :private,
+    :pipe_through,
+    :assigns,
+    :metadata,
+    :trailing_slash?,
+    :warn_on_verify?
+  ]
 
   @type t :: %Route{}
 
@@ -47,29 +60,71 @@ defmodule Phoenix.Router.Route do
   Receives the verb, path, plug, options and helper
   and returns a `Phoenix.Router.Route` struct.
   """
-  @spec build(non_neg_integer, :match | :forward, atom, String.t, String.t | nil, atom, atom, atom | nil, list(atom), map, map, map, boolean, boolean) :: t
-  def build(line, kind, verb, path, hosts, plug, plug_opts, helper, pipe_through, private, assigns, metadata, trailing_slash?, warn_on_verify?)
+  @spec build(
+          non_neg_integer,
+          :match | :forward,
+          atom,
+          String.t(),
+          String.t() | nil,
+          atom,
+          atom,
+          atom | nil,
+          list(atom),
+          map,
+          map,
+          map,
+          boolean,
+          boolean
+        ) :: t
+  def build(
+        line,
+        kind,
+        verb,
+        path,
+        hosts,
+        plug,
+        plug_opts,
+        helper,
+        pipe_through,
+        private,
+        assigns,
+        metadata,
+        trailing_slash?,
+        warn_on_verify?
+      )
       when is_atom(verb) and is_list(hosts) and
-           is_atom(plug) and (is_binary(helper) or is_nil(helper)) and
-           is_list(pipe_through) and is_map(private) and is_map(assigns) and
-           is_map(metadata) and kind in [:match, :forward] and
-           is_boolean(trailing_slash?) do
-    %Route{kind: kind, verb: verb, path: path, hosts: hosts, private: private,
-           plug: plug, plug_opts: plug_opts, helper: helper,
-           pipe_through: pipe_through, assigns: assigns, line: line, metadata: metadata,
-           trailing_slash?: trailing_slash?, warn_on_verify?: warn_on_verify?}
+             is_atom(plug) and (is_binary(helper) or is_nil(helper)) and
+             is_list(pipe_through) and is_map(private) and is_map(assigns) and
+             is_map(metadata) and kind in [:match, :forward] and
+             is_boolean(trailing_slash?) do
+    %Route{
+      kind: kind,
+      verb: verb,
+      path: path,
+      hosts: hosts,
+      private: private,
+      plug: plug,
+      plug_opts: plug_opts,
+      helper: helper,
+      pipe_through: pipe_through,
+      assigns: assigns,
+      line: line,
+      metadata: metadata,
+      trailing_slash?: trailing_slash?,
+      warn_on_verify?: warn_on_verify?
+    }
   end
 
   @doc """
   Builds the compiled expressions used by the route.
   """
-  def exprs(route, forwards) do
+  def exprs(route) do
     {path, binding} = build_path_and_binding(route)
 
     %{
       path: path,
       binding: binding,
-      dispatch: build_dispatch(route, forwards),
+      dispatch: build_dispatch(route),
       hosts: build_host_match(route.hosts),
       path_params: build_path_params(binding),
       prepare: build_prepare(route),
@@ -78,6 +133,7 @@ defmodule Phoenix.Router.Route do
   end
 
   def build_host_match([]), do: [Plug.Router.Utils.build_host_match(nil)]
+
   def build_host_match([_ | _] = hosts) do
     for host <- hosts, do: Plug.Router.Utils.build_host_match(host)
   end
@@ -91,7 +147,7 @@ defmodule Phoenix.Router.Route do
     {_params, segments} =
       case route.kind do
         :forward -> Plug.Router.Utils.build_path_match(path <> "/*_forward_path_info")
-        :match   -> Plug.Router.Utils.build_path_match(path)
+        :match -> Plug.Router.Utils.build_path_match(path)
       end
 
     rewrite_segments(segments)
@@ -101,7 +157,8 @@ defmodule Phoenix.Router.Route do
   defp rewrite_segments(segments) do
     {segments, {binding, _counter}} =
       Macro.prewalk(segments, {[], 0}, fn
-        {name, _meta, nil}, {binding, counter} when is_atom(name) and name != :_forward_path_info ->
+        {name, _meta, nil}, {binding, counter}
+        when is_atom(name) and name != :_forward_path_info ->
           var = Macro.var(:"arg#{counter}", __MODULE__)
           {var, {[{Atom.to_string(name), var} | binding], counter + 1}}
 
@@ -140,27 +197,32 @@ defmodule Phoenix.Router.Route do
     {[{key, var}], [{key, merge}]}
   end
 
-  defp build_dispatch(%Route{kind: :match, plug: plug, plug_opts: plug_opts}, _forwards) do
+  defp build_dispatch(%Route{kind: :match, plug: plug, plug_opts: plug_opts}) do
     quote do
       {unquote(plug), unquote(Macro.escape(plug_opts))}
     end
   end
 
-  defp build_dispatch(%Route{kind: :forward, plug: plug, plug_opts: plug_opts}, forwards) do
-    segments = Map.fetch!(forwards, plug)
-
+  defp build_dispatch(%Route{
+         kind: :forward,
+         plug: plug,
+         plug_opts: plug_opts,
+         metadata: metadata
+       }) do
     quote do
       {
         Phoenix.Router.Route,
-        {unquote(segments), unquote(plug), unquote(Macro.escape(plug_opts))}
+        {unquote(metadata.forward), unquote(plug), unquote(Macro.escape(plug_opts))}
       }
     end
   end
 
   defp build_params() do
     params = Macro.var(:params, :conn)
     path_params = Macro.var(:path_params, :conn)
-    merge_params = quote(do: Phoenix.Router.Route.merge_params(unquote(params), unquote(path_params)))
+
+    merge_params =
+      quote(do: Phoenix.Router.Route.merge_params(unquote(params), unquote(path_params)))
 
     {
       [{:params, params}],

lib/phoenix/router/scope.ex

@@ -54,9 +54,12 @@ defmodule Phoenix.Router.Scope do
       |> Keyword.get(:metadata, %{})
       |> Map.put(:log, Keyword.get(opts, :log, top.log))
 
-    if kind == :forward do
-      register_forwards(module, path, plug)
-    end
+    metadata =
+      if kind == :forward do
+        Map.put(metadata, :forward, validate_forward!(path, plug))
+      else
+        metadata
+      end
 
     Phoenix.Router.Route.build(
       line,
@@ -76,31 +79,18 @@ defmodule Phoenix.Router.Scope do
     )
   end
 
-  defp register_forwards(module, path, plug) when is_atom(plug) do
-    plug = expand_alias(module, plug)
-    phoenix_forwards = Module.get_attribute(module, :phoenix_forwards)
-
-    path_segments =
-      case Plug.Router.Utils.build_path_match(path) do
-        {[], path_segments} ->
-          if phoenix_forwards[plug] do
-            raise ArgumentError,
-                  "#{inspect(plug)} has already been forwarded to. A module can only be forwarded a single time"
-          end
+  defp validate_forward!(path, plug) when is_atom(plug) do
+    case Plug.Router.Utils.build_path_match(path) do
+      {[], path_segments} ->
+        path_segments
 
-          path_segments
-
-        _ ->
-          raise ArgumentError,
-                "dynamic segment \"#{path}\" not allowed when forwarding. Use a static path instead"
-      end
-
-    phoenix_forwards = Map.put(phoenix_forwards, plug, path_segments)
-    Module.put_attribute(module, :phoenix_forwards, phoenix_forwards)
-    plug
+      _ ->
+        raise ArgumentError,
+              "dynamic segment \"#{path}\" not allowed when forwarding. Use a static path instead"
+    end
   end
 
-  defp register_forwards(_, _, plug) do
+  defp validate_forward!(_, plug) do
     raise ArgumentError, "forward expects a module as the second argument, #{inspect(plug)} given"
   end
 

lib/phoenix/verified_routes.ex

@@ -757,19 +757,21 @@ defmodule Phoenix.VerifiedRoutes do
 
   defp match_route?(router, split_path) when is_list(split_path) do
     case router.__verify_route__(split_path) do
-      {_forward_plug, true = _warn_on_verify?} -> false
-      {nil = _forward_plug, false = _warn_on_verify?} -> true
-      {fwd_plug, false = _warn_on_verify?} -> match_forward_route?(router, fwd_plug, split_path)
-      :error -> false
-    end
-  end
+      {_forward_plug, true = _warn_on_verify?} ->
+        false
 
-  defp match_forward_route?(router, forward_router, split_path) do
-    if function_exported?(forward_router, :__routes__, 0) do
-      script_name = router.__forward__(forward_router)
-      match_route?(forward_router, split_path -- script_name)
-    else
-      true
+      {nil = _forward_plug, false = _warn_on_verify?} ->
+        true
+
+      {{router, script_name}, false = _warn_on_verify?} ->
+        if function_exported?(router, :__routes__, 0) do
+          match_route?(router, split_path -- script_name)
+        else
+          true
+        end
+
+      :error ->
+        false
     end
   end