Select compression algorithm using features flags

[arthurlm]

PR Type

Refactor

PR Checklist

• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• A changelog entry has been made for the appropriate packages.
• Format code with the latest stable rustfmt.
• (Team) Label with affected crates and semver status.

Overview

Update compress feature flag to allow selection of enabled algorithms with compress-brotli, compress-gzip and compress-zstd flags.

Breaking change

I totaly remove compress flag and replace it by compress-<alg> flags.

Having both compress flag and compress-<alg> flags may lead to invalid selection:

• Example 1: compress enabled but not algorithm selected
• Example 2: algorithm selected but compress disabled

Change for developpers

For developers, checking if compression is supported or not is done through new internal flag: __compress.
This flag is enabled by selecting any of compress-brotli, compress-gzip or compress-zstd flags.

Before it was:

#[cfg(feature = "compress")]

Now it is:

#[cfg(feature = "__compress")]

Other changes

Completely refact awc selection of Accept-Encoding HTTP header

This PR follow works that has been started in PR [#2244].

[jplatte]

For developper, checking if compression is enabled or not became unfortunately more verbose.
So if anyone has any good idea to improve this, feel free to share it.

Before it was:

#[cfg(feature = "compress")]

Now it is:

#[cfg(any(
    feature = "compress-br",
    feature = "compress-gz",
    feature = "compress-zstd"
))]

You can declare an internal feature like __compress-any or just __compress which is activated by the others and test for that, and leave a note in Cargo.toml that it's not meant to be activated directly. reqwest does this:

https://github.com/seanmonstar/reqwest/blob/bbeb1ede4e8098481c3de6f2cafb8ecca1db4ede/Cargo.toml#L64

[popzxc]

Totally nitpicky and a matter of taste, but I personally find import sequences combined with a lot of cfg(...) hard to read. I think it's better to separate compress-* imports with an empty line, so we have a more or less clean sequence of unconditional imports and then a sequence of imports guarded by cfg(...).

[otavio]

I second that. An empty line makes it much easier to find.

[arthurlm]

Indeed it looks better.

I have update the import related to compression but there is still other import not related to compress-* which do not have empty line before.

Would you like I update them in this PR ? create a PR for this ? or just let them as it ?

[arthurlm]

Thanks for your suggestion @jplatte.
I have update the code to use __compress flag instead of the any(...) flag check and it is much more easier to read and maintain now.

[robjtede]

this is more complicated now, suggest cfg_if!

[arthurlm]

I have add cfg_if! macro as you suggest.
However I did not add it everywhere.

• in struct definition, it make cargo fmt crash
• in awc/src/sender.rs to me it make the code less readable and produce a big change. I can update it if you think it is better with cfg_if

[robjtede]

Unless I'm mistaken, as it's written now enabling any compression flag will signal support for all of them. My suggestion for cfg_if! was to make it easier to enumerate the feature flag combinations to get the correct string here.

compress-gzip => "gzip, deflate"
compress-brotli => "br"
compress-brotli+compress-gzip => "br, gzip, deflate"
compress-brotli+compress-gzip+compress-zstd => "br, gzip, deflate, zstd"
etc...

[arthurlm]

You are right, I have not read usage of this but just replace cfg! calls.

Actually looking at the current code I do not really understand how it may works.
For now there is the current definition

const HTTPS_ENCODING: &str = {
    cfg_if::cfg_if! {
        if #[cfg(feature = "__compress")] {
            "br, gzip, deflate"
        } else {
            "br"
        }
    }
};

And the only usage of HTTPS_ENCODING is in awc/src/request.rs+495:

        if slf.response_decompress {
            let https = slf
                .head
                .uri
                .scheme()
                .map(|s| s == &uri::Scheme::HTTPS)
                .unwrap_or(true);

            if https {
                slf = slf.insert_header_if_none((header::ACCEPT_ENCODING, HTTPS_ENCODING));
            } else {
                #[cfg(feature = "__compress")]
                {
                    slf = slf.insert_header_if_none((header::ACCEPT_ENCODING, "gzip, deflate"));
                }
            };
        }

Please note, here the only change with previous code is compress becoming __compress.
Moreover the if statement related to switching HTTP / HTTPS is just weird.

To me code should be something like this:

        #[cfg(feature = "__compress")]
        if slf.response_decompress {
            let mut encoding = vec![];

            #[cfg(feature = "compress-brotli")]
            encoding.push("br");

            #[cfg(feature = "compress-gzip")]
            {
                encoding.push("gzip");
                encoding.push("deflate");
            }

            #[cfg(feature = "compress-zstd")]
            encoding.push("zstd");

            assert!(!encoding.is_empty(), "encoding cannot be empty unless __compress feature has been explictily enabled.");
            slf = slf.insert_header_if_none((header::ACCEPT_ENCODING, encoding.join(",")));
        }

[arthurlm]

If everyone is ok with this changes, maybe we can merge this PR 😄
If you have anything left to ask on this subject (or something related to this) I will be happy to contribute and help on this !

[arthurlm]

I have push new commits to fix awc decompression capabilities based on enabled features flags.
This is based on the suggestion of @robjtede.

I will be happy to get a review and discuss on this if anyone has few minutes for this subject 😄

[otavio]

What is still pending on this PR?

[arthurlm]

To me, the only thing that was not resolved since your last review was the change in awc about HTTP Accept-Encoding header.
I have fix it few days ago.

So from my point of view, all lights are now green.
This is why I was asking for a review and if all is good to maintainers a merge 😀.

[otavio]

Yep, LGTM

[robjtede]

macOS CI failure can be ignored; I've run the tests locally. Got some doc tweaks to make but will do it after this PR is merged. Thanks for adding this; good work.