New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Router: Allow route to match all HTTP verbs #977

Closed
tisba opened this Issue Jun 22, 2015 · 15 comments

Comments

Projects
None yet
6 participants
@tisba

tisba commented Jun 22, 2015

While trying to build a simple HTTP echo server to play around with phoenix, I have to write

get "/echo/*rest", ApiController, :echo
post "/echo/*rest", ApiController, :echo
put "/echo/*rest", ApiController, :echo
patch "/echo/*rest", ApiController, :echo
delete "/echo/*rest", ApiController, :echo
options "/echo/*rest", ApiController, :echo
connect "/echo/*rest", ApiController, :echo
trace "/echo/*rest", ApiController, :echo
head "/echo/*rest", ApiController, :echo

I'd like to have something similar to routes generated by resources:

match "/echo/*rest", ApiController, :echo, :all

This could also allow

match "/echo/*rest", ApiController, :echo, only: [:get, :post]
@Gazler

This comment has been minimized.

Show comment
Hide comment
@Gazler

Gazler Jun 22, 2015

Member

👍 for this.

I think this could be implemented pretty easily by doing something like:

  defmacro match(path, controller, action) do
    do_match path, controller, action, [], do: nil
  end

  #...other macro implementations

  def do_match(path, controller, action, options, do: context) do
  for verb <- @http_methods do
    method = verb |> to_string |> String.upcase
      quote do
        var!(add_route, Phoenix.Router).(
          Scope.route(__MODULE__, unquote(method), unquote(path), unquote(controller),
                                  unquote(action), unquote(options))
        )
      end
    end
  end
Member

Gazler commented Jun 22, 2015

👍 for this.

I think this could be implemented pretty easily by doing something like:

  defmacro match(path, controller, action) do
    do_match path, controller, action, [], do: nil
  end

  #...other macro implementations

  def do_match(path, controller, action, options, do: context) do
  for verb <- @http_methods do
    method = verb |> to_string |> String.upcase
      quote do
        var!(add_route, Phoenix.Router).(
          Scope.route(__MODULE__, unquote(method), unquote(path), unquote(controller),
                                  unquote(action), unquote(options))
        )
      end
    end
  end
@chrismccord

This comment has been minimized.

Show comment
Hide comment
@chrismccord

chrismccord Jun 22, 2015

Member

I think an HTTP echo server is about the only usecase where this comes up, so I don't think it's worth introducing. I would prefer for the users to have their routes listed explicitly, as match can introduce bad behaviors. Like @Gazler said you could write a macro to do this in the cases where it is needed. So I'd rather wait until we have a lot of reports of need first. Thanks!

Member

chrismccord commented Jun 22, 2015

I think an HTTP echo server is about the only usecase where this comes up, so I don't think it's worth introducing. I would prefer for the users to have their routes listed explicitly, as match can introduce bad behaviors. Like @Gazler said you could write a macro to do this in the cases where it is needed. So I'd rather wait until we have a lot of reports of need first. Thanks!

@Gazler

This comment has been minimized.

Show comment
Hide comment
@Gazler

Gazler Jun 22, 2015

Member

@chrismccord Isn't this something that could be useful in the same way that a rails engine is? You mount a plug at a particular endpoint and all request for that path get routed to that plug?

As you said, you could provide a macro in your project to do that.

Member

Gazler commented Jun 22, 2015

@chrismccord Isn't this something that could be useful in the same way that a rails engine is? You mount a plug at a particular endpoint and all request for that path get routed to that plug?

As you said, you could provide a macro in your project to do that.

@chrismccord

This comment has been minimized.

Show comment
Hide comment
@chrismccord

chrismccord Jun 22, 2015

Member

You mount a plug at a particular endpoint and all request for that path get routed to that plug?

Yes, but it wouldn't be via match, it would be with a normal plug.

Member

chrismccord commented Jun 22, 2015

You mount a plug at a particular endpoint and all request for that path get routed to that plug?

Yes, but it wouldn't be via match, it would be with a normal plug.

@chrismccord

This comment has been minimized.

Show comment
Hide comment
@chrismccord

chrismccord Jun 22, 2015

Member

@Gazler I see what you mean now. I would like to have a better story around things like this, but I want to keep it separate from match, which is usually abused. This could be with something like a forward macro or similar. I'd like to explore this area once our 1.0 milestones are taken care of.

Member

chrismccord commented Jun 22, 2015

@Gazler I see what you mean now. I would like to have a better story around things like this, but I want to keep it separate from match, which is usually abused. This could be with something like a forward macro or similar. I'd like to explore this area once our 1.0 milestones are taken care of.

@Gazler

This comment has been minimized.

Show comment
Hide comment
@Gazler

Gazler Jun 22, 2015

Member

@chrismccord I encountered this issue when toying with the idea of making something like ActiveAdmin.

I had all of my routes handled in: https://github.com/Gazler/ecto_admin/blob/2fc76fb04af482a0350b4d804b5496a1dea5f677/lib/ecto_admin/resource.ex

But the router in a Phoenix application looked like:

defmodule EctoAdminExample.Router do
  use Phoenix.Router

  pipeline :api do
    plug :accepts, ~w(json)
  end

  scope "/", EctoAdminExample do
    pipe_through :api # Use the default browser stack

    get "/", PageController, :index
  end

  get "/admin*args", EctoAdmin.Router, :dispatch
  post "/admin*args", EctoAdmin.Router, :dispatch
end
Member

Gazler commented Jun 22, 2015

@chrismccord I encountered this issue when toying with the idea of making something like ActiveAdmin.

