The removal of interfaces from v107 has made this library useless

[Florian Braun]

The reasoning for the removal of the interfaces for IRestClient and IRestRequest and IRestResonse shows a clear lack of understanding between unit and integration tests and why being able to mock dependencies is important. Not just to test the happy path but to be able to simulate error paths like timeouts and such down stream.

In the GitHubClient example the calls are just delegated to the RestClient, but if for instance the GitHubClient should return an empty array if any error occurred making the class and log that error, there is no way to simulate the RestClient throwing some sort of exception.

Instead I would need to create whole other layer ontop of the GitHubClient just so I could implement this logic in a testable way.

You ask: "What about mocking it, you might ask? The answer is: what would you do if you use a plain HttpClient instance?"

The answer is not use HttpClient because it lacks the ability to mock returns. That is one of the major shortcomings of the HttpClient and the major reason to use RestSharp instead.

I ask: Why would I use RestSharp if it provides no advantages over HttpClient? I wouldn't.

-Florian Braun

[Alexey Zimarev]

Thank you for your valuable opinion. If you think interfaces are for mocking and claim that I don't understand the difference between unit and integration tests, I can only wish you luck in your further career in software.

[Brian Hart]

I disagree.  If I may be totally blunt then I think it is YOU who needs luck in your future career in software.  Interfaces are a core component of the SOLID principles, "always code to an abstraction."

Please put them back.

Brian

--
You received this message because you are subscribed to the Google Groups "RestSharp" group.
To unsubscribe from this group and stop receiving emails from it, send an email to restsharp+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/restsharp/f72a0e33-3e70-4bf2-b3c9-4aceb7c3790bn%40googlegroups.com.

[Jay]

I am going to weigh in here and plainly disagree with you Alexey, but I'll do it respectfully. I hope you ultimately hear me out. Before I do I want to say I don't appreciate the attitudes and stabs that everyone here has taken at each other or at you. Plainly it isn't healthy for the development of this library and I am disappointed to see others do it, but I am especially disappointed to see you, Alexey, engage in it with snide remarks as the owner of this library. As the owner, it loses a lot of trust with your users and you should consider that. To let you know I empathize, I imagine it is hard to have a labor of love like RestSharp scrutinized which is why the community shouldn't bite at each other when there is a contentious change. It should seek to understand the best choice for this library, seek to explain why in order to drive consensus, and ultimately disagree and commit on a specific path forward. That said, Alexey (and everyone) if you look at the commit history this is clearly and effectively your personal package and you invest the most into it for everyone else, so do as you will. Anyone else who has issues with that should raise a PR with their explanations and actually contribute to the thing you have labored on so much instead of complaining and making brazen attacks. That's the point of open source.

My take on this change (having not used it, I mostly program in Scala anymore): What you have done seems to have plainly harmed the testability of code that directly depends on your library in the ways that your users primarily test their code with regard to your library. You are right, on one hand, there are ways to mock the underlying HttpClients and all that jazz, but ultimately the value prop for most of your users/customers is that they don't have to. When you think about their use cases (distributed across all the downloads that your library has ever received), this change doesn't meet the Liskov Substitution principal and imposes a lot of work on a lot of developers all over the world. That's not really customer obsessed/focused on current usages of your product here... you may think about that if you like. If you want this change to go forward, in my opinion, it should be a new major version of the library, not a minor version bump (so v106 would actually be v1.106 and v107 would be v2.0). You should provide a deprecation plan for support of V1.106.

Now that you have made these changes, as the owner of this library you should accept that these are valid complaints. At my software development firm, if I tried to impose a change that produces work for hundreds, thousands, or tens of thousands of teams the change would be challenged. It's highly likely the change wouldn't fly. This isn't a firm, this is open source however, so there's more leniency but I think it's fair to say that this is a costly change that impacts people negatively and that the impact is indisputable. As far as I can tell,  there's no real reason these shouldn't be mock-able, and you now have user complaints that making them unable to be mocked removes a lot of the value for your users. They are right. It's not fair to ignore that they have been mockable for years and that it is so bad when such a breaking change is imposed on a widely used public library. To me, that's simple fact but I respect that this is your library, in effect.

Ignoring the opinion laden argument about mock-ability and the less opinion laden points on forward compatibility/imposition of work, I think Brian has it right here too, interfaces describe the traits of the classes that implement them. That's to say interfaces are a key part of creating any domain specific language around something that can have interchangeable implementations. While I have no doubt you understand the value of interfaces, your change doesn't seem to, and that's plainly obvious. To remove that interface is to remove the descriptors of those traits, which, in effect, handicaps a developers ability to create their own implementations of those interfaces. The reason for why a user may need or want to contribute an alternate interchangeable implementation can't be predicted, which is why interfaces are important. This tightly couples your code to specific implementations, making the code less modular/more difficult to use/harder to adapt over time.

These are facts, not opinions... in my humble opinion and I am open to hearing your thoughts on this too.

Please be kind folks.

[Alexey Zimarev]

@Jay, things change. Even commercial software change. It's not on me that RestSharp had issues with questionable practices, like virtual methods and interfaces on the purely infrastructural library.

As you said yourself you aren't in the .NET space for a while, we've been through a substantial change over the last five years. .NET Core is completely different. I maybe heard voices of people saying "it must be compatible with .NET Framework", but Microsoft engineers did the right thing, they didn't listen, they made some things coming back for commercial reasons (which I don't have), but it happened later. Take a ASP.NET MVC project for .NET Framework 4.6 and try to convert it to .NET Core, see what happens.

The latest version changed a lot. You might argue that I should keep the synchronous methods just because people use them, although they should not (HTTP calls are IO-bound).

Removal of interfaces is a change but compared with everything else it's not big. The number of breaking changes exceeds a handful. Lots of options were moved from RestRequest to RestClientOptions because that's where they belong. Serialization changed. Sync methods are all gone. .NET Framewortk builds are gone! And, yes, interfaces are gone too.

I talk to a rather small group who essentially built an open-source community in .NET world, all of them are high-profile engineers with decades of experience. Everyone agrees that infrastructural libraries should not have interfaces by definition.

That being said, I love your phrase that "interfaces are a key part of creating any domain-specific language around something that can have interchangeable implementations". The fact is that RestSharp is a wrapper around HttpClient, and there's nothing to interchange. Neither RestSharp exposes any domain-specific language. The whole point is that people must understand the need to create domain-specific API clients that would use RestSharp internally, and write tests for those clients. In fact, Microsoft documented typed clients and advocated using them for years, if people would just listen 

https://www.hanselman.com/blog/httpclientfactory-for-typed-httpclient-instances-in-aspnet-core-21

https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests#implement-your-typed-client-classes-that-use-the-injected-and-configured-httpclient

In addition, as RestSharp uses HttpMessageHandler internally, it is perfectly possible to use something like MockHttp to replace the default message handler and properly test requests, as well as handling of expected results, without interfaces, using composition.

I would like to invite everyone who thinks that using interfaces for infrastructure is a good idea to open an issue in the .NET runtime repository and try to convince Microsoft developers to introduce an abstraction for HttpClient. As soon as they do, I will put interfaces on RestClient.

As for @Brian, I had a good laugh about 

> "If I may be totally blunt then I think it is YOU who needs luck in your future career in software. Interfaces are a core component of the SOLID principles, "always code to an abstraction.""

hilarious

[Florian Braun]

I do have to respond and apologize for my tone in the original message. Not making excuses, but with the release of 107 on a Friday and really dumb archaic rules about dependencies and major version updates and mainly because some managers are still having log4j nightmares, this version change meant I and two others had to work through the weekend to implement changes to update to 107 (well in the end we didn't update to 107 but I will get to that later) so I was a little bit annoyed when I wrote my original message.

Anyway, on topic. I think Jay  did a much better job of describing the issue with removing the interfaces so I wanted to provide some background on how we used RestSharp.

We did actually have our own implementation of IRestClient, which was a wrapper around a RestClient instance but which provided logging and auth and other functionality in a reusable way for our uses. But since our code was only working with the IRestClient interface, we could easily switch between implementations where needed. Also, other groups within my organization which were using our internal libraries making use of this interchangeability could implement their own IRestClient or just use RestSharp.RestCleint instance.

One of the original reasons we went with RestSharp as the backbone for a lot of our internal libraries over using just the HttpClient was specifically for these reason (and the mockability which is a large plus over basic HttpClient). Our assumption was that as one of the major C# rest libraries, with over 100M downloads the API would be fairly stable and breaking changes like this would be communicated better.

Since these assumptions are no longer true, and the interchangeability has been removed, we did end up creating our own Interfaces which are essentially copies of the old IRestClient (at least the API aspects we were using of it) to make migration as smooth as possible for down stream parties. And then we created an implementation of our interface around HttpClient instead of RestSharp. Both this abrupt library change to usability and the necessity to redo the work regardless meant that RestSharp lost out to the native HttpClient; the risks of using a 3rd party library, especially one which just bit us so hard, no longer outweighed the benefits when compared to using the HttpClient already supplied within .NET.

We probably should have never coupled ourselves to the interfaces to begin with, and it is our fault for not wrapping the library like this to begin with, but it definitely did not occur to us that something as universally used as RestSharp would have such an abrupt and breaking change.

-Florian

PS I see that Alexey just replied but I am going to post this and then read and then maybe respond again.

[dbdugger]

Please let me know how I can unsubscribe from this

To view this discussion on the web visit https://groups.google.com/d/msgid/restsharp/94699276-8a00-451d-8f23-b477326d6d47n%40googlegroups.com.

[Alexey Zimarev]

>  it should be a new major version of the library, not a minor version bump

RestSharp follows SemVer (at least I try to). v107 is a major version, not a "minor version bump". And no, you are wrong Jay, I do not have any obligation to support the legacy version, as I don't have anyone with any kind of paid support plan (but I do have a lot of shit over my head from time to time, and now it's one of those moments). And, I take the liberty to point out the Apache 2.0 licence of RestSharp https://github.com/restsharp/RestSharp/blob/dev/LICENSE.txt

