Resurrect the interfaces removed in v107

Someone decided that they did not like the idea of exposing interfaces for the RestSharp library, because they believe mocking them is a bad idea:

https://restsharp.dev/v107/#mocking

While I do not disagree that mocking those interfaces is probably not a smart choice for mocking in your test code, I completely disagree that they should have been removed. Removing them is enforcing someone else's design choices on everyone else. To make matters worse, the way the code was written before we had no choice but to reference the interfaces because that is what was exposed to us via the API's. So all our code is littered with references to those interfaces, so changing over to v107 is now a HUGE amount of work for us. I already started on it and gave up after wrestling with the issues for a few hours, especially due to the changes to refactor RestClient into a single instance at the same time.

As mentioned here:

https://github.com/restsharp/RestSharp/issues/1765

Since v107 is now the active version of RestSharp, not only do I have the problem of trying to port my own code, but what happens when I now import a third party library that also relies on the RestSharp library? I can no longer use a binding redirect to force that library to use v107+, because if that library is using v106 or earlier, IT WILL BREAK!

Since v107 is now the active version, is anyone maintaining and doing bug fixes for v106? I doubt it. So what happens if we find a critical vulnerability in RestSharp that exists in v106 and earlier. Nobody can produce a patch for it, so the only option is to patch v107, but then all the millions of developers now exposed to this flaw CANNOT upgrade to v107 without a significant code rewrite. We cannot simply load up Nuget and update to the latest library, recompile and deploy with the fixes.

To be blunt, the correct solution here is to deprecate EVERY SINGLE version of RestSharp v107 and implement a new v108 that is based on v106 with the design changes I mentioned here:

https://github.com/restsharp/RestSharp/issues/1765#issuecomment-1065952306

Then if folks really want to continue with a v107 like release, it should be a COMPLETELY NEW NuGet Package (RestSharp.VNext) or something. Then we won't have completely busted binding redirects and we can still rely on NuGet packages using v106 or earlier and upgrade our version to v108 or later and everything works.

If we don't solve this soon, RestSharp is going to fragment and get forked. Nobody wants that.

To be blunt, if the old behavior is desired it should be forked and maintained separately. I fully support a maintainer taking a library in a new direction at their own discretion.

On Sat, Mar 12, 2022, 13:07 Kendall Bennett @.***> wrote:

To be blunt, the correct solution here is to deprecate EVERY SINGLE version of RestSharp v107 and implement a new v108 that is based on v106 with the design changes I mentioned here:

1765 (comment)

https://github.com/restsharp/RestSharp/issues/1765#issuecomment-1065952306

Then if folks really want to continue with a v107 like release, it should be a COMPLETELY NEW NuGet Package (RestSharp.VNext) or something. Then we won't have completely busted binding redirects and we can still rely on NuGet packages using v106 or earlier and upgrade our version to v108 or later and everything works.

If we don't solve this soon, RestSharp is going to fragment and get forked. Nobody wants that.

— Reply to this email directly, view it on GitHub https://github.com/restsharp/RestSharp/issues/1784#issuecomment-1065954465, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABASFFSPUC62KOOPYNJVQDDU7T2OZANCNFSM5QSJWFVQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

To be blunt, if the old behavior is desired it should be forked and maintained separately. I fully support a maintainer taking a library in a new direction at their own discretion.

I have no issues with this either, but it should not be the same NuGet package! You can't push a brand new NuGet package for v107 that is completely different to v106 and not expect people to completely lose their mind when trying to use it.

That's why we have .NET Framework and .NET Core. That is why we have different versions of .NET Core and you can target each different version. Design decisions made for .NET 6 don't BREAK CODE for folks using .NET Core 2, or .NET Core 3.

@kendallb I have a suggestion. I am getting tired of this, really. There's no joy for me to work on RestSharp, nor any other source of satisfaction. You keep talking about "millions of developers", and I just keep spending hundreds of hours per year for free as just a single person from those mythical millions sponsor this project.

