Option to distinguish actual exceptions from "Request failed with status code"

[oliversaggau]

Overview:

We are using IRestClient.Execute[Async](request) to do our own error handling. Unfortunately response.ErrorException is not only set when an actual exception happened during the request, but also simply because response.IsSuccessStatusCode was false and unfortunately that case (also) simply uses a System.Net.Htttp.HttpRequestException for doing so. The only way for us to distinguish an actual (HttpRequest-) exception from this one, is by doing something like exception.Message.StartsWith("Request failed with status code") which feels not good.

Use case:

As I said, we want to do our own StatusCode error handling, so I would actually want response.ErrorException to be null when there was no actual exception.

Solution?

I understand that such a change might break existing code. So I would suggest to add an option to RestClientOptions where the user can decide whether response.IsSuccessStatusCode should populate response.ErrorException or not, and that option is than on by default to not change the existing behavior.

[AndrzejKozik]

I support this feature. Furthermore, I would add a parameter to decide whether Execute{Method}Async < T > should attempt to deserialize the response to T for statuses != 200 (where for 400 there is often a different data structure).

[alexeyzimarev]

Deserialisation of error responses is indeed useful. However, the error response type is not known to RestSharp as it only knows the type of T that is expected to be a successful response. In an ideal world, all error responses would comply with RFC 7087 (problem detail). Maybe you guys can check if the error response you're getting is in that format.

About swallowing the exception. In fact, RestSharp does not set the exception for unsuccessful status codes. The exception is only set if the exception occurs when sending the message or getting a response. RestSharp catches all exceptions and passes them downstream.
@oliversaggau can you explain why do you need to check the response message instead of checking the response code?

[oliversaggau]

RestSharp catches all exceptions and passes them downstream.

Yes, that indeed is the case, all exceptions are swallowed and passed along. But in my own error handling I do not want to swallow actual exceptions (like e.g. HttpRequestException when there was a network issue) so in my error handling I do something like

if (response.Exception != null) throw response.ErrorException;

which I had to change to

if (HasException(response)) throw response.ErrorException;
...
private static bool HasException(RestResponse response)
{
    if (response.ErrorException is HttpRequestException exception) return !exception.Message.StartsWith("Request failed with status code"); // workaround
    else return response.ErrorException != null;
}

In fact, RestSharp does not set the exception for unsuccessful status codes. The exception is only set if the exception occurs when sending the message or getting a response.

I'd have to double check, but I'm pretty sure that response.Exception was set, even without an actual "real" exception, because of

RestSharp/src/RestSharp/Response/RestResponse.cs

Line 68 in b034cda

ErrorException = httpResponse.MaybeException(),

RestSharp/src/RestSharp/Extensions/HttpResponseExtensions.cs

Lines 20 to 28 in b034cda

	static class HttpResponseExtensions {
	public static Exception? MaybeException(this HttpResponseMessage httpResponse)
	=> httpResponse.IsSuccessStatusCode
	? null
	#if NET
	: new HttpRequestException($"Request failed with status code {httpResponse.StatusCode}", null, httpResponse.StatusCode);
	#else
	: new HttpRequestException($"Request failed with status code {httpResponse.StatusCode}");
	#endif

(latter only returns no exception when IsSuccessStatusCode was true)

[alexeyzimarev]

Aha, I was looking at that line but somehow didn't check what MaybeException actually does. It's a good point. Should be easy to fix.

[AndrzejKozik]

In an ideal world, all error responses would comply with RFC 7087 (problem detail). Maybe you guys can check if the error response you're getting is in that format.

Indeed, my error responses are of ProblemDetails type, extended with Violations - but I would live deserialization of response.Content to the user. It would be enough to set response.Data = (T?) null if response status != 200.

Making this, together with solution to @oliversaggau's problem, enables consistent response processing - first check if there are any exceptions, then manage response status codes:

if (response.IsSuccessful)
{
return response.Data ?? throw new Exception("!");
}
else
{
if (response.ErrorException is not null)
throw response.Exception;
else if (response.StatusCode == System.Net.HttpStatusCode.UnprocessableEntity)
{ //process response.Content }
else ...

Because of reported JSON exception, I have to reverse the order of error processing - first evaluation StatusCode, then exceptions processing.