Repository: phoenixframework/phoenix (elixir)

Issue:
Defining multiple routes paths on the same controller and action is failing
### Environment

* Elixir version (elixir -v): 1.6.4
* Phoenix version (mix deps): 1.3.2
* Operating system: OS X

### Expected behavior

When defining multiple routes on the same controller and same action with different named parameters like:

```
  get "/products/:foo", ProductController, :show
  get "/products/:foo/:bar", ProductController, :show
  get "/products/:foo/:bar/:rebar", ProductController, :show
```

I would like to be able to call them using the same path name and a different number of arguments.

### Actual behavior

The `product_path` function is clearly redefined multiple times but only the first definition is working properly.

```
iex(1)> MyappWeb.Router.Helpers.product_path
** (UndefinedFunctionError) function MyappWeb.Router.Helpers.product_path/0 is undefined or private. Did you mean one of:

      * product_path/3
      * product_path/4
      * product_path/5
      * product_path/6

    (myapp) MyappWeb.Router.Helpers.product_path()
iex(1)> MyappWeb.Router.Helpers.product_path(MyappWeb.Endpoint, :show, "1")
"/products/1"

iex(2)> MyappWeb.Router.Helpers.product_path(MyappWeb.Endpoint, :show, "1", "2")
** (Protocol.UndefinedError) protocol Enumerable not implemented for "2". This protocol is implemented for: DBConnection.PrepareStream, DBConnection.Stream, Date.Range, Ecto.Adapters.SQL.Stream, File.Stream, Function, GenEvent.Stream, HashDict, HashSet, IO.Stream, List, Map, MapSet, Postgrex.Stream, Range, Scrivener.Page, Stream, Timex.Interval

iex(2)> MyappWeb.Router.Helpers.product_path(MyappWeb.Endpoint, :show, "1", "2", "3")
** (Protocol.UndefinedError) protocol Enumerable not implemented for "3". This protocol is implemented for: DBConnection.PrepareStream, DBConnection.Stream, Date.Range, Ecto.Adapters.SQL.Stream, File.Stream, Function, GenEvent.Stream, HashDict, HashSet, IO.Stream, List, Map, MapSet, Postgrex.Stream, Range, Scrivener.Page, Stream, Timex.Interval
```

If it's not possible to redefine the same route path on a different number of named argument, would it be possible to have a good warning message or a better exception?

In that case, why the path is still correctly redefined?

Thanks.


Pull request:
Fix multiple routes causing errors on unique helper clauses
Closes #2864

Patch:
diff --git a/lib/phoenix/router/helpers.ex b/lib/phoenix/router/helpers.ex
index ad0c9b031..8454ab2a5 100644
--- a/lib/phoenix/router/helpers.ex
+++ b/lib/phoenix/router/helpers.ex
@@ -176,11 +176,11 @@ defmodule Phoenix.Router.Helpers do
       defp to_param(true), do: "true"
       defp to_param(data), do: Phoenix.Param.to_param(data)
 
-      defp segments!(segments, [], _reserved, _opts) do
+      defp segments(segments, [], _reserved, _opts) do
         segments
       end
 
-      defp segments!(segments, query, reserved, _opts) when is_list(query) or is_map(query) do
+      defp segments(segments, query, reserved, _opts) when is_list(query) or is_map(query) do
         dict = for {k, v} <- query,
                not ((k = to_string(k)) in reserved),
                do: {k, v}
@@ -191,21 +191,6 @@ defmodule Phoenix.Router.Helpers do
           o  -> segments <> "?" <> o
         end
       end
-
-      defp segments!(segments, query, reserved, {helper, opts, call_vars}) do
-        arity = length(call_vars) + 3
-
-        raise ArgumentError, """
-        #{__MODULE__}.#{helper}_path/#{arity} called with invalid params.
-        The last argument to this function should be a keyword list or a map.
-        For example:
-
-            Router.#{helper}_path(#{Enum.join(["conn", ":#{opts}" | call_vars], ", ")}, page: 5, per_page: 10)
-
-        It is possible you have called this function without defining the proper
-        number of path segments in your router.
-        """
-      end
     end
 
     Module.create(Module.concat(env.module, Helpers), code,
@@ -236,8 +221,9 @@ defmodule Phoenix.Router.Helpers do
         unquote(:"#{helper}_path")(conn_or_endpoint, unquote(opts), unquote_splicing(vars), [])
       end
 
-      def unquote(:"#{helper}_path")(conn_or_endpoint, unquote(opts), unquote_splicing(vars), params) do
-        path(conn_or_endpoint, segments!(unquote(segs), params, unquote(bins),
+      def unquote(:"#{helper}_path")(conn_or_endpoint, unquote(opts), unquote_splicing(vars), params)
+          when is_list(params) or is_map(params) do
+        path(conn_or_endpoint, segments(unquote(segs), params, unquote(bins),
               {unquote(helper), unquote(opts), unquote(Enum.map(vars, &Macro.to_string/1))}))
       end
 
@@ -245,7 +231,8 @@ defmodule Phoenix.Router.Helpers do
         unquote(:"#{helper}_url")(conn_or_endpoint, unquote(opts), unquote_splicing(vars), [])
       end
 
-      def unquote(:"#{helper}_url")(conn_or_endpoint, unquote(opts), unquote_splicing(vars), params) do
+      def unquote(:"#{helper}_url")(conn_or_endpoint, unquote(opts), unquote_splicing(vars), params)
+          when is_list(params) or is_map(params) do
         url(conn_or_endpoint) <> unquote(:"#{helper}_path")(conn_or_endpoint, unquote(opts), unquote_splicing(vars), params)
       end
     end
@@ -255,7 +242,7 @@ defmodule Phoenix.Router.Helpers do
     routes =
       routes_and_exprs
       |> Enum.map(fn {routes, exprs} -> {routes.opts, Enum.map(exprs.binding, &elem(&1, 0))} end)
-      |> Enum.sort
+      |> Enum.sort()
 
     bindings =
       routes
@@ -270,30 +257,30 @@ defmodule Phoenix.Router.Helpers do
         quote @anno do
           def unquote(:"#{helper}_path")(conn_or_endpoint, action, unquote_splicing(binding)) do
             path(conn_or_endpoint, "/")
-            raise_route_error(unquote(helper), :path, unquote(arity), action)
+            raise_route_error(unquote(helper), :path, unquote(arity), action, [])
           end
 
           def unquote(:"#{helper}_path")(conn_or_endpoint, action, unquote_splicing(binding), params) do
             path(conn_or_endpoint, "/")
-            raise_route_error(unquote(helper), :path, unquote(arity + 1), action)
+            raise_route_error(unquote(helper), :path, unquote(arity + 1), action, params)
           end
 
           def unquote(:"#{helper}_url")(conn_or_endpoint, action, unquote_splicing(binding)) do
             url(conn_or_endpoint)
-            raise_route_error(unquote(helper), :url, unquote(arity), action)
+            raise_route_error(unquote(helper), :url, unquote(arity), action, [])
           end
 
           def unquote(:"#{helper}_url")(conn_or_endpoint, action, unquote_splicing(binding), params) do
             url(conn_or_endpoint)
-            raise_route_error(unquote(helper), :url, unquote(arity + 1), action)
+            raise_route_error(unquote(helper), :url, unquote(arity + 1), action, params)
           end
         end
       end
 
     [quote @anno do
-      defp raise_route_error(unquote(helper), suffix, arity, action) do
+      defp raise_route_error(unquote(helper), suffix, arity, action, params) do
         Phoenix.Router.Helpers.raise_route_error(__MODULE__, "#{unquote(helper)}_#{suffix}",
-                                                 arity, action, unquote(routes))
+                                                 arity, action, unquote(routes), params)
       end
     end | catch_alls]
   end
@@ -301,7 +288,9 @@ defmodule Phoenix.Router.Helpers do
   @doc """
   Callback for generate router catch alls.
   """
-  def raise_route_error(mod, fun, arity, action, routes) do
+  def raise_route_error(mod, fun, arity, action, routes, params)
+      when is_list(params) or is_map(params) do
+
     prelude =
       if Keyword.has_key?(routes, action) do
         "no action #{inspect action} for helper #{inspect mod}.#{fun}/#{arity}"
@@ -317,6 +306,20 @@ defmodule Phoenix.Router.Helpers do
 
     raise ArgumentError, "#{prelude}. The following actions/clauses are supported:\n#{suggestions}"
   end
+  def raise_route_error(mod, fun, arity, action, routes, _params) do
+    call_vars = Keyword.fetch!(routes, action)
+
+    raise ArgumentError, """
+    #{inspect(mod)}.#{fun}/#{arity} called with invalid params.
+    The last argument to this function should be a keyword list or a map.
+    For example:
+
+    #{fun}(#{Enum.join(["conn", ":#{action}" | call_vars], ", ")}, page: 5, per_page: 10)
+
+    It is possible you have called this function without defining the proper
+    number of path segments in your router.
+    """
+  end
 
   @doc """
   Callback for properly encoding parameters in routes.


Test patch:
diff --git a/test/phoenix/router/helpers_test.exs b/test/phoenix/router/helpers_test.exs
index e11e3d248..6d780612c 100644
--- a/test/phoenix/router/helpers_test.exs
+++ b/test/phoenix/router/helpers_test.exs
@@ -8,7 +8,6 @@ defmodule Phoenix.Router.HelpersTest do
 
   test "defhelper with :identifiers" do
     route = build(:match, :get, "/foo/:bar", nil, Hello, :world, "hello_world")
-
     assert extract_defhelper(route, 0) == String.trim """
     def(hello_world_path(conn_or_endpoint, :world, bar)) do
       hello_world_path(conn_or_endpoint, :world, bar, [])
@@ -16,8 +15,8 @@ defmodule Phoenix.Router.HelpersTest do
     """
 
     assert extract_defhelper(route, 1) == String.trim """
-    def(hello_world_path(conn_or_endpoint, :world, bar, params)) do
-      path(conn_or_endpoint, segments!((\"\" <> \"/foo\") <> \"/\" <> URI.encode(to_param(bar), &URI.char_unreserved?/1), params, [\"bar\"], {\"hello_world\", :world, [\"bar\"]}))
+    def(hello_world_path(conn_or_endpoint, :world, bar, params) when is_list(params) or is_map(params)) do
+      path(conn_or_endpoint, segments(("" <> "/foo") <> "/" <> URI.encode(to_param(bar), &URI.char_unreserved?/1), params, ["bar"], {"hello_world", :world, ["bar"]}))
     end
     """
   end
@@ -32,8 +31,8 @@ defmodule Phoenix.Router.HelpersTest do
     """
 
     assert extract_defhelper(route, 1) == String.trim """
-    def(hello_world_path(conn_or_endpoint, :world, bar, params)) do
-      path(conn_or_endpoint, segments!((\"\" <> \"/foo\") <> \"/\" <> Enum.map_join(bar, \"/\", fn s -> URI.encode(s, &URI.char_unreserved?/1) end), params, [\"bar\"], {\"hello_world\", :world, [\"bar\"]}))
+    def(hello_world_path(conn_or_endpoint, :world, bar, params) when is_list(params) or is_map(params)) do
+      path(conn_or_endpoint, segments(("" <> "/foo") <> "/" <> Enum.map_join(bar, "/", fn s -> URI.encode(s, &URI.char_unreserved?/1) end), params, ["bar"], {"hello_world", :world, ["bar"]}))
     end
     """
   end
@@ -81,6 +80,9 @@ defmodule Phoenix.Router.HelpersTest do
     end
 
     get "/", PageController, :root, as: :page
+    get "/products/:id", ProductController, :show
+    get "/products/:id/:sort", ProductController, :show
+    get "/products/:id/:sort/:page", ProductController, :show
   end
 
   # Emulate regular endpoint functions
@@ -138,13 +140,13 @@ defmodule Phoenix.Router.HelpersTest do
   end
 
   test "url helper shows an error if an id is accidentally passed" do
-    error_suggestion = ~r/Router.bottom_path\(conn, :bottom, order, count, page: 5, per_page: 10\)/
+    error_suggestion = ~r/bottom_path\(conn, :bottom, order, count, page: 5, per_page: 10\)/
 
     assert_raise ArgumentError, error_suggestion, fn ->
       Helpers.bottom_path(__MODULE__, :bottom, :asc, 8, {:not, :enumerable})
     end
 
-    error_suggestion = ~r/Router.top_path\(conn, :top, page: 5, per_page: 10\)/
+    error_suggestion = ~r/top_path\(conn, :top, page: 5, per_page: 10\)/
 
     assert_raise ArgumentError, error_suggestion, fn ->
       Helpers.top_path(__MODULE__, :top, "invalid")
@@ -548,4 +550,13 @@ defmodule Phoenix.Router.HelpersTest do
     assert Router.Helpers.post_path(__MODULE__, :show, "my path", foo: "my param") ==
       "/posts/my%20path?foo=my+param"
   end
+
+  test "duplicate helpers with unique arities" do
+    assert Helpers.product_path(__MODULE__, :show, 123) == "/products/123"
+    assert Helpers.product_path(__MODULE__, :show, 123, foo: "bar") == "/products/123?foo=bar"
+    assert Helpers.product_path(__MODULE__, :show, 123, "asc") == "/products/123/asc"
+    assert Helpers.product_path(__MODULE__, :show, 123, "asc", foo: "bar") == "/products/123/asc?foo=bar"
+    assert Helpers.product_path(__MODULE__, :show, 123, "asc", 1) == "/products/123/asc/1"
+    assert Helpers.product_path(__MODULE__, :show, 123, "asc", 1, foo: "bar") == "/products/123/asc/1?foo=bar"
+  end
 end