From c0615f28edba69cbd51f2eb60100c1de8fbbb9f7 Mon Sep 17 00:00:00 2001 From: Amit Upadhyay Date: Mon, 20 Nov 2023 00:24:08 +0530 Subject: [PATCH] Make compression middleware prefer brotli over zstd over gzip (#3189) * AcceptEncoding.preference() prefers brotli > zstd > gzip * AcceptEncoding.{ranked,negotiate}() prefers brotli > zstd > gzip * changelog entry * use browser-realistic encoding tests * fix choosing identity when q=0 --------- Co-authored-by: Rob Ede --- actix-web/CHANGES.md | 1 + actix-web/src/http/header/accept_encoding.rs | 50 +++++++++++++++++--- actix-web/tests/compression.rs | 32 ++++++++++--- 3 files changed, 69 insertions(+), 14 deletions(-) diff --git a/actix-web/CHANGES.md b/actix-web/CHANGES.md index 0079a4120..953befb7f 100644 --- a/actix-web/CHANGES.md +++ b/actix-web/CHANGES.md @@ -5,6 +5,7 @@ ### Changed - Updated `zstd` dependency to `0.13`. +- Compression middleware now prefers brotli over zstd over gzip. ### Fixed diff --git a/actix-web/src/http/header/accept_encoding.rs b/actix-web/src/http/header/accept_encoding.rs index cc80e7bb0..19d649926 100644 --- a/actix-web/src/http/header/accept_encoding.rs +++ b/actix-web/src/http/header/accept_encoding.rs @@ -149,7 +149,7 @@ impl AcceptEncoding { /// Extracts the most preferable encoding, accounting for [q-factor weighting]. /// - /// If no q-factors are provided, the first encoding is chosen. Note that items without + /// If no q-factors are provided, we prefer brotli > zstd > gzip. Note that items without /// q-factors are given the maximum preference value. /// /// As per the spec, returns [`Preference::Any`] if acceptable list is empty. Though, if this is @@ -167,6 +167,7 @@ impl AcceptEncoding { let mut max_item = None; let mut max_pref = Quality::ZERO; + let mut max_rank = 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 @@ -174,9 +175,13 @@ impl AcceptEncoding { 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 { + + let rank = encoding_rank(pref); + + if (pref.quality, rank) > (max_pref, max_rank) { max_pref = pref.quality; max_item = Some(pref.item.clone()); + max_rank = rank; } } @@ -203,6 +208,8 @@ impl AcceptEncoding { /// Returns a sorted list of encodings from highest to lowest precedence, accounting /// for [q-factor weighting]. /// + /// If no q-factors are provided, we prefer brotli > zstd > gzip. + /// /// [q-factor weighting]: https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.2 pub fn ranked(&self) -> Vec> { self.ranked_items().map(|q| q.item).collect() @@ -210,21 +217,44 @@ impl AcceptEncoding { fn ranked_items(&self) -> impl Iterator>> { if self.0.is_empty() { - return vec![].into_iter(); + return Vec::new().into_iter(); } let mut types = self.0.clone(); // use stable sort so items with equal q-factor retain listed order types.sort_by(|a, b| { - // sort by q-factor descending - b.quality.cmp(&a.quality) + // sort by q-factor descending then server ranking descending + + b.quality + .cmp(&a.quality) + .then(encoding_rank(b).cmp(&encoding_rank(a))) }); types.into_iter() } } +/// Returns server-defined encoding ranking. +fn encoding_rank(qv: &QualityItem>) -> u8 { + // ensure that q=0 items are never sorted above identity encoding + // invariant: sorting methods calling this fn use first-on-equal approach + if qv.quality == Quality::ZERO { + return 0; + } + + match qv.item { + Preference::Specific(Encoding::Known(ContentEncoding::Brotli)) => 5, + Preference::Specific(Encoding::Known(ContentEncoding::Zstd)) => 4, + Preference::Specific(Encoding::Known(ContentEncoding::Gzip)) => 3, + Preference::Specific(Encoding::Known(ContentEncoding::Deflate)) => 2, + Preference::Any => 0, + Preference::Specific(Encoding::Known(ContentEncoding::Identity)) => 0, + Preference::Specific(Encoding::Known(_)) => 1, + Preference::Specific(Encoding::Unknown(_)) => 1, + } +} + /// Returns true if "identity" is an acceptable encoding. /// /// Internal algorithm relies on item list being in descending order of quality. @@ -377,11 +407,11 @@ mod tests { ); assert_eq!( test.negotiate([Encoding::gzip(), Encoding::brotli(), Encoding::identity()].iter()), - Some(Encoding::gzip()) + Some(Encoding::brotli()) ); assert_eq!( test.negotiate([Encoding::brotli(), Encoding::gzip(), Encoding::identity()].iter()), - Some(Encoding::gzip()) + Some(Encoding::brotli()) ); } @@ -398,6 +428,9 @@ mod tests { let test = accept_encoding!("br", "gzip", "*"); assert_eq!(test.ranked(), vec![enc("br"), enc("gzip"), enc("*")]); + + let test = accept_encoding!("gzip", "br", "*"); + assert_eq!(test.ranked(), vec![enc("br"), enc("gzip"), enc("*")]); } #[test] @@ -420,5 +453,8 @@ mod tests { let test = accept_encoding!("br", "gzip", "*"); assert_eq!(test.preference().unwrap(), enc("br")); + + let test = accept_encoding!("gzip", "br", "*"); + assert_eq!(test.preference().unwrap(), enc("br")); } } diff --git a/actix-web/tests/compression.rs b/actix-web/tests/compression.rs index b911b9d1f..61ff1bff5 100644 --- a/actix-web/tests/compression.rs +++ b/actix-web/tests/compression.rs @@ -96,7 +96,7 @@ async fn negotiate_encoding_gzip() { let req = srv .post("/static") - .insert_header((header::ACCEPT_ENCODING, "gzip,br,zstd")) + .insert_header((header::ACCEPT_ENCODING, "gzip, br;q=0.8, zstd;q=0.5")) .send(); let mut res = req.await.unwrap(); @@ -109,7 +109,7 @@ async fn negotiate_encoding_gzip() { let mut res = srv .post("/static") .no_decompress() - .insert_header((header::ACCEPT_ENCODING, "gzip,br,zstd")) + .insert_header((header::ACCEPT_ENCODING, "gzip, br;q=0.8, zstd;q=0.5")) .send() .await .unwrap(); @@ -123,9 +123,11 @@ async fn negotiate_encoding_gzip() { async fn negotiate_encoding_br() { let srv = test_server!(); + // check that brotli content-encoding header is returned + let req = srv .post("/static") - .insert_header((header::ACCEPT_ENCODING, "br,zstd,gzip")) + .insert_header((header::ACCEPT_ENCODING, "br, zstd, gzip")) .send(); let mut res = req.await.unwrap(); @@ -135,10 +137,26 @@ async fn negotiate_encoding_br() { let bytes = res.body().await.unwrap(); assert_eq!(bytes, Bytes::from_static(LOREM)); + // check that brotli is preferred even when later in (q-less) list + + let req = srv + .post("/static") + .insert_header((header::ACCEPT_ENCODING, "gzip, zstd, br")) + .send(); + + let mut res = req.await.unwrap(); + assert_eq!(res.status(), StatusCode::OK); + assert_eq!(res.headers().get(header::CONTENT_ENCODING).unwrap(), "br"); + + let bytes = res.body().await.unwrap(); + assert_eq!(bytes, Bytes::from_static(LOREM)); + + // check that returned content is actually brotli encoded + let mut res = srv .post("/static") .no_decompress() - .insert_header((header::ACCEPT_ENCODING, "br,zstd,gzip")) + .insert_header((header::ACCEPT_ENCODING, "br, zstd, gzip")) .send() .await .unwrap(); @@ -154,7 +172,7 @@ async fn negotiate_encoding_zstd() { let req = srv .post("/static") - .insert_header((header::ACCEPT_ENCODING, "zstd,gzip,br")) + .insert_header((header::ACCEPT_ENCODING, "zstd, gzip, br;q=0.8")) .send(); let mut res = req.await.unwrap(); @@ -167,7 +185,7 @@ async fn negotiate_encoding_zstd() { let mut res = srv .post("/static") .no_decompress() - .insert_header((header::ACCEPT_ENCODING, "zstd,gzip,br")) + .insert_header((header::ACCEPT_ENCODING, "zstd, gzip, br;q=0.8")) .send() .await .unwrap(); @@ -207,7 +225,7 @@ async fn gzip_no_decompress() { // don't decompress response body .no_decompress() // signal that we want a compressed body - .insert_header((header::ACCEPT_ENCODING, "gzip,br,zstd")) + .insert_header((header::ACCEPT_ENCODING, "gzip, br;q=0.8, zstd;q=0.5")) .send(); let mut res = req.await.unwrap();