microsoft / kiota-http-dotnet

Kiota http provider implementation for dotnet with HttpClient
https://aka.ms/kiota/docs
MIT License
34 stars 15 forks source link

Suggestion: KiotaClientFactory should allow for setting handler options #233

Closed pschaeflein closed 4 months ago

pschaeflein commented 5 months ago

My use case is a custom user agent. Using the provided factory (in C#), I need to set request options on every call to pass the UserAgentHandlerOptions.

Ideally, I could set the options when the default handlers are created. For example:

static IList<DelegatingHandler> CreateDefaultHandlers(UserAgentHandlerOption? userAgentHandlerOption = null)
{
  return new List<DelegatingHandler>
  {
    //add the default middlewares as they are ready
    new RetryHandler(),
    new RedirectHandler(),
    new ParametersNameDecodingHandler(),
    new UserAgentHandler(userAgentHandlerOption),  // <-- this
    new HeadersInspectionHandler(),
  };
}

Obviously, having a parameter for every handler becomes unwieldy, so perhaps there is a better approach.

I thought about enumerating the resulting list, but the base DelegatingHandler class does not understand the Kiota options:

var handlers = CreateDefaultHandlers();
foreach (var handler in handlers)
{
  if (handler is UserAgentHandler userAgentHandler)
  {
    userAgentHandler.Options = userAgentHandlerOption;  // <--  compile error
  }
}

All comments appreciated. (I'm happy to submit a PR once we have a solid approach.)

baywet commented 5 months ago

Hi @pschaeflein Thanks for making the suggestion, this is something we were talking about not long ago but forgot to log an issue for. (or was it with you I talked about that? I'm getting old 🤣)

What would you think of this implementation for the create default handlers method


static IList<DelegatingHandler> CreateDefaultHandlers(IRequestOptions[] optionsForHandlers = [])
{
  return new List<DelegatingHandler>
  {
    //add the default middlewares as they are ready
    new RetryHandler(),
    new RedirectHandler(),
    new ParametersNameDecodingHandler(),
    optionsForHandlers.OfType<UserAgentHandlerOption>().FirstOrDefault() is UserAgentHandlerOption uaOption ? new UserAgentHandler(uaOption) : new UserAgentHandler(),  // <-- this
    new HeadersInspectionHandler(),
  };
}
pschaeflein commented 5 months ago

Yup. Thank you. ;)

baywet commented 5 months ago

Thanks for confirming @pschaeflein ! Would you be willing to submit a pull request to implement the change?

pschaeflein commented 5 months ago

Working on it.

Struggling with error >CSC : error CS9057: The analyzer assembly 'D:\Repos\microsoft\kiota-http-dotnet\Kiota.Generated\bin\Debug\netstandard2.0\KiotaGenerated.dll' references version '4.9.0.0' of the compiler, which is newer than the currently running version '4.8.0.0'. which is preventing me from running test.

baywet commented 5 months ago

Make sure you have the latest of net 8 installed

pschaeflein commented 5 months ago

Yup. Did that, and rebooted. No luck.

I hacked around it by removing the analyzer reference from the csproj. But the testability of the handlers is not possible in the current state.

The factory methods returns IList<DelegatingHandler>. The test method then has no way to determine if the handler options are set. Perhaps if there was an IMiddlewareHandler interface...

Would you like a PR without tests? Or should we chat further about a testing approach?

https://github.com/microsoft/kiota-http-dotnet/compare/main...pschaeflein:kiota-http-dotnet:options-in-factory?expand=1

baywet commented 5 months ago

related #245 This error is really annoying, you're not the first one to experience it, and it is usually related to an update issue on the dev environment... Please put the PR together, we can iterate on it this way.