Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CompressionHandler enhancements #598

Closed
andrueastman opened this issue Sep 8, 2021 · 7 comments
Closed

CompressionHandler enhancements #598

andrueastman opened this issue Sep 8, 2021 · 7 comments
Labels
CLI Work to support generating CLIs with Kiota Csharp Pull requests that update .net code hacktoberfest Java PHP Python TypeScript Pull requests that update Javascript code
Milestone

Comments

@andrueastman
Copy link
Member

andrueastman commented Sep 8, 2021

The current compression handler for Microsoft graph is outlined in the spec at the link below

https://github.com/microsoftgraph/msgraph-sdk-design/blob/master/middleware/CompressionHandler.md

This has two possible issues/notes

  • It outlines supports for gzip but other compression methods may be used by other APIs( e.g. deflate and brotli)
  • It outlines that AGS does not support compressed content being sent to it when ideally we would want to send compressed content and also expect back compressed content.
    AB#11068
@andrueastman andrueastman added Csharp Pull requests that update .net code Java PHP TypeScript Pull requests that update Javascript code labels Sep 8, 2021
@baywet
Copy link
Member

baywet commented Sep 8, 2021

Adding to this, it'd be nice if the next version of this middleware could support:

  • setting which compression alg to use for request payload (and not doing any request compression if nothing is set)
  • setting which compression algs to send to the server (accept encoding), validating it belongs to a range that's actually supported by the middleware, and decompressing things depending on the response header.

@baywet
Copy link
Member

baywet commented Jul 15, 2024

The current situation is a bit all over the place here. I'm going to try my best to disambiguate things so we can divide up the work and start making progress.
First off, our design guidelines calls out not one, but two handlers: request compression and response decompression.

Here is a state of the implementations:

Language Compression Decompression Notes
dotnet not implemented name is wrong, only included for graph clients https://github.com/microsoft/kiota-dotnet/blob/7c61913e32dfa6a4ccc473745033a4ab9ac2dd84/src/http/httpClient/Middleware/CompressionHandler.cs#L60 https://github.com/microsoftgraph/msgraph-sdk-dotnet-core/blob/07584bb61cc4298d75fad71c9b5e5c7d14c3b01c/src/Microsoft.Graph.Core/Requests/GraphClientFactory.cs#L121
TypeScript not implemented not implemented https://github.com/microsoft/kiota-typescript/blob/17908f5370a8410414f589b4ce8d2c70264f2ff2/packages/http/fetch/src/middlewares/middlewareFactory.ts#L20
Java not implemented not implemented https://github.com/microsoft/kiota-java/blob/86a71c852899ac90ea0f392bd1f90c58e0cc9955/components/http/okHttp/src/main/java/com/microsoft/kiota/http/KiotaClientFactory.java#L57
PHP implemented, but not used not implemented https://github.com/microsoft/kiota-http-guzzle-php/blob/dev/src/Middleware/CompressionHandler.php https://github.com/microsoft/kiota-http-guzzle-php/blob/0fb71b6eb67aece7712462e6207a08666048115e/src/KiotaClientFactory.php#L73
Go implemented not implemented https://github.com/microsoft/kiota-http-go/blob/8a1fbc8f6b5ece80a746374f913db3a76cac9879/compression_handler.go#L101
Python not implemented not implemented https://github.com/microsoft/kiota-http-python/blob/fbadd0b3f3cd7eb34b305c229d01a2ccb6ec83f1/kiota_http/kiota_client_factory.py#L93

TODO:

  • We should first create (and fix) bugs for languages that have a compression middleware implemented and in use for the content range (reflection of fix: adds content-encoding and range precisions microsoftgraph/msgraph-sdk-design#96)
  • We should then make sure the compressions handlers that are wrongly named and/or not in use, are actually used (create the issues for that)
  • We should create issues (features) to implement the missing middleware for the diverse languages.

@baywet
Copy link
Member

baywet commented Jul 22, 2024

Update: most clients we're using today support automatic decompression. Some need to be configured to handle it. I have updated the state of things in the spec microsoftgraph/msgraph-sdk-design#97.
I have also create issues where changes need to be applied for decompression.
I'll move to compression soon.

@baywet
Copy link
Member

baywet commented Jul 22, 2024

update: created the issues for the request compression handler.

@jlarmstrongiv
Copy link

When adding support for compression, would it be possible not to include it as part of the default middleware? The TypeScript release, for example, broke several of my clients. I would like to avoid the same disruption in other clients.

@baywet
Copy link
Member

baywet commented Oct 28, 2024

(let's keep the conversation going on the typescript pull request since we didn't run into any issues for other languages)

@kaakaa
Copy link

kaakaa commented Oct 30, 2024

It may also occur in other languages. The implementation of compression would cause requests to servers that do not support gzip-compressed requests to fail. (And isn't that a breaking change?)

What is the reasoning behind making it the default enabled?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Work to support generating CLIs with Kiota Csharp Pull requests that update .net code hacktoberfest Java PHP Python TypeScript Pull requests that update Javascript code
Projects
Archived in project
Development

No branches or pull requests

4 participants