Expose compression quality on the CompressionLayer

[rafaelcaricio]

Motivation

Fixes #245

Solution

Allows setting the compression quality level for responses. The ticket suggested to use .level(the_level), but after reading the full context I've settled with .quality(the_level) as it represents better the underlying feature.

[erebe]

Hello,

Thank you for the PR :)
I am not a maintainer, but I took a look at it.

Overall, it looks good to me.
I am just nitpicking of the type name of Level, which I think should be more comprehensive into CompressionLevel for the reader.

[rafaelcaricio]

@erebe Thank you for the review! I've applied your suggestion ;)

[davidpdrsn]

LGTM! Thanks :)

[davidpdrsn]

@rafaelcaricio do you have time to look into the CI error?

[rafaelcaricio]

@davidpdrsn Thank you for the review. Yes, I do have time to fix this today.

Should I ping you here when it is done or just let it be, and you will cycle back when you get some time in the future?

[davidpdrsn]

I should get a GitHub notification when you push.

[rafaelcaricio]

My trial to fix this caused way more errors. Honestly, I don't know what to do here. I need some guidance.

[erebe]

Hello @rafaelcaricio ,

The code/patch below works, the issue come down to CompressionLevel type being needed by both the compression and the decompression code, so it must be visible in all the cases.

I had to make the module compression_utils pub to fix it, it should cause no harm as everything inside is declared pub(crate) (but I am no expert in that). If you don't like it, you need to move CompressionLevel in its own module (i.e: compresion_common)

P.s: to apply the patch at root of the repo, patch -p1 < the_patch_below.patch

From 7e8888bbaf4e72492bda839943e8f30584156b7f Mon Sep 17 00:00:00 2001
From: Romain GERARD <erebe@erebe.eu>
Date: Sun, 19 Mar 2023 13:50:37 +0100
Subject: [PATCH] fix visibility

---
 tower-http/src/compression_utils.rs | 47 +++++++++--------------------
 tower-http/src/lib.rs               |  2 +-
 2 files changed, 16 insertions(+), 33 deletions(-)

diff --git a/tower-http/src/compression_utils.rs b/tower-http/src/compression_utils.rs
index 34f1c36..7b28937 100644
--- a/tower-http/src/compression_utils.rs
+++ b/tower-http/src/compression_utils.rs
@@ -15,14 +15,6 @@ use std::{
 use tokio::io::AsyncRead;
 use tokio_util::io::{poll_read_buf, StreamReader};
 
-#[cfg(any(
-    feature = "compression-br",
-    feature = "compression-deflate",
-    feature = "compression-gzip",
-    feature = "compression-zstd",
-))]
-use async_compression::Level as AsyncCompressionLevel;
-
 #[derive(Debug, Clone, Copy)]
 pub(crate) struct AcceptEncoding {
     pub(crate) gzip: bool,
@@ -345,18 +337,9 @@ where
 pub(crate) const SENTINEL_ERROR_CODE: i32 = -837459418;
 
 /// Level of compression data should be compressed with.
-#[cfg_attr(
-    not(any(
-        feature = "compression-br",
-        feature = "compression-deflate",
-        feature = "compression-gzip",
-        feature = "compression-zstd",
-    )),
-    allow(dead_code)
-)]
 #[non_exhaustive]
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
-pub(crate) enum CompressionLevel {
+pub enum CompressionLevel {
     /// Fastest quality of compression, usually produces bigger size.
     Fastest,
     /// Best quality of compression, usually produces the smallest size.
@@ -370,27 +353,27 @@ pub(crate) enum CompressionLevel {
     Precise(u32),
 }
 
-#[cfg(any(
-    feature = "compression-br",
-    feature = "compression-deflate",
-    feature = "compression-gzip",
-    feature = "compression-zstd",
-))]
-pub use self::CompressionLevel;
-
 impl Default for CompressionLevel {
     fn default() -> Self {
         CompressionLevel::Default
     }
 }
 
+#[cfg(any(
+    feature = "compression-br",
+    feature = "compression-gzip",
+    feature = "compression-deflate",
+    feature = "compression-zstd"
+))]
+use async_compression::Level as AsyncCompressionLevel;
+
+#[cfg(any(
+    feature = "compression-br",
+    feature = "compression-gzip",
+    feature = "compression-deflate",
+    feature = "compression-zstd"
+))]
 impl CompressionLevel {
-    #[cfg(any(
-        feature = "compression-br",
-        feature = "compression-deflate",
-        feature = "compression-gzip",
-        feature = "compression-zstd",
-    ))]
     pub(crate) fn into_async_compression(self) -> AsyncCompressionLevel {
         match self {
             CompressionLevel::Fastest => AsyncCompressionLevel::Fastest,
diff --git a/tower-http/src/lib.rs b/tower-http/src/lib.rs
index e6c4186..d0ac34d 100644
--- a/tower-http/src/lib.rs
+++ b/tower-http/src/lib.rs
@@ -279,7 +279,7 @@ mod content_encoding;
     feature = "decompression-gzip",
     feature = "decompression-zstd",
 ))]
-mod compression_utils;
+pub mod compression_utils;
 
 #[cfg(feature = "map-response-body")]
 pub mod map_response_body;
-- 
2.39.2

[rafaelcaricio]

@erebe Thank you for the guidance here. It solved the issues with CI. 🥳

[jplatte]

Why is this made public?

[erebe]

CompressionLevel is exposed publicly/in the api interface and is declared in this module.

You can take a look at my previous comment.

The code/patch below works, the issue come down to CompressionLevel type being needed by both the compression and the decompression code, so it must be visible in all the cases.

I had to make the module compression_utils pub to fix it, it should cause no harm as everything inside is declared pub(crate) (but I am no expert in that). If you don't like it, you need to move CompressionLevel in its own module (i.e: compresion_common)

[jplatte]

Hm, it makes sense that it must be public, but I don't think the module should be. You can maybe put a pub use at the crate root, there's already LatencyUnit there that I think only a few middleware use. cc @davidpdrsn

[rafaelcaricio]

I did make the change suggested here (pub use) instead of making the whole module public.

[jplatte]

Yes. I saw, I get notified for pushes ;)

[i18n-now]

Can I set different compression levels for different compression algorithms?
for example .gzip_with_quality(xxx)

[jplatte]

@i18n-now please open a new issue about that.