Implement OpenAPI style params for PathParams

This has been extracted from a version we created for internal use. This
keeps the current support for Phoenix-style params (`:id`) and adds
support for OpenAPI-style params (`{id}`).

Note that the test names have changed as I extracted/adapted this from
a version we developed for internal use and confusion was expressed
about what the tests were actually testing.

---

This has been opened as a *draft* pull request because there are some
corner cases that I think should be dealt with, but would prefer some
collaborative guidance.

1. This *mostly* follows the Phoenix implementation for path parameters,
   excepting that capital letters are permitted. This matches the
   previous version's implementation. I would *personally* want to have
   different parameter identifier matching rules for `:id` vs `{id}`
   formats, but that gets complicated.

2. This handles "nested" path parameters for Phoenix-style *only* (e.g.,
   `:id_post` when there is also `:id`), and it does so with applying
   `sort_by` on the parameter key length. There is, at the moment, an
   unavoidable `{to_string(name), value}` map operation, because
   otherwise we are doing *two* `to_string(name)` applications (once in
   `sort_by` and once in the `reduce`)

3. This does *not* handle "nested" OpenAPI-style parameters well (that
   is, `{post{id}}` or `{id{post}}`), and I'm not sure what the
   behaviour here should be. Strictly speaking, we should *probably* be
   doing this somewhat differently than the logic currently implemented
   *or* the previous regex-based logic:

   1. The parameters should be converted to a map with string keys at
      all times.

   2. We should *parse the URL* and apply parameters over the parts, and
      then re-encode the URL.

      Legal characters/encoding is *different* for hostnames
      (`https://{client}.api.com`) vs paths (`/users/{userId}`) vs query
      parameters (`?query={query}`), although some of the escaping for
      `URI.encode` does not seem to be correct for paths (for
      `/users/{userId}` with %{userId: "user#1"}, the substitution is
      `/users/user#1` which *absolutely* requires the `#` to be escaped
      because `#` is special, but it is not percent encoded; this is
      likely an indicator of a missing function (`URI.encode_path/1`?)
      in Elixir.

   3. Parsing the URL should be *much* closer to the actual
      implementation of Plug.Router.Utils.parse_suffix/2, and should
      have separate paths for `{OpenAPI}` vs `:phoenix` parameters.

My biggest worry for the above approach is that parsing the URL each
time *may be expensive*. Phoenix and Plug get away with it mostly
because they are building compile-time representations that get handled.

It would be possible to build a `parameterized_path` macro that would
pass an iodata-ish tokenized path to Tesla:

```elixir
defmodule MyClient do
  use Tesla

  plug Tesla.Middleware.BaseUrl, "https://api.example.com"
  plug Tesla.Middleware.Logger # or some monitoring middleware
  plug Tesla.Middleware.PathParams

  import Tesla.Middleware.PathParams, only: [parameterized_path: 1]

  @user_path parameterized_path("/users/{id}")
  @user_posts_path parameterized_path("/users/:id/posts/:post_id")

  def user(id) do
    params = [id: id]
    get(@user_path, opts: [path_params: params])
  end

  def posts(id, post_id) do
    params = [id: id, post_id: post_id]
    get(@user_posts_path, opts: [path_params: params])
  end
end
```

This would return something like `["", "users"`, {"{id}", {"id", :id}]`
and looping over that looking for a tuple would let us then do something
like (assuming Access) `params[elem(param_names, 0)] ||
params[elem(param_names, 1)]`, although we would be generating extra
atoms at compile time that way.

Any thoughts?

lib/tesla/middleware/path_params.ex

@@ -1,12 +1,29 @@
 defmodule Tesla.Middleware.PathParams do
   @moduledoc """
-  Use templated URLs with separate params.
+  Use templated URLs with provided parameters in either Phoenix (`:id`) or
+  OpenAPI (`{id}`) format.
 
   Useful when logging or reporting metric per URL.
 
+  ## Parameter Values
+
+  Parameter values may be a `t:Keyword.t/0` list, a `t:map/0`, a `t:struct/0`,
+  or any other enumerable which produces `{key, value}` when enumerated.
+
+  ## Parameter Name Restrictions
+
+  The parameters must be valid path identifers like those in Plug.Router, with
+  one exception: the parameter may begin with an uppercase character.
+  A parameter name should match this regular expression:
+
+      \A[_a-zA-Z][_a-zA-Z0-9]*\z
+
+  Parameters that begin with underscores (`_`) or otherwise do not match as
+  valid are ignored and left as-is.
+
   ## Examples
 
-  ```
+  ```elixir
   defmodule MyClient do
     use Tesla
 
@@ -16,16 +33,19 @@ defmodule Tesla.Middleware.PathParams do
 
     def user(id) do
       params = [id: id]
-      get("/users/:id", opts: [path_params: params])
+      get("/users/{id}", opts: [path_params: params])
+    end
+
+    def posts(id, post_id) do
+      params = [id: id, post_id: post_id]
+      get("/users/:id/posts/:post_id", opts: [path_params: params])
     end
   end
   ```
   """
 
   @behaviour Tesla.Middleware
 
-  @rx ~r/:([a-zA-Z]{1}[\w_]*)/
-
   @impl Tesla.Middleware
   def call(env, next, _) do
     url = build_url(env.url, env.opts[:path_params])
@@ -34,9 +54,50 @@ defmodule Tesla.Middleware.PathParams do
 
   defp build_url(url, nil), do: url
 
+  defp build_url(url, %_{} = params), do: build_url(url, Map.from_struct(params))
+
   defp build_url(url, params) do
-    Regex.replace(@rx, url, fn match, key ->
-      to_string(params[String.to_existing_atom(key)] || match)
-    end)
+    params
+    |> Enum.map(fn {name, value} -> {to_string(name), value} end)
+    |> Enum.sort_by(fn {name, _} -> String.length(name) end, :desc)
+    |> Enum.reduce(url, &replace_parameter/2)
   end
+
+  # Do not replace parameters with nil values.
+  defp replace_parameter({_name, nil}, url), do: url
+  defp replace_parameter({name, value}, url), do: replace_parameter(name, value, url)
+
+  defp replace_parameter(<<h, t::binary>>, value, url)
+       when h in ?a..?z or h in ?A..?Z do
+    case parse_name(t, <<h>>) do
+      :error ->
+        url
+
+      name ->
+        encoded_value =
+          value
+          |> to_string()
+          |> URI.encode_www_form()
+
+        url
+        |> String.replace(":#{name}", encoded_value)
+        |> String.replace("{#{name}}", encoded_value)
+    end
+  end
+
+  # Do not replace parameters that do not start with a..z or A..Z.
+  defp replace_parameter(_name, _value, url), do: url
+
+  # This is adapted from Plug.Router.Utils.parse_suffix/2. This verifies that
+  # the provided parameter *only* contains the characters documented as
+  # accepted.
+  #
+  # https://github.com/elixir-plug/plug/blob/main/lib/plug/router/utils.ex#L255-L260
+  defp parse_name(<<h, t::binary>>, acc)
+       when h in ?a..?z or h in ?A..?Z or h in ?0..?9 or h == ?_,
+       do: parse_name(t, <<acc::binary, h>>)
+
+  defp parse_name(<<>>, acc), do: acc
+
+  defp parse_name(_, _), do: :error
 end

test/tesla/middleware/path_params_test.exs

@@ -4,83 +4,222 @@ defmodule Tesla.Middleware.PathParamsTest do
 
   @middleware Tesla.Middleware.PathParams
 
-  test "no params" do
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:id"}, [], nil)
-    assert env.url == "/users/:id"
+  defmodule TestUser do
+    defstruct [:id]
   end
 
-  test "passed params" do
-    opts = [path_params: [id: 42]]
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:id", opts: opts}, [], nil)
-    assert env.url == "/users/42"
-  end
+  describe "Phoenix-style params (:id)" do
+    test "leaves the identifier with no parameters" do
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/:id"}, [], nil)
+      assert env.url == "/users/:id"
+    end
 
-  test "value is not given" do
-    opts = [path_params: [y: 42]]
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:x", opts: opts}, [], nil)
-    assert env.url == "/users/:x"
-  end
+    test "replaces the identifier with passed params" do
+      opts = [path_params: [id: 42]]
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/:id", opts: opts}, [], nil)
+      assert env.url == "/users/42"
+    end
 
-  test "value is nil" do
-    opts = [path_params: [id: nil]]
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:id", opts: opts}, [], nil)
-    assert env.url == "/users/:id"
-  end
+    test "leaves the identifier if no value is given" do
+      opts = [path_params: [y: 42]]
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/:x", opts: opts}, [], nil)
+      assert env.url == "/users/:x"
+    end
 
-  test "placeholder contains another placeholder" do
-    opts = [path_params: [id: 1, id_post: 2]]
+    test "leaves the identifier if the value is nil" do
+      opts = [path_params: [id: nil]]
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/:id", opts: opts}, [], nil)
+      assert env.url == "/users/:id"
+    end
 
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:id/p/:id_post", opts: opts}, [], nil)
+    test "correctly handles shorter identifiers in longer identifiers" do
+      opts = [path_params: [id: 1, id_post: 2]]
 
-    assert env.url == "/users/1/p/2"
-  end
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/:id/p/:id_post", opts: opts}, [], nil)
 
-  test "placeholder starts by number" do
-    opts = [path_params: ["1id": 1, id_post: 2]]
+      assert env.url == "/users/1/p/2"
+    end
 
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:1id/p/:id_post", opts: opts}, [], nil)
+    test "leaves identifiers that start with a number" do
+      opts = [path_params: ["1id": 1, id_post: 2]]
 
-    assert env.url == "/users/:1id/p/2"
-  end
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/:1id/p/:id_post", opts: opts}, [], nil)
 
-  test "placeholder with only 1 number" do
-    opts = [path_params: ["1": 1, id_post: 2]]
+      assert env.url == "/users/:1id/p/2"
+    end
 
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:1/p/:id_post", opts: opts}, [], nil)
+    test "leaves identifiers that are a single digit" do
+      opts = [path_params: ["1": 1, id_post: 2]]
 
-    assert env.url == "/users/:1/p/2"
-  end
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/:1/p/:id_post", opts: opts}, [], nil)
 
-  test "placeholder with only 1 character" do
-    opts = [path_params: [i: 1, id_post: 2]]
+      assert env.url == "/users/:1/p/2"
+    end
 
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:i/p/:id_post", opts: opts}, [], nil)
+    test "replaces identifiers one character long" do
+      opts = [path_params: [i: 1, id_post: 2]]
 
-    assert env.url == "/users/1/p/2"
-  end
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/:i/p/:id_post", opts: opts}, [], nil)
 
-  test "placeholder with multiple numbers" do
-    opts = [path_params: ["123": 1, id_post: 2]]
+      assert env.url == "/users/1/p/2"
+    end
 
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:123/p/:id_post", opts: opts}, [], nil)
+    test "leaves identifiers that are only numbers" do
+      opts = [path_params: ["123": 1, id_post: 2]]
 
-    assert env.url == "/users/:123/p/2"
-  end
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/:123/p/:id_post", opts: opts}, [], nil)
+
+      assert env.url == "/users/:123/p/2"
+    end
+
+    test "leaves identifiers that start with underscore" do
+      opts = [path_params: [_id: 1, id_post: 2]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/:_id/p/:id_post", opts: opts}, [], nil)
+
+      assert env.url == "/users/:_id/p/2"
+    end
 
-  test "placeholder starts by underscore" do
-    opts = [path_params: [_id: 1, id_post: 2]]
+    test "replaces any valid identifier" do
+      opts = [path_params: [id_1_a: 1, id_post: 2]]
 
-    assert {:ok, env} = @middleware.call(%Env{url: "/users/:_id/p/:id_post", opts: opts}, [], nil)
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/:id_1_a/p/:id_post", opts: opts}, [], nil)
 
-    assert env.url == "/users/:_id/p/2"
+      assert env.url == "/users/1/p/2"
+    end
+
+    test "replaces identifiers that start with a capital letter" do
+      opts = [path_params: [id_1_a: 1, IdPost: 2]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/:id_1_a/p/:IdPost", opts: opts}, [], nil)
+
+      assert env.url == "/users/1/p/2"
+    end
+
+    test "replaces identifiers where the path params is a struct" do
+      opts = [path_params: %TestUser{id: 1}]
+
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/:id", opts: opts}, [], nil)
+      assert env.url == "/users/1"
+    end
+
+    test "URI-encodes path parameters with reserved characters" do
+      opts = [path_params: [id: "user#1", post_id: "post#2"]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/:id/p/:post_id", opts: opts}, [], nil)
+
+      assert env.url == "/users/user%231/p/post%232"
+    end
   end
 
-  test "placeholder with numbers, underscore and characters" do
-    opts = [path_params: [id_1_a: 1, id_post: 2]]
+  describe "OpenAPI-style params ({id})" do
+    test "leaves the identifier with no parameters" do
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/{id}"}, [], nil)
+      assert env.url == "/users/{id}"
+    end
+
+    test "replaces the identifier with passed params" do
+      opts = [path_params: [id: 42]]
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/{id}", opts: opts}, [], nil)
+      assert env.url == "/users/42"
+    end
+
+    test "leaves the identifier if no value is given" do
+      opts = [path_params: [y: 42]]
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/{x}", opts: opts}, [], nil)
+      assert env.url == "/users/{x}"
+    end
+
+    test "leaves the identifier if the value is nil" do
+      opts = [path_params: [id: nil]]
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/{id}", opts: opts}, [], nil)
+      assert env.url == "/users/{id}"
+    end
+
+    test "leaves identifiers that start with a number" do
+      opts = [path_params: ["1id": 1, id_post: 2]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/{1id}/p/{id_post}", opts: opts}, [], nil)
+
+      assert env.url == "/users/{1id}/p/2"
+    end
+
+    test "leaves identifiers that are a single digit" do
+      opts = [path_params: ["1": 1, id_post: 2]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/{1}/p/{id_post}", opts: opts}, [], nil)
+
+      assert env.url == "/users/{1}/p/2"
+    end
+
+    test "replaces identifiers one character long" do
+      opts = [path_params: [i: 1, id_post: 2]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/{i}/p/{id_post}", opts: opts}, [], nil)
+
+      assert env.url == "/users/1/p/2"
+    end
+
+    test "leaves identifiers that are only numbers" do
+      opts = [path_params: ["123": 1, id_post: 2]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/{123}/p/{id_post}", opts: opts}, [], nil)
+
+      assert env.url == "/users/{123}/p/2"
+    end
+
+    test "leaves identifiers that start with underscore" do
+      opts = [path_params: [_id: 1, id_post: 2]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/{_id}/p/{id_post}", opts: opts}, [], nil)
+
+      assert env.url == "/users/{_id}/p/2"
+    end
+
+    test "replaces any valid identifier" do
+      opts = [path_params: [id_1_a: 1, id_post: 2]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/{id_1_a}/p/{id_post}", opts: opts}, [], nil)
+
+      assert env.url == "/users/1/p/2"
+    end
+
+    test "replaces identifiers that start with a capital letter" do
+      opts = [path_params: [id_1_a: 1, IdPost: 2]]
+
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/{id_1_a}/p/{IdPost}", opts: opts}, [], nil)
+
+      assert env.url == "/users/1/p/2"
+    end
+
+    test "replaces identifiers where the path params is a struct" do
+      opts = [path_params: %TestUser{id: 1}]
+
+      assert {:ok, env} = @middleware.call(%Env{url: "/users/{id}", opts: opts}, [], nil)
+      assert env.url == "/users/1"
+    end
+
+    test "URI-encodes path parameters with reserved characters" do
+      opts = [path_params: [id: "user#1", post_id: "post#2"]]
 
-    assert {:ok, env} =
-             @middleware.call(%Env{url: "/users/:id_1_a/p/:id_post", opts: opts}, [], nil)
+      assert {:ok, env} =
+               @middleware.call(%Env{url: "/users/{id}/p/{post_id}", opts: opts}, [], nil)
 
-    assert env.url == "/users/1/p/2"
+      assert env.url == "/users/user%231/p/post%232"
+    end
   end
 end