-
Notifications
You must be signed in to change notification settings - Fork 352
Middleware.PathParams :erlang.binary_to_existing_atom error for google APIs #566
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
Comments
Another possible solution is to do away with the regex altogether and do an 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
|
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. |
String.replace is tricky- #270 (comment) |
@teamon wouldn't be better to avoid the atom situation altogether? |
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 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 |
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 That way we are closer and more aligned with the rest of the developer community, even outside Elixir (less stuff to learn and reinvent) |
Tesla.Middleware.PathParams has been extended to support OpenAPI style parameters in addition to the existing Phoenix style parameters. - Phoenix parameters begin with `:` and a letter (`a-zA-Z`) and continue to the next non-word character (not `a-zA-Z0-9_`). Examples include `:id`, `:post_id`, `:idPost`. - OpenAPI parameters are contained in braces (`{}`), must begin with a letter (`a-zA-Z`) and may contain word characters and hyphens (`-a-zA-Z0-9_`). Examples inlucde `{id}`, `{post_id}`, `{IdPost}`. Parameter value objects may be provided as maps with atom or string keys, keyword lists, or structs. During path resolution, to avoid potential atom exhaustion, the parameter value object is transformed to a string-keyed map. Parameter values that are `nil` or that are not present in the value object are not replaced; all other values are converted to string (with `to_string`) and encoded with `application/x-www-form-urlencoded` formatting. If the value does not implement the `String.Chars` protocol, it is the caller's responsibility to convert it to a string prior to passing it to the Tesla client using this middleware. Execution time is determined by the number of parameter patterns in the path. Closes: #566
Uh oh!
There was an error while loading. Please reload this page.
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:
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:
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 onlib/tesla/middleware/path_params.ex:39
we could rescue it if it fails.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).What do you think? I can work on opening a pull request for that as well. for now, I'm passing as a param 😬.
The text was updated successfully, but these errors were encountered: