ExecuteRequestAsync as public

[RomanSoloweow]

Continue discussion #1952
Api was changed, it's nice. But it still difficult to implement generic logic for all requests.
If we check implementation for ExecuteAsync and DownloadStreamAsync methods, they have common part: ExecuteRequestAsync.
Why don't you want to make this method public? This will allow library developers to write more versatile and more enjoyable code.

[RomanSoloweow]

@alexeyzimarev any news?

[alexeyzimarev]

I have no issue with that but it would mean the function will become a part of the interface, which will introduce breaking changes for everyone who use the interface.

[RomanSoloweow]

Which interface you mean?

[RomanSoloweow]

I think it's can be not included in interface. Only as part of RestClient object

[rassilon]

There is RestRequest.OnAfterRequest for the brave..

[alexeyzimarev]

@rassilon I think the idea here to enable creating extensions that would use ExecuteRequestAsync. However, it will require exposing the internal HttpResponse type as public. I don't really see a problem doing that.

One option is to make ExecuteRequestAsync the only function in the interface, and convert both ExecuteAsync and DownloadStreamAsync to extensions. It would also allow resolving issues with extending the basic functionality, like returning a tuple for downloads (there's an open issue for that, and a PR).

[rassilon]

That definitely sounds like a good idea.

[RomanSoloweow]

Sounds perfect

[RomanSoloweow]

@alexeyzimarev any news?

[alexeyzimarev]

I looked it now and tried to be "smart" by using default interface implementation for that function, but it didn't work because of .NET Framework target.

Essentially, both ExecuteAsync and DownloadStreamAsync depend on ExecuteRequestAsync. If that function becomes the only one in the interface, people who, for example, use the interface for mocking the client, will struggle to return RestResponse back as they'd need to properly populate HttpResponseMessage and it is not trivial.

The default interface implementation could have worked, but apart from issues with .NET Framework (I tried to use partials and pragmas to make it class/interface), the function uses private properties and functions, so it quickly became a mess.

So, right now, unfortunately, I don't have a good solution there.

[niwrA]

Our team chose to use this, but it has so far taken me hours to find a way to properly test this. The solutions that I found were against older versions and no longer work. It wasn't trivial to mock with IHttpClient either, mind you. I will try and see if I can spare some time to help, because otherwise I fear this otherwise excellent and long standing library will lose out to regular IHttpClient. Intuitively I'm thinking that a solution could start with wrapping all code in a new object with a good interface. This could leave everything else working as it is now, but provide a new interface and code block that could eventually take over without breaking changes for existing users.

[alexeyzimarev]

I mentioned it before that mocking the HTTP client isn't the best idea as you can only ensure that your test fake receives the right parameters, but it won't guarantee that the request will be sent over the wire in the shape and form expected by the remote endpoint. Neither can you test how the response is deserialised.

There are better way to test API calls, and RestSharp tests uses three methods:

• TestServer when you also have access to the API server project
• WireMock when you actually want to send requests over the wire (locally) and mock the expected response, so the client can handle those
• RichardSzalay.MockHttp for asserting that HttpRequestMessage is correctly formed, and also for validating response handling by providing the mocked response back, but without actually making an HTTP call

Those three methods allow us to ensure that RestSharp works correctly, and they can also be applied easily to test the code that uses RestSharp.

MockHttp might not be the best choice as it requires configuring a delegating handler, which often requires creating a special branch in the application code that'd be only used for tests. This I understand. But using WireMock doesn't pose this issue as it's just a server running locally.

I don't think this issue was ever meant to be about testing. If you still need to mock the client, there's a simple interface to implement.