From 46c194ff27b816a905a014b5d4c1ea0405c62a89 Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Thu, 28 Apr 2022 08:42:51 -0500
Subject: [PATCH] Overhaul modify_list() (#130)

Now uses strategy suggested by @jchrom, where we operate on "homonym groups". This makes it possible to use duplicate names in url parameters and form/multipart bodies.

Fixes #97. Fixes #107.
---
 NEWS.md                        | 3 +++
 R/utils.R                      | 6 ++++--
 tests/testthat/test-req-body.R | 7 +++++--
 tests/testthat/test-req-url.R  | 6 +++++-
 tests/testthat/test-utils.R    | 7 +++++++
 5 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 105ee4b0..89283f09 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,8 @@
 # httr2 (development version)
 
+* `req_body_form()`, `req_body_multipart()`, and `req_url_query()` now 
+  support multiple arguments with the same name (#97, #107).
+
 * `req_throttle()` correctly sets throttle rate (@jchrom, #101).
 
 * `req_error()` can now correct force successful HTTP statuses to fail (#98).
diff --git a/R/utils.R b/R/utils.R
index 00a6e948..067ce16b 100644
--- a/R/utils.R
+++ b/R/utils.R
@@ -28,8 +28,10 @@ modify_list <- function(.x, ...) {
   if (!is_named(dots)) {
     abort("All components of ... must be named")
   }
-  .x[names(dots)] <- dots
-  out <- compact(.x)
+
+  out <- .x[!names(.x) %in% names(dots)]
+  out <- c(out, compact(dots))
+
   if (length(out) == 0) {
     names(out) <- NULL
   }
diff --git a/tests/testthat/test-req-body.R b/tests/testthat/test-req-body.R
index f6fd1e1b..6ff4877c 100644
--- a/tests/testthat/test-req-body.R
+++ b/tests/testthat/test-req-body.R
@@ -49,10 +49,13 @@ test_that("can send named list as json/form/multipart", {
 
 test_that("can modify body data", {
   req1 <- request_test() %>% req_body_form(list(a = 1))
-  req2 <- req1 %>% req_body_form(list(b = 2))
-
   expect_equal(req1$body$data, list(a = 1))
+
+  req2 <- req1 %>% req_body_form(list(b = 2))
   expect_equal(req2$body$data, list(a = 1, b = 2))
+
+  req3 <- req1 %>% req_body_form(list(a = 3, a = 4))
+  expect_equal(req3$body$data, list(a = 3, a = 4))
 })
 
 test_that("can upload file with multipart", {
diff --git a/tests/testthat/test-req-url.R b/tests/testthat/test-req-url.R
index b16cb2ba..71c62ddd 100644
--- a/tests/testthat/test-req-url.R
+++ b/tests/testthat/test-req-url.R
@@ -30,6 +30,9 @@ test_that("can set query params", {
   expect_equal(req_url_query(req, a = 1, b = 2)$url, "http://example.com/?a=1&b=2")
   expect_equal(req_url_query(req, a = 1, b = 2, c = NULL)$url, "http://example.com/?a=1&b=2")
   expect_equal(req_url_query(req, !!!list(a = 1, b = 2))$url, "http://example.com/?a=1&b=2")
+
+  expect_equal(req_url_query(req, a = 1, a = 2)$url, "http://example.com/?a=1&a=2")
+  expect_equal(req_url_query(req, !!!list(a = 1, a = 2))$url, "http://example.com/?a=1&a=2")
 })
 
 test_that("empty query doesn't affect url", {
@@ -41,7 +44,8 @@ test_that("empty query doesn't affect url", {
 test_that("can modify query params iteratively", {
   req <- request("http://example.com/?a=1&b=2")
   expect_equal(req_url_query(req, c = 3)$url, "http://example.com/?a=1&b=2&c=3")
-  expect_equal(req_url_query(req, a = 2)$url, "http://example.com/?a=2&b=2")
+  expect_equal(req_url_query(req, a = 2)$url, "http://example.com/?b=2&a=2")
+  expect_equal(req_url_query(req, a = 1, a = 2)$url, "http://example.com/?b=2&a=1&a=2")
   expect_equal(req_url_query(req, b = NULL)$url, "http://example.com/?a=1")
 })
 
diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R
index 7a18704b..c1825087 100644
--- a/tests/testthat/test-utils.R
+++ b/tests/testthat/test-utils.R
@@ -9,6 +9,13 @@ test_that("modify list adds, removes, and overrides", {
   expect_snapshot(modify_list(x, a = 1, 2), error = TRUE)
 })
 
+test_that("replacement affects all components with name", {
+  x <- list(a = 1, a = 2)
+  expect_equal(modify_list(x, a = NULL), list())
+  expect_equal(modify_list(x, a = 3), list(a = 3))
+  expect_equal(modify_list(x, a = 3, a = 4), list(a = 3, a =4))
+})
+
 test_that("can check arg types", {
   expect_snapshot(error = TRUE, {
     check_string(1, "x")