remove from<string> bound on compress middleware

CHANGES.md

@@ -4,13 +4,17 @@
 ### Added
 * Re-export actix-service `ServiceFactory` in `dev` module. [#2325]
 
-### Changes
+### Changed
-* Fix quality parse error in Accept-Encoding HTTP header. [#2344]
 * Minimum supported Rust version (MSRV) is now 1.51.
+* Compress middleware will return 406 Not Acceptable when no content encoding is acceptable to the client. [#2344]
+
+### Fixed
+* Fix quality parse error in Accept-Encoding header. [#2344]
 
 [#2325]: https://github.com/actix/actix-web/pull/2325
 [#2344]: https://github.com/actix/actix-web/pull/2344
 
+
 ## 4.0.0-beta.8 - 2021-06-26
 ### Added
 * Add `ServiceRequest::parts_mut`. [#2177]

actix-http/CHANGES.md

@@ -1,13 +1,13 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
-### Changes
+### Changed
 * Minimum supported Rust version (MSRV) is now 1.51.
 
 ### Fixed
 * Remove slice creation pointing to potential uninitialized data on h1 encoder. [#2364]
 * Remove `Into<Error>` bound on `Encoder` body types. [#2375]
-* Fix quality parse error in Accept-Encoding HTTP header. [#2344]
+* Fix quality parse error in Accept-Encoding header. [#2344]
 
 [#2364]: https://github.com/actix/actix-web/pull/2364
 [#2375]: https://github.com/actix/actix-web/pull/2375

actix-http/src/header/shared/content_encoding.rs

@@ -51,7 +51,7 @@ impl ContentEncoding {
         matches!(self, ContentEncoding::Identity | ContentEncoding::Auto)
     }
 
-    /// Convert content encoding to string
+    /// Convert content encoding to string.
     #[inline]
     pub fn as_str(self) -> &'static str {
         match self {
@@ -74,14 +74,6 @@ impl FromStr for ContentEncoding {
     type Err = ContentEncodingParseError;
 
     fn from_str(val: &str) -> Result<Self, Self::Err> {
-        Self::try_from(val)
-    }
-}
-
-impl TryFrom<&str> for ContentEncoding {
-    type Error = ContentEncodingParseError;
-
-    fn try_from(val: &str) -> Result<Self, Self::Error> {
         let val = val.trim();
 
         if val.eq_ignore_ascii_case("br") {
@@ -98,6 +90,14 @@ impl TryFrom<&str> for ContentEncoding {
     }
 }
 
+impl TryFrom<&str> for ContentEncoding {
+    type Error = ContentEncodingParseError;
+
+    fn try_from(val: &str) -> Result<Self, Self::Error> {
+        val.parse()
+    }
+}
+
 impl IntoHeaderValue for ContentEncoding {
     type Error = InvalidHeaderValue;
 

actix-http/src/header/shared/quality_item.rs

@@ -1,11 +1,14 @@
 use std::{
     cmp,
     convert::{TryFrom, TryInto},
-    fmt, str,
+    fmt,
+    str::{self, FromStr},
 };
 
 use derive_more::{Display, Error};
 
+use crate::error::ParseError;
+
 const MAX_QUALITY: u16 = 1000;
 const MAX_FLOAT_QUALITY: f32 = 1.0;
 
@@ -113,12 +116,12 @@ impl<T: fmt::Display> fmt::Display for QualityItem<T> {
     }
 }
 
-impl<T: str::FromStr> str::FromStr for QualityItem<T> {
+impl<T: FromStr> FromStr for QualityItem<T> {
-    type Err = crate::error::ParseError;
+    type Err = ParseError;
 
-    fn from_str(qitem_str: &str) -> Result<QualityItem<T>, crate::error::ParseError> {
+    fn from_str(qitem_str: &str) -> Result<Self, Self::Err> {
         if !qitem_str.is_ascii() {
-            return Err(crate::error::ParseError::Header);
+            return Err(ParseError::Header);
         }
 
         // Set defaults used if parsing fails.
@@ -139,7 +142,7 @@ impl<T: str::FromStr> str::FromStr for QualityItem<T> {
             if parts[0].len() < 2 {
                 // Can't possibly be an attribute since an attribute needs at least a name followed
                 // by an equals sign. And bare identifiers are forbidden.
-                return Err(crate::error::ParseError::Header);
+                return Err(ParseError::Header);
             }
 
             let start = &parts[0][0..2];
@@ -148,25 +151,21 @@ impl<T: str::FromStr> str::FromStr for QualityItem<T> {
                 let q_val = &parts[0][2..];
                 if q_val.len() > 5 {
                     // longer than 5 indicates an over-precise q-factor
-                    return Err(crate::error::ParseError::Header);
+                    return Err(ParseError::Header);
                 }
 
-                let q_value = q_val
+                let q_value = q_val.parse::<f32>().map_err(|_| ParseError::Header)?;
-                    .parse::<f32>()
-                    .map_err(|_| crate::error::ParseError::Header)?;
 
                 if (0f32..=1f32).contains(&q_value) {
                     quality = q_value;
                     raw_item = parts[1];
                 } else {
-                    return Err(crate::error::ParseError::Header);
+                    return Err(ParseError::Header);
                 }
             }
         }
 
-        let item = raw_item
+        let item = raw_item.parse::<T>().map_err(|_| ParseError::Header)?;
-            .parse::<T>()
-            .map_err(|_| crate::error::ParseError::Header)?;
 
         // we already checked above that the quality is within range
         Ok(QualityItem::new(item, Quality::from_f32(quality)))
@@ -224,7 +223,7 @@ mod tests {
         }
     }
 
-    impl str::FromStr for Encoding {
+    impl FromStr for Encoding {
         type Err = crate::error::ParseError;
         fn from_str(s: &str) -> Result<Encoding, crate::error::ParseError> {
             use Encoding::*;

src/middleware/compress.rs

@@ -18,6 +18,7 @@ use actix_http::{
 use actix_service::{Service, Transform};
 use actix_utils::future::{ok, Either, Ready};
 use futures_core::ready;
+use once_cell::sync::Lazy;
 use pin_project::pin_project;
 
 use crate::{
@@ -57,7 +58,7 @@ impl Default for Compress {
 
 impl<S, B> Transform<S, ServiceRequest> for Compress
 where
-    B: MessageBody + From<String>,
+    B: MessageBody,
     S: Service<ServiceRequest, Response = ServiceResponse<B>, Error = Error>,
 {
     type Response = ServiceResponse<ResponseBody<Encoder<B>>>;
@@ -79,7 +80,7 @@ pub struct CompressMiddleware<S> {
     encoding: ContentEncoding,
 }
 
-fn supported_algorithm_names() -> String {
+static SUPPORTED_ALGORITHM_NAMES: Lazy<String> = Lazy::new(|| {
     let mut encoding = vec![];
 
     #[cfg(feature = "compress-brotli")]
@@ -98,16 +99,16 @@ fn supported_algorithm_names() -> String {
 
     assert!(
         !encoding.is_empty(),
-        "encoding can not be empty unless __compress feature has been explicitly enabled"
+        "encoding can not be empty unless __compress feature has been explicitly enabled by itself"
     );
 
     encoding.join(", ")
-}
+});
 
 impl<S, B> Service<ServiceRequest> for CompressMiddleware<S>
 where
-    B: MessageBody + From<String>,
     S: Service<ServiceRequest, Response = ServiceResponse<B>, Error = Error>,
+    B: MessageBody,
 {
     type Response = ServiceResponse<ResponseBody<Encoder<B>>>;
     type Error = Error;
@@ -143,12 +144,12 @@ where
             Some(Err(_)) => {
                 let res = HttpResponse::with_body(
                     StatusCode::NOT_ACCEPTABLE,
-                    supported_algorithm_names().into(),
+                    SUPPORTED_ALGORITHM_NAMES.as_str(),
                 );
                 let enc = ContentEncoding::Identity;
 
                 Either::right(ok(req.into_response(res.map_body(move |head, body| {
-                    Encoder::response(enc, head, ResponseBody::Body(body))
+                    Encoder::response(enc, head, ResponseBody::Other(body.into()))
                 }))))
             }
         }
@@ -195,6 +196,7 @@ where
 
 struct AcceptEncoding {
     encoding: ContentEncoding,
+    // TODO: use Quality or QualityItem<ContentEncoding>
     quality: f64,
 }
 
@@ -221,17 +223,17 @@ impl PartialOrd for AcceptEncoding {
 
 impl PartialEq for AcceptEncoding {
     fn eq(&self, other: &AcceptEncoding) -> bool {
-        self.quality == other.quality && self.encoding == other.encoding
+        self.encoding == other.encoding && self.quality == other.quality
     }
 }
 
-/// Parse qfactor from HTTP header
+/// Parse q-factor from quality strings.
 ///
-/// If parse fail, then fallback to default value which is 1
+/// If parse fail, then fallback to default value which is 1.
-/// More details available here: https://developer.mozilla.org/en-US/docs/Glossary/Quality_values
+/// More details available here: <https://developer.mozilla.org/en-US/docs/Glossary/Quality_values>
 fn parse_quality(parts: &[&str]) -> f64 {
     for part in parts {
-        if part.starts_with("q=") {
+        if part.trim().starts_with("q=") {
             return part[2..].parse().unwrap_or(1.0);
         }
     }
@@ -241,9 +243,8 @@ fn parse_quality(parts: &[&str]) -> f64 {
 
 #[derive(Debug, PartialEq, Eq)]
 enum AcceptEncodingError {
-    /// This error occurs when client only support compressed response
+    /// This error occurs when client only support compressed response and server do not have any
-    /// and server do not have any algorithm that match client accepted
+    /// algorithm that match client accepted algorithms.
-    /// algorithms
     CompressionAlgorithmMismatch,
 }
 
@@ -262,11 +263,12 @@ impl AcceptEncoding {
         if quality <= 0.0 || quality > 1.0 {
             return None;
         }
+
         Some(AcceptEncoding { encoding, quality })
     }
 
-    /// Parse a raw Accept-Encoding header value into an ordered list
+    /// Parse a raw Accept-Encoding header value into an ordered list then return the best match
-    /// then return the best match based on middleware configuration.
+    /// based on middleware configuration.
     pub fn try_parse(
         raw: &str,
         encoding: ContentEncoding,
@@ -285,8 +287,9 @@ impl AcceptEncoding {
             }
         }
 
-        // Special case if user cannot accept uncompressed data
+        // Special case if user cannot accept uncompressed data.
         // See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding
+        // TODO: account for whitespace
         if raw.contains("*;q=0") || raw.contains("identity;q=0") {
             return Err(AcceptEncodingError::CompressionAlgorithmMismatch);
         }
@@ -356,7 +359,7 @@ mod tests {
 
     #[test]
     fn test_parse_compression_required() {
-        // Check we fallback to identity if there is an unsuported compression algorithm
+        // Check we fallback to identity if there is an unsupported compression algorithm
         assert_parse_eq!("compress", ContentEncoding::Identity);
 
         // User do not want any compression

src/middleware/mod.rs

@@ -53,7 +53,7 @@ mod tests {
         #[cfg(feature = "__compress")]
         {
             let _ = App::new().wrap(Compress::default()).wrap(Logger::default());
-            // let _ = App::new().wrap(Logger::default()).wrap(Compress::default());
+            let _ = App::new().wrap(Logger::default()).wrap(Compress::default());
             let _ = App::new().wrap(Compat::new(Compress::default()));
             let _ = App::new().wrap(Condition::new(true, Compat::new(Compress::default())));
         }