What is my suggestion: I stepped in as a maintainer for RestSharp after a call for maintainers being unanswered for many months. Apparently, none of those "millions" was ready to maintain RestSharp. So, I propose that I will call for maintainers again, and step down as the maintainer. I also suggest that you, @kendallb, would take over the maintenance, since you have a very strong opinion about the design choices in RS 107, so you "fix" it the way you find fit.

@alexeyzimarev if you are going to make huge breaking changes in a library that you maintain, while all of us who use the library appreciate the work you put into maintaining it, you can't expect us not to complain when a new version is completely incompatible with a prior version, when it's published as the same NuGet package? Your standard response when people complain about breakages is to say 'it is by design', but when someone second guesses that design, you throw up your hands and say you will now step down as maintainer?

You have not yet actually responded to the core issues in my complaint, which are:

1) v107 completely breaks functionality with the v106 and earlier releases, for what does not seem like a reasonable reason. What is the reason? You don't think people should mock IRestRequest etc? You believe folks should create a singleton instance of RestClient for every API they consume? All valid goals in their own right, but not really a valid reason to break compatibility?

2) v107 with such huge breaking changes was released as the same NuGet package. Which means someone cannot update to it and then rely on another Open Source project that relies on v106 and earlier, without also making sure those projects are updated to v107+ at the same time.

I do not myself have enough understanding of the internals of RestSharp to maintain it for the entire suite of users who use it as we only use a minor subset of it in our own code. I also maintain plenty of my own Open Source projects and contribute to many others, so I do understand what it takes to be a maintainer. But I do think as a maintainer of other projects and a contributor to other projects, that is really important to make trade offs between compatibility with existing code and complete re-writes. Rewrites that do not change the API surface area are fine, but re-writes that change the API surface area should really be published as an entire new NuGet package stream.

As I said in the other thread, I do not have any issues at all with the desire to change over to using HttpClient internally. It's a good goal and one that I welcome. But I do have significant issues with how it was achieved.

As for what I plan to do, I just loaded up Visual Studio 2022 and plan to take a good hard look at all the changes made in v107 to date and see if I can refactor the code in such a way to achieve some of the goals you wanted to achieve, without completely breaking the API surface area.

One of my biggest gripes about HttpClient is that I think it is fundamentally broken also (I complained about that to Microsoft ages ago, but it won't get fixed). The problem is the fact that HttpClient accepts a message handler pipeline but there is no way to change that on a per request basis. Within the scope of RestSharp itself the HttpClientHandler instance is configured with a large amount of what is set up in RestClientOptions:

    void ConfigureHttpMessageHandler(HttpClientHandler handler) {
        handler.Credentials            = Options.Credentials;
        handler.UseDefaultCredentials  = Options.UseDefaultCredentials;
        handler.CookieContainer        = CookieContainer;
        handler.AutomaticDecompression = Options.AutomaticDecompression;
        handler.PreAuthenticate        = Options.PreAuthenticate;
        handler.AllowAutoRedirect      = Options.FollowRedirects;
        handler.Proxy                  = Options.Proxy;

        if (Options.RemoteCertificateValidationCallback != null)
            handler.ServerCertificateCustomValidationCallback =
                (request, cert, chain, errors) => Options.RemoteCertificateValidationCallback(request, cert, chain, errors);

        if (Options.ClientCertificates != null) {
            handler.ClientCertificates.AddRange(Options.ClientCertificates);
            handler.ClientCertificateOptions = ClientCertificateOption.Manual;
        }

        if (Options.MaxRedirects.HasValue) handler.MaxAutomaticRedirections = Options.MaxRedirects.Value;
    }

So if I do try to use my own single instance of HttpClient, it is not really possible to do that if RestSharp is relying on the handler to implement a lot of this functionality (such as the cookie container, credentials, proxy support etc). Unless I also configure my single instance of HttpClient to support that, but if it's shared with other code I can't infect that other code with that either. We currently use a shared httpClient single instance across all our Swagger API proxy code, which internally can use HttpClient, but it does not rely on anything in the pipeline so it works fine. But I cannot share that one with RestSharp.

I actually don't think there is any real value in exposing a constructor in RestClient to accept an external HttpClient since a lot of what is configured in RestClientOptions will no longer apply. The constructor is below:

    public RestClient(HttpClient httpClient, RestClientOptions? options = null, bool disposeHttpClient = false) {
        if (options?.CookieContainer != null) {
            throw new ArgumentException("Custom cookie container cannot be added to the HttpClient instance", nameof(options.CookieContainer));
        }

        UseDefaultSerializers();

        HttpClient         = httpClient;
        Options            = options ?? new RestClientOptions();
        CookieContainer    = new CookieContainer();
        _disposeHttpClient = disposeHttpClient;

        if (httpClient.BaseAddress != null && Options.BaseUrl == null) {
            Options.BaseUrl = httpClient.BaseAddress;
        }

        ConfigureHttpClient(HttpClient);
    }

As you can see it does not configure the handler (it cannot as it does not own it), yet it accept RestClientOptions as a parameter. A user would fully expect those options would apply to requests, but would be disappointed to discover they do not.

The only correct way to use the library in it's current form, is to have a single instance per API so it can have it's own single instance of HttpClient with it's own handler. This is fundamentally part of what I think is broken with HttpClient as there is no way to set some stuff on a per request basis (like just the cookie container). I would have helped tremendously if a large amount of what is handled in the HttpClientHandler could be done via a request message, or if a handler instance could be passed to HttpClient.SendAsync(), but it cannot:

https://github.com/dotnet/runtime/issues/1904

You can set cookies on a per request basis by manually stuffing in the headers (that is how we ended up doing it), but if you needed any of the other functionality currently implemented by the HttpClientHandler, you are sorely out of luck. This is pretty much the same conclusion I came to last time I wrestled with HttpClient and trying to use it in our own code.

As such I am not sure it is even feasible to implement RestClient on top of HttpClient without expecting RestClient to be a single instance per API endpoint as you have suggested, or at the very least a single instance shared across the entire application stack with no base URL configured.

I am going to continue work on trying to get our code moved over to RestSharp v107 and the dependent libraries I maintain that also use it. I really wish RestSharp Next was a different NuGet package to the current one.

I know for a fact that people use both of the new constructors: the one that accepts HttpClient, and the one that accepts HttpMessageHandler. So, if you don't find it useful, it doesn't mean that nobody finds it useful.

Regarding the API changes, I also know for a fact that up to 80% of the code that was using RestSharp 106 and written according to the very old RestSharp documentation (Twilio API client and so on), compiles and works. Issues come with explicit typing (IRestClient client instead of var client), dependencies on IRestClient, synchronous calls, and, of course, mocking.

The IRestRequest and IRestResponse are particularly nonsense as these should be property bags. No one should ever put an interface on a property bag unless it's a "marker interface" (which I don't like either, but I understand it).

As for the IRestClient interface, it was astonishingly large. RestSharp even had a test that ensured that not a single public member of RestClient would not be a part of the interface, so even public properties landed in the interface, and it became a huge burden to maintain. Again, for a lot of people changing IRestClient to RestClient as a dependency worked just fine.

Here comes mocking. My arguments are explicitly listed, and I received a lot of nonsense coming to me including:

I don't know how to write software
It's not SOLID
Everything must be coded to an abstraction
We must code to abstraction because our managers tell us so

I consider none of those arguments valid, and some were clearly offensive, and, therefore, I had, and I still have no intention to discuss those. I am writing it here to make it very clear.

With regards to mocking for unit tests, I asked, in particular, to give some samples of such tests in this issue: https://github.com/restsharp/RestSharp/issues/1696

Even after I asked multiple times, not a single example of a test with mocked IRestClient was added to this issue. It is extremely annoying, and that's the reason why the issue never moved forward. Despite being against bringing the IRestClient interface back, I am ok with doing it. However, I don't understand what the API surface of that interface should be. Again, I received very little constructive feedback.

Your way to do a hard fork is totally fine. But, are you looking at getting interfaces back only? I can assure you that I have considered an option to have some sort of HttpMessageHandler factory, cache and lifecycle management as Microsoft did in DefaultHttpClientFactory. But, to me, that code looks insanely complex for no particular reason, as it's much easier to use separate, pre-configured HttpClient instances.

If one can't, or doesn't want to, it can be solved in a similar way by building IRestClientFactory with a default implementation, which would not do any voodoo, but will instead use named options. That would allow using the factory itself as a dependency, and request particular instances as transient dependencies, as the factory would use pooled handler instances based on registered options by name. I have already done it in another library. It's reasonably complex, but still a lot simple compared with DefaultHttpClientFactory magic.

So, if you skip my counter-rant part, I am still trying to figure out what you want. Interfaces for backwards compatibility is one thing. It can be solved with a shim (for example). The shim won't solve the issues with libs that use RS 106 anyway, you are right here. But, using RestSharp as a transient dependency is a completely different issue, which should be treaded differently as I explained.

Final words. I understand what triggers your rage. But I don't agree with your way to address it, by aggressively taking on me personally. I maintained this, as I said before, almost single-handedly, for many years, without any financial support, without much code contributions, etc. The changes I made were not only targeting the switch to HttpClient, but also to reduce the code surface, making sense of the API, and making the whole thing easier to maintain. That goal was achieved, and it was not an easy task. The old RestSharp was a big mess to maintain. With regards to OSS and breaking changes, you can look at AutoMapper and MediatR. AutoMapper at one point lost all the static configuration, which was used by millions. They were eventually brought back (as far as I know, but I don't use it anyway), but the change was made by Jimmy because he did the change as he sees fit. MediatR at some point lost all sync code, and all the signatures became invalid at once. I did a refactoring to the newer version once (I don't use MediatR for a while), and it took days. The last thing I thought about it going to the MediatR repository, opening an issue about that, and starting to rage about Jimmy being a poor OSS mantainer.

I know for a fact that people use both of the new constructors: the one that accepts HttpClient, and the one that accepts HttpMessageHandler. So, if you don't find it useful, it doesn't mean that nobody finds it useful.

True

Regarding the API changes, I also know for a fact that up to 80% of the code that was using RestSharp 106 and written according to the very old RestSharp documentation (Twilio API client and so on), compiles and works. Issues come with explicit typing (IRestClient client instead of var client), dependencies on IRestClient, synchronous calls, and, of course, mocking.

Well I also know for a fact that I have spent three full days already porting our code to v107, and it is still not done (more PR's will be coming for stuff that I think needs fixing). The fact that a couple of simple examples work out of the box does not help someone port existing code to v107.

The IRestRequest and IRestResponse are particularly nonsense as these should be property bags. No one should ever put an interface on a property bag unless it's a "marker interface" (which I don't like either, but I understand it).

I do not disagree, as I said at the start of this issue.

Here comes mocking. My arguments are explicitly listed, and I received a lot of nonsense coming to me including:

Again, I never disagreed with that. I 100% agree there is zero value in mocking IRestRequest or especially IRestClient. We mock our code at the client API level, and have integration tests to make sure the actual very thin client code works. So no arguments from me there.

Your way to do a hard fork is totally fine. But, are you looking at getting interfaces back only?

Yes. Not because I plan to mock them (we don't), but simply because the old API returned them all over the place and relied on them all over the place. So without the interfaces, it makes porting code to v107 much, much harder. For instance there are 35 references to IRestRequest and 18 references to IRestClient in our code base. That is a lot of stuff that needs to be changed.

Granted the other half of the problem is that the API surface area for RestRequest and RestClient have changed so dramatically that a simple search and replace of IRestRequest to RestRequest and IRestClient to RestClient does not suffice either.

So perhaps resurrecting them really won't help solve the porting pain that myself or others will experience. To ease the pain, the only real solution is to bring back the ability to have RestClient be instance per request, and have it internally manage an appropriate instance cache of HttpClient's. But that in itself is a huge amount of work (I know as I started to look into it) given the amount of changes already accomplished.

I can assure you that I have considered an option to have some sort of HttpMessageHandler factory, cache and lifecycle management as Microsoft did in DefaultHttpClientFactory. But, to me, that code looks insanely complex for no particular reason, as it's much easier to use separate, pre-configured HttpClient instances.

Yes, there are really good reasons for having an HttpClientFactory, the biggest of which is to use pool HttpClientHandlers as outlined in the Microsoft documentation on it:

https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests

All of this gets very messy when trying to support multiple frameworks which is really well outlined in this post:

https://github.com/dotnet/aspnetcore/issues/28385#issuecomment-853766480

Taking a dependency on HttpClientFactory might help solve the problem but introduces it's own. The way the use of HttpClient is currently configured in RestSharp, it's a singleton tied to the singleton RestClient, which means it will suffer from the DNS change issues as outlined here:

http://byterot.blogspot.com/2016/07/singleton-httpclient-dns.html

There are ways to fix that but it's different for each target framework. Thankfully in our case we don't have DNS changes to worry about at runtime, so it won't affect me but it may well affect other users.

If one can't, or doesn't want to, it can be solved in a similar way by building IRestClientFactory with a default implementation, which would not do any voodoo, but will instead use named options. That would allow using the factory itself as a dependency, and request particular instances as transient dependencies, as the factory would use pooled handler instances based on registered options by name. I have already done it in another library. It's reasonably complex, but still a lot simple compared with DefaultHttpClientFactory magic.

Yes, I would agree and removes an external dependency. I have already built my own RestClientFactory for the code I am porting to v107 right now, but I keep running into stuff that can use problems across re-use of the client (such as setting the Authentication handler in the client, not the request which means it will blow up if two threads need different authentication values for each request).

Final words. I understand what triggers your rage. But I don't agree with your way to address it, by aggressively taking on me personally.

For that I am sorry.

My intention was not to piss you off, but to outline the significant amount of work it is going to take me to refactor all our code to use v107 which will by extension affect every other developer who takes on this task as well. There are REALLY good reasons to want to move to v107 and utilize HttpClient, it's just making it very difficult to migrate. It would have been much nicer if the changes to take advantage of HttpClient were much smaller than they are today.

I still believe v106 should have remained as it's own NuGet package and v107 should have been RestSharp.Next or something. Then it would be really clear to anyone wanting to move over to it that it's entirely different.

I maintained this, as I said before, almost single-handedly, for many years, without any financial support, without much code contributions, etc.

Yes, maintaining Open Source code is a thankless task and nobody is going to get rich doing it. Most folks get involved to scratch their own itch (myself included), but when the itch goes away, they move on. At this point to show my support I will contribute financially to this project.

The changes I made were not only targeting the switch to HttpClient, but also to reduce the code surface, making sense of the API, and making the whole thing easier to maintain. That goal was achieved, and it was not an easy task. The old RestSharp was a big mess to maintain.

Agreed and I appreciate the work you have done. I have wanted to migrate all this code to HttpClient for years, so now I will accept the pain of porting to v107 and get on with the job.

With regards to OSS and breaking changes, you can look at AutoMapper and MediatR. AutoMapper at one point lost all the static configuration, which was used by millions. They were eventually brought back (as far as I know, but I don't use it anyway), but the change was made by Jimmy because he did the change as he sees fit. MediatR at some point lost all sync code, and all the signatures became invalid at once. I did a refactoring to the newer version once (I don't use MediatR for a while), and it took days. The last thing I thought about it going to the MediatR repository, opening an issue about that, and starting to rage about Jimmy being a poor OSS mantainer.

I use AutoMapper and have been down that path. But just because Jimmy did something does not mean it was right, but to Jimmy's credit he does listen to feedback and did go back and make changes to accommodate the users of the libraries and make it easier for those folks to migrate their code.

And BTW I was not ever tying to say you are a poor OSS maintainer, I was just expressing my opinion that the approach taken is going to cause a lot of pain for developers like myself, and as more folks discover v107 and try to move to it, a lot more questions will crop up.

But what is done is done now and I am just going to deal with it and move on. So we can close this issue and call it a day.

I will provide PR's for things I believe need to change to make using this library in the new singleton manner safer, and hopefully we can get those merged upstream. I really do not want to maintain my own fork.

BTW, here is a library that uses RestSharp and has updated their library to v107 already, but have done is completely wrong:

https://github.com/EasyPost/easypost-csharp/blob/b04f71c5b505a9ed09ab0e106b7121e344571bbe/EasyPost.Net/Client.cs#L70

I do not use this exact library, but I have my own Async version I wrote that I have already ported to v107 using a common HttpClient cache. These guys have done it wrong for two reasons:

1) They assumed like v106 and earlier, that it's safe to create an instance per request of RestClient (well we know it's not really safe, but everyone does it).

