wojtekmach / req

Req is a batteries-included HTTP client for Elixir.
https://hexdocs.pm/req
Apache License 2.0
1.1k stars 117 forks source link

`Req.merge`: Deep merge options #319

Open wojtekmach opened 8 months ago

wojtekmach commented 8 months ago

Right now we special case :headers and :params. For example:

Req.new(headers: [a: 1]) |> Req.merge(headers: [b: 2]) == Req.new(headers: [a: 1, b: 2])

Another one when this might be useful is aws_sigv4:

Req.new(aws_sigv4: ...) |> Req.merge(aws_sigv4: [service: :s3])

We could special case that one too or have a more generic mechanism when registering the options, something like:

Req.Request.register_options(req, aws_sigv4: [update: :replace | :merge])
wojtekmach commented 8 months ago

Req.Request.register_options(req, aws_sigv4: [update: :replace | :merge])

this choice would prevent us from validating nested option names like:

Req.Request.register_options(req, aws_sigv4: [:access_key_id, :region, ...])

so need to carefully consider it, maybe we can have it both ways but maybe we don't.

gaberduty commented 2 months ago

I found myself on this issue when looking into merging the json option. I'm using Req right now to query DynamoDB, and it's got a pretty verbose request form:

Req.post(
  req,
  headers: %{"X-Amz-Target" => "DynamoDB_20120810.Query"},
  json: %{
    "TableName" => "tablename",
    "IndexName" => "indexname",
    "KeyConditionExpression" => "someVal = :someval",
    "ExpressionAttributeValues" => %{
      ":someval" => %{
        "S" => "the value you're querying"
      }
    }
  }
)

If the response includes a %{"LastEvaluatedKey" => key}, then you have to make a followup request that repeats all of that, but also includes the key:

Req.post(
  req,
  headers: %{"X-Amz-Target" => "DynamoDB_20120810.Query"},
  json: %{
    "TableName" => "tablename",
    "IndexName" => "indexname",
    "KeyConditionExpression" => "someVal = :someval",
    "ExpressionAttributeValues" => %{
      ":someval" => %{
        "S" => "the value you're querying"
      }
    },
    "ExclusiveStartKey" => key
  }
)

It would be nice to just set the base json in Req.new and then merge in that key if necessary. As it is, I'm doing

req = update_in(req.options.json, fn j -> Map.put(j, "ExclusiveStartKey", key) end)

but I'm not sure if that's kosher. I'm wary of messing directly with structs beneath the exposed functional interface.

wojtekmach commented 3 weeks ago

Another idea is to have a separate function like Req.deep_merge/2.