Proposal: Allow all non-option fields in `Req.Request` to be specified as options

[zachallaun]

Hi there! First off, thanks for your work on req. I've been using it as my primary request library and really enjoying it, but wanted to offer a proposal that I think would alleviate a current source of friction.

Proposal:

All non-options Req.Request fields (:adapter, :url, :method, etc.) passed to Req.new/1 should be able to be merged into the request as options. I think the cleanest way to do this is to move much of the code in Req.new/1 to Request.merge_options/2, and to use it in the implementation of Req.new.

Context:

I've been really enjoying using Req for some API work I'm doing. I'm interacting with multiple APIs, both REST and GraphQL, and have been using Req for the whole thing. My strategy has been to define a general APIs.Helpers.base_req/1 that sets up general-purpose, shared logging/etc. and then APIs.SomeAPI.base_req/1 that wraps it and sets up API-specific stuff like the authentication mechanism, etc.

This has worked really well and been really flexible and easy to work with, except for one point of friction: the use of Req.Request fields vs. options. There were a number of cases where I was using something like Request.merge_options when setting up a request and then getting strange results, only to find that I was actually merging in something that is special-cased in Req.new and attached to the request directly. Ultimately, you have to look at the type spec to determine what you need to do -- whether something needs to be Map.put directly into the request, or merged in as an option.

This ends up making composition more difficult. For instance, if I pass :url in to Req.new/1, it will parse the URI for me. But if I merge it in later, I need to remember to parse the URI myself. This actually just bit me here -- prior to v3.1, you could merge in a :url string (even though it violates the typespec) so long as you were using a :base_url, because join/2 allowed the URL to be a string instead of a URI. Now obviously this was a bug in my code -- the typespec said it should have been a URI.t, but I didn't check and it worked. I think it highlights the problem, however. It would be better if I could Request.merge_options(req, url: "/some_path") and not have to concern myself with it.

Note: Digging through the code while writing this, I realized that the current behavior may actually be considered a bug, since validate_options/2 allows for :method, :url, etc.

[wojtekmach]

Thank you for reporting this.

Note: Digging through the code while writing this, I realized that the current behavior may actually be considered a bug, since validate_options/2 allows for :method, :url, etc.

Right. Currently we have:

iex> Req.new() |> Req.Request.merge_options(url: "foo") |> Req.request!()
** (ArgumentError) scheme is required for url:

and so we allow setting :url but it ultimately is not used which is bad.

If I understand your proposal correctly you're saying that merge_options(req, url: url) should update req.url (and pass it through URI.parse), right? If so I think that would be good, that would be more predictable. My only concern is special-casing :method/:url/:headers/:body not only in Req.new but now in other functions too which doesn't feel great but that's probably the way to go after all, I'll think about it.

[zachallaun]

Thanks very much for considering it! Yes, you have the gist of it.

Thinking through implementation, here's the definition of merge_options right now:

def merge_options(%Req.Request{} = request, options) when is_list(options) do
  validate_options(request, options)
  update_in(request.options, &Map.merge(&1, Map.new(options)))
end

I wonder if much of this could be alleviated by putting much of the behavior currently in Req.new into merge_options, and having Req.new call into it itself:

def merge_options(%Req.Request{} = request, options) when is_list(options) do
  validate_options(request, options)  
  
  {method, options} = Keyword.pop(options, :method, request.method)
  {headers, options} = Keyword.pop(options, :headers, request.headers)
  {url, options} = Keyword.pop(options, :url, request.url)
  {body, options} = Keyword.pop(options, :body, request.body)
  # ...

  %{
    request |
    method: method,
    headers: headers,
    url: url,
    body: body,
    options: Map.merge(request.options, Map.new(options))
  }
end

# Then back in req.ex

def new(options \\ []) do
  request = 
    %Req.Request{
      # all the defaults
    }
    |> Req.Request.merge_options(options)

  # ...
end

You obviously know the codebase very well, so I apologize if I missed some obvious (or even not-obvious) reason that the above wouldn't work!

[mcrumm]

I have also been bitten by this :)

special-casing :method/:url/:headers/:body not only in Req.new but now in other functions too

This is already the case– in fact, I think it is the primary source of the problem. Currently Req.request/2 performs a number of just-in-time operations on the additional options provided to ensure they end up in the right place:

https://github.com/wojtekmach/req/blob/12a0c000265c7542345407c69f02a1b474d746af/lib/req.ex#L763-L786

Note there are some other operations in request/2 as well but I think they are all legacy– the block above is the pertinent bit.

