Fix compression middleware images

[cptrodolfox]

PR Type

Adds predicate for compression of responses with default value that skips compression for video and images.

PR Checklist

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

Overview

Closes #2981

[robjtede]

The solution to this issue should allow user customization of the excluded mime types.

[cptrodolfox]

The solution to this issue should allow user customization of the excluded mime types.

Thanks, I will take it mind.

[cptrodolfox]

The solution to this issue should allow user customization of the excluded mime types.

So, I was taking a look into the Compress struct that is used to build the CompressMiddleware. And, I think that a good solution would be to add a field to the struct that holds a function fn(Mime) -> bool. This function would be the one to indicate whether or not we should compress content based on the Mime type of the Content-type header. WDYT @robjtede .

#[derive(Debug, Clone)]
#[non_exhaustive]
pub struct Compress {
    pub compress: fn(Mime) -> bool,
}

impl Default for Compress {
    fn default() -> Self {
	Compress {
	    compress: |_| { true }
	}
    }
}

[robjtede]

Use of a predicate function is a good idea imo. Then we just set the default to ignore images to solve #2981.

[cptrodolfox]

Only one question for the moment @robjtede. How do we return an uncompressed response?. After skimming through the code. I believe the decision to compress or not should be done in the Future implementation of CompressResponse (Am I correct on thinking this?). If I understand correctly, the snippet Encoder::response(enc, head, body) is where the actual compression takes place.

[cptrodolfox]

@robjtede I was able to add a proof of concept of using a predicate function for deciding whether or not to compress based on the value of the Content-Type header of the response. I think there are points to improve though the first one is that currently the compress function has the following signature fn(Option<&HeaderValue>) -> bool, I had to use the HeaderValue since the headers map of the head of the response only returns this. Also, I had to change a little bit the signature of the poll function of the Future implementation of the CompressResponse struct, I am not sure what this change entails but it was needed to call the compress predicate function. What are your thoughts?

[robjtede]

@cptrodolfox I spent some time working on this PR today but I can't push to your branch. Can you check the settings for this PR?

Alternatively, apply this patch here: 0216cb186ea95e4c8f2e645dafe74f5df383c2d3

[cptrodolfox]

@cptrodolfox I spent some time working on this PR today but I can't push to your branch. Can you check the settings for this PR?

Alternatively, apply this patch here: 0216cb1

Hey @robjtede, It appears since the fork is from a organization I can not change the "Allow edit from maintainers" setting. I will apply manually the patch you mentioned.

[cptrodolfox]

@cptrodolfox I spent some time working on this PR today but I can't push to your branch. Can you check the settings for this PR? Alternatively, apply this patch here: 0216cb1

Hey @robjtede, It appears since the fork is from a organization I can not change the "Allow edit from maintainers" setting. I will apply manually the patch you mentioned.

Hey @robjtede , I have added the changes you made. I look through the changes. And, If you don't mind, I want to ask you the following: What is the idea behind using a counted reference for predicate?

[robjtede]

The advantage over taking a function pointer is that you can still avoid the generic param and also pass closures as well.

[cptrodolfox]

The advantage over taking a function pointer is that you can still avoid the generic param and also pass closures as well.

Thanks for the answer.

[robjtede]

Should be noted that some auto traits are lost in this PR, wondering if this is okay.

https://github.com/actix/actix-web/actions/runs/4662131547/jobs/8252158730?pr=2996#step:6:410

Removed items from the public API
=================================
-impl core::marker::Send for actix_web::middleware::Compress
-impl core::marker::Sync for actix_web::middleware::Compress
-impl core::panic::unwind_safe::RefUnwindSafe for actix_web::middleware::Compress
-impl core::panic::unwind_safe::UnwindSafe for actix_web::middleware::Compress

[cptrodolfox]

Should be noted that some auto traits are lost in this PR, wondering if this is okay.

https://github.com/actix/actix-web/actions/runs/4662131547/jobs/8252158730?pr=2996#step:6:410

Removed items from the public API
=================================
-impl core::marker::Send for actix_web::middleware::Compress
-impl core::marker::Sync for actix_web::middleware::Compress
-impl core::panic::unwind_safe::RefUnwindSafe for actix_web::middleware::Compress
-impl core::panic::unwind_safe::UnwindSafe for actix_web::middleware::Compress

I am unsure if losing these traits would affect users down stream. If so we might want to implement them, manually. WDYT @robjtede

[robjtede]

From the rebase this needs a rustfmt run.

On the blocker for merging this, I'd be willing to give up the UnwindSafe impls but not so much Send and Sync, unfortunately. I can definitely think of common patterns where it would break and that's not acceptable. Thinking about how to progress, though it'll probably just be holding off until v5.0 for the customization part and we backpace this PR to just solve the original issue and emulate the "default" behavior.

[robjtede]

I've created #3075 with the subset of changes that do not cause breakage to the public API.