Merge pull request #199 from danschultzer/prevent-duplicate-routes

Filter duplicate routes

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## v1.0.9 (TBA)
 
+* `Pow.Phoenix.Router` will now only add specific routes if there is no matching route already defined
 * Removed call to `Pow.Ecto.Context.repo/1`
 * Fixed bug with exception raised in `Pow.Ecto.Schema.normalize_user_id_field_value/1` when calling `Pow.Ecto.Context.get_by/2` with a non binary user id
 * Fixed bug with exception raised in `Pow.Ecto.Schema.normalize_user_id_field_value/1` when calling `Pow.Ecto.Context.authenticate/2` with a non binary user id

lib/extensions/email_confirmation/phoenix/router.ex

@@ -2,9 +2,11 @@ defmodule PowEmailConfirmation.Phoenix.Router do
   @moduledoc false
   use Pow.Extension.Phoenix.Router.Base
 
+  alias Pow.Phoenix.Router
+
   defmacro routes(_config) do
     quote location: :keep do
-      resources "/confirm-email", ConfirmationController, only: [:show]
+      Router.pow_resources "/confirm-email", ConfirmationController, only: [:show]
     end
   end
 end

lib/extensions/invitation/phoenix/router.ex

@@ -2,9 +2,11 @@ defmodule PowInvitation.Phoenix.Router do
   @moduledoc false
   use Pow.Extension.Phoenix.Router.Base
 
+  alias Pow.Phoenix.Router
+
   defmacro routes(_config) do
     quote location: :keep do
-      resources "/invitations", InvitationController, only: [:new, :create, :show, :edit, :update]
+      Router.pow_resources "/invitations", InvitationController, only: [:new, :create, :show, :edit, :update]
     end
   end
 end

lib/extensions/reset_password/phoenix/router.ex

@@ -2,10 +2,12 @@ defmodule PowResetPassword.Phoenix.Router do
   @moduledoc false
   use Pow.Extension.Phoenix.Router.Base
 
+  alias Pow.Phoenix.Router
+
   defmacro routes(_config) do
     quote location: :keep do
-      resources "/reset-password", ResetPasswordController, only: [:new, :create, :update]
-      get "/reset-password/:id", ResetPasswordController, :edit
+      Router.pow_resources "/reset-password", ResetPasswordController, only: [:new, :create, :update]
+      Router.pow_route :get, "/reset-password/:id", ResetPasswordController, :edit
     end
   end
 end

lib/pow/phoenix/router.ex

@@ -2,6 +2,14 @@ defmodule Pow.Phoenix.Router do
   @moduledoc """
   Handles Phoenix routing for Pow.
 
+  Resources are build with `pow_resources/3` and individual routes are build
+  with `pow_route/5`. The Pow routes will be filtered if a route has already
+  been defined with the same action and router helper alias. This makes it easy
+  to override pow routes with no conflicts.
+
+  The scope will be validated to ensure that there is no aliases. An exception
+  will be raised if an alias was defined in any scope around the pow routes.
+
   ## Usage
 
   Configure `lib/my_project_web/router.ex` the following way:
@@ -36,9 +44,10 @@ defmodule Pow.Phoenix.Router do
   end
 
   @doc """
-  Pow router macro.
+  Pow routes macro.
 
-  Use this macro to define the Pow routes.
+  Use this macro to define the Pow routes. This will call
+  `pow_session_routes/0` and `pow_registration_routes/0`.
 
   ## Example
 
@@ -68,7 +77,7 @@ defmodule Pow.Phoenix.Router do
   defmacro pow_session_routes do
     quote location: :keep do
       pow_scope do
-        resources "/session", SessionController, singleton: true, only: [:new, :create, :delete]
+        unquote(__MODULE__).pow_resources "/session", SessionController, singleton: true, only: [:new, :create, :delete]
       end
     end
   end
@@ -77,7 +86,43 @@ defmodule Pow.Phoenix.Router do
   defmacro pow_registration_routes do
     quote location: :keep do
       pow_scope do
