Expose compression quality on the CompressionLayer by rafaelcaricio · Pull Request #333 · tower-rs/tower-http

rafaelcaricio · 2023-02-26T22:29:56Z

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.

Allows setting the compression quality level for responses. Fixes tower-rs#245

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.

tower-http/src/compression_utils.rs

rafaelcaricio · 2023-03-09T11:46:01Z

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

tower-http/src/compression_utils.rs

davidpdrsn

LGTM! Thanks :)

davidpdrsn · 2023-03-17T11:53:55Z

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

rafaelcaricio · 2023-03-17T11:57:45Z

@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 · 2023-03-17T12:03:07Z

I should get a GitHub notification when you push.

rafaelcaricio · 2023-03-17T19:47:41Z

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

erebe · 2023-03-19T13:02:31Z

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 · 2023-03-20T19:36:30Z

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

jplatte · 2023-03-21T14:01:40Z

tower-http/src/lib.rs

    feature = "decompression-zstd",
 ))]
-mod compression_utils;
+pub mod compression_utils;


Why is this made public?

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)

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

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

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

Allows setting the compression quality level for responses.

i18nsite · 2024-04-30T07:46:48Z

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

jplatte · 2024-04-30T09:00:07Z

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

rafaelcaricio added 4 commits February 26, 2023 19:26

Expose compression level on CompressionLayer

59ece57

Allows setting the compression quality level for responses. Fixes tower-rs#245

Apply cargo fmt

c46ff14

Test compression with explicit quality level

429b72d

Use streaming encoder in tests

158fa8e

jplatte mentioned this pull request Mar 7, 2023

[tracing] Allow to customize TracingLayer based on request #330

Closed

erebe suggested changes Mar 9, 2023

View reviewed changes

tower-http/src/compression_utils.rs Outdated Show resolved Hide resolved

Rename Level to CompressionLevel for clarity

578effc

jplatte reviewed Mar 12, 2023

View reviewed changes

tower-http/src/compression_utils.rs Outdated Show resolved Hide resolved

Do not expand the public API

62b538c

jplatte approved these changes Mar 12, 2023

View reviewed changes

erebe approved these changes Mar 14, 2023

View reviewed changes

davidpdrsn approved these changes Mar 17, 2023

View reviewed changes

Fix visibility errors

a3a36a8

Fix visibility errors, thanks @erebe

582d4c1

jplatte reviewed Mar 21, 2023

View reviewed changes

Do not set compression_utils mod as public

c1f63c6

jplatte merged commit 4ebe811 into tower-rs:master Mar 22, 2023

rafaelcaricio deleted the expose-compression-quality branch March 22, 2023 14:13

gezihuzi pushed a commit to gezihuzi/tower-http that referenced this pull request Jul 19, 2023

Expose compression quality on the CompressionLayer (tower-rs#333)

72ff1d7

Allows setting the compression quality level for responses.

Conversation

rafaelcaricio commented Feb 26, 2023

Motivation

Solution

Uh oh!

erebe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rafaelcaricio commented Mar 9, 2023

Uh oh!

Uh oh!

davidpdrsn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidpdrsn commented Mar 17, 2023

Uh oh!

rafaelcaricio commented Mar 17, 2023

Uh oh!

davidpdrsn commented Mar 17, 2023

Uh oh!

rafaelcaricio commented Mar 17, 2023

Uh oh!

erebe commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaelcaricio commented Mar 20, 2023

Uh oh!

jplatte Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

erebe Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jplatte Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

rafaelcaricio Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

jplatte Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

i18nsite commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jplatte commented Apr 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidpdrsn left a comment •

edited

Loading

erebe commented Mar 19, 2023 •

edited

Loading

erebe Mar 21, 2023 •

edited

Loading

i18nsite commented Apr 30, 2024 •

edited

Loading