diff --git a/Cargo.toml b/Cargo.toml index dbe28df4a..96c8fef7b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -97,6 +97,7 @@ log = "0.4" mime = "0.3" paste = "1" pin-project = "1.0.0" +pin-project-lite = "0.2.7" regex = "1.4" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" diff --git a/actix-http/CHANGES.md b/actix-http/CHANGES.md index 1cc7ae5fd..d3aa7fc0e 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 to be same as insertion order. [#2467] +* Expose `header::{GetAll, Removed}` iterators. [#2467] + +[#2467]: https://github.com/actix/actix-web/pull/2467 ## 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 a8fd9715b..c7e4921a8 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 18494f555..a9483a9ff 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 5e9652380..cf8636e9d 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 fdcde5eac..8ec04a5c3 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. diff --git a/src/extract.rs b/src/extract.rs index 29fd0d05e..bb2dabb9f 100644 --- a/src/extract.rs +++ b/src/extract.rs @@ -10,6 +10,7 @@ use std::{ use actix_http::http::{Method, Uri}; use actix_utils::future::{ok, Ready}; use futures_core::ready; +use pin_project_lite::pin_project; use crate::{dev::Payload, Error, HttpRequest}; @@ -139,10 +140,11 @@ where } } -#[pin_project::pin_project] -pub struct FromRequestOptFuture { - #[pin] - fut: Fut, +pin_project! { + pub struct FromRequestOptFuture { + #[pin] + fut: Fut, + } } impl Future for FromRequestOptFuture @@ -226,10 +228,11 @@ where } } -#[pin_project::pin_project] -pub struct FromRequestResFuture { - #[pin] - fut: Fut, +pin_project! { + pub struct FromRequestResFuture { + #[pin] + fut: Fut, + } } impl Future for FromRequestResFuture @@ -297,102 +300,104 @@ impl FromRequest for () { } } -macro_rules! tuple_from_req ({$fut_type:ident, $(($n:tt, $T:ident)),+} => { - - // This module is a trick to get around the inability of - // `macro_rules!` macros to make new idents. We want to make - // a new `FutWrapper` struct for each distinct invocation of - // this macro. Ideally, we would name it something like - // `FutWrapper_$fut_type`, but this can't be done in a macro_rules - // macro. - // - // Instead, we put everything in a module named `$fut_type`, thus allowing - // us to use the name `FutWrapper` without worrying about conflicts. - // This macro only exists to generate trait impls for tuples - these - // are inherently global, so users don't have to care about this - // weird trick. - #[allow(non_snake_case)] - mod $fut_type { - - // Bring everything into scope, so we don't need - // redundant imports - use super::*; - - /// A helper struct to allow us to pin-project through - /// to individual fields - #[pin_project::pin_project] - struct FutWrapper<$($T: FromRequest),+>($(#[pin] $T::Future),+); - - /// FromRequest implementation for tuple - #[doc(hidden)] - #[allow(unused_parens)] - impl<$($T: FromRequest + 'static),+> FromRequest for ($($T,)+) - { - type Error = Error; - type Future = $fut_type<$($T),+>; - - fn from_request(req: &HttpRequest, payload: &mut Payload) -> Self::Future { - $fut_type { - items: <($(Option<$T>,)+)>::default(), - futs: FutWrapper($($T::from_request(req, payload),)+), - } - } - } - - #[doc(hidden)] - #[pin_project::pin_project] - pub struct $fut_type<$($T: FromRequest),+> { - items: ($(Option<$T>,)+), - #[pin] - futs: FutWrapper<$($T,)+>, - } - - impl<$($T: FromRequest),+> Future for $fut_type<$($T),+> - { - type Output = Result<($($T,)+), Error>; - - fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - let mut this = self.project(); - - let mut ready = true; - $( - if this.items.$n.is_none() { - match this.futs.as_mut().project().$n.poll(cx) { - Poll::Ready(Ok(item)) => { - this.items.$n = Some(item); - } - Poll::Pending => ready = false, - Poll::Ready(Err(e)) => return Poll::Ready(Err(e.into())), - } - } - )+ - - if ready { - Poll::Ready(Ok( - ($(this.items.$n.take().unwrap(),)+) - )) - } else { - Poll::Pending - } - } - } - } -}); - -#[rustfmt::skip] -mod m { +#[doc(hidden)] +#[allow(non_snake_case)] +mod tuple_from_req { use super::*; - tuple_from_req!(TupleFromRequest1, (0, A)); - tuple_from_req!(TupleFromRequest2, (0, A), (1, B)); - tuple_from_req!(TupleFromRequest3, (0, A), (1, B), (2, C)); - tuple_from_req!(TupleFromRequest4, (0, A), (1, B), (2, C), (3, D)); - tuple_from_req!(TupleFromRequest5, (0, A), (1, B), (2, C), (3, D), (4, E)); - tuple_from_req!(TupleFromRequest6, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F)); - tuple_from_req!(TupleFromRequest7, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F), (6, G)); - tuple_from_req!(TupleFromRequest8, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F), (6, G), (7, H)); - tuple_from_req!(TupleFromRequest9, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F), (6, G), (7, H), (8, I)); - tuple_from_req!(TupleFromRequest10, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F), (6, G), (7, H), (8, I), (9, J)); + macro_rules! tuple_from_req { + ($fut: ident; $($T: ident),*) => { + /// FromRequest implementation for tuple + #[allow(unused_parens)] + impl<$($T: FromRequest + 'static),+> FromRequest for ($($T,)+) + { + type Error = Error; + type Future = $fut<$($T),+>; + + fn from_request(req: &HttpRequest, payload: &mut Payload) -> Self::Future { + $fut { + $( + $T: ExtractFuture::Future { + fut: $T::from_request(req, payload) + }, + )+ + } + } + } + + pin_project! { + pub struct $fut<$($T: FromRequest),+> { + $( + #[pin] + $T: ExtractFuture<$T::Future, $T>, + )+ + } + } + + impl<$($T: FromRequest),+> Future for $fut<$($T),+> + { + type Output = Result<($($T,)+), Error>; + + fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + let mut this = self.project(); + + let mut ready = true; + $( + match this.$T.as_mut().project() { + ExtractProj::Future { fut } => match fut.poll(cx) { + Poll::Ready(Ok(output)) => { + let _ = this.$T.as_mut().project_replace(ExtractFuture::Done { output }); + }, + Poll::Ready(Err(e)) => return Poll::Ready(Err(e.into())), + Poll::Pending => ready = false, + }, + ExtractProj::Done { .. } => {}, + ExtractProj::Empty => unreachable!("FromRequest polled after finished"), + } + )+ + + if ready { + Poll::Ready(Ok( + ($( + match this.$T.project_replace(ExtractFuture::Empty) { + ExtractReplaceProj::Done { output } => output, + _ => unreachable!("FromRequest polled after finished"), + }, + )+) + )) + } else { + Poll::Pending + } + } + } + }; + } + + pin_project! { + #[project = ExtractProj] + #[project_replace = ExtractReplaceProj] + enum ExtractFuture { + Future { + #[pin] + fut: Fut + }, + Done { + output: Res, + }, + Empty + } + } + + tuple_from_req! { TupleFromRequest1; A } + tuple_from_req! { TupleFromRequest2; A, B } + tuple_from_req! { TupleFromRequest3; A, B, C } + tuple_from_req! { TupleFromRequest4; A, B, C, D } + tuple_from_req! { TupleFromRequest5; A, B, C, D, E } + tuple_from_req! { TupleFromRequest6; A, B, C, D, E, F } + tuple_from_req! { TupleFromRequest7; A, B, C, D, E, F, G } + tuple_from_req! { TupleFromRequest8; A, B, C, D, E, F, G, H } + tuple_from_req! { TupleFromRequest9; A, B, C, D, E, F, G, H, I } + tuple_from_req! { TupleFromRequest10; A, B, C, D, E, F, G, H, I, J } } #[cfg(test)] @@ -494,4 +499,26 @@ mod tests { let method = Method::extract(&req).await.unwrap(); assert_eq!(method, Method::GET); } + + #[actix_rt::test] + async fn test_concurrent() { + let (req, mut pl) = TestRequest::default() + .uri("/foo/bar") + .method(Method::GET) + .insert_header((header::CONTENT_TYPE, "application/x-www-form-urlencoded")) + .insert_header((header::CONTENT_LENGTH, "11")) + .set_payload(Bytes::from_static(b"hello=world")) + .to_http_parts(); + let (method, uri, form) = <(Method, Uri, Form)>::from_request(&req, &mut pl) + .await + .unwrap(); + assert_eq!(method, Method::GET); + assert_eq!(uri.path(), "/foo/bar"); + assert_eq!( + form, + Form(Info { + hello: "world".into() + }) + ); + } }