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

Add router path to Plug.Conn #630

Merged
merged 1 commit into from Dec 9, 2017

Conversation

Projects
None yet
4 participants
@mgartner
Copy link
Contributor

mgartner commented Nov 27, 2017

In certain situations, it's very valuable to be able to access the matched router path of a request. Some examples include:

  • Rate limiting—you want to rate limit on a route /hello/:name, not a variation of the route /hello/robert
  • Metrics—you want to track metrics for an endpoint, but not necessarily each individual variation of the endpoint

I think that access to the matched router string is critically important information in these examples, and most likely others. I've added a function Plug.Router.match_path which returns the string path that the request was matched to.

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Nov 27, 2017

@mgartner

This comment has been minimized.

Copy link
Contributor

mgartner commented Nov 27, 2017

@josevalim in this PR I added a new field, path, instead of overriding request_path. I agree that request_path should definitely not change.

I can definitely move this to private—it looks like plug_route is a function being set in the private field of the conn. If so, I'm curious why you think this belongs in private, for future reference.

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Nov 27, 2017

@mgartner

This comment has been minimized.

Copy link
Contributor

mgartner commented Nov 27, 2017

I'm confused by your comments. This PR adds the field to the Plug.Conn struct, and the build is passing. Why would this error? Do you mean this cannot be accepted for compatibility with other libraries?

I'm not proposing adding any framework-specific data, only Plug router data.

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Nov 27, 2017

Sorry, when reviewing I missed the part of where a field was actually added to the struct.

I don't think we should be adding new fields to conn because plug needs them. Private is exactly for that.

@mgartner

This comment has been minimized.

Copy link
Contributor

mgartner commented Nov 27, 2017

Ok, I can make that change. How would you want to change the docs to reflect the new philosophy of the private field? The docs currently say:

These fields are reserved for libraries/framework usage.

If we add path to the private field in Plug, then private is no longer reserved for frameworks.

@mgartner mgartner force-pushed the mgartner:include-path-in-conn branch 2 times, most recently from e201949 to d23eb9b Nov 27, 2017

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Nov 27, 2017

@mgartner

This comment has been minimized.

Copy link
Contributor

mgartner commented Nov 27, 2017

Sounds good. Updated!

@mgartner mgartner force-pushed the mgartner:include-path-in-conn branch 2 times, most recently from a10c961 to c044e0c Nov 28, 2017

@michalmuskala

This comment has been minimized.

Copy link
Contributor

michalmuskala commented Nov 28, 2017

Should there be a function in Plug.Router for retrieving this path?

@@ -452,7 +452,8 @@ defmodule Plug.Router do
conn = update_in unquote(conn).params, merge_params
conn = update_in conn.path_params, merge_params

Plug.Conn.put_private(conn, :plug_route, fn var!(conn) -> unquote(body) end)
conn = Plug.Conn.put_private(conn, :plug_route, fn var!(conn) -> unquote(body) end)
Plug.Conn.put_private(conn, :plug_path, unquote(path))

This comment has been minimized.

@josevalim

josevalim Nov 28, 2017

Member

I believe we can store both on a single private field in Plug.Conn and then we provide a function for reading it out? Since you are not supposed to access a private field set by a library?

This comment has been minimized.

@mgartner

mgartner Nov 28, 2017

Contributor

Well the whole point of this field is to be able to access it in other plugs, like a plug for metrics or rate limiting, for example. That was my reasoning for initially leaving it out of private, since private to me meant information set and only used by a framework/library. My goal is instead to provide this information to other libraries, much like request_path is available to them on the Plug.Conn.

Can you explain more about how you are thinking this field should be stored? I'm happy to make any changes.

This comment has been minimized.

@josevalim

josevalim Nov 30, 2017

Member

It should be stored like this:

conn = Plug.Conn.put_private(conn, :plug_route, {unquote(path), fn var!(conn) -> unquote(body) end})

and then you should have a function such as Plug.Router.match_route(conn) that access the private field and returns only the path (the first element of the tuple).

This comment has been minimized.

@mgartner

mgartner Dec 4, 2017

Contributor

Thanks for the explanation! I've made the changes you suggested. Let me know if this is what you had in mind, or if there are any improvements I can make.

@mgartner

This comment has been minimized.

Copy link
Contributor

mgartner commented Nov 29, 2017

Any more thoughts on the best place to store this information?

@mgartner mgartner force-pushed the mgartner:include-path-in-conn branch from c044e0c to bf3ffde Dec 4, 2017

@@ -213,7 +213,8 @@ defmodule Plug.Router do

@doc false
def dispatch(%Plug.Conn{assigns: assigns} = conn, _opts) do
Map.get(conn.private, :plug_route).(conn)
{_path, fun} = Map.get(conn.private, :plug_route)

This comment has been minimized.

@whatyouhide

whatyouhide Dec 4, 2017

Member

Since :plug_route should be here, I think Map.get/2 is not the optimal choice. If it returns nil, you'll have a nasty match error (which is fine since we expect the field there), but we don't communicate this requirement in this code. I'd go with either

{_path, fun} = Map.fetch!(conn.private, :plug_route)

or with

%{plug_route: {_path, fun}} = conn.private

(leaning on the first for my personal tastes)

"""
@spec match_path(Plug.Conn.t) :: String.t
def match_path(%Plug.Conn{} = conn) do
{path, _fun} = Map.get(conn.private, :plug_route)

This comment has been minimized.

@whatyouhide

whatyouhide Dec 4, 2017

Member

Same as above with Map.fetch!/2/pattern matching.

@mgartner mgartner force-pushed the mgartner:include-path-in-conn branch from bf3ffde to 71089c3 Dec 4, 2017

@mgartner

This comment has been minimized.

Copy link
Contributor

mgartner commented Dec 4, 2017

thanks @whatyouhide! updated.

@josevalim josevalim merged commit e7b20b3 into elixir-plug:master Dec 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mgartner mgartner deleted the mgartner:include-path-in-conn branch Dec 11, 2017

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