Repository: actix/actix-web (language: rust)

Issue:
`.negotiate()` prefers `gzip` over `brotli`
If I enable `actix_web::middleware::Compress` middleware, it's serving `gzip`, even though `brotli` feature is on.

 
## Expected Behavior

I am expecting it to pick the best compression method supported by Browser, and between brotli and gzip, brotli is better (my site compresses to ~196KB with brotli and ~250kb with gzip). 

## Current Behavior

In the 4.4 release and on main, if you look at the test cases, `gzip` is given higher priority over `brotli`.

 https://github.com/actix/actix-web/blob/e50bceb91493087d4059e605d94c13a11b09e747/actix-web/src/http/header/accept_encoding.rs#L378-L385

## Possible Solution

Based on https://news.ycombinator.com/item?id=19678985 I recommend `brotli` > `zstd` > `gzip` as the priority in which one should pick the compression Algo to use if browser supports all three.

Since every browser that supports `brotli` or `zstd` also supports `gzip`, with current implementation of giving `gzip` priority over the two means we are compiling support for `brotli` and `zstd` for no real benefit.

Happy to send a PR if my proposal is accepted. 

## Steps to Reproduce (for bugs)

The test linked above demonstrates the problem. 

## Context

I am trying to optimise a [`fastn`](https://github.com/fastn-stack/fastn/) powered website: [`fastn.com`](https://fastn.com). Currently it is implemented as SSG, deployed on Vercel, and Vercel uses brotli and about 190KB is downloaded for a test page, and when I power it by running `fastn serve` on a Heroku instance, and enable compression middleware, it serves gzipped content, and browser downloads ~250KB of content. 

## Your Environment

- Rust Version (I.e, output of `rustc -V`): 1.73.0
- Actix Web Version: 4.4

Site is deployed on Heroku. [Code in question](https://github.com/fastn-stack/fastn/blob/85f972eb49818423cab055e43dafe71c5af9cc05/fastn-core/src/commands/serve.rs#L737). 



Discussion / hints:
Since equally weighted q-values are not discriminated according to order (it's left up to the origin), I do think we should preference brotli if it is enabled 👍🏻 

Fix patch:
diff --git a/actix-web/CHANGES.md b/actix-web/CHANGES.md
index 0079a412057..953befb7f0e 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 cc80e7bb08c..19d64992684 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<Preference<Encoding>> {
         self.ranked_items().map(|q| q.item).collect()
@@ -210,21 +217,44 @@ impl AcceptEncoding {
 
     fn ranked_items(&self) -> impl Iterator<Item = QualityItem<Preference<Encoding>>> {
         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<Preference<Encoding>>) -> 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"));
     }
 }


Test patch:
diff --git a/actix-web/tests/compression.rs b/actix-web/tests/compression.rs
index b911b9d1f57..61ff1bff54a 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,25 @@ 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"))
+        .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 brotli is preferred even when later in (q-less) list
+
     let req = srv
         .post("/static")
-        .insert_header((header::ACCEPT_ENCODING, "br,zstd,gzip"))
+        .insert_header((header::ACCEPT_ENCODING, "gzip, zstd, br"))
         .send();
 
     let mut res = req.await.unwrap();
@@ -135,10 +151,12 @@ async fn negotiate_encoding_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();