Repository: algesten/ureq
Pull request #221 (2020-11-13 07:54:38)

Remove qstring dependency.
Instead, rely on Url's built-in query parameter handling. A Request now
accumulates a list of query param pairs, and joins them with a parsed
URL at the time do_call is called.

In the process, remove some getters that rely on parsing the URL.
Adapting these getters was going to be awkward, and they mostly
duplicate things people can readily get by parsing the URL.


Patch:
diff --git a/Cargo.toml b/Cargo.toml
index a57f17b..a088b79 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -28,7 +28,6 @@ base64 = "0.13"
 chunked_transfer = "1.2.0"
 cookie = { version = "0.14", features = ["percent-encode"], optional = true}
 once_cell = "1"
-qstring = "0.7"
 url = "2"
 socks = { version = "0.3.2", optional = true }
 rustls = { version = "0.18", optional = true, features = [] }
diff --git a/src/request.rs b/src/request.rs
index 2c616ae..00393ca 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -1,7 +1,6 @@
 use std::fmt;
 use std::io::Read;
 
-use qstring::QString;
 use url::{form_urlencoded, Url};
 
 use crate::agent::Agent;
@@ -33,22 +32,15 @@ pub struct Request {
     url: String,
     return_error_for_status: bool,
     pub(crate) headers: Vec<Header>,
-    pub(crate) query: QString,
+    query_params: Vec<(String, String)>,
 }
 
 impl fmt::Debug for Request {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        let (path, query) = self
-            .to_url()
-            .map(|u| {
-                let query = unit::combine_query(&u, &self.query, true);
-                (u.path().to_string(), query)
-            })
-            .unwrap_or_else(|_| ("BAD_URL".to_string(), "BAD_URL".to_string()));
         write!(
             f,
-            "Request({} {}{}, {:?})",
-            self.method, path, query, self.headers
+            "Request({} {} {:?}, {:?})",
+            self.method, self.url, self.query_params, self.headers
         )
     }
 }
@@ -61,7 +53,7 @@ impl Request {
             url,
             headers: vec![],
             return_error_for_status: true,
-            query: QString::default(),
+            query_params: vec![],
         }
     }
 
@@ -86,11 +78,16 @@ impl Request {
         for h in &self.headers {
             h.validate()?;
         }
-        let response = self.to_url().and_then(|url| {
-            let reader = payload.into_read();
-            let unit = Unit::new(&self, &url, true, &reader);
-            unit::connect(&self, unit, true, 0, reader, false)
-        })?;
+        let mut url: Url = self
+            .url
+            .parse()
+            .map_err(|e: url::ParseError| Error::BadUrl(e.to_string()))?;
+        for (name, value) in self.query_params.clone() {
+            url.query_pairs_mut().append_pair(&name, &value);
+        }
+        let reader = payload.into_read();
+        let unit = Unit::new(&self, &url, &reader);
+        let response = unit::connect(&self, unit, true, 0, reader, false)?;
 
         if response.error() && self.return_error_for_status {
             Err(Error::HTTP(response.into()))
@@ -296,22 +293,8 @@ impl Request {
     /// println!("{:?}", r);
     /// ```
     pub fn query(mut self, param: &str, value: &str) -> Self {
-        self.query.add_pair((param, value));
-        self
-    }
-
-    /// Set query parameters as a string.
-    ///
-    /// For example, to set `?format=json&dest=/login`
-    ///
-    /// ```
-    /// let r = ureq::get("/my_page")
-    ///     .query_str("?format=json&dest=/login")
-    ///     .call();
-    /// println!("{:?}", r);
-    /// ```
-    pub fn query_str(mut self, query: &str) -> Self {
-        self.query.add_str(query);
+        self.query_params
+            .push((param.to_string(), value.to_string()));
         self
     }
 
@@ -334,90 +317,6 @@ impl Request {
         self
     }
 
-    /// Get the method this request is using.
-    ///
-    /// Example:
-    /// ```
-    /// let req = ureq::post("/somewhere");
-    /// assert_eq!(req.get_method(), "POST");
-    /// ```
-    pub fn get_method(&self) -> &str {
-        &self.method
-    }
-
-    /// Get the url this request was created with.
-    ///
-    /// This value is not normalized, it is exactly as set.
-    /// It does not contain any added query parameters.
-    ///
-    /// Example:
-    /// ```
-    /// let req = ureq::post("https://cool.server/innit");
-    /// assert_eq!(req.get_url(), "https://cool.server/innit");
-    /// ```
-    pub fn get_url(&self) -> &str {
-        &self.url
-    }
-
-    /// Normalizes and returns the host that will be used for this request.
-    ///
-    /// Example:
-    /// ```
-    /// let req1 = ureq::post("https://cool.server/innit");
-    /// assert_eq!(req1.get_host().unwrap(), "cool.server");
-    ///
-    /// let req2 = ureq::post("http://localhost/some/path");
-    /// assert_eq!(req2.get_host().unwrap(), "localhost");
-    /// ```
-    pub fn get_host(&self) -> Result<String> {
-        match self.to_url() {
-            Ok(u) => match u.host_str() {
-                Some(host) => Ok(host.to_string()),
-                None => Err(Error::BadUrl("No hostname in URL".into())),
-            },
-            Err(e) => Err(e),
-        }
-    }
-
-    /// Returns the scheme for this request.
-    ///
-    /// Example:
-    /// ```
-    /// let req = ureq::post("https://cool.server/innit");
-    /// assert_eq!(req.get_scheme().unwrap(), "https");
-    /// ```
-    pub fn get_scheme(&self) -> Result<String> {
-        self.to_url().map(|u| u.scheme().to_string())
-    }
-
-    /// The complete query for this request.
-    ///
-    /// Example:
-    /// ```
-    /// let req = ureq::post("https://cool.server/innit?foo=bar")
-    ///     .query("format", "json");
-    /// assert_eq!(req.get_query().unwrap(), "?foo=bar&format=json");
-    /// ```
-    pub fn get_query(&self) -> Result<String> {
-        self.to_url()
-            .map(|u| unit::combine_query(&u, &self.query, true))
-    }
-
-    /// The normalized url of this request.
-    ///
-    /// Example:
-    /// ```
-    /// let req = ureq::post("https://cool.server/innit");
-    /// assert_eq!(req.get_path().unwrap(), "/innit");
-    /// ```
-    pub fn get_path(&self) -> Result<String> {
-        self.to_url().map(|u| u.path().to_string())
-    }
-
-    fn to_url(&self) -> Result<Url> {
-        Url::parse(&self.url).map_err(|e| Error::BadUrl(format!("{}", e)))
-    }
-
     // Returns true if this request, with the provided body, is retryable.
     pub(crate) fn is_retryable(&self, body: &SizedReader) -> bool {
         // Per https://tools.ietf.org/html/rfc7231#section-8.1.3
@@ -441,16 +340,6 @@ impl Request {
     }
 }
 
-#[test]
-fn no_hostname() {
-    let req = Request::new(
-        Agent::new(),
-        "GET".to_string(),
-        "unix:/run/foo.socket".to_string(),
-    );
-    assert!(req.get_host().is_err());
-}
-
 #[test]
 fn request_implements_send_and_sync() {
     let _request: Box<dyn Send> = Box::new(Request::new(
diff --git a/src/unit.rs b/src/unit.rs
index 207fcd1..c9b3e45 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -2,7 +2,6 @@ use std::io::{self, Write};
 use std::time;
 
 use log::{debug, info};
-use qstring::QString;
 use url::Url;
 
 #[cfg(feature = "cookies")]
@@ -23,7 +22,6 @@ pub(crate) struct Unit {
     pub req: Request,
     pub url: Url,
     pub is_chunked: bool,
-    pub query_string: String,
     pub headers: Vec<Header>,
     pub deadline: Option<time::Instant>,
 }
@@ -31,7 +29,7 @@ pub(crate) struct Unit {
 impl Unit {
     //
 
-    pub(crate) fn new(req: &Request, url: &Url, mix_queries: bool, body: &SizedReader) -> Self {
+    pub(crate) fn new(req: &Request, url: &Url, body: &SizedReader) -> Self {
         //
 
         let (is_transfer_encoding_set, mut is_chunked) = req
@@ -48,8 +46,6 @@ impl Unit {
             // otherwise, no chunking.
             .unwrap_or((false, false));
 
-        let query_string = combine_query(&url, &req.query, mix_queries);
-
         let extra_headers = {
             let mut extra = vec![];
 
@@ -106,7 +102,6 @@ impl Unit {
             req: req.clone(),
             url: url.clone(),
             is_chunked,
-            query_string,
             headers,
             deadline,
         }
@@ -225,7 +220,7 @@ pub(crate) fn connect(
                 301 | 302 | 303 => {
                     let empty = Payload::Empty.into_read();
                     // recreate the unit to get a new hostname and cookies for the new host.
-                    let mut new_unit = Unit::new(req, &new_url, false, &empty);
+                    let mut new_unit = Unit::new(req, &new_url, &empty);
                     // this is to follow how curl does it. POST, PUT etc change
                     // to GET on a redirect.
                     new_unit.req.method = match &method[..] {
@@ -271,16 +266,6 @@ fn extract_cookies(agent: &Agent, url: &Url) -> Option<Header> {
     }
 }
 
-/// Combine the query of the url and the query options set on the request object.
-pub(crate) fn combine_query(url: &Url, query: &QString, mix_queries: bool) -> String {
-    match (url.query(), !query.is_empty() && mix_queries) {
-        (Some(urlq), true) => format!("?{}&{}", urlq, query),
-        (Some(urlq), false) => format!("?{}", urlq),
-        (None, true) => format!("?{}", query),
-        (None, false) => "".to_string(),
-    }
-}
-
 /// Connect the socket, either by using the pool or grab a new one.
 fn connect_socket(unit: &Unit, hostname: &str, use_pooled: bool) -> Result<(Stream, bool), Error> {
     match unit.url.scheme() {
@@ -323,10 +308,11 @@ fn send_prelude(unit: &Unit, stream: &mut Stream, redir: bool) -> io::Result<()>
     // request line
     write!(
         prelude,
-        "{} {}{} HTTP/1.1\r\n",
+        "{} {}{}{} HTTP/1.1\r\n",
         unit.req.method,
         unit.url.path(),
-        &unit.query_string
+        if unit.url.query().is_some() { "?" } else { "" },
+        unit.url.query().unwrap_or_default(),
     )?;
 
     // host header if not set by user.


Test patch:
diff --git a/src/test/query_string.rs b/src/test/query_string.rs
index 87a8692..e3a6f57 100644
--- a/src/test/query_string.rs
+++ b/src/test/query_string.rs
@@ -25,7 +25,11 @@ fn escaped_query_string() {
         .unwrap();
     let vec = resp.to_write_vec();
     let s = String::from_utf8_lossy(&vec);
-    assert!(s.contains("GET /escaped_query_string?foo=bar&baz=yo%20lo HTTP/1.1"))
+    assert!(
+        s.contains("GET /escaped_query_string?foo=bar&baz=yo+lo HTTP/1.1"),
+        "req: {}",
+        s
+    );
 }
 
 #[test]
@@ -50,5 +54,5 @@ fn query_in_path_and_req() {
         .unwrap();
     let vec = resp.to_write_vec();
     let s = String::from_utf8_lossy(&vec);
-    assert!(s.contains("GET /query_in_path_and_req?foo=bar&baz=1%202%203 HTTP/1.1"))
+    assert!(s.contains("GET /query_in_path_and_req?foo=bar&baz=1+2+3 HTTP/1.1"))
 }
diff --git a/src/test/simple.rs b/src/test/simple.rs
index 2e1dda1..498c672 100644
--- a/src/test/simple.rs
+++ b/src/test/simple.rs
@@ -136,7 +136,7 @@ fn request_debug() {
 
     assert_eq!(
         s,
-        "Request(GET /my/page, [Authorization: abcdef, \
+        "Request(GET http://localhost/my/page [], [Authorization: abcdef, \
          Content-Length: 1234, Content-Type: application/json])"
     );
 
@@ -148,7 +148,7 @@ fn request_debug() {
 
     assert_eq!(
         s,
-        "Request(GET /my/page?q=z&foo=bar%20baz, [Authorization: abcdef])"
+        "Request(GET http://localhost/my/page?q=z [(\"foo\", \"bar baz\")], [Authorization: abcdef])"
     );
 }