guarantee ordering of header map get_all (#2467)

actix-http/src/header/map.rs

@@ -288,7 +288,7 @@ impl HeaderMap {
     /// Returns an iterator over all values associated with a header name.
     ///
     /// The returned iterator does not incur any allocations and will yield no items if there are no
-    /// values associated with the key. Iteration order is **not** guaranteed to be the same as
+    /// values associated with the key. Iteration order is guaranteed to be the same as
     /// insertion order.
     ///
     /// # Examples
@@ -355,6 +355,19 @@ impl HeaderMap {
     ///
     /// assert_eq!(map.len(), 1);
     /// ```
+    ///
+    /// A convenience method is provided on the returned iterator to check if the insertion replaced
+    /// any values.
+    /// ```
+    /// # use actix_http::http::{header, HeaderMap, HeaderValue};
+    /// let mut map = HeaderMap::new();
+    ///
+    /// let removed = map.insert(header::ACCEPT, HeaderValue::from_static("text/plain"));
+    /// assert!(removed.is_empty());
+    ///
+    /// let removed = map.insert(header::ACCEPT, HeaderValue::from_static("text/html"));
+    /// assert!(!removed.is_empty());
+    /// ```
     pub fn insert(&mut self, key: HeaderName, val: HeaderValue) -> Removed {
         let value = self.inner.insert(key, Value::one(val));
         Removed::new(value)
@@ -393,6 +406,9 @@ impl HeaderMap {
 
     /// Removes all headers for a particular header name from the map.
     ///
+    /// Providing an invalid header names (as a string argument) will have no effect and return
+    /// without error.
+    ///
     /// # Examples
     /// ```
     /// # use actix_http::http::{header, HeaderMap, HeaderValue};
@@ -409,6 +425,21 @@ impl HeaderMap {
     /// assert!(removed.next().is_none());
     ///
     /// assert!(map.is_empty());
+    /// ```
+    ///
+    /// A convenience method is provided on the returned iterator to check if the `remove` call
+    /// actually removed any values.
+    /// ```
+    /// # use actix_http::http::{header, HeaderMap, HeaderValue};
+    /// let mut map = HeaderMap::new();
+    ///
+    /// let removed = map.remove("accept");
+    /// assert!(removed.is_empty());
+    ///
+    /// map.insert(header::ACCEPT, HeaderValue::from_static("text/html"));
+    /// let removed = map.remove("accept");
+    /// assert!(!removed.is_empty());
+    /// ```
     pub fn remove(&mut self, key: impl AsHeaderName) -> Removed {
         let value = match key.try_as_name(super::as_name::Seal) {
             Ok(Cow::Borrowed(name)) => self.inner.remove(name),
@@ -571,7 +602,7 @@ impl<'a> IntoIterator for &'a HeaderMap {
     }
 }
 
-/// Iterator for all values with the same header name.
+/// Iterator for references of [`HeaderValue`]s with the same associated [`HeaderName`].
 ///
 /// See [`HeaderMap::get_all`].
 #[derive(Debug)]
@@ -613,18 +644,32 @@ impl<'a> Iterator for GetAll<'a> {
     }
 }
 
-/// Iterator for owned [`HeaderValue`]s with the same associated [`HeaderName`] returned from methods
-/// on [`HeaderMap`] that remove or replace items.
+/// Iterator for owned [`HeaderValue`]s with the same associated [`HeaderName`] returned from
+/// methods that remove or replace items.
+///
+/// See [`HeaderMap::insert`] and [`HeaderMap::remove`].
 #[derive(Debug)]
 pub struct Removed {
     inner: Option<smallvec::IntoIter<[HeaderValue; 4]>>,
 }
 
-impl<'a> Removed {
+impl Removed {
     fn new(value: Option<Value>) -> Self {
         let inner = value.map(|value| value.inner.into_iter());
         Self { inner }
     }
+
+    /// Returns true if iterator contains no elements, without consuming it.
+    ///
+    /// If called immediately after [`HeaderMap::insert`] or [`HeaderMap::remove`], it will indicate
+    /// wether any items were actually replaced or removed, respectively.
+    pub fn is_empty(&self) -> bool {
+        match self.inner {
+            // size hint lower bound of smallvec is the correct length
+            Some(ref iter) => iter.size_hint().0 == 0,
+            None => true,
+        }
+    }
 }
 
 impl Iterator for Removed {
@@ -945,6 +990,53 @@ mod tests {
         assert_eq!(vals.next(), removed.next().as_ref());
     }
 
+    #[test]
+    fn get_all_iteration_order_matches_insertion_order() {
+        let mut map = HeaderMap::new();
+
+        let mut vals = map.get_all(header::COOKIE);
+        assert!(vals.next().is_none());
+
+        map.append(header::COOKIE, HeaderValue::from_static("1"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"1");
+        assert!(vals.next().is_none());
+
+        map.append(header::COOKIE, HeaderValue::from_static("2"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"1");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"2");
+        assert!(vals.next().is_none());
+
+        map.append(header::COOKIE, HeaderValue::from_static("3"));
+        map.append(header::COOKIE, HeaderValue::from_static("4"));
+        map.append(header::COOKIE, HeaderValue::from_static("5"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"1");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"2");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"3");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"4");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"5");
+        assert!(vals.next().is_none());
+
+        let _ = map.insert(header::COOKIE, HeaderValue::from_static("6"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"6");
+        assert!(vals.next().is_none());
+
+        let _ = map.insert(header::COOKIE, HeaderValue::from_static("7"));
+        let _ = map.insert(header::COOKIE, HeaderValue::from_static("8"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"8");
+        assert!(vals.next().is_none());
+
+        map.append(header::COOKIE, HeaderValue::from_static("9"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"8");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"9");
+        assert!(vals.next().is_none());
+    }
+
     fn owned_pair<'a>(
         (name, val): (&'a HeaderName, &'a HeaderValue),
     ) -> (HeaderName, HeaderValue) {