From cce7abb008e89266fae681ffaa14af30a8b95d4e Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sun, 28 Nov 2021 00:16:31 +0000 Subject: [PATCH] guarantee ordering of header map get_all closes #2466 --- actix-http/CHANGES.md | 5 ++ actix-http/src/header/map.rs | 102 +++++++++++++++++++++++++++++++-- actix-http/src/header/mod.rs | 16 +++--- actix-http/src/header/utils.rs | 1 + actix-http/src/ws/proto.rs | 2 +- 5 files changed, 111 insertions(+), 15 deletions(-) diff --git a/actix-http/CHANGES.md b/actix-http/CHANGES.md index 1cc7ae5fd75..31cafe8eb5b 100644 --- a/actix-http/CHANGES.md +++ b/actix-http/CHANGES.md @@ -1,6 +1,11 @@ # Changes ## Unreleased - 2021-xx-xx +### Changed +* Guarantee ordering of `header::GetAll` iterator. [#????] +* Expose `header::{GetAll, Removed}` iterators. [#????] + +[#????]: https://github.com/actix/actix-web/pull/???? ## 3.0.0-beta.13 - 2021-11-22 diff --git a/actix-http/src/header/map.rs b/actix-http/src/header/map.rs index a8fd9715bc9..c7e4921a88a 100644 --- a/actix-http/src/header/map.rs +++ b/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>, } -impl<'a> Removed { +impl Removed { fn new(value: Option) -> 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) { diff --git a/actix-http/src/header/mod.rs b/actix-http/src/header/mod.rs index 18494f5557b..a9483a9ffaf 100644 --- a/actix-http/src/header/mod.rs +++ b/actix-http/src/header/mod.rs @@ -29,16 +29,14 @@ pub use http::header::{ X_FRAME_OPTIONS, X_XSS_PROTECTION, }; -use crate::error::ParseError; -use crate::HttpMessage; +use crate::{error::ParseError, HttpMessage}; mod as_name; mod into_pair; mod into_value; -mod utils; - pub(crate) mod map; mod shared; +mod utils; #[doc(hidden)] pub use self::shared::*; @@ -46,10 +44,10 @@ pub use self::shared::*; pub use self::as_name::AsHeaderName; pub use self::into_pair::IntoHeaderPair; pub use self::into_value::IntoHeaderValue; -#[doc(hidden)] -pub use self::map::GetAll; -pub use self::map::HeaderMap; -pub use self::utils::*; +pub use self::map::{GetAll, HeaderMap, Removed}; +pub use self::utils::{ + fmt_comma_delimited, from_comma_delimited, from_one_raw_str, http_percent_encode, +}; /// A trait for any object that already represents a valid header field and value. pub trait Header: IntoHeaderValue { @@ -68,7 +66,7 @@ impl From for HeaderMap { } /// This encode set is used for HTTP header values and is defined at -/// https://tools.ietf.org/html/rfc5987#section-3.2. +/// . pub(crate) const HTTP_VALUE: &AsciiSet = &CONTROLS .add(b' ') .add(b'"') diff --git a/actix-http/src/header/utils.rs b/actix-http/src/header/utils.rs index 5e965238094..cf8636e9d86 100644 --- a/actix-http/src/header/utils.rs +++ b/actix-http/src/header/utils.rs @@ -56,6 +56,7 @@ where /// Percent encode a sequence of bytes with a character set defined in /// +#[inline] pub fn http_percent_encode(f: &mut fmt::Formatter<'_>, bytes: &[u8]) -> fmt::Result { let encoded = percent_encoding::percent_encode(bytes, HTTP_VALUE); fmt::Display::fmt(&encoded, f) diff --git a/actix-http/src/ws/proto.rs b/actix-http/src/ws/proto.rs index fdcde5eac3a..8ec04a5c309 100644 --- a/actix-http/src/ws/proto.rs +++ b/actix-http/src/ws/proto.rs @@ -220,7 +220,7 @@ impl> From<(CloseCode, T)> for CloseReason { } } -/// The WebSocket GUID as stated in the spec. See https://tools.ietf.org/html/rfc6455#section-1.3. +/// The WebSocket GUID as stated in the spec. See . static WS_GUID: &[u8] = b"258EAFA5-E914-47DA-95CA-C5AB0DC85B11"; /// Hashes the `Sec-WebSocket-Key` header according to the WebSocket spec.