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

C# Netcore WebRequest and HttpClient functionality #8821

Merged

Conversation

Blackclaws
Copy link
Contributor

@Blackclaws Blackclaws commented Feb 24, 2021

This is WIP work tracking the addition of WebRequest and HttpClient as replacements for RestSharp, as some environments might find it undesirable to include an additional library (Unity, Blazor, etc.).

This is a bit orthogonal to the already open pull request and could be considered a different take on the subject:

#8331

It tries to keep as much of the original interface intact as possible. WebRequest more so than HttpClient which simply does not offer many of the customizations that WebRequest and RestSharp do.

You can try it out by setting the library to httpclient by cliswitch or adding:

library: httpclient

to the global config in the config file you're using. Should hopefully alleviate/fix #6800 and #853

If you also want to get rid of the dependency on Polly add

additionalProperties:
supportsRetry: false

aswell

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

 @mandrean @frankyjuang @shibayan

@auto-labeler auto-labeler bot added the WIP Work in Progress label Feb 24, 2021
@auto-labeler
Copy link

auto-labeler bot commented Feb 24, 2021

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@Blackclaws Blackclaws marked this pull request as draft February 24, 2021 11:34
@Blackclaws Blackclaws force-pushed the csharp-netcore-httpclient-alternative branch from d7f9c5f to cbabf1c Compare February 24, 2021 16:56
@Blackclaws
Copy link
Contributor Author

I think this is at a point now where it is ready to be reviewed. It still doesn't work with Blazor but it should work with Unity, I'll test that later today.

In general I think a separate generator such as @shibayan or @devhl-labs has been working on might be needed to fully support Blazor because quite a few options simply aren't possible there.

@Blackclaws Blackclaws changed the title WIP: C# Netcore WebRequest and HttpClient functionality C# Netcore WebRequest and HttpClient functionality Feb 25, 2021
@Blackclaws Blackclaws marked this pull request as ready for review February 25, 2021 11:04
@wing328 wing328 added Client: C-Sharp Enhancement: Feature and removed WIP Work in Progress labels Feb 28, 2021
@wing328
Copy link
Member

wing328 commented Feb 28, 2021

Thanks for the PR. I played with it locally and it looks very good to start with 👍

I'll merge it so that users can try it out in the upcoming 5.1.0 release (this month or next), and will submit PRs to further improve it (e.g. README, petstore integration tests in CI, etc)

@@ -19,18 +19,28 @@ using System.Web;
{{/netStandard}}
using Newtonsoft.Json;
using Newtonsoft.Json.Serialization;
using ErrorEventArgs = Newtonsoft.Json.Serialization.ErrorEventArgs;
{{#useRestSharp}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create a separate ApiClient.mustache just for httpclient library similar to how we support multiple HTTP libraries in other client generators as too many {{#useRestSharp}} ... {{/useRestSharp}} etc make the template hard to maintain.

@wing328 wing328 merged commit e815d7c into OpenAPITools:master Feb 28, 2021
@wing328 wing328 added this to the 5.1.0 milestone Mar 16, 2021
@gbt518
Copy link

gbt518 commented Mar 24, 2021

Thank you for this PR!

I found one small issue as I was testing this out in 5.1.0. I noticed that a new HttpClient is created for each call.

From https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient?view=net-5.0:

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors.

@VincentNe
Copy link

I would like to try out the new HttpClient generator but I don't really have a clue on how to enable the new generator with the cli.

So far I've tried the following command:
openapi-generator-cli generate -i api.json -g csharp-netcore -o Api.Client.NetCore --additional-properties library=httpclient
And I see that it's still using the RestSharp library, can anyone give me an example cli command on how to make use of the new library.

@devhl-labs
Copy link
Contributor

devhl-labs commented Mar 25, 2021

There are two versions of HttpClient, mine and Blackclaws. Mine only uses one instance of HttpClient that you pass in, however, it only supports basic requests and JWT tokens at the moment. Looks like Blackclaw's version may have made the 5.1 release, but mine didn't. I have a pr to add it though.

@VincentNe try just --library httpclient without the additional properties.

@Blackclaws
Copy link
Contributor Author

Thank you for this PR!

I found one small issue as I was testing this out in 5.1.0. I noticed that a new HttpClient is created for each call.

From https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient?view=net-5.0:

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors.

Yes, this was indeed the case in order to be mostly a drop in replacement for restsharp. As creating new HttpClients for each request is indeed problematic in some cases there is now a new pull request to change this behaviour to be by default a static HttpClient for each ApiClient which however is also customizable from outside.

@sleipper
Copy link

Some great work here @Blackclaws. I have some feedback if you're interested:

  1. ApiClient attempts to deserialise to a model regardless of status code. For example a 404, 401, 500 would likely have a different model say with validation or problem details (https://docs.microsoft.com/en-us/aspnet/core/web-api/handle-errors?view=aspnetcore-5.0). Could also not return JSON or no content in the case of a 204 response code. Typically in the case at present the error returned is:
    Most of ApiException : Unexpected character encountered while parsing value: P. Path '', line 0, position 0.
    Stack Trace:

  2. Generated API doesn't work with just HttpClient only, this is called out in the Readme but would be better to not permit invalid use. E.g new PetApi(client); the use of this will error as you need to set disableHandlerFeatures to true.

  3. ApiClient NewRequest method uses Properties e.g. request.Properties["CookieContainer"], but this is obsolete, should used options instead. There is a TODO but due to your PR to avoid HttpClient being used for each call looks like this is per request but doesn't appear to be a way to set Cookies as localVarRequestOptions is always created.

  4. Minor issue

    • RetryConfiguration class not static

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REQ] New C# Generator that uses HttpClient instead of RestSharp
6 participants