kiota-typescript icon indicating copy to clipboard operation
kiota-typescript copied to clipboard

feat: request body compression

Open rkodev opened this issue 1 year ago • 2 comments

Resolves https://github.com/microsoft/kiota-typescript/issues/1288

Adds support for compression of request body

rkodev avatar Sep 10 '24 12:09 rkodev

@baywet I think we have a bit off on issue with compression. Native support for compression in node for the web is a bit on an issue, as of node 22 zlib has native support and it works great, but for node 18 / 20 we do need to import it to support browser compression.

rkodev avatar Sep 10 '24 18:09 rkodev

@rkodev the compression stream API is available on 90+% of browsers out there, we should use that for browsers and assume if applications run into an issue they can selectively disable the middleware based on the browser they want to support. https://caniuse.com/?search=compressionstream https://developer.mozilla.org/en-US/docs/Web/API/Compression_Streams_API

As for nod ethe zlib gzip API is available since "forever" https://nodejs.org/api/zlib.html#zlibgzipbuffer-options-callback https://nodejs.org/api/zlib.html#zlibcreategzipoptions

The way around the issue you're facing is to:

  1. have two implementation methods of the compression (one for browser, one for node)
  2. leverage dynamic imports in those methods (and not at the top of the document)
  3. selectively use the right method based on the context (node or browser)

I think we have other examples of doing that across the code.

If this is two complex, we could have two implementations of the middleware (a bit more duplication) and insert the right implementation in the right factory (we already have that in place as well)

Let me know if you have any additional comments or questions.

baywet avatar Sep 10 '24 18:09 baywet

@baywet Thanks for taking a look

rkodev avatar Oct 17 '24 04:10 rkodev

It took me a while to track this bug down! Enabling request body compression by default broke several of my clients. Could it be an optional middleware that’s not enabled by default?

jlarmstrongiv avatar Oct 28 '24 05:10 jlarmstrongiv

@jlarmstrongiv thanks for reporting this. Can you share more information as to how were your clients brokens? Did the target service not support decompress request bodies? or was something wrong with how the middleware handles the compression/headers?

The design is to enable it by default across languages, of course you always have the option to disable it for a specific client.

Additionally, was your client running as a node or a browser application?

@rkodev to follow up on that as I'll be out of office soon.

baywet avatar Oct 28 '24 09:10 baywet

@baywet @rkodev

While the kiota client supports compressed request bodies just fine, the API I use does not. Instead, the @microsoft/kiota-http-fetchlibrary transitive dependency updated without me knowing it, and broke my app. The server in question responds with 500 errors when 'Content-Encoding': 'gzip' bodies are found in the request.

I would like to avoid having the request compression middleware enabled by default on all languages and avoid running into this issue 5 more times. Or, at least, have an example on how to disable the middleware for each language (typescript, python, php, java, c#, go).

I run the client as both a Node.js and Browser application. It’s the server that has the issue, and it’s a server I do not have control over. Without the request content encoding, everything works great.

jlarmstrongiv avatar Oct 28 '24 12:10 jlarmstrongiv

Thank you for the additional information.

Is this server not supporting compression for all operations/paths or just for a few?

The reason why I'm asking is because you can pass a per request option if this is just for a few to disable body compression on a per request base.

Otherwise your best bet is to copy the middleware and client factory to tweak which handlers are selected before passing things to the request adapter.

This advice applies to all languages as this is the general design of the middleware + options + client part of the infrastructure.

As for the fact that the compression handler has been added in a preview bump, while this is an important behavior change, it's not an API breaking change, and TypeScript is still in preview anyway, be aware that other changes of the sort might happen before the language reaches a stable maturity.

baywet avatar Oct 28 '24 13:10 baywet

Thank you for your help!

Unfortunately, it’s all operations. I tried the following code:

const httpClient = new HttpClient(
    undefined,
    new RetryHandler(),
    new RedirectHandler(),
    new ParametersNameDecodingHandler(),
    new UserAgentHandler(),
    new HeadersInspectionHandler(),
  );
const fetchAdapter = new FetchRequestAdapter(
    authenticationProvider,
    undefined,
    undefined,
    httpClient,
  );

But I receive the error next middleware is undefined.

. Do you have an example for disabling compression for all operations/paths? It’s not immediately clear how to do so.

As for the fact that the compression handler has been added in a preview bump, while this is an important behavior change, it's not an API breaking change, and TypeScript is still in preview anyway, be aware that other changes of the sort might happen before the language reaches a stable maturity.

I have subscribed to all issues related to request compression to ensure I am notified when breaking changes occur. In the several months I’ve worked with kiota, this issue is the first to have caused outages.

jlarmstrongiv avatar Oct 28 '24 13:10 jlarmstrongiv

Thank you for the additional information.

Can you try the following? It's not the most obvious way of setting it up.

const httpClient = new HttpClient(
-   undefined,
+   fetch, 
    new RetryHandler(),
    new RedirectHandler(),
    new ParametersNameDecodingHandler(),
    new UserAgentHandler(),
    new HeadersInspectionHandler(),
  );

see the implementation

baywet avatar Oct 28 '24 14:10 baywet

Yes, that worked! Thank you. Interesting that it needs the CustomFetchHandler; good to know. I’ll update my packages with the fix.

jlarmstrongiv avatar Oct 28 '24 15:10 jlarmstrongiv