2) Since v107 gutted all the non-async functions, they simply implemented the Sync over Async completely wrong using GetAwaiter().GetResult() which WILL result in deadlocks (I know, I have run into that exact issue):

https://github.com/EasyPost/easypost-csharp/blob/b04f71c5b505a9ed09ab0e106b7121e344571bbe/EasyPost.Net/Client.cs#L80

These are the reasons why I believe that RestClient really should have been developed to allow for instance per request, but would internally have support for caching and reusing HttpClient's (or if we have short lived HttpClients, we have pooled HttpClientHandlers that are used per request, so cookies etc work correctly).

Sounds good. I am very happy we changed the tone of the conversation to be a lot more constructive. I am all for making the migration easier 👍

These are the reasons why I believe that RestClient really should have been developed to allow for instance per request, but would internally have support for caching and reusing HttpClient's

That was my first attempt, and it got really wicked. Honestly, I spent many hours trying to make it work like that, and I looked at the code of DefaultHttpClientFactory, and it's way too complex considering the issue it's aiming to solve.

As for GetAwaiter().GetResult(), when you run into the issue, have you tried to use ConfigureAwait(false)? I believe that GetAwaiter().GetResult() is the only way to call async IO in the sync world. Honestly, I don't know why EasyPost didn't just get rid of all the sync functions.

