Request decompression #282

wanderinglethe · 2022-07-09T08:41:18Z

Motivation

Currently compression and decompression of response bodies are supported with the Compression and Decompression services. Issue #276 proposes to also implement decompression for request bodies, this PR addresses that. To complete the compression and decompression modules, request compression could be added in another PR.

Solution

RequestDecompression decompresses the request body with the algorithm defined in the Content-Encoding header, it does not support multiple/stacked algorithms. When an algorithm is not supported it will return a 415 Unsupported Media Type by default, but it can also be configured to pass-through the request to the inner service.

Questions

This is my first PR and I am not sure if I am going in the right direction, I would already like some feedback while working on the issue.

Refactor `decompression` module

Will be done in a different PR.

Service and Future implementation

The service and future are based on the Decompression service and ResponseFuture, but I have the feeling the constraints are too complex. Can these be simplified?

And the same for the boxing of the bodies and errors, has this been done correctly?

API

I have copied the layer implementation from the request decompression layer to make the API uniform. The extra functionality is the pass-through of unsupported encodings, maybe later this can be changed into a single final decompression, that return the 415, and a stacked decompression that pops an encoding and pass-through to the next service.

Would this be a good first way to go?

General code style / conventions

Since I have the basic functionalities tested I can now refactor code. Are there any structures that can be written clearer or more concise? Or are there things that do not follow Rust or Tower conventions?

Todos

Documentation
Non-breaking decompression::Decompression, including docs
Refactor code structure / style

Thank you

Thank you for looking into this.

Still needs some refactor to remove duplicate code and needs documentation

Which either polls its inner future or returns 415 Unsupported Media Type

davidpdrsn

I haven't read through the code yet and while I appreciate that you wanna refactor I would prefer if it was done in a separate PR. Makes it easier to review and bisect in case of bugs.

Is it possible for you to split things up?

wanderinglethe · 2022-07-09T09:24:23Z

Is it possible for you to split things up?

Of course, where should I locate the request decompression module and what should I name it?
Also how do you think we should name the layer and service? RequestDecompression or just Decompression distiquished by its module name?

davidpdrsn · 2022-07-09T09:58:12Z

I'd call it RequestDecompression and put it in the decompression module if possible

davidpdrsn · 2022-07-09T10:21:03Z

I think we should also consider making separate middleware for (de)compressing requests and responses rather than making one middleware do both. It's easy to apply both if that's what users want.

wanderinglethe · 2022-07-09T20:04:34Z

I think we should also consider making separate middleware for (de)compressing requests and responses rather than making one middleware do both.

That is what I have done, right?

Refactoring of `decompression` module will be done in a later PR

davidpdrsn · 2022-07-09T21:57:45Z

Yes sorry. It was mostly a note to myself. Haven't actually read your code yet 😅

wanderinglethe · 2022-07-15T12:04:42Z

I think most of the PR is completed and can be reviewed. Although my questions, in the first message, still stand.
@davidpdrsn could you run the workflow?

Thank you

davidpdrsn · 2022-07-15T12:16:57Z

Thanks!

I'm not sure when I'll get time to review this. Going on vacation in a few days. So might not get time before I'm back in a few weeks.

wanderinglethe · 2022-07-15T12:20:01Z

I'm not sure when I'll get time to review this. Going on vacation in a few days. So might not get time before I'm back in a few weeks.

Have a good vacation. I will see if I can work on some other PR or maybe I will take some vacation as well. :)

reneklacan · 2022-08-18T16:49:00Z

@davidpdrsn any chance you are back from vacation? 🙏 I'd also hugely appreciate getting this merged ❤️

davidpdrsn · 2022-08-18T17:08:32Z

I am 😊 I'll try and have a look soon.

wanderinglethe · 2022-08-18T19:35:48Z

Thank you

reneklacan · 2022-08-19T08:37:02Z

@davidpdrsn Thanks! Hope you had a lovely vacation

Nehliin

LGTM!

wanderinglethe · 2022-08-23T07:18:00Z

Do you have any critique on conventions, design or anything else?

LGTM!

davidpdrsn · 2022-08-23T07:23:42Z

I still wanna look through. Please remain patient.

wanderinglethe · 2022-08-23T07:26:23Z

I still wanna look through. Please remain patient.

Thank you. That's what I expected, but maybe @Nehliin has some critique as well

frxstrem · 2022-10-07T10:28:12Z

Any progress on getting this merged? At my workplace we have a need to support request decompression in our Axum/Hyper based services, and it would be much better if we get support from that in tower-http rather than doing it in our own codebase.

mmd-osm · 2022-10-25T06:28:10Z

Apologies if this question is asking the obvious. Would it be possible to specify the maximum permitted request body size after decompression in a lazy way somehow, ie. without uncompressing the whole payload, as to avoid excessive memory consumption?

frxstrem · 2022-10-25T10:27:07Z

Apologies if this question is asking the obvious. Would it be possible to specify the maximum permitted request body size after decompression in a lazy way somehow, ie. without uncompressing the whole payload, as to avoid excessive memory consumption?

@mmd-osm As far as I can tell, this is doing streaming decompression, so you should be able to apply a tower_http::limit::RequestBodyLimitLayer layer to limit the number of bytes in the request body. Applying that layer after the request decompression layer should apply the limit to the decompressed bytes.

davidpdrsn

Sorry for taking forever to review. First round of feedback is in! Generally I think this looks good. Mostly minor nitpicks here and there.

tower-http/src/decompression/mod.rs

tower-http/src/decompression/request/future.rs

tower-http/src/decompression/request/layer.rs

davidpdrsn · 2022-12-02T11:13:16Z

tower-http/src/decompression/request/layer.rs

+    }
+
+    /// Passes through the request even when the encoding is not supported.
+    pub fn pass_through_unaccepted(mut self) -> Self {


I think this method should take a bool to similar to the set_* methods.

Then users can easily configure things based on things like env vars:

layer.passthrough_unaccepted(std::env::var("FOO").is_ok())

I believe it is also spelled passthrough and not pass through

In my native language it would also be passthrough, but in English that is a noun and the verb is to pass through.

davidpdrsn · 2022-12-02T11:57:59Z

Oh and CI should be fixed if you merge the latest master.

…epted

wanderinglethe · 2022-12-05T20:54:17Z

Thank you for reviewing David. I have made the changes you requested.

bruwozniak · 2023-01-12T16:41:44Z

Would be amazing to see this merged :) thanks for the effort @wanderinglethe

davidpdrsn

Thanks for working on this! Sorry for taking so long to review it.

wanderinglethe added 4 commits July 6, 2022 23:49

Add layer to decompress request bodies

dfca3fb

Still needs some refactor to remove duplicate code and needs documentation

Refactor decompression modules

43786f4

Fix incorrect rename

80d2a74

Add ResponseFuture for RequestDecompression

80e1283

Which either polls its inner future or returns 415 Unsupported Media Type

davidpdrsn reviewed Jul 9, 2022

View reviewed changes

Refactor DecompressionService

afde567

wanderinglethe added 2 commits July 9, 2022 22:26

Rollback rename and move of Decompression

1400518

Refactoring of `decompression` module will be done in a later PR

Re-add request module to the decompression module

9fbb858

wanderinglethe added 3 commits July 12, 2022 18:05

Send "identity" encoding when no encodings are accepted

5555ef8

Add documentation

e669c18

Add example

561db25

wanderinglethe marked this pull request as ready for review July 13, 2022 22:11

Fix styling

d009ab0

Nehliin approved these changes Aug 19, 2022

View reviewed changes

wanderinglethe requested a review from davidpdrsn October 23, 2022 11:01

davidpdrsn reviewed Dec 2, 2022

View reviewed changes

wanderinglethe added 4 commits December 4, 2022 22:07

Fix some styling of imports and documentation

deba218

Add enable parameter to RequestDecompressionLayer::pass_through_unacc…

b06ac0a

…epted

Merge branch 'master' into request-decompression

cc1a78f

Cleanup redundant code

3575cd4

wanderinglethe requested a review from davidpdrsn December 5, 2022 20:53

davidpdrsn added 5 commits February 24, 2023 13:05

Merge branch 'master' into request-decompression

d62837b

fix imports

115cacb

actually fix import

301da26

check for zstd

6a8532e

zstd

3cd8c76

davidpdrsn approved these changes Feb 24, 2023

View reviewed changes

davidpdrsn merged commit 67130ab into tower-rs:master Feb 24, 2023

leio mentioned this pull request Mar 21, 2023

Decompression service decompress the request #276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request decompression #282

Request decompression #282

wanderinglethe commented Jul 9, 2022 •

edited

davidpdrsn left a comment

wanderinglethe commented Jul 9, 2022 •

edited

davidpdrsn commented Jul 9, 2022

davidpdrsn commented Jul 9, 2022

wanderinglethe commented Jul 9, 2022

davidpdrsn commented Jul 9, 2022

wanderinglethe commented Jul 15, 2022

davidpdrsn commented Jul 15, 2022

wanderinglethe commented Jul 15, 2022

reneklacan commented Aug 18, 2022

davidpdrsn commented Aug 18, 2022

wanderinglethe commented Aug 18, 2022

reneklacan commented Aug 19, 2022

Nehliin left a comment

wanderinglethe commented Aug 23, 2022

davidpdrsn commented Aug 23, 2022

wanderinglethe commented Aug 23, 2022 •

edited

frxstrem commented Oct 7, 2022 •

edited

mmd-osm commented Oct 25, 2022

frxstrem commented Oct 25, 2022

davidpdrsn left a comment

davidpdrsn Dec 2, 2022

wanderinglethe Dec 4, 2022

davidpdrsn commented Dec 2, 2022

wanderinglethe commented Dec 5, 2022

bruwozniak commented Jan 12, 2023 •

edited

davidpdrsn left a comment

Request decompression #282

Request decompression #282

Conversation

wanderinglethe commented Jul 9, 2022 • edited

Motivation

Solution

Questions

Refactor decompression module

Service and Future implementation

API

General code style / conventions

Todos

Thank you

davidpdrsn left a comment

Choose a reason for hiding this comment

wanderinglethe commented Jul 9, 2022 • edited

davidpdrsn commented Jul 9, 2022

davidpdrsn commented Jul 9, 2022

wanderinglethe commented Jul 9, 2022

davidpdrsn commented Jul 9, 2022

wanderinglethe commented Jul 15, 2022

davidpdrsn commented Jul 15, 2022

wanderinglethe commented Jul 15, 2022

reneklacan commented Aug 18, 2022

davidpdrsn commented Aug 18, 2022

wanderinglethe commented Aug 18, 2022

reneklacan commented Aug 19, 2022

Nehliin left a comment

Choose a reason for hiding this comment

wanderinglethe commented Aug 23, 2022

davidpdrsn commented Aug 23, 2022

wanderinglethe commented Aug 23, 2022 • edited

frxstrem commented Oct 7, 2022 • edited

mmd-osm commented Oct 25, 2022

frxstrem commented Oct 25, 2022

davidpdrsn left a comment

Choose a reason for hiding this comment

davidpdrsn Dec 2, 2022

Choose a reason for hiding this comment

wanderinglethe Dec 4, 2022

Choose a reason for hiding this comment

davidpdrsn commented Dec 2, 2022

wanderinglethe commented Dec 5, 2022

bruwozniak commented Jan 12, 2023 • edited

davidpdrsn left a comment

Choose a reason for hiding this comment

wanderinglethe commented Jul 9, 2022 •

edited

Refactor `decompression` module

wanderinglethe commented Jul 9, 2022 •

edited

wanderinglethe commented Aug 23, 2022 •

edited

frxstrem commented Oct 7, 2022 •

edited

bruwozniak commented Jan 12, 2023 •

edited