Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RudolfMan
Copy link

@RudolfMan RudolfMan commented Feb 3, 2023

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".

Tesla.get("http://example.com", query: [username: "John Smith"], opts: [query_encoding: :rfc3986])

will build the following url

http://example.com?username=John%20Smith

Also adds optional argument to Tesla.build_url and Tesla.encode_query.


Tesla uses URI.encode_query/2 to encode query params. Since Elixir 1.12 that function allows to specify the encoding strategy.

You can specify one of the following encoding strategies:

  • :www_form - (default, since v1.12.0) keys and values are URL encoded as per encode_www_form/1. This is the format typically used by browsers on query strings and form data. It encodes " " as "+".

  • :rfc3986 - (since v1.12.0) the same as :www_form except it encodes " " as "%20" according RFC 3986. This is the best option if you are encoding in a non-browser situation, since encoding spaces as "+" can be ambiguous to URI parsers. This can inadvertently lead to spaces being interpreted as literal plus signs.

Encoding defaults to :www_form for backward compatibility.

@yordis
Copy link
Member

yordis commented Feb 3, 2023

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@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?

Copy link
Author

@RudolfMan RudolfMan Feb 3, 2023

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

tesla/lib/tesla.ex

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

|> 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 🤷🏻‍♂️

@yordis
Copy link
Member

yordis commented Feb 3, 2023

Thank you so much for taking the time for this. I appreciate it.

Co-authored-by: Yordis Prieto <[email protected]>
@yordis
Copy link
Member

yordis commented Feb 16, 2023

Any final thoughts related to our conversation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants