IRestClient interface

[alexeyzimarev]

Version 107 got several interfaces removed, which is one of the major breaking changes in that version.

It seems most of the interfaces were okay to remove, except IRestClient. My arguments are described in the docs.

This issue is a place where we should have a civilised discussion about it. I will not follow up on the interface issue anywhere else, except here.

Some details about the current state of the RestClient API signature.

• All the sync methods were removed. Those who need it can make extensions, wrapping up async calls in some async helper.
• Most of the overloads for making requests are now extensions, as they just call each other, so they would never be part of any interface anyway.
• Most of the options that were previously on IRestClient and IRestRequest are now in RestClientOptions, which is a property bag and, therefore, won't have any interface

So, what's there in stock for IRestClient? Basically, it boils down to this signature:

namespace RestSharp; 

public interface IRestClient {
    RestClient AddDefaultParameter(Parameter parameter);

    Task<RestResponse> ExecuteAsync(RestRequest request, CancellationToken cancellationToken = default);
}

The question now is if it makes sense to introduce an interface with those two functions? Please comment.

[astrohart]

I would think it would, in order to maintain SOLID code. Also, as several other people pointed out previously, we are not always immune to managers etc. making us code to interfaces, do gratuitous TDD and automated-unit-test writing etc/mocking, and/or use IoC containers. Bosses like to make software engineers do things "because I heard about it at a conference," or "the insurance is making us," etc.

The interface just supporting the two methods is, in principle, okay -- just make sure that the static extension methods refer to the object through the interface and not the actual class.

[alexeyzimarev]

Brian, I understand the argument about managers, although in my 30 years of engineering I never encountered a single manager who was telling me how to write code... I mean, if you write your code against an interface it's one thing. But how some managers can tell a third-party library how to write their code? HttpClient doesn't have any interfaces implemented and it seems to be fine, no?

Automated testing and mocking HTTP requests are totally possible with RestSharp in its current shape. I have shown an option in the docs.

I see no issues with using DI containers as well, as HttpClient does with its nice extensions. In fact, I plan to add some DI support to make it easier and push people to use RestSharp properly (typed clients, etc).

The SOLID argument is not solid :) What do you mean by that, could you elaborate?

Then again, I am seriously not against adding that small interface. However, I would not be happy to add IRestRequest and IRestResponse back as those are glorified property bags.

[fjmorel]

A typical method in a wrapper service for my HTTP calls does the following

1. Take some inputs to form a request
2. Execute
3. Handle response (maybe retry, return data)

If I can mock IRestClient, then I can verify the contents of the RestRequest given to it by my method. That lets me avoid splitting 1 into a separate method just for testing that I correctly crafted the web request (which I want because string formatting is tricky).

If I can mock IRestClient, then I can setup responses for 200, 404, 401, etc and don't need to split out 3 either.

Each method is already short enough that I don't want to split it up just for test coverage. Example:

public async Task<List<Something>> GetData(DateTimeOffset startEpoch, DateTimeOffset endEpoch)
{
    var request = new RestRequest("/url", Method.GET);
    await AddBearerToken(request);// other method which might make a request to authenticate or use cached token
    request.AddQueryParameter("start", startEpoch.ToUnixTimeSeconds().ToString());
    request.AddQueryParameter("end", endEpoch.ToUnixTimeSeconds().ToString());

    var result = await _client.ExecuteAsync<ApiResponseModel>(request);
    if (result.StatusCode != HttpStatusCode.OK)
        throw new Exception($"Failed to fetch data. Status code: \"{(int)result.StatusCode}\", message: \"{result.StatusDescription}\"");

    return result.Data.ThingICareAbout;
}

[alexeyzimarev]

@fjmorel what of that is not possible to do with MockHttp? It gives you even better options to ensure the request comes in the correct format and has the correct content type, authorisation headers, etc.

[danielwagn3r]

I see no issues with using DI containers as well, as HttpClient does with its nice extensions. In fact, I plan to add some DI support to make it easier and push people to use RestSharp properly (typed clients, etc).

Can you tell a little bit more about your plans on DI support? Perhaps a word on DI in the docs could clarify many questions before the've to be asked.

[alexeyzimarev]

The DI registration heavily depends if you want to follow the same pattern as Microsoft does, using IHttpClientFactory, registering the client as a transient dependency.

For many use cases it's enough to register a typed client as a singleton:

services.AddSingleton<IMyApiClient>(new MyApiClient(new RestClient(options)));

If I'd to provide some use of IHttpClientFactory registration extensions, I'd need to know if people want to configure the HttpClient instance used by RestClient on the outside. In this case, lots of options that RestSharp uses to configure the message handler won't be applicable.

[fjmorel]

@fjmorel what of that is not possible to do with MockHttp? It gives you even better options to ensure the request comes in the correct format and has the correct content type, authorisation headers, etc.

Thanks! I didn't know about that library. I've been using Moq.Contrib.HttpClient in places we used HttpClient rather than RestSharp.

This library using HttpClient is an implementation detail that I don't think library users should have to think about, unless they have specific requirements making the need for a custom HttpClient necessary. Having the interface makes testing easier for all users without having to add extra dependencies.

[alexeyzimarev]

The library you mentioned would work as well. As long as you can add a delegating handler, it will be possible to test RestSharp calls. All you need to do is to compose or override the message handler used by the wrapped HttpClient:

var client = new RestClient(...) { ConfigureMessageHandler = _ => mockedHandler };