I had all of my routes handled in: https://github.com/Gazler/ecto_admin/blob/2fc76fb04af482a0350b4d804b5496a1dea5f677/lib/ecto_admin/resource.ex

But the router in a Phoenix application looked like:

defmodule EctoAdminExample.Router do
  use Phoenix.Router

  pipeline :api do
    plug :accepts, ~w(json)
  end

  scope "/", EctoAdminExample do
    pipe_through :api # Use the default browser stack

    get "/", PageController, :index
  end

  get "/admin*args", EctoAdmin.Router, :dispatch
  post "/admin*args", EctoAdmin.Router, :dispatch
end
@tisba

This comment has been minimized.

Show comment
Hide comment
@tisba

tisba Jun 22, 2015

@chrismccord the echo server is more or less just a playground. My goal is to build a comprehensive test endpoint to test my product (load testing as a service). My idea is that I have a target app (possibly using phoenix) whose behavior can be dynamically configured.

tisba commented Jun 22, 2015

@chrismccord the echo server is more or less just a playground. My goal is to build a comprehensive test endpoint to test my product (load testing as a service). My idea is that I have a target app (possibly using phoenix) whose behavior can be dynamically configured.

@christopheradams

This comment has been minimized.

Show comment
Hide comment
@christopheradams

christopheradams Dec 11, 2016

Contributor

I think an HTTP echo server is about the only usecase where this comes up

I have a use case for catch-all path matching that I'd like to mention: creating Webmachine-like Resource plugs to replace Controllers, where the supported HTTP methods are encoded in the Resource module, not the Router.

This is feasible in Plug's Router using the match macro, but in Phoenix you need to generate a route for each method (at compile time).

You mount a plug at a particular endpoint and all request for that path get routed to that plug?

Yes, but it wouldn't be via match, it would be with a normal plug.

It is a match because we want to match on the path and compile a match_route/4 for it; otherwise the router will crash if there's no function head for that path.

I would prefer for the users to have their routes listed explicitly, as match can introduce bad behaviors.

Can you say more about this? My guess is that there's an underlying fear that developers might accidentally put GET and POST requests on the same code path.

Phoenix Router is nice because it doesn't force us to use Controllers (any Plugs will do); it would be even more flexible if it didn't require a verb as part of the route.

So this match all feature is definitely a wontfix?

Contributor

christopheradams commented Dec 11, 2016

I think an HTTP echo server is about the only usecase where this comes up

I have a use case for catch-all path matching that I'd like to mention: creating Webmachine-like Resource plugs to replace Controllers, where the supported HTTP methods are encoded in the Resource module, not the Router.

This is feasible in Plug's Router using the match macro, but in Phoenix you need to generate a route for each method (at compile time).

You mount a plug at a particular endpoint and all request for that path get routed to that plug?

Yes, but it wouldn't be via match, it would be with a normal plug.

It is a match because we want to match on the path and compile a match_route/4 for it; otherwise the router will crash if there's no function head for that path.

I would prefer for the users to have their routes listed explicitly, as match can introduce bad behaviors.

Can you say more about this? My guess is that there's an underlying fear that developers might accidentally put GET and POST requests on the same code path.

Phoenix Router is nice because it doesn't force us to use Controllers (any Plugs will do); it would be even more flexible if it didn't require a verb as part of the route.

So this match all feature is definitely a wontfix?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 11, 2016

Member

@christopheradams forwarding won't work for your use case?

Member

josevalim commented Dec 11, 2016

@christopheradams forwarding won't work for your use case?

@christopheradams

This comment has been minimized.

Show comment
Hide comment
@christopheradams

christopheradams Dec 11, 2016

Contributor

@josevalim Forwarding in Phoenix Router doesn't support dynamic path segments, for one (like we added to Plug). But it is another possible workaround depending.

Contributor

christopheradams commented Dec 11, 2016

@josevalim Forwarding in Phoenix Router doesn't support dynamic path segments, for one (like we added to Plug). But it is another possible workaround depending.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 11, 2016

Member

@christopheradams good point. So I think we should probably support a way for matching on all verbs but it has to be explicit to misuse.

Member

josevalim commented Dec 11, 2016

@christopheradams good point. So I think we should probably support a way for matching on all verbs but it has to be explicit to misuse.

@chrismccord

This comment has been minimized.

Show comment
Hide comment
@chrismccord

chrismccord Dec 24, 2016

Member

@josevalim I just realized we already support this via a special, private verb that we use for forward. This works today to match on all verbs:

    match :*, "/any", SomeController, :any

Shall we doc and expose this publicly? This issue would be resolved then.

Member

chrismccord commented Dec 24, 2016

@josevalim I just realized we already support this via a special, private verb that we use for forward. This works today to match on all verbs:

    match :*, "/any", SomeController, :any

Shall we doc and expose this publicly? This issue would be resolved then.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 24, 2016

Member

@chrismccord sounds good to me!

Member

josevalim commented Dec 24, 2016

@chrismccord sounds good to me!

@christopheradams

This comment has been minimized.

Show comment
Hide comment
@christopheradams

christopheradams Dec 24, 2016

Contributor

Amazing!

Contributor

christopheradams commented Dec 24, 2016

Amazing!

@mukulpanchal

This comment has been minimized.

Show comment
Hide comment
@mukulpanchal

mukulpanchal Oct 27, 2017

@chrismccord - I didn't find the 'match' option in the doc, only learnt about it after seeing this Issue. Is it kept out of the documentation on purpose?

Thank you for exposing "match", helped me a lot

@chrismccord - I didn't find the 'match' option in the doc, only learnt about it after seeing this Issue. Is it kept out of the documentation on purpose?

Thank you for exposing "match", helped me a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment