Url::mutate_query_pairs return a value with Drop rather than take a closure.

src/form_urlencoded.rs

@@ -186,30 +186,74 @@ impl<'a> Iterator for ByteSerialize<'a> {
 
 /// The [`application/x-www-form-urlencoded` serializer](
 /// https://url.spec.whatwg.org/#concept-urlencoded-serializer).
-pub struct Serializer<'a> {
-    string: &'a mut String,
+pub struct Serializer<T: Target> {
+    target: Option<T>,
     start_position: usize,
     encoding: EncodingOverride,
 }
 
-impl<'a> Serializer<'a> {
+pub trait Target {
+    fn as_mut_string(&mut self) -> &mut String;
+    fn finish(self) -> Self::Finished;
+    type Finished;
+}
+
+impl Target for String {
+    fn as_mut_string(&mut self) -> &mut String { self }
+    fn finish(self) -> Self { self }
+    type Finished = Self;
+}
+
+impl<'a> Target for &'a mut String {
+    fn as_mut_string(&mut self) -> &mut String { &mut **self }
+    fn finish(self) -> Self { self }
+    type Finished = Self;
+}
+
+// `as_mut_string` string here exposes the internal serialization of an `Url`,
+// which should not be exposed to users.
+// We achieve that by not giving users direct access to `UrlQuery`:
+// * Its fields are private
+//   (and so can not be constructed with struct literal syntax outside of this crate),
+// * It has no constructor
+// * It is only visible (on the type level) to users in the return type of
+//   `Url::mutate_query_pairs` which is `Serializer<UrlQuery>`
+// * `Serializer` keeps its target in a private field
+// * Unlike in other `Target` impls, `UrlQuery::finished` does not return `Self`.
+impl<'a> Target for ::UrlQuery<'a> {
+    fn as_mut_string(&mut self) -> &mut String { &mut self.url.serialization }
+    fn finish(self) -> &'a mut ::Url { self.url }
+    type Finished = &'a mut ::Url;
+}
+
+impl<T: Target> Serializer<T> {
+    /// Create a new `application/x-www-form-urlencoded` serializer for the given target.
+    ///
+    /// If the target is non-empty,
+    /// its content is assumed to already be in `application/x-www-form-urlencoded` syntax.
+    pub fn new(target: T) -> Self {
+        Self::for_suffix(target, 0)
+    }
+
     /// Create a new `application/x-www-form-urlencoded` serializer
-    /// for the given range of the given string.
+    /// for a suffix of the given target.
     ///
-    /// If the range is non-empty, the corresponding slice of the string is assumed
-    /// to already be in `application/x-www-form-urlencoded` syntax.
-    pub fn new(string: &'a mut String, start_position: usize) -> Self {
-        &string[start_position..];  // Panic if out of bounds
+    /// If that suffix is non-empty,
+    /// its content is assumed to already be in `application/x-www-form-urlencoded` syntax.
+    pub fn for_suffix(mut target: T, start_position: usize) -> Self {
+        &target.as_mut_string()[start_position..];  // Panic if out of bounds
         Serializer {
-            string: string,
+            target: Some(target),
             start_position: start_position,
             encoding: EncodingOverride::utf8(),
         }
     }
 
     /// Remove any existing name/value pair.
+    ///
+    /// Panics if called after `.finish()`.
     pub fn clear(&mut self) -> &mut Self {
-        self.string.truncate(self.start_position);
+        string(&mut self.target).truncate(self.start_position);
         self
     }
 
@@ -220,18 +264,11 @@ impl<'a> Serializer<'a> {
         self
     }
 
-    fn append_separator_if_needed(&mut self) {
-        if self.string.len() > self.start_position {
-            self.string.push('&')
-        }
-    }
-
     /// Serialize and append a name/value pair.
+    ///
+    /// Panics if called after `.finish()`.
     pub fn append_pair(&mut self, name: &str, value: &str) -> &mut Self {
-        self.append_separator_if_needed();
-        self.string.extend(byte_serialize(&self.encoding.encode(name.into())));
-        self.string.push('=');
-        self.string.extend(byte_serialize(&self.encoding.encode(value.into())));
+        append_pair(string(&mut self.target), self.start_position, self.encoding, name, value);
         self
     }
 
@@ -240,11 +277,16 @@ impl<'a> Serializer<'a> {
     /// This simply calls `append_pair` repeatedly.
     /// This can be more convenient, so the user doesn’t need to introduce a block
     /// to limit the scope of `Serializer`’s borrow of its string.
+    ///
+    /// Panics if called after `.finish()`.
     pub fn append_pairs<I, K, V>(&mut self, iter: I) -> &mut Self
     where I: IntoIterator, I::Item: Borrow<(K, V)>, K: AsRef<str>, V: AsRef<str> {
-        for pair in iter {
-            let &(ref k, ref v) = pair.borrow();
-            self.append_pair(k.as_ref(), v.as_ref());
+        {
+            let string = string(&mut self.target);
+            for pair in iter {
+                let &(ref k, ref v) = pair.borrow();
+                append_pair(string, self.start_position, self.encoding, k.as_ref(), v.as_ref());
+            }
         }
         self
     }
@@ -252,11 +294,50 @@ impl<'a> Serializer<'a> {
     /// Add a name/value pair whose name is `_charset_`
     /// and whose value is the character encoding’s name.
     /// (See the `encoding_override()` method.)
+    ///
+    /// Panics if called after `.finish()`.
     #[cfg(feature = "query_encoding")]
     pub fn append_charset(&mut self) -> &mut Self {
-        self.append_separator_if_needed();
-        self.string.push_str("_charset_=");
-        self.string.push_str(self.encoding.name());
+        {
+            let string = string(&mut self.target);
+            append_separator_if_needed(string, self.start_position);
+            string.push_str("_charset_=");
+            string.push_str(self.encoding.name());
+        }
         self
     }
+
+    /// If this serializer was constructed with a string, take and return that string.
+    ///
+    /// ```rust
+    /// use url::form_urlencoded;
+    /// let encoded: String = form_urlencoded::Serializer::new(String::new())
+    ///     .append_pair("foo", "bar & baz")
+    ///     .append_pair("saison", "Été+hiver")
+    ///     .finish();
+    /// assert_eq!(encoded, "foo=bar+%26+baz&saison=%C3%89t%C3%A9%2Bhiver");
+    /// ```
+    ///
+    /// Panics if called more than once.
+    pub fn finish(&mut self) -> T::Finished {
+        self.target.take().expect("url::form_urlencoded::Serializer double finish").finish()
+    }
+}
+
+fn append_separator_if_needed(string: &mut String, start_position: usize) {
+    if string.len() > start_position {
+        string.push('&')
+    }
+}
+
+fn string<T: Target>(target: &mut Option<T>) -> &mut String {
+    target.as_mut().expect("url::form_urlencoded::Serializer finished").as_mut_string()
+}
+
+fn append_pair(string: &mut String, start_position: usize, encoding: EncodingOverride,
+               name: &str, value: &str) {
+    append_separator_if_needed(string, start_position);
+    string.extend(byte_serialize(&encoding.encode(name.into())));
+    string.push('=');
+    string.extend(byte_serialize(&encoding.encode(value.into())));
 }

src/lib.rs

@@ -492,76 +492,103 @@ impl Url {
         }
     }
 
+    fn take_fragment(&mut self) -> Option<String> {
+        self.fragment_start.take().map(|start| {
+            debug_assert!(self.byte_at(start) == b'#');
+            let fragment = self.slice(start + 1..).to_owned();
+            self.serialization.truncate(start as usize);
+            fragment
+        })
+    }
+
+    fn restore_already_parsed_fragment(&mut self, fragment: Option<String>) {
+        if let Some(ref fragment) = fragment {
+            assert!(self.fragment_start.is_none());
+            self.fragment_start = Some(to_u32(self.serialization.len()).unwrap());
+            self.serialization.push('#');
+            self.serialization.push_str(fragment);
+        }
+    }
+
     /// Change this URL’s query string.
     pub fn set_query(&mut self, query: Option<&str>) {
-        self.set_query_internal(|url| {
-            // Remove any previous query
-            if let Some(start) = url.query_start.take() {
-                debug_assert!(url.byte_at(start) == b'?');
-                url.serialization.truncate(start as usize);
-            }
-            // Write the new query, if any
-            if let Some(input) = query {
-                url.query_start = Some(to_u32(url.serialization.len()).unwrap());
-                url.serialization.push('?');
-                let scheme_end = url.scheme_end;
-                url.mutate(|parser| parser.parse_query(scheme_end, input));
-            }
-        })
+        let fragment = self.take_fragment();
+
+        // Remove any previous query
+        if let Some(start) = self.query_start.take() {
+            debug_assert!(self.byte_at(start) == b'?');
+            self.serialization.truncate(start as usize);
+        }
+        // Write the new query, if any
+        if let Some(input) = query {
+            self.query_start = Some(to_u32(self.serialization.len()).unwrap());
+            self.serialization.push('?');
+            let scheme_end = self.scheme_end;
+            self.mutate(|parser| parser.parse_query(scheme_end, input));
+        }
+
+        self.restore_already_parsed_fragment(fragment);
     }
 
-    /// Change this URL’s query string, viewed as a sequence of name/value pairs
+    /// Manipulate this URL’s query string, viewed as a sequence of name/value pairs
     /// in `application/x-www-form-urlencoded` syntax.
     ///
-    /// Example:
+    /// The return value has a method-chaining API:
     ///
     /// ```rust
     /// # use url::Url;
-    /// let mut url = Url::parse("https://example.net?...#nav").unwrap();
-    /// assert_eq!(url.query(), Some("..."));
-    /// url.mutate_query_pairs(|query| {
-    ///     query.clear();
-    ///     query.append_pair("foo", "bar & baz");
-    /// });
-    /// assert_eq!(url.query(), Some("foo=bar+%26+baz"));
-    /// assert_eq!(url.as_str(), "https://example.net/?foo=bar+%26+baz#nav");
-    /// url.mutate_query_pairs(|query| {
-    ///     query.append_pair("saison", "Été+hiver");
-    /// });
-    /// assert_eq!(url.query(), Some("foo=bar+%26+baz&saison=%C3%89t%C3%A9%2Bhiver"));
+    /// let mut url = Url::parse("https://example.net?lang=fr#nav").unwrap();
+    /// assert_eq!(url.query(), Some("lang=fr"));
+    ///
+    /// url.mutate_query_pairs().append_pair("foo", "bar");
+    /// assert_eq!(url.query(), Some("lang=fr&foo=bar"));
+    /// assert_eq!(url.as_str(), "https://example.net/?lang=fr&foo=bar#nav");
+    ///
+    /// url.mutate_query_pairs()
+    ///     .clear()
+    ///     .append_pair("foo", "bar & baz")
+    ///     .append_pair("saisons", "Été+hiver");
+    /// assert_eq!(url.query(), Some("foo=bar+%26+baz&saisons=%C3%89t%C3%A9%2Bhiver"));
+    /// assert_eq!(url.as_str(),
+    ///            "https://example.net/?foo=bar+%26+baz&saisons=%C3%89t%C3%A9%2Bhiver#nav");
     /// ```
-    pub fn mutate_query_pairs<F: FnOnce(&mut form_urlencoded::Serializer)>(&mut self, f: F) {
-        self.set_query_internal(|url| {
-            let query_start;
-            if let Some(start) = url.query_start {
-                debug_assert!(url.byte_at(start) == b'?');
-                query_start = start as usize;
-            } else {
-                query_start = url.serialization.len();
-                url.query_start = Some(to_u32(query_start).unwrap());
-                url.serialization.push('?');
-            }
-            let query_start = query_start + "?".len();
-            f(&mut form_urlencoded::Serializer::new(&mut url.serialization, query_start))
-        })
+    ///
+    /// Note: `url.mutate_query_pairs().clear();` is equivalent to `url.set_query(Some(""))`,
+    /// not `url.set_query(None)`.
+    ///
+    /// The state of `Url` is unspecified if this return value is leaked without being dropped.
+    pub fn mutate_query_pairs(&mut self) -> form_urlencoded::Serializer<UrlQuery> {
+        let fragment = self.take_fragment();
+
+        let query_start;
+        if let Some(start) = self.query_start {
+            debug_assert!(self.byte_at(start) == b'?');
+            query_start = start as usize;
+        } else {
+            query_start = self.serialization.len();
+            self.query_start = Some(to_u32(query_start).unwrap());
+            self.serialization.push('?');
+        }
+
+        let query = UrlQuery { url: self, fragment: fragment };
+        form_urlencoded::Serializer::for_suffix(query, query_start + "?".len())
     }
+}
 
-    fn set_query_internal<F: FnOnce(&mut Url)>(&mut self, f: F) {
-        // Stash any fragment
-        let fragment = self.fragment_start.map(|start| {
-            let f = self.slice(start..).to_owned();
-            self.serialization.truncate(start as usize);
-            f
-        });
-        f(self);
-        // Restore the fragment, if any
-        if let Some(ref fragment) = fragment {
-            self.fragment_start = Some(to_u32(self.serialization.len()).unwrap());
-            debug_assert!(fragment.starts_with('#'));
-            self.serialization.push_str(fragment)  // It’s already been through the parser
-        }
+
+/// Implementation detail of `Url::mutate_query_pairs`. Typically not used directly.
+pub struct UrlQuery<'a> {
+    url: &'a mut Url,
+    fragment: Option<String>,
+}
+
+impl<'a> Drop for UrlQuery<'a> {
+    fn drop(&mut self) {
+        self.url.restore_already_parsed_fragment(self.fragment.take())
     }
+}
 
+impl Url {
     /// Change this URL’s path.
     pub fn set_path(&mut self, path: &str) {
         let (old_after_path_pos, after_path) = match (self.query_start, self.fragment_start) {

tests/tests.rs

@@ -217,19 +217,17 @@ fn test_form_urlencoded() {
         ("bar".into(), "".into()),
         ("foo".into(), "#".into())
     ];
-    let mut encoded = String::new();
-    form_urlencoded::Serializer::new(&mut encoded, 0).append_pairs(pairs);
+    let encoded = form_urlencoded::Serializer::new(String::new()).append_pairs(pairs).finish();
     assert_eq!(encoded, "foo=%C3%A9%26&bar=&foo=%23");
     assert_eq!(form_urlencoded::parse(encoded.as_bytes()).collect::<Vec<_>>(), pairs.to_vec());
 }
 
 #[test]
 fn test_form_serialize() {
-    let mut encoded = String::new();
-    form_urlencoded::Serializer::new(&mut encoded, 0).append_pairs(&[
-        ("foo", "é&"),
-        ("bar", ""),
-        ("foo", "#")
-    ]);
+    let encoded = form_urlencoded::Serializer::new(String::new())
+        .append_pair("foo", "é&")
+        .append_pair("bar", "")
+        .append_pair("foo", "#")
+        .finish();
     assert_eq!(encoded, "foo=%C3%A9%26&bar=&foo=%23");
 }