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

Garbled request body #1439

Open
qwertycho opened this issue Oct 24, 2024 · 35 comments
Open

Garbled request body #1439

qwertycho opened this issue Oct 24, 2024 · 35 comments

Comments

@qwertycho
Copy link

When sending a post request the json body becomes an arbitrary data.
Example

Image

Sample

vite react + typescript project

import './App.css'

import { AnonymousAuthenticationProvider } from '@microsoft/kiota-abstractions';
import { FetchRequestAdapter, HttpClient } from '@microsoft/kiota-http-fetchlibrary';
import {createWebClient} from '../WebClient/webClient'

function App() {
  return (
    <>
        <button onClick={async () => {
            const authProvider = new AnonymousAuthenticationProvider();
            const adapter = new FetchRequestAdapter(authProvider);
            adapter.baseUrl = "https://localhost:7139";

            const client = createWebClient(adapter);
            await client.api.values.post({
              name: "test",
              value: "test",
            });
        }}>
          test
        </button>
    </>
  )
}

export default App

api

dotnet 8 webapplication

{
  "openapi": "3.0.1",
  "info": {
    "title": "WebApplication2",
    "version": "1.0"
  },
  "paths": {
    "/api/Values": {
      "post": {
        "tags": [
          "Values"
        ],
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "$ref": "#/components/schemas/UploadModel"
              }
            },
            "text/json": {
              "schema": {
                "$ref": "#/components/schemas/UploadModel"
              }
            },
            "application/*+json": {
              "schema": {
                "$ref": "#/components/schemas/UploadModel"
              }
            }
          }
        },
        "responses": {
          "200": {
            "description": "OK"
          }
        }
      },
      "get": {
        "tags": [
          "Values"
        ],
        "responses": {
          "200": {
            "description": "OK"
          }
        }
      }
    },
    "/WeatherForecast": {
      "get": {
        "tags": [
          "WeatherForecast"
        ],
        "operationId": "GetWeatherForecast",
        "responses": {
          "200": {
            "description": "OK",
            "content": {
              "text/plain": {
                "schema": {
                  "type": "array",
                  "items": {
                    "$ref": "#/components/schemas/WeatherForecast"
                  }
                }
              },
              "application/json": {
                "schema": {
                  "type": "array",
                  "items": {
                    "$ref": "#/components/schemas/WeatherForecast"
                  }
                }
              },
              "text/json": {
                "schema": {
                  "type": "array",
                  "items": {
                    "$ref": "#/components/schemas/WeatherForecast"
                  }
                }
              }
            }
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "UploadModel": {
        "type": "object",
        "properties": {
          "name": {
            "type": "string",
            "nullable": true
          },
          "value": {
            "type": "string",
            "nullable": true
          }
        },
        "additionalProperties": false
      },
      "WeatherForecast": {
        "type": "object",
        "properties": {
          "date": {
            "type": "string",
            "format": "date"
          },
          "temperatureC": {
            "type": "integer",
            "format": "int32"
          },
          "temperatureF": {
            "type": "integer",
            "format": "int32",
            "readOnly": true
          },
          "summary": {
            "type": "string",
            "nullable": true
          }
        },
        "additionalProperties": false
      }
    }
  }
}
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Oct 24, 2024
@rkodev rkodev self-assigned this Oct 24, 2024
@rkodev
Copy link
Contributor

rkodev commented Oct 24, 2024

Hi @qwertycho, Thanks for trying out the SDK.
This issue is caused by the compression middleware, which is enabled by default. If you would like to view the request body, you can disable the compression middleware or de-compress the request

@rkodev rkodev moved this from Needs Triage 🔍 to Waits for author 🔁 in Kiota Oct 24, 2024
@baywet baywet added the status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close label Oct 24, 2024
@andrueastman andrueastman added this to the Kiota-TS-GA milestone Oct 30, 2024
@qwertycho
Copy link
Author

Thank you @rkodev ! I have disabled the compression with this snipped:

    const http = KiotaClientFactory.create(undefined, [
      new RetryHandler(), new RedirectHandler(), new ParametersNameDecodingHandler(), new UserAgentHandler(),  new HeadersInspectionHandler()
    ])
  
    const adapter = new FetchRequestAdapter(new BearerAuthenticationProvider(), undefined, undefined, http);
    const client = createApiClient(adapter);

    client.api.endpoint.post({
      field: "testdata",
    })

The compression is a bit annoying when the api does not support it.

I did notice that the CompressionHandler is not in exported in the http fetch library, so that limits discoverability. Although it seems there isn't much to configure anyway.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close Status: No Recent Activity labels Oct 31, 2024
@baywet
Copy link
Member

baywet commented Nov 9, 2024

@qwertycho
Thank you for the additional information.

When the API you're targeting replies to the compressed request, does it send back a 415 status code? Or something else?
The handler should retry with an uncompressed request body making failures "transparent" (although haven't a negative impact on bandwidth usage and latency).

We didn't get any feedback about APIs not supporting request body compression in other languages where it's been implemented to date as far as I know.

The addition of this handler by default is guided by our design guidelines

Can you expand on the export comment you were making please?

@brad-technologik
Copy link

I find it bizarre that requests are compressed by default. In my decade+ of web development, I've never heard of compressing json payloads sent to a server (only responses). Furthermore, given that Kiota is closely related to ASP.NET core and ASP.NET core fails to recognized compressed data by default, Kiota basically is broken out of the box for ASP.NET backends.

Disabling compression as per the comment above worked for me.

@qwertycho
Copy link
Author

qwertycho commented Nov 12, 2024

@baywet
The api does not send a 415 statuscode but a 400. When i tested it with a api that send a 415 it works perfectly fine.
As @brad-technologik said ASP.NET does not support compressed content by default.

The kiota-http-fetchlibrary exports all the middlewares (RedirectHandler, RetryHandler, etc) except for the CompressionHandler. So if somebody wanted to build a HttpClient with a custom set of middleware, they won't be able to use the CompressionHandler.

As far as i can see the only thing that is configurable in the CompressionHandler is the 'enableCompression' flag, so that's why i said that there isn't much to configure.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Nov 12, 2024
@andreaTP
Copy link
Contributor

Hitting this same issue here: Apicurio/apicurio-registry#5498

So, I can confirm that Java server are being affected by this issue too.

@baywet
Copy link
Member

baywet commented Nov 14, 2024

Thank you for the additional information.

@qwertycho are you referring to the missing entries here and there ?? This is most likely an oversight.
Is this something you'd like to submit a pull request for provided some guidance?

As for request compression being enabled by default, extrapolating from Microsoft Graph we might have made two assumptions that are not holding:

  • Broad support for request body compression in APIs. (seems the support is lower than we expected)
  • Broad support for replying with 415 from APIs that do not support request body compression. (it seems that the ones that don't support it are oblivious to it, and just send a 400 back).

What's strange is that go has been our canary and has supported request body compression for over 2 years now. While we received feedback about content range and other aspects where compression should NOT be applied, we have not received feedback asking to disable request body compression all together.

We could consider disabling request compression by default (and only enabling it in Microsoft Graph core), but before we go there, I'd love to brainstorm potential solutions to:

  • Make the issue easier to find for the developer, and disable when API does not "do the right thing".
  • (assuming disabled by default) make it easier for people to discover this feature/performance improvement for there clients.

@andreaTP
Copy link
Contributor

For reference, in the Java world, either in Spring Boot and Quarkus enabling compression is still opt in(i.e. disabled by default).

@qwertycho
Copy link
Author

@baywet
Yes, That are the points where the CompressionHandler is missing.

As for for compression being enabled by default: I think it's fine, as long as it's properly documented. When looking at the documentation there is no mention of compression being enabled. Currently the only mention of default middleware is Create a middleware handlers array and use the existing middleware already implemented within Microsoft.Kiota.HttpClientLibrary that includes existing handlers like retry, redirect, and more.

I think that it would be good to mention the different middleware handlers that are enabled by default in the quickstart guides and then link to the middleware page.

@baywet
Copy link
Member

baywet commented Nov 19, 2024

Thank you for the additional information.

Can you please create an issue here for the gap in public documentation?

@illunix
Copy link
Contributor

illunix commented Nov 25, 2024

So there is any workaround for this or this still need to be resolved? @baywet

@baywet
Copy link
Member

baywet commented Nov 26, 2024

The only workaround today it to remove the compression middleware from the default middlewares when initializing the client.

We (the internal kiota team) still need to make a decision regarding what to do here:

  • disable request compression by default for all languages
  • disable request compression by default only for TypeScript so browser tools can be used
  • somehow detect that browser tools are open, and disable request compression in that context only (I did a quick search the other day, there doesn't seem to be any reliable API to do that)
  • disable it by default for kiota clients, but enable it by default for generated microsoft graph clients through core
  • simply leave things as is, improve the public documentation
  • is there any other option I missed?

Input is appreciated.

CC @maisarissi @sebastienlevert for the experience part.

@rkodev rkodev removed their assignment Nov 26, 2024
@illunix
Copy link
Contributor

illunix commented Nov 26, 2024

How to remove it from base client? @baywet

@baywet
Copy link
Member

baywet commented Nov 26, 2024

@illunix please create another issue to add typescript example on the middleware documentation page here

@qwertycho
Copy link
Author

@illunix

You need to create a httpAdaper with a custom set of middleware.
After that simply use the httpAdapter in a FetchRequestAdapter.

factory
middlewares

Thank you @rkodev ! I have disabled the compression with this snipped:

    const http = KiotaClientFactory.create(undefined, [
      new RetryHandler(), new RedirectHandler(), new ParametersNameDecodingHandler(), new UserAgentHandler(),  new HeadersInspectionHandler()
    ])
  
    const adapter = new FetchRequestAdapter(new BearerAuthenticationProvider(), undefined, undefined, http);
    const client = createApiClient(adapter);

    client.api.endpoint.post({
      field: "testdata",
    })

The compression is a bit annoying when the api does not support it.

I did notice that the CompressionHandler is not in exported in the http fetch library, so that limits discoverability. Although it seems there isn't much to configure anyway.

@maisarissi
Copy link

My assumption would that based on the feedback (or lack of complaint) we got from community over the years, the default request compression was not a major issue for most people using Kiota generated API clients until now and having it enabled by default gives users increased performance out of the box. So I don't think my first option would be in favor of disabling in it. This might be a bit different for folks using TypeScript though.

Also I don't think having different experiences (like changing the set of the default middleware) for different languages will be a good solution. That might cause more confusion than help folks out in the long run, as each language will have different flavors and we would need to make sure to have dedicated language docs to cover all different aspects for each one.

Instead of relying on 415 code response, is there another way we could identify compression is not supported to do the uncompressed body retry?

@baywet
Copy link
Member

baywet commented Nov 26, 2024

Instead of relying on 415 code response, is there another way we could identify compression is not supported to do the uncompressed body retry?

Not as far as I know. I think we made an assumption that APIs that do not support compression would reply with a 415 in the majority of cases. I now believe it's far more nuanced than that:

  • APIs that do support request body compression, but do not support the algorithm we're using would reply with a 415 in the vast majority of cases. This is very unlikely to happen in our case though as we're using the most "popular" compression algorithm (gzip).
  • APIs that do not support request body compression might send back a 415, or a 4XX status code. 415 is clearly linked to the content-type or encoding. Content type is unlikely to happen unless the description is wrong. So we can safely assume at the handler level it's an encoding issue and retry without the encoding. 4XX could be anything, from something not found, to the payload not being formatted properly for whatever reason, to throttling, etc... We could expand retrying on any 4XX, but that might have serious performance/latency impacts for clients to these APIs, with no added benefits. And it's not clear what's the proportion of APIs in the wild that are sending back 4XX for an unsupported content encoding.

And then there's the developer experience issue, I think most front end devs would expect to be able to inspect the payload with the developer tools in browser. It's interesting that those tools automatically decompress the response payload, but not the request payload. Or don't even offer a button to do so.

@maisarissi
Copy link

maisarissi commented Nov 26, 2024

Thanks for the additional information. And yes, I agree that going the 4xx route wouldn't be the way.

I think my first option would be to keep as it is, so with default request body compression, but make that very clear in our docs with additional content of how to disable that which is actually a adding your default set of middleware article.
Second would be disabling it for Kiota TypeScript and adding the relevant code for request compression in the Graph Core.

Curious to hear @sebastienlevert @darrelmiller opinions.

@baywet
Copy link
Member

baywet commented Nov 26, 2024

I also had a quick discussion with @captainsafia from the aspnet team. And she offered to look into any potential routing issue that might be happening here causing the wrong status code to be returned. @brad-technologik @qwertycho would one of you be able to create a reproduction API and share a repository here please?

@sebastienlevert
Copy link

Can we add some console.error when we "can" identify that this is an issue and then add a link to our docs? This would make the developer experience better (they would know it's happening, why, and what to do). This would be probably a TypeScript only requirement? Or would WASM also bring this challenge?

@baywet
Copy link
Member

baywet commented Nov 26, 2024

WASM also brings the same challenge, but the developer tooling might be different and bring different expectations from the developer. E.g. if I'm building a blazor web app, my IDE might be attached to the browser instead of using the developer tools on the browser. And that debugger might or might not do the request body decompression for me.

Console logging in production can have performance (and security if we start logging payloads) impacts. Some of which might be partially mitigated by using different levels like debug etc which get filtered automatically. And ultimately, should we log just on 415? also on any 4XX status code?

@baywet
Copy link
Member

baywet commented Nov 27, 2024

I went spelunking a little more on the developer experience aspect.

Chromium has a bug logged for this, so does firefox, I couldn't find an equivalent bug for safari or even test it, but I wouldn't be surprised if it were also impacted, Safari generally being behind the other two.

I tried identifying in chromium where the fix should be, but between the size of the repo, most of it being C++ and my C++ being extremely rusty, it wasn't clear to me.

It seems that both fix efforts are stalled on a missing repro, I'll try to reactive them by replying to the posts.

To test that in the tools, here is a quick snippet that can be pasted in the console.

async function compress(str) {
  // Convert the string to a byte stream.
  const stream = new Blob([str]).stream();

  // Create a compressed stream.
  const compressedStream = stream.pipeThrough(
    new CompressionStream("gzip")
  );

  // Read all the bytes from this stream.
  const chunks = [];
  for await (const chunk of compressedStream) {
    chunks.push(chunk);
  }
  return await concatUint8Arrays(chunks);
}
async function concatUint8Arrays(uint8arrays) {
  const blob = new Blob(uint8arrays);
  const buffer = await blob.arrayBuffer();
  return new Uint8Array(buffer);
}
const clearPayload = {
  displayName: "Jane doe"
};
const strPaylaod = JSON.stringify(clearPayload);
const compressedBytes = await compress(strPaylaod);
const requestOptions = {
   method: 'POST',
   headers: {'Content-Type': 'application/json', 'Content-Encoding': 'gzip'},
   body: compressedBytes
};
const response = await fetch('https://graph.microsoft.com/v1.0/me', requestOptions);

@illunix
Copy link
Contributor

illunix commented Nov 28, 2024

@illunix

You need to create a httpAdaper with a custom set of middleware. After that simply use the httpAdapter in a FetchRequestAdapter.

factory middlewares

Thank you @rkodev ! I have disabled the compression with this snipped:

    const http = KiotaClientFactory.create(undefined, [
      new RetryHandler(), new RedirectHandler(), new ParametersNameDecodingHandler(), new UserAgentHandler(),  new HeadersInspectionHandler()
    ])
  
    const adapter = new FetchRequestAdapter(new BearerAuthenticationProvider(), undefined, undefined, http);
    const client = createApiClient(adapter);

    client.api.endpoint.post({
      field: "testdata",
    })

The compression is a bit annoying when the api does not support it.
I did notice that the CompressionHandler is not in exported in the http fetch library, so that limits discoverability. Although it seems there isn't much to configure anyway.

This works but when I serve in development environment my Angular app, but for example when I try do prod build I get
Image

I'm using windows btw

@baywet
Copy link
Member

baywet commented Nov 28, 2024

@illunix this was fixed in #1499, please make sure your packages are up to date.

@ItsJustRuby
Copy link

We didn't get any feedback about APIs not supporting request body compression in other languages where it's been implemented to date as far as I know.

Just want to chime in on this: I set up a (what I thought would be) very straightforward C# ASP .NET Core (9.0) based API and tried to use Kiota as OpenAPI generator.

And even though these are both Microsoft products, they seemed to hold contradictory views:

ASP .NET Core does not respond with 415 to compressed requests - compression is in fact, entirely opt-in.

There were no error messages on either the Kiota client nor the ASP .NET Core server suggesting that there was a mismatch of expectations with regards to compression - even though they were configured in non-production settings (i.e. Release "Development" for the server).

This may not seem a critical issue to resolve - but it makes for very bad developer experience to already stumble when setting up a new tool that should be one of the "safest" choices - a USP that I feel Kiota aims to, but currently fails to provide.

@baywet
Copy link
Member

baywet commented Dec 27, 2024

Thanks for the additional input!

I did have a quick chat about that with @captainsafia internally today. It doesn't seem like turning support for request body decompression on by default is part of the asp.net philosophy. Same for updating the templates (dotnet new & co) to include it by default.

The best course of actions from an asp.net perspective is to include that aspect in the "performance" documentation section, and accept the fact that some people might turn it on, some people might not. And beyond asp.net developers, other ecosystems (java, etc...) might have the same fragmentation. So we should account for this.

This is before we even consider the experience impact of not being able to see the request body in the developer console. (no traction of the browsers issues so far)

Sending requests the service might not understand by default is a bit aggressive, we should probably evaluate having two "handlers profiles":

  • default: all the safe things that do not impact the request being sent out
  • performance: this would build on default and have request body compression for now, and things like caching, etags & co in the future.

Documentation would lead customers to select the performance profile when required. Graph clients would have their own selection of middleware handlers (default + a few things from performance we know graph supports + a few things that are in the graph core layer).

Thoughts?

@ItsJustRuby
Copy link

I appreciate you further digging into this!

From my POV as a developer who had just started out using Kiota, the thing I would have appreciated most would have been a clear communication about the implicit choices made for me. At that point in time, it was the default compression middleware, in the future it might be the default "handler profile".

Any solution like logging the selected default middleware/"handler profile" or making it necessary on an API level that the developer explicitly opt into one would have made my (debugging) life easier, so I welcome any movement in that direction.

@baywet
Copy link
Member

baywet commented Dec 30, 2024

Thank you for the additional information.

That's a great suggestion! Would a console.debug enumerating the active handlers (would log every time a client is created) been enough in your opinion?

@ItsJustRuby
Copy link

That would work!

@baywet
Copy link
Member

baywet commented Dec 30, 2024

Thanks for confirming @ItsJustRuby !

While we wait for other decisions to be made (most of the team is out on our side), is this something you'd like to submit a pull request for provided some guidance?

@ItsJustRuby
Copy link

Thank you for the opportunity 😄.
I have, however, moved on quite a lot from my original project that involved Kiota and do not have any interest in pursuing this further at this time.

@andreaTP
Copy link
Contributor

Hey 👋,
sorry for chiming in really quickly in this conversation, I'm on vacation with a very limited screen time ATM.

I love how this discussion progressed, but I lack to see taking in serious consideration turning compression off by default:

  • at least asp, quarkus and spring do not enable it by default (it would be great to have a full matrix of popular langs/frameworks)
  • as noticed, it doesn't play well with debugging
  • it easily makes the user experience troublesome for newcomers: i.e. it's arguably hard to find out this is the root cause of something not working out of the box with Kiota (this issue matches my own experience too FWIW)

I can foresee users trying out Kiota and moving away quickly as it doesn't work, without tweaks, with a pretty standard/expected setup.

On the other hand we have performances, and that's a very valid argument, still, making compression opt-in instead of opt-out would turn into a situation where:

  • ease newcomers experience
  • makes everything easier to debug
  • isn't surprising in some ecosystems
  • ppl concerned about perf can easily turn it on (documentation needed here)

Given Microsoft's APIs usually support it you can easily turn it on by default in the Graph SDKs, while keeping the default off for the above mentioned reasons.

Happy to hear feedback on this possible course of action :-)

@baywet
Copy link
Member

baywet commented Dec 31, 2024

@andreaTP thanks for chiming in, I think we're all on the same page given the last few replies here:

  • default profile (works for the vast majority of the APIs out there, at a performance trade-off in some scenarios)
  • perf profile (everything in default + compression and other potentially compatibility impacting handlers like caching, e-tag... opt-in)
  • a console debug to tell the developer what's active at glance.
  • Microsoft Graph SDK (and other applications) will mix and match to fine tune the set of handlers for the scenario.

There are two main reasons why this isn't progressing at this time:

  • I'd like a confirmation for this plan of actions from the PMs @maisarissi @sebastienlevert (both out of office)
  • On the engineering side, I'm pretty much the only one holding the fort at the moment.

Thank you for your patience. :)

@andreaTP
Copy link
Contributor

Looks like an awesome plan!
I missed some context here, sorry for the noise.

No rush and happy new year :-)

@sebastienlevert
Copy link

I like this idea. Making this (compression) opt-in seems reasonable and offers a great onboarding experience. We can chat more @baywet to identify the DevX of this thing and how it affects the developer wanting to write code and consume our docs. While I believe docs is a good way to describe this, I'd love to have something that is self-documented as part of the libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waits for author 🔁
Development

No branches or pull requests

10 participants