-        resources "/registration", RegistrationController, singleton: true, only: [:new, :create, :edit, :update, :delete]
+        unquote(__MODULE__).pow_resources "/registration", RegistrationController, singleton: true, only: [:new, :create, :edit, :update, :delete]
+      end
+    end
+  end
+
+  @doc false
+  defmacro pow_resources(path, controller, opts) do
+    quote location: :keep do
+      opts = unquote(__MODULE__).__filter_resource_actions__(@phoenix_routes, __ENV__.line, __ENV__.module, unquote(path), unquote(controller), unquote(opts))
+
+      resources unquote(path), unquote(controller), opts
+    end
+  end
+
+  @doc false
+  def __filter_resource_actions__(phoenix_routes, line, module, path, controller, options) do
+    resource     = Phoenix.Router.Resource.build(path, controller, options)
+    action_verbs = [index: :get, new: :get, create: :post, show: :get, edit: :get, update: :patch]
+    only         = Enum.reject(resource.actions, &__route_defined__(phoenix_routes, line, module, action_verbs[&1], path, controller, &1, options))
+
+    Keyword.put(options, :only, only)
+  end
+
+  @doc false
+  def __route_defined__(phoenix_routes, line, module, verb, path, plug, plug_opts, options) do
+    matching_params =
+      line
+      |> Phoenix.Router.Scope.route(module, :match, verb, path, plug, plug_opts, options)
+      |> Map.take([:opts, :helper])
+
+    Enum.any?(phoenix_routes, &Map.take(&1, [:opts, :helper]) == matching_params)
+  end
+
+  defmacro pow_route(verb, path, plug, plug_opts, options \\ []) do
+    quote location: :keep do
+      unless unquote(__MODULE__).__route_defined__(@phoenix_routes, __ENV__.line, __ENV__.module, unquote(verb), unquote(path), unquote(plug), unquote(plug_opts), unquote(options)) do
+        unquote(verb)(unquote(path), unquote(plug), unquote(plug_opts), unquote(options))
       end
     end
   end

test/pow/extension/phoenix/router_test.exs

@@ -1,9 +1,11 @@
 defmodule Pow.Test.Extension.Phoenix.Router.Phoenix.Router do
   use Pow.Extension.Phoenix.Router.Base
 
+  alias Pow.Phoenix.Router
+
   defmacro routes(_config) do
     quote location: :keep do
-      resources "/test", TestController, only: [:new, :create, :edit, :update]
+      Router.pow_resources "/test", TestController, only: [:new, :create, :edit, :update]
     end
   end
 end
@@ -14,10 +16,16 @@ defmodule Pow.Test.Extension.Phoenix.Router do
   use Pow.Extension.Phoenix.Router,
     extensions: [Pow.Test.Extension.Phoenix.Router]
 
+  scope "/", as: "pow_test_extension_phoenix_router" do
+    get "/test/:id/overidden", TestController, :edit
+  end
+
   scope "/" do
     pow_routes()
     pow_extension_routes()
   end
+
+  def phoenix_routes(), do: @phoenix_routes
 end
 
 module_raised_with =
@@ -51,11 +59,16 @@ defmodule Pow.Extension.Phoenix.RouterTest do
 
   test "has routes" do
     assert unquote(Routes.pow_session_path(@conn, :new)) == "/session/new"
-    assert unquote(Routes.pow_test_extension_phoenix_router_test_path(@conn, :new)) = "/test/new"
+    assert unquote(Routes.pow_test_extension_phoenix_router_test_path(@conn, :new)) == "/test/new"
   end
 
   test "validates no aliases" do
     assert unquote(module_raised_with) =~ "Pow routes should not be defined inside scopes with aliases: Test"
     assert unquote(module_raised_with) =~ "scope \"/\", Test do"
   end
+
+  test "can override routes" do
+    assert unquote(Routes.pow_test_extension_phoenix_router_test_path(@conn, :edit, 1)) == "/test/1/overidden"
+    assert Enum.count(Pow.Test.Extension.Phoenix.Router.phoenix_routes(), &(&1.plug == TestController && &1.opts == :edit)) == 1
+  end
 end

test/pow/phoenix/router_test.exs

@@ -15,12 +15,39 @@ module_raised_with =
     _ -> raise "Scope with alias didn't throw any error"
   end
 
+defmodule Pow.Test.Phoenix.OverriddenRouteRouter do
+  @moduledoc false
+
+  use Phoenix.Router
+  use Pow.Phoenix.Router
+
+  scope "/", Pow.Phoenix, as: "pow" do
+    get "/registration/overidden", RegistrationController, :new
+  end
+
+  scope "/" do
+    pow_routes()
+  end
+
+  def phoenix_routes(), do: @phoenix_routes
+end
+
 defmodule Pow.Phoenix.RouterTest do
   use ExUnit.Case
   doctest Pow.Phoenix.Router
 
+  alias Pow.Test.Phoenix.OverriddenRouteRouter.Helpers, as: OverriddenRoutes
+  alias Phoenix.ConnTest
+
+  @conn ConnTest.build_conn()
+
   test "validates no aliases" do
     assert unquote(module_raised_with) =~ "Pow routes should not be defined inside scopes with aliases: Test"
     assert unquote(module_raised_with) =~ "scope \"/\", Test do"
   end
+
+  test "can override routes" do
+    assert unquote(OverriddenRoutes.pow_registration_path(@conn, :new)) == "/registration/overidden"
+    assert Enum.count(Pow.Test.Phoenix.OverriddenRouteRouter.phoenix_routes(), &(&1.plug == Pow.Phoenix.RegistrationController && &1.opts == :new)) == 1
+  end
 end