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

Query parameters do not support nested maps #538

Open
mfeckie opened this issue Jul 13, 2022 · 7 comments
Open

Query parameters do not support nested maps #538

mfeckie opened this issue Jul 13, 2022 · 7 comments
Assignees

Comments

@mfeckie
Copy link

mfeckie commented Jul 13, 2022

When using Plug, we are able to serialize nested maps to query params.

e.g. Plug.Conn.Query.encode(%{filters: %{ name: "dave", direction: "asc", pagination: %{ page: 1} } }) which would give us something like filters[direction]=asc&filters[name]=dave&filters[pagination][page]=1"

Would you be open to a PR which either used Plug's implementation directly or re-implemented the algorithm?

@yordis
Copy link
Member

yordis commented Jul 13, 2022

Ideally, we shouldn't depend on Plug since Telsa is about something else, so in the best scenario, Plug and Tesla would use the same package.

If I am not mistaken, these types of serialization became popular with OpenAPI and they have some names for it https://swagger.io/docs/specification/serialization/ maybe it is worth having some packages shared, but I am not sure if Plug peeps would take the suggestion.

Worth mentioning, that Tesla doesn't know about the serialization strategy of such query params and most likely are pluggable based on the specification of their APIs, so whatever we do, it needs to be taken into consideration.

Thoughts?

@mfeckie
Copy link
Author

mfeckie commented Jul 13, 2022

Plug doesn't have a dependency, it has it's own implementation https://github.com/elixir-plug/plug/blob/v1.13.6/lib/plug/conn/query.ex#L203-L279

I'm not sure what the deal is with OpenAPI. Looking at that reference list, the only strategy I've very seen is deepObject and (infrequently) form.

Plug seemed like the logical implementation to me, because Tesla is very Plug like.

Seems like the issue hasn't come up before when I looked through the issues list, so maybe it's just something a small number of users would care about.

We were able to work around it by directly encoding using Plug.Conn.Query.encode and appending to the URL used by Tesla.

Personally, I'd only add the deepObject variant because it would be compatible with what already exists, just extending support for some extra data structures.

@yordis
Copy link
Member

yordis commented Sep 9, 2022

To collect more implementations, I recently had to do the following one

defp encode_query(query) do
    query
    |> Enum.map(fn
      {k, v} when is_binary(v) -> "#{k}=#{v}"
      {k, v} when is_list(v) -> Enum.map(v, &"#{k}=#{&1}")
    end)
    |> List.flatten()
    |> Enum.join("&")
  end

Haven't done objects because we don't have a need for it

@halostatue
Copy link
Contributor

Twilio's Ruby implementation (based on OpenAPI) has a flatten function which turns {a: {b: {c: 3}}} to {"a.b.c" => 3} to that using your encode_query approach, we would see a.b.c=3 or a.b.c=3&a.b.c=4 for a list.

@yordis
Copy link
Member

yordis commented Oct 25, 2024

After #558 is merged; I will allow a function to be passed as the query encoding; I will try to provide the default

Tesla.get(
  "http://example.com",
  query: [username: "John Smith"],
  opts: [query_encoding: &Plug.Conn.Query.encode\1]
)

Something around those lines

@yordis yordis self-assigned this Oct 25, 2024
@teamon
Copy link
Member

teamon commented Oct 25, 2024

I wonder if shouldn't allow passing binary as query instead, like:

Tesla.get("/", query: Plug.Conn.Query.encode([username: "John Smith"]))

🤔

@yordis
Copy link
Member

yordis commented Oct 25, 2024

@teamon I do not see why not, at that point we just concat strings; noted!

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

No branches or pull requests

4 participants