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.

Sign up for GitHub

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

Jump to bottom

Handle nil params in get requests #423

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

Handle nil params in get requests #423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f514cb
Handle nil params in get requests
thiagomajesk Oct 16, 2024
1338d4d
Coalessce nil params and allow unsetting params
thiagomajesk Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/req.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,12 @@ defmodule Req do
update_in(
request.options,
&Map.merge(&1, Map.new(options), fn
:params, nil, new ->
new

:params, _old, nil ->
nil

:params, old, new ->
Keyword.merge(old, new)

Expand Down
6 changes: 2 additions & 4 deletions lib/req/steps.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,12 +534,10 @@ defmodule Req.Steps do
put_params(request, Req.Request.get_option(request, :params, []))
end

defp put_params(request, []) do
request
end
defp put_params(request, []), do: request

defp put_params(request, params) do
encoded = URI.encode_query(params)
encoded = URI.encode_query(params || [])

update_in(request.url.query, fn
nil -> encoded
Expand Down
24 changes: 24 additions & 0 deletions test/req_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,28 @@ defmodule ReqTest do
resp = Req.get!(origin_url, into: :self)
assert Req.put!(echo_url, body: resp.body).body == "foobarbaz"
end

test "merge params" do
req = Req.new(url: "/", params: [a: 1])
assert %{options: %{params: [a: 1, b: 2]}} = Req.merge(req, params: [b: 2])
end

test "merge with empty params" do
req = Req.new(url: "/", params: nil)
assert %{options: %{params: [a: 1]}} = Req.merge(req, params: [a: 1])

req = Req.new(url: "/", params: [a: 1])
assert %{options: %{params: nil}} = Req.merge(req, params: nil)
end

test "request with empty params", c do
Bypass.expect(c.bypass, "GET", "/", fn conn ->
Plug.Conn.send_resp(conn, 200, "")
end)


assert {:ok, %Req.Response{}} = Req.get(Req.new(url: c.url, params: [a: 1]), params: nil)
assert {:ok, %Req.Response{}} = Req.get(Req.new(url: c.url, params: nil), params: [a: 1])
assert %Req.Response{status: 200} = Req.get!(c.url, params: nil)
end
Copy link
Owner

@wojtekmach wojtekmach Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

params are merged, i.e. Req.get(Req.new(params: [a: 1]), params: [b: 2]) is equivalent to Req.new(params: [a: 1, b: 2]). If we were to support nil as no-op, we should document it and add a test. But now I'm getting cold feet, what if the user ever wanted to unset params by setting them to nil? I suppose one way to do it would be then put_in(req.options.params, nil) but I'm not sure. WDYT? (There is no backwards compatibility concern of course since passing nil today crashes.)

Copy link
Author

@thiagomajesk thiagomajesk Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @wojtekmach I think it's a valid concern. IMO, it depends on how you want to communicate "emptiness". I usually read something like [] or %{} as empty and nil as uninitialized / invalid.

For instance, it seems that Plug handles %{}, [] and nil as a no-op:

Plug.Conn.fetch_query_params(Plug.Adapters.Test.Conn.conn(%Plug.Conn{}, :get, "/", nil))
# => %Plug.Conn{query_params: %{}, ...}
Plug.Conn.fetch_query_params(Plug.Adapters.Test.Conn.conn(%Plug.Conn{}, :get, "/", %{}))
# => %Plug.Conn{query_params: %{}, ...} 
Plug.Conn.fetch_query_params(Plug.Adapters.Test.Conn.conn(%Plug.Conn{}, :get, "/", []))
# => %Plug.Conn{query_params: %{}, ...}

However, if you want to stay closer to Map.merge/2 semantics, it makes total sense to accept nil.

FYI: The code below also raises, so merging nil params is already invalid:

Req.get(Req.new(url: c.url, params: [a: 1]), params: nil)

I pushed a new commit that handles the proper merging of nil params and coalesces while encoding the query params (instead of no-oping when you pass nil): 1338d4d (I might need your help to improve the request asserts if you also want to check for the params 👍).

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you. It's always a good reminder what Plug and similar libraries do. I'd like to take some time thinking this through. FWIW we could raise a good error message for nil telling users to change their code, for example, nudging users to make a change like this:

- Req.get(Req.new(url: "/", params: opts[:query]))
+ Req.get(Req.new(url: "/", params: opts[:query] || []))

Copy link
Author

@thiagomajesk thiagomajesk Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My personal opinion is that this is such a common (expected) use case that I'd rather avoid raise. IMHO coalescing the value in the lib code is a more elegant solution than branching on the client, but please take your time 💪

PS.: Also, let me know if there's something I can do to help because the current behavior is kinda annoying and will catch a lot of people off guard 😅

end