From 5b20bf5446a2d8b4d8b2ee918c04a8a8b301bfad Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 8 Feb 2021 03:58:57 +0000 Subject: [PATCH] clean up header map --- actix-http/Cargo.toml | 1 - actix-http/src/header/map.rs | 528 ++++++++++++++++++++++------------- actix-http/src/request.rs | 6 +- actix-http/src/response.rs | 4 +- 4 files changed, 338 insertions(+), 201 deletions(-) diff --git a/actix-http/Cargo.toml b/actix-http/Cargo.toml index 22a54f569..402462362 100644 --- a/actix-http/Cargo.toml +++ b/actix-http/Cargo.toml @@ -53,7 +53,6 @@ bytes = "1" bytestring = "1" cookie = { version = "0.14.1", features = ["percent-encode"] } derive_more = "0.99.5" -either = "1.5.3" encoding_rs = "0.8" futures-channel = { version = "0.3.7", default-features = false, features = ["alloc"] } futures-core = { version = "0.3.7", default-features = false, features = ["alloc"] } diff --git a/actix-http/src/header/map.rs b/actix-http/src/header/map.rs index 8f20f3e6f..c35fab5ee 100644 --- a/actix-http/src/header/map.rs +++ b/actix-http/src/header/map.rs @@ -1,16 +1,12 @@ -use std::{ - collections::hash_map::{self, Entry}, - convert::TryFrom, -}; +use std::{borrow::Cow, collections::hash_map, str::FromStr}; use ahash::AHashMap; -use either::Either; -use http::header::{HeaderName, HeaderValue}; +use http::header::{HeaderName, HeaderValue, InvalidHeaderName}; use smallvec::{smallvec, SmallVec}; /// A multi-map of HTTP headers. /// -/// `HeaderMap` is a "multi-map" of [`HeaderName`] to one or more values. +/// `HeaderMap` is a "multi-map" of [`HeaderName`] to one or more [`HeaderValue`]s. #[derive(Debug, Clone, Default)] pub struct HeaderMap { pub(crate) inner: AHashMap, @@ -54,33 +50,29 @@ impl Value { impl HeaderMap { /// Create an empty `HeaderMap`. /// - /// The map will be created without any capacity. This function will not - /// allocate. + /// The map will be created without any capacity. This function will not allocate. pub fn new() -> Self { - HeaderMap { - inner: AHashMap::default(), - } + HeaderMap::default() } /// Create an empty `HeaderMap` with the specified capacity. /// - /// The returned map will allocate internal storage in order to hold about - /// `capacity` elements without reallocating. However, this is a "best - /// effort" as there are usage patterns that could cause additional - /// allocations before `capacity` headers are stored in the map. + /// The returned map will allocate internal storage in order to hold about `capacity` elements + /// without reallocating. However, this is a "best effort" as there are usage patterns that + /// could cause additional allocations before `capacity` headers are stored in the map. /// /// More capacity than requested may be allocated. pub fn with_capacity(capacity: usize) -> HeaderMap { HeaderMap { - inner: AHashMap::with_capacity_and_hasher(capacity, Default::default()), + inner: AHashMap::with_capacity(capacity), } } /// Returns the number of keys stored in the map. /// - /// This number could be be less than or equal to actual headers stored in - /// the map. + /// This number could be be less than or equal to actual headers stored in the map. pub fn len(&self) -> usize { + // TODO: wat!? that's messed up self.inner.len() } @@ -89,206 +81,210 @@ impl HeaderMap { self.inner.len() == 0 } - /// Clears the map, removing all key-value pairs. Keeps the allocated memory - /// for reuse. + /// Clears the map, removing all name-value pairs. Keeps the allocated memory for reuse. pub fn clear(&mut self) { self.inner.clear(); } /// Returns the number of headers the map can hold without reallocating. /// - /// This number is an approximation as certain usage patterns could cause - /// additional allocations before the returned capacity is filled. + /// This number is an approximation as certain usage patterns could cause additional allocations + /// before the returned capacity is filled. pub fn capacity(&self) -> usize { self.inner.capacity() } - /// Reserves capacity for at least `additional` more headers to be inserted - /// into the `HeaderMap`. + /// Reserves capacity for at least `additional` more headers to be inserted in the map. /// - /// The header map may reserve more space to avoid frequent reallocations. - /// Like with `with_capacity`, this will be a "best effort" to avoid - /// allocations until `additional` more headers are inserted. Certain usage - /// patterns could cause additional allocations before the number is - /// reached. + /// The header map may reserve more space to avoid frequent reallocations. Like with + /// `with_capacity`, this will be a "best effort" to avoid allocations until `additional` more + /// headers are inserted. Certain usage patterns could cause additional allocations before the + /// number is reached. pub fn reserve(&mut self, additional: usize) { self.inner.reserve(additional) } - /// Returns a reference to the value associated with the key. - /// - /// If there are multiple values associated with the key, then the first one - /// is returned. Use `get_all` to get all values associated with a given - /// key. Returns `None` if there are no values associated with the key. - pub fn get(&self, name: N) -> Option<&HeaderValue> { - self.get2(name).map(|v| v.first()) - } - - fn get2(&self, name: N) -> Option<&Value> { - match name.as_name() { - Either::Left(name) => self.inner.get(name), - Either::Right(s) => { - if let Ok(name) = HeaderName::try_from(s) { - self.inner.get(&name) - } else { - None - } - } + fn get_value(&self, key: impl AsHeaderName) -> Option<&Value> { + match key.try_as_name().ok()? { + Cow::Borrowed(name) => self.inner.get(name), + Cow::Owned(name) => self.inner.get(&name), } } - /// Returns an iterator of all values associated with a key. + /// Returns a reference to the first value associated with a header name. + /// + /// If there are multiple values associated with the key, then the first one is returned. + /// Use `get_all` to get all values associated with a given key. Returns `None` if there are no + /// values associated with the key. + pub fn get(&self, key: impl AsHeaderName) -> Option<&HeaderValue> { + self.get_value(key).map(|val| val.first()) + } + + /// Returns a mutable reference to the first value associated a header name. + /// + /// If there are multiple values associated with the key, then the first one is returned. + /// Use `get_all` to get all values associated with a given key. Returns `None` if there are no + /// values associated with the key. + pub fn get_mut(&mut self, key: impl AsHeaderName) -> Option<&mut HeaderValue> { + match key.try_as_name().ok()? { + Cow::Borrowed(name) => self.inner.get_mut(name).map(|v| v.first_mut()), + Cow::Owned(name) => self.inner.get_mut(&name).map(|v| v.first_mut()), + } + } + + /// Returns an iterator of all values associated with a header name. /// /// The returned view does not incur any allocations and allows iterating the values associated - /// with the key. Returns `None` if there are no values associated with the key. Iteration order - /// is not guaranteed to be the same as insertion order. - pub fn get_all(&self, name: N) -> GetAll<'_> { - GetAll { - idx: 0, - item: self.get2(name), - } - } - - /// Returns a mutable reference to the value associated with the key. - /// - /// If there are multiple values associated with the key, then the first one - /// is returned. Use `entry` to get all values associated with a given - /// key. Returns `None` if there are no values associated with the key. - pub fn get_mut(&mut self, name: N) -> Option<&mut HeaderValue> { - match name.as_name() { - Either::Left(name) => self.inner.get_mut(name).map(|v| v.first_mut()), - Either::Right(s) => { - if let Ok(name) = HeaderName::try_from(s) { - self.inner.get_mut(&name).map(|v| v.first_mut()) - } else { - None - } - } - } + /// with the key. Iterator will yield no items if there are no values associated with the key. + /// Iteration order is not guaranteed to be the same as insertion order. + pub fn get_all(&self, key: impl AsHeaderName) -> GetAll<'_> { + GetAll::new(self.get_value(key)) } /// Returns true if the map contains a value for the specified key. - pub fn contains_key(&self, key: N) -> bool { - match key.as_name() { - Either::Left(name) => self.inner.contains_key(name), - Either::Right(s) => { - if let Ok(name) = HeaderName::try_from(s) { - self.inner.contains_key(&name) - } else { - false - } - } + /// + /// Invalid header names will simply return false. + pub fn contains_key(&self, key: impl AsHeaderName) -> bool { + match key.try_as_name() { + Ok(Cow::Borrowed(name)) => self.inner.contains_key(name), + Ok(Cow::Owned(name)) => self.inner.contains_key(&name), + Err(_) => false, } } - /// An iterator visiting all key-value pairs. + /// An iterator visiting all name-value pairs. /// - /// The iteration order is arbitrary, but consistent across platforms for - /// the same crate version. Each key will be yielded once per associated - /// value. So, if a key has 3 associated values, it will be yielded 3 times. + /// The iteration order is arbitrary but consistent across platforms for the same crate version. + /// Each key will be yielded once per associated value. So, if a key has 3 associated values, it + /// will be yielded 3 times. pub fn iter(&self) -> Iter<'_> { Iter::new(self.inner.iter()) } /// An iterator visiting all keys. /// - /// The iteration order is arbitrary, but consistent across platforms for - /// the same crate version. Each key will be yielded only once even if it - /// has multiple associated values. + /// The iteration order is arbitrary but consistent across platforms for the same crate version. + /// Each key will be yielded only once even if it has multiple associated values. pub fn keys(&self) -> Keys<'_> { Keys(self.inner.keys()) } - /// Inserts a key-value pair into the map. + /// Inserts a name-value pair into the map. /// - /// If the map did not previously have this key present, then `None` is - /// returned. - /// - /// If the map did have this key present, the new value is associated with - /// the key and all previous values are removed. **Note** that only a single - /// one of the previous values is returned. If there are multiple values - /// that have been previously associated with the key, then the first one is - /// returned. See `insert_mult` on `OccupiedEntry` for an API that returns + /// If the map did have this key present, the new value is associated with the key and all + /// previous values are removed. **Note** that only a single one of the previous values + /// is returned. If there are multiple values that have been previously associated with the key, + /// then the first one is returned. See `insert_mult` on `OccupiedEntry` for an API that returns /// all values. /// /// The key is not updated, though; this matters for types that can be `==` /// without being identical. pub fn insert(&mut self, key: HeaderName, val: HeaderValue) { - let _ = self.inner.insert(key, Value::One(val)); + self.inner.insert(key, Value::One(val)); } - /// Inserts a key-value pair into the map. + /// Inserts a name-value pair into the map. /// - /// If the map did not previously have this key present, then `false` is - /// returned. - /// - /// If the map did have this key present, the new value is pushed to the end - /// of the list of values currently associated with the key. The key is not - /// updated, though; this matters for types that can be `==` without being - /// identical. + /// If the map did have this key present, the new value is pushed to the end of the list of + /// values currently associated with the key. The key is not updated, though; this matters for + /// types that can be `==` without being identical. pub fn append(&mut self, key: HeaderName, value: HeaderValue) { match self.inner.entry(key) { - Entry::Occupied(mut entry) => entry.get_mut().append(value), - Entry::Vacant(entry) => { + hash_map::Entry::Occupied(mut entry) => { + entry.get_mut().append(value); + } + hash_map::Entry::Vacant(entry) => { entry.insert(Value::One(value)); } - } + }; } /// Removes all headers for a particular header name from the map. - pub fn remove(&mut self, key: N) { - match key.as_name() { - Either::Left(name) => { - let _ = self.inner.remove(name); - } - Either::Right(s) => { - if let Ok(name) = HeaderName::try_from(s) { - let _ = self.inner.remove(&name); - } - } + // TODO: return value somehow + pub fn remove(&mut self, key: impl AsHeaderName) { + match key.try_as_name() { + Ok(Cow::Borrowed(name)) => self.inner.remove(name), + Ok(Cow::Owned(name)) => self.inner.remove(&name), + Err(_) => None, + }; + } +} + +impl IntoIterator for HeaderMap { + type Item = (HeaderName, HeaderValue); + type IntoIter = IntoIter; + + #[inline] + fn into_iter(self) -> Self::IntoIter { + IntoIter::new(self.inner.into_iter()) + } +} + +impl<'a> IntoIterator for &'a HeaderMap { + type Item = (&'a HeaderName, &'a HeaderValue); + type IntoIter = Iter<'a>; + + #[inline] + fn into_iter(self) -> Self::IntoIter { + Iter::new(self.inner.iter()) + } +} + +pub trait AsHeaderName: sealed::Sealed {} + +mod sealed { + use super::*; + + pub trait Sealed { + fn try_as_name(&self) -> Result, InvalidHeaderName>; + } + + impl Sealed for HeaderName { + fn try_as_name(&self) -> Result, InvalidHeaderName> { + Ok(Cow::Borrowed(self)) } } -} + impl AsHeaderName for HeaderName {} -#[doc(hidden)] -pub trait AsName { - fn as_name(&self) -> Either<&HeaderName, &str>; -} - -impl AsName for HeaderName { - fn as_name(&self) -> Either<&HeaderName, &str> { - Either::Left(self) + impl Sealed for &HeaderName { + fn try_as_name(&self) -> Result, InvalidHeaderName> { + Ok(Cow::Borrowed(*self)) + } } -} + impl AsHeaderName for &HeaderName {} -impl<'a> AsName for &'a HeaderName { - fn as_name(&self) -> Either<&HeaderName, &str> { - Either::Left(self) + impl Sealed for &str { + fn try_as_name(&self) -> Result, InvalidHeaderName> { + HeaderName::from_str(self).map(Cow::Owned) + } } -} + impl AsHeaderName for &str {} -impl<'a> AsName for &'a str { - fn as_name(&self) -> Either<&HeaderName, &str> { - Either::Right(self) + impl Sealed for String { + fn try_as_name(&self) -> Result, InvalidHeaderName> { + HeaderName::from_str(self).map(Cow::Owned) + } } -} + impl AsHeaderName for String {} -impl AsName for String { - fn as_name(&self) -> Either<&HeaderName, &str> { - Either::Right(self.as_str()) - } -} - -impl<'a> AsName for &'a String { - fn as_name(&self) -> Either<&HeaderName, &str> { - Either::Right(self.as_str()) + impl Sealed for &String { + fn try_as_name(&self) -> Result, InvalidHeaderName> { + HeaderName::from_str(self).map(Cow::Owned) + } } + impl AsHeaderName for &String {} } /// Iterator for all values in a `HeaderMap` with the same name. pub struct GetAll<'a> { idx: usize, - item: Option<&'a Value>, + value: Option<&'a Value>, +} + +impl<'a> GetAll<'a> { + fn new(value: Option<&'a Value>) -> Self { + Self { idx: 0, value } + } } impl<'a> Iterator for GetAll<'a> { @@ -296,25 +292,25 @@ impl<'a> Iterator for GetAll<'a> { #[inline] fn next(&mut self) -> Option<&'a HeaderValue> { - if let Some(ref val) = self.item { - match val { - Value::One(ref val) => { - self.item.take(); + let val = self.value?; + + match val { + Value::One(ref val) => { + // remove value to fast-path future next calls + self.value = None; + Some(val) + } + Value::Multi(ref vec) => match vec.get(self.idx) { + Some(val) => { + self.idx += 1; Some(val) } - Value::Multi(ref vec) => { - if self.idx < vec.len() { - let item = Some(&vec[self.idx]); - self.idx += 1; - item - } else { - self.item.take(); - None - } + None => { + // current index is none; remove value to fast-path future next calls + self.value = None; + None } - } - } else { - None + }, } } } @@ -330,27 +326,18 @@ impl<'a> Iterator for Keys<'a> { } } -impl<'a> IntoIterator for &'a HeaderMap { - type Item = (&'a HeaderName, &'a HeaderValue); - type IntoIter = Iter<'a>; - - fn into_iter(self) -> Self::IntoIter { - self.iter() - } -} - pub struct Iter<'a> { - idx: usize, - current: Option<(&'a HeaderName, &'a SmallVec<[HeaderValue; 4]>)>, iter: hash_map::Iter<'a, HeaderName, Value>, + multi_inner: Option<(&'a HeaderName, &'a SmallVec<[HeaderValue; 4]>)>, + multi_idx: usize, } impl<'a> Iter<'a> { fn new(iter: hash_map::Iter<'a, HeaderName, Value>) -> Self { Self { iter, - idx: 0, - current: None, + multi_idx: 0, + multi_inner: None, } } } @@ -360,26 +347,171 @@ impl<'a> Iterator for Iter<'a> { #[inline] fn next(&mut self) -> Option<(&'a HeaderName, &'a HeaderValue)> { - if let Some(ref mut item) = self.current { - if self.idx < item.1.len() { - let item = (item.0, &item.1[self.idx]); - self.idx += 1; - return Some(item); - } else { - self.idx = 0; - self.current.take(); - } - } - if let Some(item) = self.iter.next() { - match item.1 { - Value::One(ref value) => Some((item.0, value)), - Value::Multi(ref vec) => { - self.current = Some((item.0, vec)); - self.next() + // handle in-progress multi value lists first + if let Some((ref name, ref mut vals)) = self.multi_inner { + match vals.get(self.multi_idx) { + Some(val) => { + self.multi_idx += 1; + return Some((name, val)); + } + None => { + // no more items in value list; reset state + self.multi_idx = 0; + self.multi_inner = None; } } - } else { - None + } + + let (name, value) = self.iter.next()?; + + match value { + Value::One(ref value) => Some((name, value)), + Value::Multi(ref vals) => { + // set up new multi value inner iter and recurse into it + self.multi_inner = Some((name, vals)); + self.next() + } } } } + +pub struct IntoIter { + iter: hash_map::IntoIter, + multi_inner: Option<(HeaderName, smallvec::IntoIter<[HeaderValue; 4]>)>, +} + +impl IntoIter { + fn new(iter: hash_map::IntoIter) -> Self { + Self { + iter, + multi_inner: None, + } + } +} + +impl Iterator for IntoIter { + type Item = (HeaderName, HeaderValue); + + #[inline] + fn next(&mut self) -> Option<(HeaderName, HeaderValue)> { + // handle in-progress multi value iterators first + if let Some((ref name, ref mut vals)) = self.multi_inner { + match vals.next() { + Some(val) => { + return Some((name.clone(), val)); + } + None => { + // no more items in value iterator; reset state + self.multi_inner = None; + } + } + } + + let (name, value) = self.iter.next()?; + + match value { + Value::One(value) => Some((name, value)), + Value::Multi(vals) => { + // set up new multi value inner iter and recurse into it + self.multi_inner = Some((name, vals.into_iter())); + self.next() + } + } + } +} + +#[cfg(test)] +mod tests { + use http::header; + + use super::*; + + #[test] + fn test_new() { + let map = HeaderMap::new(); + assert_eq!(map.len(), 0); + assert_eq!(map.capacity(), 0); + + let map = HeaderMap::with_capacity(16); + assert_eq!(map.len(), 0); + assert!(map.capacity() >= 16); + } + + #[test] + fn test_insert() { + let mut map = HeaderMap::new(); + + map.insert(header::LOCATION, HeaderValue::from_static("/test")); + assert_eq!(map.len(), 1); + } + + #[test] + fn test_contains() { + let mut map = HeaderMap::new(); + assert!(!map.contains_key(header::LOCATION)); + + map.insert(header::LOCATION, HeaderValue::from_static("/test")); + assert!(map.contains_key(header::LOCATION)); + assert!(map.contains_key("Location")); + assert!(map.contains_key("Location".to_owned())); + assert!(map.contains_key("location")); + } + + #[test] + fn test_entries_iter() { + let mut map = HeaderMap::new(); + + map.append(header::HOST, HeaderValue::from_static("duck.com")); + map.append(header::COOKIE, HeaderValue::from_static("one=1")); + map.append(header::COOKIE, HeaderValue::from_static("two=2")); + + let mut iter = map.iter(); + assert!(iter.next().is_some()); + assert!(iter.next().is_some()); + assert!(iter.next().is_some()); + assert!(iter.next().is_none()); + + let pairs = map.iter().collect::>(); + assert!(pairs.contains(&(&header::HOST, &HeaderValue::from_static("duck.com")))); + assert!(pairs.contains(&(&header::COOKIE, &HeaderValue::from_static("one=1")))); + assert!(pairs.contains(&(&header::COOKIE, &HeaderValue::from_static("two=2")))); + } + + #[test] + fn test_entries_into_iter() { + let mut map = HeaderMap::new(); + + map.append(header::HOST, HeaderValue::from_static("duck.com")); + map.append(header::COOKIE, HeaderValue::from_static("one=1")); + map.append(header::COOKIE, HeaderValue::from_static("two=2")); + + let mut iter = map.into_iter(); + assert!(iter.next().is_some()); + assert!(iter.next().is_some()); + assert!(iter.next().is_some()); + assert!(iter.next().is_none()); + } + + #[test] + fn test_entries_iter_and_into_iter_same_order() { + let mut map = HeaderMap::new(); + + map.append(header::HOST, HeaderValue::from_static("duck.com")); + map.append(header::COOKIE, HeaderValue::from_static("one=1")); + map.append(header::COOKIE, HeaderValue::from_static("two=2")); + + let mut iter = map.iter(); + let mut into_iter = map.clone().into_iter(); + + assert_eq!(iter.next().map(owned_pair), into_iter.next()); + assert_eq!(iter.next().map(owned_pair), into_iter.next()); + assert_eq!(iter.next().map(owned_pair), into_iter.next()); + assert_eq!(iter.next().map(owned_pair), into_iter.next()); + } + + fn owned_pair<'a>( + (name, val): (&'a HeaderName, &'a HeaderValue), + ) -> (HeaderName, HeaderValue) { + (name.clone(), val.clone()) + } +} diff --git a/actix-http/src/request.rs b/actix-http/src/request.rs index 1031d7dce..fbf4d3c23 100644 --- a/actix-http/src/request.rs +++ b/actix-http/src/request.rs @@ -177,13 +177,17 @@ impl

fmt::Debug for Request

{ self.method(), self.path() )?; + if let Some(q) = self.uri().query().as_ref() { writeln!(f, " query: ?{:?}", q)?; } + writeln!(f, " headers:")?; - for (key, val) in self.headers() { + + for (key, val) in self.headers().iter() { writeln!(f, " {:?}: {:?}", key, val)?; } + Ok(()) } } diff --git a/actix-http/src/response.rs b/actix-http/src/response.rs index 110514e05..b9850f750 100644 --- a/actix-http/src/response.rs +++ b/actix-http/src/response.rs @@ -752,9 +752,11 @@ impl<'a> From<&'a ResponseHead> for ResponseBuilder { let mut msg = BoxedResponseHead::new(head.status); msg.version = head.version; msg.reason = head.reason; - for (k, v) in &head.headers { + + for (k, v) in head.headers.iter() { msg.headers.append(k.clone(), v.clone()); } + msg.no_chunking(!head.chunked()); ResponseBuilder {