OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.74k stars 6.56k forks source link

[REQ][csharp-netcore][client] Move `RestClient` to `Configuration` #11271

Open Anakael opened 2 years ago

Anakael commented 2 years ago

Is your feature request related to a problem? Please describe.

In csharp generator inner RestClient is saved in Configuration, but in csharp-netcore not and it created for each request. It doesn't allow to configure RestClient, for example, add IAuthenticator from RestSharp and also less perfomant as reusage of RestClient as it was in csharp generator.

Describe the solution you'd like

I'd like to move creating RestClient in Configuration class.

Describe alternatives you've considered

As alternative solution for only authorization IAuthenticator can be added to *Api and can be consumed for new RestSharp when it created.

If moving RestClient in Configuration is ok, I can try to implement it by myself.

Anakael commented 2 years ago

@mandrean @frankyjuang @shibayan @Blackclaws @lucamazzanti

Blackclaws commented 2 years ago

Is this just relevant for the restsharp version csharp-netcore or does this also have any relevance for the httpclient version?

I unfortunately have little to know knowledge about the restsharp version as I don't use it and never have.

Anakael commented 2 years ago

Is this just relevant for the restsharp version csharp-netcore or does this also have any relevance for the httpclient version?

I unfortunately have little to know knowledge about the restsharp version as I don't use it and never have.

Only restshasp.

Is httpclient version ready for usage? Does it have any benefits compare to restshasp version?

Blackclaws commented 2 years ago

Httpclient is ready for usage. RestSharp is not really supported for .net 5 and up and using it leads to socket exhaustion if too many connections are made too fast.

Anakael commented 2 years ago

Httpclient is ready for usage. RestSharp is not really supported for .net 5 and up and using it leads to socket exhaustion if too many connections are made too fast.

Is there any reason why it is marked as experimental?

Anakael commented 2 years ago

using it leads to socket exhaustion if too many connections are made too fast.

Because it creates new connection on every request.

I was talking about that:

also less perfomant as reusage of RestClient as it was in csharp generator.

Blackclaws commented 2 years ago

I think this project is still using a restsharp version before the vnext change (which is v107). This has general problems with .net 5 and up namely socket exhaustion due to the way it uses HttpWebRequest internally. Therefore Restsharp is simply a bad choice for .net 5 and up unless you are willing to also up that version in the process (which might break some htings or not, no clue honestly).

I have no clue about the performance issues that might arise from where the client is.

Would what you propose be a breaking change? If not feel free to open a pull request.

As for the httpclient being still marked as experimental, that's a good question. Its stable enough for everyday use. I think the background for that was that we wanted to keep the door open to breaking changes.

Anakael commented 2 years ago

-> unless you are willing to also up that version in the process (which might break some htings or not, no clue honestly). It will, because restsharp have changed RestRequest https://github.com/OpenAPITools/openapi-generator/blob/7129cdebc5524fb2516822d33b4d2000096cb72b/modules/openapi-generator/src/main/resources/csharp-netcore/ApiClient.mustache#L189 to: https://github.com/restsharp/RestSharp/blob/c9b8cfad1f7556c1066739db7724cfd670c31fc9/src/RestSharp/Request/RestRequest.cs#L32 -> I have no clue about the performance issues that might arise from where the client is. Reusing HttpClient (which RestSharp is based on) can lead to reusing connection.

-> Would what you propose be a breaking change? If not feel free to open a pull request. Actually, as for now I'm not sure if it needed for me anymore, since httpclient has more convenient API for custom authorization. But I plan to apply bugfix to httpclient also.

Fell free to close this issue if nobody else is interested in it.

Thank you!

segevfiner commented 1 month ago

This annoyingly seems to also prevent customization of the HttpClient, e.g. for using it over a named pipe...