Basically, one of the benefits of moving to HttpClient (besides that it's obvious and HttpWebRequest is heavy legacy) is the ability to use delegating handlers and build message processing pipelines.

The point about testing real requests is obvious to me. If you look at StackOverflow questions about RestSharp, most of the issues are caused by incorrectly added parameters, content type overrides and other weird things. But, if people would test against a RestShar interface, they would have all their tests passing. But it won't work anyway, because the request is just not correctly formed. It's a great benefit to at least inspect the actual request and ensure that it looks exactly as it should, rather than inspecting the parameters collection on a RestRequest instance. The latter gives you zero idea how the actual request would look like.

[sspates]

This is not necessary if the existing RestClient methods are marked virtual. This will allow for mocking overrides of the public methods instead of needing a full interface.

One of the two is necessary as the handler system is not able to simulate invalid http responses. We switched from Http client to RestSharp because RestSharp had an interface that allowed us to simulate the result of those responses as well as other error conditions not simulatable through the mock handler. Some of these result in StatusCode 0 or exceptions being thrown which require an interface or virtual method.

[alexeyzimarev]

@sspates-starbucks why can't you simulate the error response with a testing handler like MockHttp? It provides a behaviour that is much closer to reality. It can return anything you want based on its configuration.

[alexeyzimarev]

@maor-rosenfeld thanks for your extensive and elaborative feedback for this issue.

[alexeyzimarev]

If we keep discussing testing, maybe it's a good idea to post some test samples and check how these tests can be refactored. So far, the discussion goes quite theoretical imo.

[maor-rozenfeld]

@alexeyzimarev we can argue all day about use cases of using the interface for tests, extension methods, tests on these extension methods and other reasons of why suddenly removing all interfaces from an existing infrastructure package is a bad idea, but at this point it's clear that you're here to tell us how to write code.

I'm sorry if I choose not to align with your views, but at this point upgrading to the latest version of RestSharp is so painful that there's really no justification to even try.

On a positive note, I'm completely with you regarding the separation of services and settings objects. I like RestClientOptions.

[alexeyzimarev]

@maor-rosenfeld you definitely know how to motivate OSS maintainers to keep doing their community work for free.

[lukos]

I've come here from a breaking scenario that I'm not sure I can fix other than backing out the upgrade to 107 due to missing IRestClient.

I updated one of my libraries to use 107. Why not right? I'm trying to be proactive with keeping libs up to date. It all seemed to work OK until some tests starting failing. This is because we use a third-party library and they reference IRestClient and v106 of the library. I could ask them to update but I don't see that they would prioritise that and it would then force all of their customers to update to v107 as well which might be a big deal. If I use an assembly redirect to 107, it won't work because it won't find IRestClient and if I use 106, my code won't work because the method signatures changed.

I'm not sure what I would have done differently but perhaps something as simple a a different nuget package with different namespaces and mark the old one as deprecated. Then we can run them side-by-side until everyone is on 107?

Not sure. Any suggestions?

[alexeyzimarev]

@lukos I don't think it would work in any scenario, interfaces or not. RestSharp v107 internals are completely different due to migration to HttpClient and ditching HttpWebRequest. It made it impossible to keep the API backwards compatible, and even with interfaces, the API scope of those interfaces won't ever be compatible. Lots of properties of RestClient and RestRequest are moved to RestClientOptions and there's no way to keep them where those properties were before.

[sspates]

We are running into cases now where one library requires the interface and another requires the v107 structure, we can't upgrade either one due to the lack of backwards compatibility with IRestClient. Adding a compatibility wrapper that implements the interfaces might be a way to handle this better rather than implementing the interface on the current client.

[alexeyzimarev]

@sspates-starbucks Even if there's something to make things build-time compatible, these won't be compatible at runtime anyway. As I mentioned, most of the properties of those (IRestClient and IRestRequest) interfaces were (by necessity) moved to RestClientOptions, and it's something that simply cannot be undone due to the configuration of HttpMessageHandler.

I can only suggest opening issues for those libraries that still use RestSharp 106 and helping them with the migration. It's not a lot of work honestly.

[Terebi42]

The new class having internal only options generates problems for dependency injection. I set up my DI container to hand out RestClient, but different consumers need different options. There is now now way to set the options after having an instantiated Client.

[alexeyzimarev]

@Terebi42 that's why I advocate wrapping RestClient in a particular API client, so you can wire it like this:

services.AddSingleton<ITwitterClient>(new TwitterClient(new RestClient(twitterOptions)));

But I plan to address it better, just haven't found a good way to do it yet. Either I will make some extensions for IHttpClientFactory or make a new factory with named registrations for options.

[Terebi42]

@Terebi42 that's why I advocate wrapping RestClient in a particular API client, so you can wire it like this:

services.AddSingleton<ITwitterClient>(new TwitterClient(new RestClient(twitterOptions)));

But I plan to address it better, just haven't found a good way to do it yet. Either I will make some extensions for IHttpClientFactory or make a new factory with named registrations for options.

Is there a reason why (some) options can't be set after the constructor? Its a HUGE code change to say go make a new interface and options class for every location that is going to use restsharp, vs just setting the timeout and baseurl after the DI injection is done

[saarea]

Every man and his dog has an opinion on software design/standards and the debates will rage on forever - they're usually just different mindsets/approaches. Those mindsets aren't even usually the mindset of the developer using your library, they're probably dictated by the organisation they work for and having a conversation to change those standards is either way above their pay grade or a battle that's not worth fighting.

The problem here isn't one of who is right or wrong, it's one of practicality. The removal of the interfaces causes the amount of work some consumers have to do to significantly increase. Most devs really just need to deliver the work they had promised within the timeframe promised. I'm a huge fan of MockHttp but in my case, updating over 1000 tests to use it (which were written 5 years ago) is a huge amount of work. No matter how you plan your pipeline, the sizing of the work item is now completely invalid. The amount of work required to update your library has become inhibitive to doing so - it will either have to be picked up as a separate work item or it just won't ever get updated.

Irrespective of whether consumers are testing the right thing, using IoC correctly or just plain writing crap code, they no longer have the flexibility to choose like they used to. I think most devs would agree that the code changes make sense but at the end of the day we all need to deliver functionality to a business and I guarantee that those business users really don't care how well the code is written as long as it reliably does what they need.

The best development tools out there make our lives easier. That's why we use them - they abstract us away from the detail/hard work we really don't need to care about and they provide flexibility to customise how we utilise them. They help us do more, faster. Removing these interfaces does the opposite of that, irrespective of whether it's "better" code or not.

[lukos]

What everyone is saying is correct. Of course, we appreciate Alexey for making this library but the changes are very breaking.

It seems like the best way out of it would be to create a separate library with a different name/package name (and importantly, different namespaces) to contain your newer/better way of doing things and which should be used by people going forwards. Any serious bugs found in the old library will probably be fixed but otherwise it won't get any new features.

This way, we can have both packages alongside each other so if we can update our own code for the new library we can do that without forcing all of our dependencies to update all of their code at the same time. If they eventually do, then we can eventually uninstal the old package.

Not sure if you think that sounds fair or not?

[alexeyzimarev]

The question is why is there such a strong need to migrate to v107 if v106 works fine for most? Migration to HttpClient is a big change in itself, the IRestRequest (for example) is impossible to shape to the same form as it was before anyway, as well as IRestClient interface API surface cannot be recovered.

[saarea]

Why update? People want the stated benefits that this update (and future updates) will bring.

As a consumer, I shouldn't have to care what implementations are used internally as long as it works. This is a debate about the impact of the removal of interfaces and the consequences that has for those who use the library. (The fact that I'm even aware of what components are used internally screams leaky abstraction to me but that's just my opinion and somewhat off topic)

Changing the method signatures is something that's not overly complex to deal with (the compiler will help identify what needs changing and, in our case, this library is abstracted away from our code anyway) however, removing all the interfaces entirely fundamentally changes how to set up and work with the library and how easy/difficult that is given what's gone before.

The work you've done is great - we're just providing feedback of the real world issues it causing us. Rightly or wrongly, people consume this library is a myriad of ways and for us, the migration path is so large that it's currently a barrier to entry.

[alexeyzimarev]

I don't know why, but my message is not getting through. I will try again:

• The API surface of IRestClient and IRestRequest is impossible to hold intact after the migration
• Removal of interfaces made the maintenance work much easier.

[saarea]

*** EDITED ***

I really don't want to get bogged down in specifics as I don't think it's helpful

The question now is if it makes sense to introduce an interface with those two functions?

For us, yes it does.

There are probably many others who will find that interface useful too for many different reasons.

Adding them in provides flexibility to choose and reduces the impact of the changes for many people especially given that this library has been around for such a long time. This isn't about what code/approach is better, it's about what's practical and helpful for existing consumers.

[alexeyzimarev]

Thanks very helpful @alexsaare, thank you.

The scope of refactoring, however, would be significant, both for me (all the extensions need to change) and for those who use the previous version of the library.

I am not against it, that's why I opened this issue. I still, though, wait for some sample test code, so I can understand the need better.

[alexeyzimarev]

As I mentioned before, I am trying to understand how people use interfaces in tests. Tests aren't PI, nor IP. Please, share your tests.

[RedVinchenzo]

Hello dear community,
I'm using RestSharp since some times (7 years more or less), I like it as it do the job.
For a new project, I used it, by reflex, and surprise ! No more interface 😨
I will not discuss with project owner about, it's a good choice or not, I will just write quickly my opinion.
I will give up RestSharp usage, and stop recommand it to team members.
I'm using library to save time, I'm using library as a tool to make my work easier.
Now I'll have to invest time to introduce "tricks" for my unit test ? But why ?
I choose a tool because it fit to my need, RestSharp doesn't fit anymore.
I'm sure this breaking change is relevant, but not for me.
Most of dev will update lib, keep it in 106 (this proposal is a shame 😅), update their code.
For my part, I choose another tool to save time.

[alexeyzimarev]

@RedVinchenzo thank you for your feedback, but I am having a hard time understanding how your comment contributed to the discussion. Keeping the API intact just so people don't need to change their code, hm. I don't remember a single library I used that never changed its API.

Now I see what is going to happen. With .NET you have no choice but to use HttpClient one way or another. Flurl, Refit, or whatever wrapper you use, will either use their own HttpMessageHandler, or force you to use the same MockHttp. I am quite puzzled hearing "I will just use HttpClient because RestSharp has no interfaces", I wish someone explains that.

[encore121]

Hi @alexeyzimarev

If you use your library in your projects then you are familiarized with how can you use it and tested it. But I think many of us use the library in the next way.

For DI just define in Startup.cs
services.AddTransient<RestSharp.IRestClient, RestSharp.RestClient>();

Then just need to define this in the constructor and thats it, you can use RestSharp in the class
public ValidationDataAccess(IRestClient restClient)

For the unit test I use Moq, so I only neet to define the result of method Execute like this
`var moq = new Mock();

moq.Setup(m => m.Execute(It.IsAny()))
.Returns(new RestResponse()
{
ErrorMessage = string.Empty,
Data = data
});`

And pass to the constructor like this
var validationDA = new ValidationDataAccess(moq);

If I need that RestClient response with an error then I define the result of Execute with an error to test that escenario.

So, for me the problems is the next

• We neet to refactory not just new projects but projects in production only for use the new version
• You say "just don't update the version". @Terebi42 say one reason what we neet to update
• You say "in your unit test you can call a simulation", but again it's needed time to refactory the projects
• You say "I don't remember a single library I used that never changed its API", yes you have right but always that cause troubles and discomfort in developers
• You say this in using of mock "it doesn't guarantee that the actual HTTP request will be valid", maybe but that´s why we test local and QA

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[repo-ranger]

⚠️ This issue has been marked wontfix and will be closed in 3 days

[tzuge]

I am starting a new project making use of RestSharp, so more focused on the recommended way to mock / unit test today. It appears the example in the docs might be incorrect.

var client = new RestClient(...) { ConfigureMessageHandler = _ => mockHttp };

versus

var client = new RestClient(new RestClientOptions { ConfigureMessageHandler = _ => mockHttp });

Also the single quotes in the json value isn't valid; MockHttp example has this issue as well but there's no actual deserialization in that example.

From my perspective, I like being able to verify through the request using MockHttp, but would also like IRestClient for easier mocking at that level. Having both options is nice, and it doesn't seem onerous for the lib to support it. It sounds like the latest version is a new library from compatibility standpoint, so makes sense that IRestClient in this version would be a new interface.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[repo-ranger]

⚠️ This issue has been marked wontfix and will be closed in 3 days

[alexeyzimarev]

@richardbuckle sorry, who are you again? good bye.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[repo-ranger]

⚠️ This issue has been marked wontfix and will be closed in 3 days

[alexeyzimarev]

I reviewed #1952 and have a proposal:

• Rename ExecuteInternal to ExecuteRequestAsync and make it public
• Rename InternalResponse to HttpResponse and make it public
• Convert ExecuteAsync and DownloadStreamAsync to extension methods; both are calling ExecuteRequestAsync
• Convert Execute and DownloadStream sync methods to extension methods
• Add the IRestClient interface with only one method: ExecuteRequestAsync
• Change all extensions of RestClient to IRestClient. There will be issues with client members that should be included in the interface, so add those too. Options is the property I expect to be there.

This will allow composition. For example, I can add a Polly-based wrapped client to retry particular status codes. Btw, I found that the number of retries in the response Request property might not work as intended.

How does that sound?

[alexeyzimarev]

Hmm, it doesn't really work well as Options become public again... One solution is to make it a record again, with init-only properties, and risk incompatibilities with old SDKs. Not sure what's more important.

[kendallb]

Seems reasonable to me, but why would the options need to be part of the IRestClient interface anyway? The way I set it up in my branch where options is private again, I have some properties to get back important stuff like the baseurl etc. Those could be part of the IRestClient interface if needed. But I don't think exposing options is necessary at all in order for someone to mock up IRestClient is it?

[alexeyzimarev]

Because lots of extensions need those options. So, it's either to extend the API surface of the interface, which makes little sense or expose a shared options container via the interface.

[alexeyzimarev]

I guess I took the wrong approach. The internal execute method should remain internal or private. Both ExecuteAsync and DownloadStreamAsync should be in the interface as it's the basic public API.

[alexeyzimarev]

I appreciate some of the feedback on this issue (not all of it).

However, I see a lot of negativity and unwillingness to understand that:

• the library needs to move on, and breaking changes are inevitable
• keeping the previous API service was hard to maintain, and I am doing most of the work here (thanks to all the contributors, too) for free, which many participants of this discussion, apparently, fail to appreciate
• the number of aggressive and counterproductive comments here is unacceptable

Therefore:

• the interface will be brought back in v109, but its purpose is not to make mocking easier
• I will open another issue for the new interface
• any attack on the maintainers will not be tolerated. those who do it will be blocked for violation of CoC and contribution guidelines