-
Notifications
You must be signed in to change notification settings - Fork 342
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
Allow to specify encoding strategy for query params #558
base: master
Are you sure you want to change the base?
Allow to specify encoding strategy for query params #558
Conversation
Related to: #538 |
lib/tesla.ex
Outdated
""" | ||
@spec build_url(Tesla.Env.url(), Tesla.Env.query()) :: binary | ||
def build_url(url, []), do: url | ||
@spec build_url(Tesla.Env.url(), Tesla.Env.query(), :rfc3986 | :www_form) :: binary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spec build_url(Tesla.Env.url(), Tesla.Env.query(), :rfc3986 | :www_form) :: binary | |
@type encoding_strategy :: :rfc3986 | :www_form | |
@spec build_url(Tesla.Env.url(), Tesla.Env.query(), encoding_strategy) :: binary |
Also related to this, what happen if some weird encoding is needed?
As an example: #538 (comment) I had to do that for a project, and, I am not too familiar with whatever RFCs exists out there.
So, what I created was part of some RFC?
Should we allow some callback to handle the encoding as a scape goat? Maybe add it when somebody asks for it? What would be the workload then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for quick response!
Well, my thought was since we are using URI.encode_query
underneath, we just expose that option to the user as it's given.
I like the idea of callback for encoder. I would imagine it looking something like this: d5e8527
Lines 521 to 535 in d5e8527
@spec build_url(Tesla.Env.url(), Tesla.Env.query(), (Enumerable.t() -> String.t())) :: binary | |
def build_url(url, query, encoder \\ &encode_query/1) | |
def build_url(url, [], _encoder), do: url | |
def build_url(url, query, encoder) do | |
join = if String.contains?(url, "?"), do: "&", else: "?" | |
url <> join <> encoder.(query) | |
end | |
def encode_query(query) do | |
query | |
|> Enum.flat_map(&encode_pair/1) | |
|> URI.encode_query() | |
end |
While that gives more flexibility, now in order to apply elixir's built-in URI.encode_query
with :rfc3986
it's not enough to do just
Tesla.build_url(url, [page: 2, status: true], &URI.encode_query(&1, :rfc3986))
because it would be lacking encoding nested params and lists which is currently done here by private encode_pair/1
Line 531 in dfc4ea5
|> Enum.flat_map(&encode_pair/1) |
At the same time, I don't think it would be right to run custom encoder only after that encode_pair
because, as you mentioned - someone could want to encode lists differently 🤷🏻♂️
Thank you so much for taking the time for this. I appreciate it. |
Co-authored-by: Yordis Prieto <yordis.prieto@gmail.com>
Any final thoughts related to our conversation? |
I was adapting Tesla in a project when we encode query params with
:rfc3986
and I noticed that, while Tesla provides conveniences to build url encoding query params for us, it doesn't allow to specify the encoding strategy.TL;DR
This PR allows specifying option
query_encoding: :rfc3986
so that whitespaces in query params will be encoded as "%20".will build the following url
Also adds optional argument to
Tesla.build_url
andTesla.encode_query
.Tesla uses URI.encode_query/2 to encode query params. Since Elixir 1.12 that function allows to specify the encoding strategy.