Middleware.PathParams :erlang.binary_to_existing_atom error for google APIs

[hiagomeels]

Middleware.PathParams :erlang.binary_to_existing_atom error for google APIs

I'm migrating from another lib to Tesla and I found an issue while trying to use got integrated this API.

This is the error that I got:

** (ArgumentError) argument error
     code: @middleware.call(%Env{url: "/v1/brands/:brand_id/agents/:agent_id:requestLaunch", opts: opts}, [], nil)
     stacktrace:
       :erlang.binary_to_existing_atom("requestLaunch", :utf8)
       (tesla) lib/tesla/middleware/path_params.ex:39: anonymous fn/3 in Tesla.Middleware.PathParams.build_url/2
       (elixir) lib/regex.ex:731: Regex.apply_list/5
       (elixir) lib/regex.ex:732: Regex.apply_list/5
       (elixir) lib/regex.ex:726: Regex.apply_list/5
       (elixir) lib/regex.ex:668: Regex.do_replace/4
       (tesla) lib/tesla/middleware/path_params.ex:31: Tesla.Middleware.PathParams.call/3
       test/tesla/middleware/path_params_test.exs:91: (test)

The problem is that the path for this API request is using a colon to indicate one action in the path /v1/brands/__BRAND_ID__/agents/__AGENT_ID__:requestLaunch (see doc).

I know that I could just pass the ":requestLaunch" as a path param as well:

path = "/v1/brands/:brand_id/agents/:agent_id:requestLaunch"
path_params = [
  agent_id: ...,
  brand_id: ...,
  requestLaunch:  ":requestLaunch" 
]
...

But it is very odd/ugly, and I think we should fix it on the middleware.

I have the following suggestion that we could:

1 - Add a rescue clause

Since we are using the String.to_existing_atom/1 function on lib/tesla/middleware/path_params.ex:39 we could rescue it if it fails.

defmodule Tesla.Middleware.PathParams do
  ...
  defp build_url(url, params) do
    Regex.replace(@rx, url, fn match, key ->
      try do
        to_string(params[String.to_existing_atom(key)] || match)
      rescue
        ArgumentError -> match
      end
    end)
  end
end

I'm unsure if we will want it not to throw an error if the param does not exist.

2 - Regex change

We can also change the regex to only fetch params that come after a slash / (it would imply on add back the slash again while we are doing the replacement).

defmodule Tesla.Middleware.PathParams do
  ...
  @rx ~r/\/:([a-zA-Z]{1}[\w_]*)/
  ...
  defp build_url(url, params) do
    Regex.replace(@rx, url, fn match, key ->
        "/" <> to_string(params[String.to_existing_atom(key)] || match)
    end)
  end
end

What do you think? I can work on opening a pull request for that as well. for now, I'm passing as a param 😬.

[EPortela2013]

Another possible solution is to do away with the regex altogether and do an Enum.reduce instead.

defmodule Tesla.Middleware.PathParams do
  ...
  defp build_url(url, params) do
    Enum.reduce(params, url, fn {key, value}, final_url ->
      pattern = ":#{Atom.to_string(key)}"
      String.replace(final_url, pattern, value)
    end)
end

[yordis]

I prefer to remove the atom as @EPortela2013 suggested. The dilemma is between URLs vs. Params to define what should be replaced.

I lean towards whatever is not relying on Atoms since they are not garbage collected, and I can see a place where people use Tesla as a pass-thru client from an untrusted environment that could DDos the server creating atoms.

Being said, all it matters it to try to avoid breaking changes.

[teamon]

String.replace is tricky- #270 (comment)

[yordis]

@teamon wouldn't be better to avoid the atom situation altogether?

[teamon]

I think the best option would be to go with something like this:

Enum.reduce(params, url, fn {key, value}, url ->
  String.replace(url, "<#{key}>", value)
end)

i.e. using "/users/<user_id>/" params with delimiters on both sides to prevent the issue described in the comment on #270.

We'd need to choose delimiters that are super unlikely to be part of any regular URL to prevent breaking change. This should be relatively safe, since the replace is driven by given args, not the url placeholders. The only case that would be broken would be something like this:

url = "/x/:user_id/y/<user_id>"
params = [user_id: 1]

# before the change
url == "/x/1/y/<user_id>"

# after the change
url == "/x/1/y/1"

I'm not sure < > is the ultimate best choice here - but overall I think this approach could potentially solve all issues at once.

[yordis]

Strongly agree with using something that has a start and end; my only nitpick and strong bias is towards OpenAPI if that will be the case: https://swagger.io/docs/specification/describing-parameters/

So it would be {name}

That way we are closer and more aligned with the rest of the developer community, even outside Elixir (less stuff to learn and reinvent)