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

Add support to provide default middleware options in kiota factory #130

Closed
andreaTP opened this issue Dec 15, 2023 · 5 comments · Fixed by #139
Closed

Add support to provide default middleware options in kiota factory #130

andreaTP opened this issue Dec 15, 2023 · 5 comments · Fixed by #139
Assignees
Labels
enhancement New feature or request WIP

Comments

@andreaTP
Copy link

andreaTP commented Dec 15, 2023

Hi everyone! 👋
I started building a new SDK in Go for Apicurio Registry, my experience performing a GET has been completely smooth, great job!

It required quite some digging to find out that an interceptor for compression is enabled by default, and that's surprising, I haven't found a better way to disable it than creating a new client without it:

	adapter, err := kiotaHttp.NewNetHttpRequestAdapterWithParseNodeFactoryAndSerializationWriterFactoryAndHttpClient(
		&authProvider,
		nil,
		nil,
		kiotaHttp.GetDefaultClient(
			kiotaHttp.NewRetryHandler(),
			kiotaHttp.NewRedirectHandler(),
			kiotaHttp.NewParametersNameDecodingHandler(),
			// NewCompressionHandler(),
			kiotaHttp.NewUserAgentHandler(),
			kiotaHttp.NewHeadersInspectionHandler(),
		),
	)

My expectation would be: compression should be disabled by default and we should document how to toggle it on and off

@baywet
Copy link
Member

baywet commented Dec 15, 2023

Hi @andreaTP
haha I thought you already had published some go kiota based SDK.
quick question around the compression handler: was it causing any issue? if so I'd rather solve the underlying issue than disable it by default. Ideally we'd want kiota clients to make the fastest/smallest request possible.

@andreaTP
Copy link
Author

haha I thought you already had published some go kiota based SDK.

done something but haven't finalized it at the time.

was it causing any issue?

Yes, the server doesn't support it 🙂 , if you wanna keep the default turned on we should have an easy switch, e.g. a method on the RequestAdapter.

@baywet
Copy link
Member

baywet commented Dec 15, 2023

I'm assuming the issue is only with requests which have a request body?
In which case, could we:

  1. add a "Should Compress Request Body" flag on the option (default false)
  2. add a "Should Compress Response Body" flag on the option (default true)
  3. change the description of "Should Compress" to point out this flag is deprecated
  4. change the default value of "Should Compress" to true
  5. only compress the request body when Should Compress and Should Compress Request Body are both true. (so we don't impact existing behavior for people who passed a value
  6. only sent the accept-encoding header when Should Compress and Should Compress Response Body are both true.

This way the only behaviour change we introduce in most cases is to stop compressing request bodies.

For Microsoft Graph we'll have to coordinate to set the new options/middleware properly.

As for "an easy switch", we'd have to think of a design that works across languages and doesn't produce a "global settings object" (mixing concerns). I think we could potentially have a design where the client factory accepts a set of default handler options for the requests. This way you could do something like this:

kiotaHttp.GetDefaultClientWithOptions([] {compressionOptionYouWant])

Which would:

  1. add all the default middleware
  2. add those options on each request (via another middleware handler)

What do you think?

@andreaTP
Copy link
Author

I'm assuming the issue is only with requests which have a request body?

Possible, I have experienced it on a POST and haven't dug deeper.

Regarding how to roll out the change I don't have strong opinions, whatever fits the best for you.

I thought about this a little more and:

  • having the option to use compression is good
  • the sensible default is disabled as it's the same default as the underlying Http client (changing it breaks the principle of least surprise)
  • this should be documented a bit more clearly

@baywet baywet added this to Kiota Dec 18, 2023
@github-project-automation github-project-automation bot moved this to Todo in Kiota Dec 18, 2023
@baywet baywet added the enhancement New feature or request label Dec 18, 2023
@baywet
Copy link
Member

baywet commented Dec 20, 2023

We discussed that further internally. The downside of a dedicated middleware is that it will result in a lot of allocations at runtime.

We could still add this overload with the default options, and have spaghetti code in there, the implementation would look something like this:

if (options[keyForCompression] != nil) {
    middlewares = append(middlewares, NewCompressionHandlerWithOptions(options[keyForCompression]))
} else {
    middlewares = append(middlewares, NewCompressionHandler())
}

This way the allocations only happen once per client, and rely on existing defaults mechanisms

@baywet baywet changed the title Compression Middleware Add support to provide default middleware options in kiota factory Dec 20, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Kiota Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants