Add RestClient.DownloadStreamAsync() error handler

[marcoburato-ecutek]

Background

Our API, in case or error, returns a suitable HTTP status code and also provides more detailed information as JSON in the response body. Something like:

{
    "errorCode": "CustomerNotFound",
    "errorDescription": "No customer matching the given ID was found"
}

For regular requests that send JSON and receive JSON, I could use RestClient.ExecuteAsync() to obtain a RestResponse and deserialise the content as either the expected object or the error object based on the HTTP status code.

When downloading files, I have found no good way of doing this. In case of error, RestClient.DownloadStreamAsync() would either return null or throw an exception without info about the exact server response ("Request failed with status code XXX").

Alternatives considered before adding the new functionality

Using DownloadStreamAsync() + RestRequest.OnAfterRequest

This would work in principle. The raw response could be inspected and a suitable application-specific exception could be thrown if it's an error response.
The problem is that the consuming code would have to deal with both RestSharp responses and raw HttpClient responses, which duplicates code. RestSharp also has code to distinguish between a canceled request and a timeout, which would have to be duplicated too.

Using ExecuteAsync() + RestRequest.ResponseWriter

This would allow to save the file to a FileStream and get a RestResponse to check the HTTP status code. The obvious problem is that, in case of error, the response error JSON would be saved to the file instead of the expected file content, which is not ideal.
Also, in case of success, RestSharp would still make a byte[] and possibly a string of the response, which is not required and undesired (especially if the file is big).

Using ExecuteAsync() + RestRequest.AdvancedResponseWriter to save the response body

Compared to the above, this would be passed the raw HTTP response, so it has more control. But this has the same problem as RestRequest.OnAfterRequest (dealing with the raw HTTP response).

Description

This PR adds the following overload:
Task<Stream?> DownloadStreamAsync(RestRequest request, Action<RestResponse> errorHandler, CancellationToken cancellationToken = default)

If the request fails or the server returns an error HTTP status code, errorHandler is executed, passing the error response as a RestResponse object.
After executing the handler, the method returns null or throws an exception as usual.

Initially, I considered returning a different object which would wrap either the Stream or the RestResponse, but I didn't like the idea of returning a different type on the new overload. I also thought about using an out RestResponse? argument but they're not allowed on async methods.

Example usage:

var stream = client.DownloadStreamAsync(request, (errorResponse) => {
    //Inspect response, throw application-specific exception if needed
});

A single error handler can be defined, to be used for many requests

//Somewhere
var stream = client.DownloadStreamAsync(request, ThrowExceptionForErrorResponse);

//Somewhere else
var stream = client.DownloadStreamAsync(request, ThrowExceptionForErrorResponse);

void ThrowExceptionForErrorResponse(RestResponse errorResponse)
{
    //...
}

Purpose

This pull request is a:

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

[alexeyzimarev]

Thank you for the PR. Please sign the CLA, and I will merge it.

[marcoburato-ecutek]

@microsoft-github-policy-service agree company="EcuTek Technologies Ltd"

[marcoburato-ecutek]

@alexeyzimarev Sorry for the delay. I followed the instructions of the CLA tool to "sign" the agreement. Is that correct?

[alexeyzimarev]

Doesn't seem like it. The check is still queued.

[marcoburato-ecutek]

@dotnet-policy-service agree company="EcuTek Technologies Ltd"

[marcoburato-ecutek]

@alexeyzimarev That worked. I looked at other PRs to see what other people did. It would be nice to mention how to accept the CLA in CONTRIBUTING.md.

[alexeyzimarev]

Yeah, it seems like contribution guidelines need updating regardless :)

[alexeyzimarev]

@marcoburato-ecutek do you think your PR fixes this one? #1692

[marcoburato-ecutek]

@alexeyzimarev I don't think so. My change only introduces a new method overload, it does not change previous behaviour. The assumption was that the existing behaviour was correct and intentional.

My memory is short, but according to my description above, it was possible to make RestClient.DownloadStreamAsync() throw on error, my problem was that it was not possible to access the information about the error (HTTP status code and message).

I reckon the other person should open a separate ticket with a test case, if they believe there's a bug.

[marcoburato-ecutek]

Reopening, I thought this had been merged already.

[alexeyzimarev]

There's quite a big issue here as:

1. Custom error handler is only on the client, not on the interface. Also you added inheritdoc but there's nothing to inherit from.
2. It only works for downloads, which creates yet another functionality gap as all other request types have no error handlers.

[marcoburato-ecutek]

With regards to the interface, the documentation says

The IRestClient interface is deprecated in v107, but brought back in v109. The new interface, however, has a much smaller API compared to previous versions. You will be using the RestClient class instance.

I interpreted that as "IRestClient is deprecated and we only partially brought it back for compatibility". So, I didn't worry about the interface. It seems I misinterpreted the documentation.

The inheritdoc must be my mistake.

About the fact that it only works for downloads, it's already possible to obtain the same error-handling by using RestClient.ExecuteAsync and checking the returned RestResponse. This PR was meant to address the lack of error-handling functionality when using RestClient.DownloadStreamAsync().

IMHO, the best alternative solution would be to use the existing ThrowOnAnyError behaviour to throw an exception when the server returns an error code, which would include the HTTP status code, HTTP status message and response body (under the assumption that, for errors, it would be reasonably short). This would allow the consuming code to just use a try-catch instead of passing an error handler. However, since RestClient currently throws an HttpRequestException in case of HTTP errors (which only exposes the HTTP status code as a property), this would mean throwing a different type of exception, which would be a breaking change.

[alexeyzimarev]

The thing with the interface is the whole API scope is available as extension methods for the interface, so they are basically syntactic sugar. It allows creating a new implementation of the interface for whatever reason, and the whole API will work. It was a substantial effort to archive this. I am thinking about a way to accommodate your change in a similar way.

I understand the RestResponse bit, and the challenge with handling exceptions for downloads. I just want to way to make it fit to the current API conceptually, considering what I wrote above. Please give me a bit of time, I might figure out something.

[alexeyzimarev]

I have another suggestion. Why don't we extend interceptors and add OnError there? This way, a custom error handler can be used for any kind of request, not just downloads. It also keeps the interface and the API surface the same, so there will be no ambiguity where a Download with the custom error handler is only available for the implementation.

[marcoburato-ecutek]

Maybe.

I think that would work for me, as long as the error handler is passed a RestResponse. But that would only allow to use a single error handler for most requests. Someone might want to have different error handling for each request.

If you add an OnError then you should also add an OnSuccess for balance. Both taking a RestResponse instead of an HttpResponseMessage. InterceptBeforeDeserialize would not be the same as it would not be called for a download, I imagine.

What about having a method overload like Task<Stream?> DownloadStreamAsync(RestRequest request, out RestResponse? errorResponse), where on success errorResponse is null and a stream is returned, while on error errorResponse is non-null and null is returned? That would make error handling code closer to ExecuteAsync(): no lambda/interceptor, the code gets a RestResponse and inspects it.

[alexeyzimarev]

The problem with out would be that, first, it still requires signature change, second, it forces people to add an out var even if they don't need it.

InterceptBeforeDeserialize indeed is only called in generic extensions, when deserialisation indeed takes place.

I would remove it, rename InterceptBeforeRequest and InterceptAfterRequest to InterceptBeforeHttpRequest and InterceptAfterHttpRequest. Then, add InterseptAfterSuccessfulRequest(RestResponse reponse) and InterseptAfterFailedRequest(RestResponse reponse). The last one would give you the error response.

[marcoburato-ecutek]

I meant to keep DownloadStreamAsync(RestRequest) and add the DownloadStreamAsync(RestRequest, out RestResponse?) overload, on both class and interface. Are you saying that you prefer to not add overloads to the interface? Not sure if that complicates extension methods.

About your proposed changes, that's sensible naming.

One limitation of both my original solution and the interceptor is that the error handling would be limited to checking the response and throw a specific exception based on it. That would probably be the common case, but getting a RestResponse as an output would be more flexible.

[alexeyzimarev]

Adding stuff to the interface breaks all custom interface implementations as people would need to add this new method there even if they don't use it.

[alexeyzimarev]

Another thing is that the error response example you provided when you opened the PR is quite common. It's not only limited to downloads. Often, normal calls return similar results or, for example, problem details. Ultimately, the solution could be that the response deserialisation would support two types.

But, indeed, the download case is different because you expect a stream back, but there's nothing there because where's an error. Still, the implementation in this PR would still produce an exception unless you disable ThrowOnAnyError, and you have a completely separate flow for error handlings, which is still quite inconvenient (interceptors won't help there though).

I guess the best solution for downloads would be to change the signature of DownloadStreamAsync to return a tuple of Stream and TError?. RestSharp can then auth-deserialise the response to TError, which would be convenient to inspect in the same execution path instead of separate function/interceptor. The interface will be broken, but it will be easy to fix downstream. Also, it will be possible to retrofit the old API there by an extension with the current signature, which will deserialise errors to JObject a not return it. So only the code that implements the interface will be broken, but, again, it's an easy fix.

[marcoburato-ecutek]

Regarding the interface, my understanding is that it's possible to add default implementations to avoid breaking existing consumers. But, I've never used the feature myself and I don't know if it's compatible with old runtimes.

Following your idea, I reckon the signature on the interface should be a tuple of Stream?, RestResponse?. Automatically deserializing an error object could then be done in the same way ExecuteAsync does it (extension methods, if I understand correctly). That way one could deal with the low-level RestResponse if needed.
For instance, right now I'm dealing with another API that gives a plain 503 Service Unavailable error response that can include a Retry-After header, which is of interest. I don't need to use RestSharp in this case, just providing it as an example.