These are the reasons why I believe that RestClient really should have been developed to allow for instance per request, but would internally have support for caching and reusing HttpClient's

That was my first attempt, and it got really wicked. Honestly, I spent many hours trying to make it work like that, and I looked at the code of DefaultHttpClientFactory, and it's way too complex considering the issue it's aiming to solve.

I have not looked at how DefaultHttpClientFactory is designed, but I believe you. As I have already discovered writing my own client instance cache, part of the problem is making sure all the things that need to be configured on a per request basis but are part of the client API surface need to be set when the client is initialized, and if necessary used as part of the cache key. So it is sometimes more than just caching it based on the API URL endpoint. In the case of EasyPost, the API endpoint is all that matters so the cache there is simple enough:

https://github.com/EasyPost/easypost-csharp/blob/ce2ee23c9d1f732d4adcb7d8620190e01535d3b8/EasyPost.Net/RestClientFactory.cs

As for GetAwaiter().GetResult(), when you run into the issue, have you tried to use ConfigureAwait(false)? I believe that GetAwaiter().GetResult() is the only way to call async IO in the sync world.

No, that is not correct. GetAwaiter().GetResult() can only work deadlock free if the resulting task is not actually an async operation. Otherwise it will deadlock. I thought that would work also and we went to production with that in our code and got deadlocks. The correct solution is the one implemented in Rebus and a few other libraries, which I took and put into the EasyPost library to fix it for them:

https://github.com/EasyPost/easypost-csharp/blob/ce2ee23c9d1f732d4adcb7d8620190e01535d3b8/EasyPost.Net/AsyncHelpers.cs

Then it's very easy to use:

return AsyncHelpers.RunSync(() => myAsyncFunction());

Honestly, I don't know why EasyPost didn't just get rid of all the sync functions.

Most folks don't know how to call sync functions in an non-sync world and get it wrong (as they did, and I did initially). I wish they had at least simply added Async functions but they were not receptive to it years ago so I ended up forking the library and built my own Async only version. I also cleaned up the API surface areas as they use a really ugly dictionary of properties to define the values to pass to the API calls (super error prone!) so I redid it all using POCO objects. It is a royal PITA to maintain this against their changing API's, but it works for what we need it for:

https://github.com/kendallb/easypost-async-csharp

Besides, we moved to ShipEngine anyway and only use EasyPost as a fallback when ShipEngine is down (or they have bugs that they won't fix).

restsharp / RestSharp

Resurrect the interfaces removed in v107 #1784

1765 (comment)