`fetch_query_params/2` discards previous `conn.params` with path parameters populated by `Phoenix.Router`

[ymtszw]

conn.params is stated as:

* `params` - the request params, the result of merging the `:path_params` on top of
  `:body_params` on top of `:query_params`

So I assumed that if we use plug :fetch_query_params with Phoenix.Router (WITHOUT plug Plug.Parser) we should get path parameters AND query parameters merged, in conn.params (or 2nd argument in action callback).

Code example:

defmodule MyApp.Router do
  use Phoenix.Router # NOT Plug.Router

  post "/foo/:id", MyApp.FooController, :create
end

defmodule MyApp.FooController do
  use Phoenix.Controller

  plug :fetch_query_params

  def create(conn, params) do
    IO.inspect(params)
    send_resp(conn, 204, "")
  end
end

# in test
conn = post(conn, "/foo/1?query=true", "")
# inspection should result in:
# => %{"id" => "1", "query" => "true"}

BUT instead we only get %{"query" => "true"}, path parameters discarded.

Investigation

The point is we are using Phoenix.Router instead of Plug.Router.

Plug.Router populates conn.path_params AND conn.params while initializing conn.params as a map instead of Unfetched struct:

https://github.com/elixir-plug/plug/blob/fb6b952cf93336dc79ec8d033e09a424d522ce56/lib/plug/router.ex#L565-L571

Then, fetch_query_params/2 (if invoked) fetches query parameters and populates conn.params: https://github.com/elixir-plug/plug/blob/fb6b952cf93336dc79ec8d033e09a424d522ce56/lib/plug/conn.ex#L993-L996

In this process, fetch_query_params/2 initializes conn.params from scratch if it is still Unfetched, while it merges with previous conn.params if already fetched.

So I suspect Phoenix.Router puts path parameters into conn.params WITHOUT properly initializing it to "fetched" state unlike Plug.Router, which I could not reveal exactly where.

On the other hand, if we use plug Plug.Parsers instead of plug :fetch_qeury_params: https://github.com/elixir-plug/plug/blob/fb6b952cf93336dc79ec8d033e09a424d522ce56/lib/plug/parsers.ex#L367-L382 It reads from conn.path_params AND calls fetch_query_params/2 inside, and merges results. So the above problem does not surface. In another word, params merge behavior is slightly different between Plug.Parsers and fetch_query_params, where the former depends on previous state of conn.params while the latter does not.

Conclusion

I am surprised (and bitten) by this inconsistency so hope it gets unified (fixed) or more clearly documented if it has some intentions behind that.

Please tell me if I am missing something obvious. Thanks!

[josevalim]

Hi @ymtszw, thanks for the great report! Fixed here: https://github.com/phoenixframework/phoenix/commit/e5628e198989265201857f53f2d990c88967b89a

[ymtszw]

Amazing as always @josevalim, the speed!