[Alexey Zimarev]

There's a Subscribe checkbox on the top right corner of the page.

[Alexey Zimarev]

Brian, I can understand the rush, although it was surprising.

On a serious note, v107 was in preview for quite some time. The migration guide was there in the docs. Plus, v106 is a decent version, a lot of people use it without issues. There is no particular need to upgrade. As I mentioned in my previous reply, the number of breaking changes is extensive, and my aim was to do them all at once. Nobody ever got forced to do an urgent shift from v106 to v107, so I don't really understand the urgency of the migration that you had to do, or the statement that changes in RestSharp suddenly bit you so hard. I don't attempt to undervalue your time and effort, but I am questioning the urgency.

I am also aware that lots of issues with hanging connections, thread pool starvation, and sudden request timeouts are all linked to improper use of RestSharp, although the library itself suffered from its dependency on HttpWebRequest (kind of breaking change in .NET as it's heavily obsolete now). About 30% of IRestClient properties were linked to changes in HttpMessageHandler configuration that forced HttpWebRequest (which is a weak wrapper around HttpClient nowadays) to create more and more instances of HttpMessageHandler, leading to those issues. All of it is gone now, for good.  

On a final more, I remember major breaking API changes in AutoMapper, MediatR, MassTransit, NServiceBus, and even Newtonsoft.Json. I don't know how much backfire Jimmy, Chris or Udi had to deal with, I can ask. For me, it's the first time, and it is demotivating. Reminds me of the Identity Server story, honestly.

[Rui Ribeiro]

If anyone was using a 3^rd party library without actually wrapping it up under their own code, with the proper interfaces, then I am sorry to say you were doing it wrong. The use of interfaces is meant to easily allow you to change one implementation for another, thus freeing your code from hard dependencies. RestSharp is precisely the kind of hard dependency you want to avoid in your code.

 

I am not saying Alex was right in adding the breaking changes, but this in fact will lead to better designed code, if from now on, you wrap the use of RestSharp and use an interface for your wrapper, this will allow you to just replace RestSharp by whatever you want with ease, whenever you want to or need to. I have gone that route a long time ago and I mostly use my own web client implementations over HttpClient, with my own interfaces.

 

You all take care.

To view this discussion on the web visit https://groups.google.com/d/msgid/restsharp/94699276-8a00-451d-8f23-b477326d6d47n%40googlegroups.com.

[Jay]

Alexey your point about the license is totally fair. Hope you understand I was just trying to facilitate a healthier version of this conversation. Looks like people are having that healthier version of the conversation. Good luck all. 

To view this discussion on the web visit https://groups.google.com/d/msgid/restsharp/CAP66vQXYNPDZtOohp764cXOU03-GxKnmZ7JmaEjXrM3%2B34d-aQ%40mail.gmail.com.

[Florian Braun]

I agree that us relying in the RestSharp interfaces was the wrong choice, one also made before my time but it is what it is, and yes, there is no reason to upgrade in a hurry unless managers and others are forcing you to for stupid reasons, sadly not all of us are immune to such things.

And yes, us hiding the http connection and communication behind our own interfaces is a better implementation.

I do wish you, Alexey, luck in the future so please don't take this thread as a personal attack on you or your work. I definitely don't ever want to see someone demotivated from an OSS project.

-Florian

[Alexey Zimarev]

Ok people, as the discussion went in the proper direction, I want to move it to the proper place. Please, post your comments here https://github.com/restsharp/RestSharp/issues/1696

[talln...@gmail.com]

I also want to respond and apologize for my tone in my message about “if I may be blunt.”  It’s from my English heritage (I’m American but British ancestry) LOL

 

However, I do find it interesting that the evolution of this discussion seems to have proved my point about the importance of “code to an abstraction.”

 

Brian Hart

To view this discussion on the web visit https://groups.google.com/d/msgid/restsharp/94699276-8a00-451d-8f23-b477326d6d47n%40googlegroups.com.

[Charles Giddens]

hmmmm...missing RemoteCertificateValidationCallback so cannot debug iOS or Android App using Ngrok to test locally.  Oh well....going back to using Refit.

Bummer.

[Alexey Zimarev]

Oh dear...

[Custodio Malilong]

Removal of interfaces from v107 has made this library unacceptable.  It has invalidated all of the unit tests written with this NuGet package.  It is not just an issue of technology;  it's a terrible business decision.  We are restricted as to which NuGet packages we can use.  This is necessary in a company with greater than 200,000 employees.  RestSharp will likely  become "prohibited" by our architects due to this capricious change that offered no clear benefits.  I know I'll have to find another package and to not recommend it to my fellow developers.

[Alexey Zimarev]

Can you elaborate a bit about "business decision"? I don't remember your great company contributing in any way that would justify your rants about "business" as there's no business here.

[Rui Ribeiro]

I would think that your architects should really be worried about the fact that you are using 3rd party libraries without properly wrapping them up in your own implementation and interfaces. Had you done that, you would be ensuring that your software would be resilient to changes in such hard dependencies and any such hard dependency could be easily replaced by registering a different implementation of your wrapper.
If this is any indication of how software is developed in your 200,000 employees company, for sure there is reason for them to be seriously concerned and their problems are way more serious than a few breaking changes in a 3rd party open source library.
I find it mind boggling how people vent in such a way about software they never contributed to. I must confess, though, that what all this venting reveals about software development practices in place is even more mind boggling.

It is never late to improve coding practices. Get v106, wrap it properly and learn a good lesson about using 3rd party software. Once you do that, you can change the wrapper implementation for v107 and distribute that in your company and your in-house coding practices will havedone a quantum leap in quality...

--
You received this message because you are subscribed to the Google Groups "RestSharp" group.
To unsubscribe from this group and stop receiving emails from it, send an email to restsharp+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/restsharp/615bb608-ba85-4860-b829-1e171a7792fdn%40googlegroups.com.

[Alexey Zimarev]

I second that, and for as long as RestSharp exists, that was always the way we suggest for RestSharp to be used. There's no use for RestClient or HttpClient to be randomly invoked throughout the codebase. It's not only unsustainable but also doesn't make the code less readable and way less maintainable. If one reads Microsoft documentation about the preferred way to use HttpClient as a dependency of a typed client, that's exactly the way for RestSharp to be used, and it's on the front page of the documentation. I find it hard to believe that people were ignoring the advice of the library maintainers about how to use it properly, until after some years a breaking change comes that would keep all those typed clients mostly intact (except for the configuration), and now complain that everything suddenly breaks.

I also keep answering questions on StackOverflow where people point to the list of parameters that seems to be correct, but the HTTP request comes out in the wrong way. And I keep pointing out that the only way to ensure that the request is formed correctly is to trace the request itself. It was hard to do with RS <= 106, and it's dead easy to do with RS 107. Still, instead of grabbing the opportunity to finally make their code and tests meaningful, people keep bragging about tests that just check if some parameters are present in the Parameters collection, and so on. That's both frustrating and disappointing.

Still, the statement about "bad business decision" made me laugh and cry at the same time. Throughout the years of maintaining RestSharp, I got one financial contributor on OpenCollective, and one person buying me coffee for helping out with their single issue even before the issue was resolved, just as a sign of appreciation for my effort. However, nothing ever came from a "200K+ employees company" or similar, except massive, despicable and annoying rants.

On Thursday, January 6, 2022 at 7:29:19 PM UTC+1 f.bra...@gmail.com wrote:

[Custodio Malilong]

Sad to see our valid concerns dismissed as "rants".  Obviously, this package has become a hobbyist plaything.  It is dead to me as far as I'm concerned (I'm sure this will be dismissed too).  Time to move on.

--
You received this message because you are subscribed to a topic in the Google Groups "RestSharp" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/restsharp/SOViQe8KS6c/unsubscribe.
To unsubscribe from this group and all its topics, send an email to restsharp+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/restsharp/d0e61fda-0edd-4f5d-8b5b-f2389871ca3dn%40googlegroups.com.

[Alexey Zimarev]

A "valid concern" would be something else than telling the maintainer that a "200K size company, which doesn't contribute in any way to the library they use, suddenly got issues and it's unacceptable" because you have not provided any reasoning that would make me reconsider any decision made during the refactoring towards HttpClient. You expressed issues your employer experiences, exposed some dubious practices, which you consider "reasonable", and called my decisions "bad business", where there's no business for me. That is a pure rant, sorry.