I assumed this was the case because, as currently written, Request.merge_options/2 is a well-understood low-level function: it does what it says and only what it says. However I will argue that consistency/clarity is more important here and therefore it should properly handle all options (including Request keys).

One final thought: in discussing the virtue of functions to test run a request, I used the following example:

Req.request!(adapter: &{&1, Response.new(body: &1)})

It is worth noting that this example is only valid as of v0.3.1, due to a missing key in the options split in Req.request/2. Prior to v0.3.1 the example would need to be written as follows:

Req.request!(Req.new(adapter: &{&1, Response.new(body: &1)})

[wojtekmach]

Thank you both for feedback, I think we should definitely address this.

I'm uneasy about merge_options accepting say :auth and storing it in request.options.auth but also accepting headers and storing it in request.headers.

WDYT about deprecating merge_options in favour of a new Req.Request.update(request, options)?

[zachallaun]

That seems perfectly fine to me. A unified interface to update both the struct and the options is really what's needed, and that would provide it!

On Mon, Oct 10, 2022 at 5:28 AM Wojtek Mach @.***> wrote:

Thank you both for feedback, I think we should definitely address this.

I'm uneasy about merge_options accepting say :auth and storing it in request.options.auth but also accepting headers and storing it in request.headers.

WDYT about deprecating merge_options in favour of a new Req.Request.update(request, options)?

— Reply to this email directly, view it on GitHub https://github.com/wojtekmach/req/issues/139#issuecomment-1273033741, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD3BAWIFGXJNYJF5QPLKALWCPOTDANCNFSM6AAAAAAQRFF43U . You are receiving this because you authored the thread.Message ID: @.***>

[mcrumm]

I like the idea of introducing a new function for this, and Request.update/2 is a good name :)

[wojtekmach]

Instead of Req.Request.update I decided to go with Req.update. The reason is I wanted to keep the behaviour of automatic headers merging and normalisation (e.g.: Req.new(headers: [x: 1]) |> Req.get!(headers: [x: 2]) sends [{"x", "1"}, {"x", "2"}]) and I thought that didn't belong in the lower-level Req.Request module.

I decided to keep Req.Request.merge_options/2 but deprecate passing :url, :headers and friends to it in favour of the new Req.update. I kept it because I thought this as an example plugin looks awkward:

  def attach(%Req.Request{} = request, options \\ []) do
    request
    |> Req.Request.register_options([:print_headers])
-   |> Req.Request.merge_options(options)
+   |> Req.update(options)
    |> Req.Request.append_request_steps(print_headers: &print_request_headers/1)
    |> Req.Request.prepend_response_steps(print_headers: &print_response_headers/1)
  end

I'm really curious what you think about this change and I'm happy to revisit it.

[zachallaun]

Thanks very much for this work, @wojtekmach!

I'm really curious what you think about this change and I'm happy to revisit it.

Definitely happy with the addition of Req.update/2! My personal opinion regarding Req.update vs. Req.Request.update is that either 1) it should be Req.Request.update and it's okay for the header-concatenation to occur in the lower-level module, or 2) all "request building functions", possibly including append/prepend steps/etc., should live in the top-level Req module, and all functions in Req.Request become implementation details.

I personally lean towards moving it into Req.Request, and then all of the various request functions (get, post, request, etc.) can point to Req.Request.update/2 as the canonical documentation for how options are merged into the request struct.

My justification for the second option above is basically that one would need only to refer to the Req module documentation to understand the full library API (which is not large), and Request and Response are more data descriptions / internal helpers.

I don't feel very strongly about this, however. I think the change as a whole is positive!

[wojtekmach]

I think you raise good points, thank you! I guess my counter argument is there's I think nice symmetry between Req.new and Req.update, they are basically the same thing.

I often find myself looking at things from two perspectives, the regular usage (Req module) and writing custom steps (Req.Request) module, and I guess that's part of my mistake because the boundary isn't perfect, it should be part of regular usage to be able to use custom steps. 😅 Another thing that is awkward at times is the fact that Req.new() #=> %Req.Request{}, as opposed to having either %Req{} or instead Req.Request.new. Will keep at it for sure.

[zachallaun]

it should be part of regular usage to be able to use custom steps

I think this is key! Almost every time I've used Req, I add some custom step. The ease of "extending the library" is what, in my opinion, makes it so compelling and productive. I've wondered if it is maybe doing Req a disservice to "hide" custom steps behind the label of "low level API" -- usually, when I see that, I think of it as something to almost always stay away from unless I really need it. But steps are so simple/fundamental/composable in Req that bringing them "to the surface" may be worth doing. :)