Handle nil params in get requests

[thiagomajesk]

Closes: #422 (comment)

[wojtekmach]

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

[thiagomajesk]

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 👍).

[wojtekmach]

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] || []))

[thiagomajesk]

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 😅