Repository: seanmonstar/reqwest (language: rust)

Issue:
Calling RequestBuilder.query() with an empty argument adds a ? to the URL
If you pass a `Serialize` argument that does not produce any values (for example, an empty `Vec`) to `RequestBuilder.query()`, it results in a URL that ends in "?". Here is a test that shows the behavior:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=1647711889f5df31a99edbe18ce59256

Fundamentally, I find it strange that reqwest would generate such URLs, since it's never useful to have the extra ? at the end if it's not followed by any query parameters. It also makes it harder for me to write integration tests, because I have to explicitly encode the ? in the expected path for the request, even though it doesn't belong there.

Of course, you can just not call `query()` if you don't have any parameters, but that makes the `RequestBuilder` a lot more awkward to use. Here's my use case:

I want to write this:
```rust
let results: Vec<SearchResult> = client
    .get(&url)
    .headers(headers)
    .query(&self.get_query_params())
    .send()?
    .json()?;
```
In my case, `get_query_params()` may or may not return parameters, depending on the state of the system.

But instead I have to write something like this:
```rust
let mut request = client.get(&url).headers(headers);

let params = self.get_query_params();
if params.is_some() {
    request = request.query(&params);
}

let results: Vec<SearchResult> = request.send()?.json()?;
```


Discussion / hints:
Seems like a good thing to fix!

The `query` method makes use of `query_pairs_mut` from the url crate, and then serde_urlencoded. Likely one of those is setting the question mark even when empty...
It's `query_pairs_mut` that adds the question mark:
https://github.com/servo/rust-url/blob/a1d8c8849fd35098c90a6a05dc772e827303ce41/src/lib.rs#L1344
What is the recommended approach here? modify the logic of querying in some way, or just removing the query if it's empty with something like below? (feels a bit hacky)

```
if let Some("") = url.query() {
    url.set_query(None);
}
```
My opinion is it's odd behavior in `url`, so maybe warrants a fix there. But we can probably fix it faster in reqwest, so I'd be fine with that too.
I have a possible solution on this [branch](https://github.com/aaneto/rust-url/tree/remove-trailing-question-mark-on-empty-query). I can create a PR on rust-url linking this issue, if that's not a problem.

Note 1: I'm a little lost with OSS contribution proceedings, that's why I'm asking a bunch of questions.

Fix patch:
diff --git a/src/async_impl/request.rs b/src/async_impl/request.rs
index dff3a6a52..431765fcd 100644
--- a/src/async_impl/request.rs
+++ b/src/async_impl/request.rs
@@ -267,6 +267,11 @@ impl RequestBuilder {
                 error = Some(::error::from(err));
             }
         }
+        if let Ok(ref mut req) = self.request {
+            if let Some("") = req.url().query() {
+                req.url_mut().set_query(None);
+            }
+        }
         if let Some(err) = error {
             self.request = Err(err);
         }
@@ -514,6 +519,22 @@ mod tests {
         assert_eq!(foo[1], "baz");
     }
 
+    #[test]
+    fn normalize_empty_query() {
+        let client = Client::new();
+        let some_url = "https://google.com/";
+        let empty_query: &[(&str, &str)] = &[];
+
+        let req = client
+            .get(some_url)
+            .query(empty_query)
+            .build()
+            .expect("request build");
+
+        assert_eq!(req.url().query(), None);
+        assert_eq!(req.url().as_str(), "https://google.com/");
+    }
+
     /*
     use {body, Method};
     use super::Client;
diff --git a/src/request.rs b/src/request.rs
index 31fb26b2e..160255630 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -353,6 +353,11 @@ impl RequestBuilder {
                 error = Some(::error::from(err));
             }
         }
+        if let Ok(ref mut req) = self.request {
+            if let Some("") = req.url().query() {
+                req.url_mut().set_query(None);
+            }
+        }
         if let Some(err) = error {
             self.request = Err(err);
         }
@@ -837,4 +842,20 @@ mod tests {
         assert_eq!(foo[0], "bar");
         assert_eq!(foo[1], "baz");
     }
+
+    #[test]
+    fn normalize_empty_query() {
+        let client = Client::new();
+        let some_url = "https://google.com/";
+        let empty_query: &[(&str, &str)] = &[];
+
+        let req = client
+            .get(some_url)
+            .query(empty_query)
+            .build()
+            .expect("request build");
+
+        assert_eq!(req.url().query(), None);
+        assert_eq!(req.url().as_str(), "https://google.com/");
+    }
 }


Test patch: