tower-http icon indicating copy to clipboard operation
tower-http copied to clipboard

Request decompression

Open wanderinglethe opened this issue 3 years ago • 16 comments

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

  • [x] Documentation
  • [x] Non-breaking decompression::Decompression, including docs
  • [x] Refactor code structure / style

Thank you

Thank you for looking into this.

wanderinglethe avatar Jul 09 '22 08:07 wanderinglethe

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?

wanderinglethe avatar Jul 09 '22 09:07 wanderinglethe

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

davidpdrsn avatar Jul 09 '22 09:07 davidpdrsn

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.

davidpdrsn avatar Jul 09 '22 10:07 davidpdrsn

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?

wanderinglethe avatar Jul 09 '22 20:07 wanderinglethe

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

davidpdrsn avatar Jul 09 '22 21:07 davidpdrsn

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

wanderinglethe avatar Jul 15 '22 12:07 wanderinglethe

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.

davidpdrsn avatar Jul 15 '22 12:07 davidpdrsn

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. :)

wanderinglethe avatar Jul 15 '22 12:07 wanderinglethe

@davidpdrsn any chance you are back from vacation? :pray: I'd also hugely appreciate getting this merged :heart:

reneklacan avatar Aug 18 '22 16:08 reneklacan

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

davidpdrsn avatar Aug 18 '22 17:08 davidpdrsn

Thank you

wanderinglethe avatar Aug 18 '22 19:08 wanderinglethe

@davidpdrsn Thanks! Hope you had a lovely vacation

reneklacan avatar Aug 19 '22 08:08 reneklacan

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

LGTM!

wanderinglethe avatar Aug 23 '22 07:08 wanderinglethe

I still wanna look through. Please remain patient.

davidpdrsn avatar Aug 23 '22 07:08 davidpdrsn

I still wanna look through. Please remain patient.

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

wanderinglethe avatar Aug 23 '22 07:08 wanderinglethe

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.

frxstrem avatar Oct 07 '22 10:10 frxstrem

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 avatar Oct 25 '22 06:10 mmd-osm

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.

frxstrem avatar Oct 25 '22 10:10 frxstrem

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

davidpdrsn avatar Dec 02 '22 11:12 davidpdrsn

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

wanderinglethe avatar Dec 05 '22 20:12 wanderinglethe