diff --git a/actix-http/src/header/mod.rs b/actix-http/src/header/mod.rs index 721f4ea79..308cb0123 100644 --- a/actix-http/src/header/mod.rs +++ b/actix-http/src/header/mod.rs @@ -55,7 +55,7 @@ pub trait Header: IntoHeaderValue { fn name() -> HeaderName; /// Parse a header - fn parse(msg: &T) -> Result; + fn parse(msg: &M) -> Result; } /// Convert `http::HeaderMap` to our `HeaderMap`. diff --git a/actix-http/src/header/utils.rs b/actix-http/src/header/utils.rs index 94c5b6dcb..2168202b9 100644 --- a/actix-http/src/header/utils.rs +++ b/actix-http/src/header/utils.rs @@ -12,7 +12,8 @@ where I: Iterator + 'a, T: FromStr, { - let mut result = Vec::new(); + let size_guess = all.size_hint().1.unwrap_or(2); + let mut result = Vec::with_capacity(size_guess); for h in all { let s = h.to_str().map_err(|_| ParseError::Header)?; @@ -26,6 +27,7 @@ where .filter_map(|x| x.trim().parse().ok()), ) } + Ok(result) } @@ -34,10 +36,12 @@ where pub fn from_one_raw_str(val: Option<&HeaderValue>) -> Result { if let Some(line) = val { let line = line.to_str().map_err(|_| ParseError::Header)?; + if !line.is_empty() { return T::from_str(line).or(Err(ParseError::Header)); } } + Err(ParseError::Header) } @@ -48,13 +52,16 @@ where T: fmt::Display, { let mut iter = parts.iter(); + if let Some(part) = iter.next() { fmt::Display::fmt(part, f)?; } + for part in iter { f.write_str(", ")?; fmt::Display::fmt(part, f)?; } + Ok(()) } @@ -65,3 +72,25 @@ pub fn http_percent_encode(f: &mut fmt::Formatter<'_>, bytes: &[u8]) -> fmt::Res let encoded = percent_encoding::percent_encode(bytes, HTTP_VALUE); fmt::Display::fmt(&encoded, f) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn comma_delimited_parsing() { + let headers = vec![]; + let res: Vec = from_comma_delimited(headers.iter()).unwrap(); + assert_eq!(res, vec![0; 0]); + + let headers = vec![ + HeaderValue::from_static(""), + HeaderValue::from_static(","), + HeaderValue::from_static(" "), + HeaderValue::from_static("1 ,"), + HeaderValue::from_static(""), + ]; + let res: Vec = from_comma_delimited(headers.iter()).unwrap(); + assert_eq!(res, vec![1]); + } +} diff --git a/src/http/header/accept.rs b/src/http/header/accept.rs index 91f7b27d2..bc794c02c 100644 --- a/src/http/header/accept.rs +++ b/src/http/header/accept.rs @@ -77,7 +77,7 @@ crate::http::header::common_header! { /// ]) /// ); /// ``` - (Accept, header::ACCEPT) => (QualityItem)+ + (Accept, header::ACCEPT) => (QualityItem)* test_parse_and_format { // Tests from the RFC @@ -207,12 +207,29 @@ impl Accept { /// If no q-factors are provided, the first mime type is chosen. Note that items without /// q-factors are given the maximum preference value. /// - /// Returns `None` if contained list is empty. + /// As per the spec, will return [`Mime::STAR_STAR`] (indicating no preference) if the contained + /// list is empty. /// /// [q-factor weighting]: https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.2 - pub fn preference(&self) -> Option { - // PERF: creating a sorted list is not necessary - self.ranked().into_iter().next() + pub fn preference(&self) -> Mime { + use actix_http::header::q; + + let mut max_item = None; + let mut max_pref = q(0); + + // uses manual max lookup loop since we want the first occurrence in the case of same + // preference but `Iterator::max_by_key` would give us the last occurrence + + for pref in &self.0 { + // only change if strictly greater + // equal items, even while unsorted, still have higher preference if they appear first + if pref.quality > max_pref { + max_pref = pref.quality; + max_item = Some(pref.item.clone()); + } + } + + max_item.unwrap_or(mime::STAR_STAR) } } @@ -264,7 +281,7 @@ mod tests { QualityItem::new("application/xml".parse().unwrap(), q(0.9)), QualityItem::new(mime::STAR_STAR, q(0.8)), ]); - assert_eq!(test.preference(), Some(mime::TEXT_HTML)); + assert_eq!(test.preference(), mime::TEXT_HTML); let test = Accept(vec![ QualityItem::new("video/*".parse().unwrap(), q(0.8)), @@ -273,6 +290,6 @@ mod tests { qitem(mime::IMAGE_SVG), QualityItem::new(mime::IMAGE_STAR, q(0.8)), ]); - assert_eq!(test.preference(), Some(mime::IMAGE_PNG)); + assert_eq!(test.preference(), mime::IMAGE_PNG); } } diff --git a/src/http/header/accept_language.rs b/src/http/header/accept_language.rs index b451673fb..e96d1d13e 100644 --- a/src/http/header/accept_language.rs +++ b/src/http/header/accept_language.rs @@ -1,6 +1,6 @@ use language_tags::LanguageTag; -use super::{common_header, AnyOrSome, QualityItem}; +use super::{common_header, Preference, QualityItem}; use crate::http::header; common_header! { @@ -55,9 +55,13 @@ common_header! { /// ]) /// ); /// ``` - (AcceptLanguage, header::ACCEPT_LANGUAGE) => (QualityItem>)+ + (AcceptLanguage, header::ACCEPT_LANGUAGE) => (QualityItem>)* parse_and_fmt_tests { + common_header_test!(no_headers, vec![b""; 0], Some(AcceptLanguage(vec![]))); + + common_header_test!(empty_header, vec![b""; 1], Some(AcceptLanguage(vec![]))); + common_header_test!( example_from_rfc, vec![b"da, en-gb;q=0.8, en;q=0.7"] @@ -92,7 +96,7 @@ impl AcceptLanguage { /// for [q-factor weighting]. /// /// [q-factor weighting]: https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.2 - pub fn ranked(&self) -> Vec> { + pub fn ranked(&self) -> Vec> { if self.0.is_empty() { return vec![]; } @@ -113,12 +117,28 @@ impl AcceptLanguage { /// If no q-factors are provided, the first language is chosen. Note that items without /// q-factors are given the maximum preference value. /// - /// As per the spec, returns [`AnyOrSome::Any`] if contained list is empty. + /// As per the spec, returns [`Preference::Any`] if contained list is empty. /// /// [q-factor weighting]: https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.2 - pub fn preference(&self) -> AnyOrSome { - // PERF: creating a sorted list is not necessary - self.ranked().into_iter().next().unwrap_or(AnyOrSome::Any) + pub fn preference(&self) -> Preference { + use actix_http::header::q; + + let mut max_item = None; + let mut max_pref = q(0); + + // uses manual max lookup loop since we want the first occurrence in the case of same + // preference but `Iterator::max_by_key` would give us the last occurrence + + for pref in &self.0 { + // only change if strictly greater + // equal items, even while unsorted, still have higher preference if they appear first + if pref.quality > max_pref { + max_pref = pref.quality; + max_item = Some(pref.item.clone()); + } + } + + max_item.unwrap_or(Preference::Any) } } @@ -181,7 +201,10 @@ mod tests { QualityItem::new("*".parse().unwrap(), q(500)), QualityItem::new("de".parse().unwrap(), q(700)), ]); - assert_eq!(test.preference(), AnyOrSome::Item("fr-CH".parse().unwrap())); + assert_eq!( + test.preference(), + Preference::Specific("fr-CH".parse().unwrap()) + ); let test = AcceptLanguage(vec![ qitem("fr".parse().unwrap()), @@ -190,9 +213,12 @@ mod tests { qitem("*".parse().unwrap()), qitem("de".parse().unwrap()), ]); - assert_eq!(test.preference(), AnyOrSome::Item("fr".parse().unwrap())); + assert_eq!( + test.preference(), + Preference::Specific("fr".parse().unwrap()) + ); let test = AcceptLanguage(vec![]); - assert_eq!(test.preference(), AnyOrSome::Any); + assert_eq!(test.preference(), Preference::Any); } } diff --git a/src/http/header/any_or_some.rs b/src/http/header/any_or_some.rs deleted file mode 100644 index e5a37e495..000000000 --- a/src/http/header/any_or_some.rs +++ /dev/null @@ -1,70 +0,0 @@ -use std::{ - fmt::{self, Write as _}, - str, -}; - -/// A wrapper for types used in header values where wildcard (`*`) items are allowed but the -/// underlying type does not support them. -/// -/// For example, we use the `language-tags` crate for the [`AcceptLanguage`](super::AcceptLanguage) -/// typed header but it does parse `*` successfully. On the other hand, the `mime` crate, used for -/// [`Accept`](super::Accept), has first-party support for wildcard items so this wrapper is not -/// used in those header types. -#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Hash)] -pub enum AnyOrSome { - /// A wildcard value. - Any, - - /// A valid `T`. - Item(T), -} - -impl AnyOrSome { - /// Returns true if item is wildcard (`*`) variant. - pub fn is_any(&self) -> bool { - matches!(self, Self::Any) - } - - /// Returns true if item is a valid item (`T`) variant. - pub fn is_item(&self) -> bool { - matches!(self, Self::Item(_)) - } - - /// Returns reference to value in `Item` variant, if it is set. - pub fn item(&self) -> Option<&T> { - match self { - AnyOrSome::Item(ref item) => Some(item), - AnyOrSome::Any => None, - } - } - - /// Consumes the container, returning the value in the `Item` variant, if it is set. - pub fn into_item(self) -> Option { - match self { - AnyOrSome::Item(item) => Some(item), - AnyOrSome::Any => None, - } - } -} - -impl fmt::Display for AnyOrSome { - #[inline] - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - AnyOrSome::Any => f.write_char('*'), - AnyOrSome::Item(item) => fmt::Display::fmt(item, f), - } - } -} - -impl str::FromStr for AnyOrSome { - type Err = T::Err; - - #[inline] - fn from_str(s: &str) -> Result { - match s.trim() { - "*" => Ok(Self::Any), - other => other.parse().map(AnyOrSome::Item), - } - } -} diff --git a/src/http/header/macros.rs b/src/http/header/macros.rs index 0da331349..7fed7f286 100644 --- a/src/http/header/macros.rs +++ b/src/http/header/macros.rs @@ -19,16 +19,18 @@ macro_rules! common_header_deref { }; } +/// Sets up a test module with some useful imports for use with [`common_header_test!`]. macro_rules! common_header_test_module { ($id:ident, $tm:ident{$($tf:item)*}) => { - #[allow(unused_imports)] #[cfg(test)] mod $tm { + #![allow(unused_imports)] + use std::str; use actix_http::http::Method; use mime::*; use $crate::http::header::*; - use super::$id as HeaderField; + use super::{$id as HeaderField, *}; $($tf)* } } @@ -71,7 +73,7 @@ macro_rules! common_header_test { } }; - ($id:ident, $raw:expr, $typed:expr) => { + ($id:ident, $raw:expr, $exp:expr) => { #[test] fn $id() { use actix_http::test; @@ -83,28 +85,35 @@ macro_rules! common_header_test { } let req = req.finish(); let val = HeaderField::parse(&req); - let typed: Option = $typed; + let exp: Option = $exp; - // Test parsing - assert_eq!(val.ok(), typed); + println!("req: {:?}", &req); + println!("val: {:?}", &val); + println!("exp: {:?}", &exp); - // Test formatting - if typed.is_some() { + // test parsing + assert_eq!(val.ok(), exp); + + // test formatting + if let Some(exp) = exp { let raw = &($raw)[..]; let mut iter = raw.iter().map(|b| str::from_utf8(&b[..]).unwrap()); let mut joined = String::new(); - joined.push_str(iter.next().unwrap()); - for s in iter { - joined.push_str(", "); + if let Some(s) = iter.next() { joined.push_str(s); + for s in iter { + joined.push_str(", "); + joined.push_str(s); + } } - assert_eq!(format!("{}", typed.unwrap()), joined); + assert_eq!(format!("{}", exp), joined); } } }; } macro_rules! common_header { + // TODO: these docs are wrong, there's no $n or $nn // $a:meta: Attributes associated with the header item (usually docs) // $id:ident: Identifier of the header // $n:expr: Lowercase name of the header @@ -123,7 +132,7 @@ macro_rules! common_header { } #[inline] - fn parse(msg: &T) -> Result { + fn parse(msg: &M) -> Result { let headers = msg.headers().get_all(Self::name()); $crate::http::header::from_comma_delimited(headers).map($id) } @@ -162,9 +171,7 @@ macro_rules! common_header { $name } #[inline] - fn parse(msg: &T) -> Result - where T: $crate::HttpMessage - { + fn parse(msg: &M) -> Result { $crate::http::header::from_comma_delimited( msg.headers().get_all(Self::name())).map($id) } @@ -204,7 +211,7 @@ macro_rules! common_header { } #[inline] - fn parse(msg: &T) -> Result { + fn parse(msg: &M) -> Result { let header = msg.headers().get(Self::name()); $crate::http::header::from_one_raw_str(header).map($id) } @@ -244,7 +251,7 @@ macro_rules! common_header { } #[inline] - fn parse(msg: &T) -> Result { + fn parse(msg: &M) -> Result { let is_any = msg .headers() .get(Self::name()) diff --git a/src/http/header/mod.rs b/src/http/header/mod.rs index fb478d1a0..45d5b8d1a 100644 --- a/src/http/header/mod.rs +++ b/src/http/header/mod.rs @@ -22,7 +22,6 @@ mod accept_charset; mod accept; mod accept_language; mod allow; -mod any_or_some; mod cache_control; mod content_disposition; mod content_language; @@ -40,6 +39,7 @@ mod if_range; mod if_unmodified_since; mod last_modified; mod macros; +mod preference; // mod range; #[cfg(test)] @@ -51,7 +51,6 @@ pub use self::accept_charset::AcceptCharset; pub use self::accept::Accept; pub use self::accept_language::AcceptLanguage; pub use self::allow::Allow; -pub use self::any_or_some::AnyOrSome; pub use self::cache_control::{CacheControl, CacheDirective}; pub use self::content_disposition::{ContentDisposition, DispositionParam, DispositionType}; pub use self::content_language::ContentLanguage; @@ -68,6 +67,7 @@ pub use self::if_none_match::IfNoneMatch; pub use self::if_range::IfRange; pub use self::if_unmodified_since::IfUnmodifiedSince; pub use self::last_modified::LastModified; +pub use self::preference::Preference; //pub use self::range::{Range, ByteRangeSpec}; /// Format writer ([`fmt::Write`]) for a [`BytesMut`]. diff --git a/src/http/header/preference.rs b/src/http/header/preference.rs new file mode 100644 index 000000000..9a69e1a1a --- /dev/null +++ b/src/http/header/preference.rs @@ -0,0 +1,70 @@ +use std::{ + fmt::{self, Write as _}, + str, +}; + +/// A wrapper for types used in header values where wildcard (`*`) items are allowed but the +/// underlying type does not support them. +/// +/// For example, we use the `language-tags` crate for the [`AcceptLanguage`](super::AcceptLanguage) +/// typed header but it does not parse `*` successfully. On the other hand, the `mime` crate, used +/// for [`Accept`](super::Accept), has first-party support for wildcard items so this wrapper is not +/// used in those header types. +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Hash)] +pub enum Preference { + /// A wildcard value. + Any, + + /// A valid `T`. + Specific(T), +} + +impl Preference { + /// Returns true if the preference is the any/wildcard (`*`) value. + pub fn is_any(&self) -> bool { + matches!(self, Self::Any) + } + + /// Returns true if variant is a specific item (`T`) variant. + pub fn is_specific(&self) -> bool { + matches!(self, Self::Specific(_)) + } + + /// Returns reference to value in `Specific` variant, if it is set. + pub fn item(&self) -> Option<&T> { + match self { + Preference::Specific(ref item) => Some(item), + Preference::Any => None, + } + } + + /// Consumes the container, returning the value in the `Specific` variant, if it is set. + pub fn into_item(self) -> Option { + match self { + Preference::Specific(item) => Some(item), + Preference::Any => None, + } + } +} + +impl fmt::Display for Preference { + #[inline] + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Preference::Any => f.write_char('*'), + Preference::Specific(item) => fmt::Display::fmt(item, f), + } + } +} + +impl str::FromStr for Preference { + type Err = T::Err; + + #[inline] + fn from_str(s: &str) -> Result { + match s.trim() { + "*" => Ok(Self::Any), + other => other.parse().map(Preference::Specific), + } + } +}