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

Defaulting to HTTP/2.0 breaks consumers using .NET Framework #237

Closed BodrickLight closed 5 months ago

BodrickLight commented 5 months ago

231 introduces a breaking change for consumers that use .NET Framework, as HTTP/2.0 is not a supported version in HttpRequestMessage.

Minimal repro with <TargetFramework>net472</TargetFramework>:

var adapter = new HttpClientRequestAdapter(new AnonymousAuthenticationProvider());
await adapter.SendNoContentAsync(new RequestInformation()
{
    URI = new Uri("http://example.com")
});

This works as expected on Microsoft.Kiota.Http.HttpClientLibrary version 1.3.8, but throws the following exception on 1.3.9 and 1.3.10:

Unhandled Exception: System.ArgumentException: Only HTTP/1.0 and HTTP/1.1 version requests are currently supported.
Parameter name: value
   at System.Net.HttpWebRequest.set_ProtocolVersion(Version value)
   at System.Net.Http.HttpClientHandler.CreateAndPrepareWebRequest(HttpRequestMessage request)
   at System.Net.Http.HttpClientHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
jcsawyer commented 5 months ago

I missed the PR but when the target is net462, the package System.Net.Http.WinHttpHandler is referenced. If the target of the consumer is net472, surely this package will not be pulled in and WinHttpHandler will not be available?

baywet commented 5 months ago

@jcsawyer this directive #if NETFRAMEWORK covers all net framework versions (anything not net core effectively), so I'm not sure what the concern is?

jcsawyer commented 5 months ago

@baywet if the package System.Net.Http.WinHttpHandler is referenced when the target is net462, will this work for consumers with a target of net472 or will it result in a The type or namespace name 'WinHttpHandler' could not be found?

I'm suspecting the latter?

The #if NETFRAMEWORK will resolve the type for net462 but no other NETFRAMEWORK target?

baywet commented 5 months ago

Ah, you're referring to this

I'm not sure whether other versions of netfx will favour the net462 version or the net standard versions. Maybe this is something that @andrueastman can shed a light on? We might have to update the directive, or the condition for the package.

jcsawyer commented 5 months ago

I am indeed. If I drop:

<ItemGroup Condition="'$(TargetFramework)' == 'net462'">
    <PackageReference Include="System.Net.Http.WinHttpHandler" Version="[6.0,9.0)" />
</ItemGroup>

in a net472 project, any references to WinHttpHandler will fail to build.

baywet commented 5 months ago

Thanks for the additional information. This is a fix we had in the previous version of the microsoft graph sdk for years nad here which only had net462 and netstandard2 targets while net472 was already available for years and we didn't get complaints about missing dependencies?

andrueastman commented 5 months ago

I believe in the context of a net472 project the package manager will pull the net462 dll and resolve the dependency. This is validated with the latest https://www.nuget.org/packages/Microsoft.Kiota.Http.HttpClientLibrary/1.3.11 release. The sample net4.7.2 project builds and runs as expected.

Any chance you can confirm if you are referencing the project as a submodule or as a package?

jcsawyer commented 5 months ago

TIL NET Framework packages are confusing as anything! It looks like everything is working with the 1.3.11 package - running a full e2e pipeline through on component to fully confirm for specific case, but I can re-open/open new issue if there are any problems there.

Many thanks

baywet commented 5 months ago

Thanks for keeping us in check! Closing