Skip to content

Conversation

@PressXtoChris
Copy link

What kind of change does this PR introduce?

Feature with breaking changes

What is the current behavior?

Refit currently doesn't catch/wrap any exceptions that occur before receiving a response from the target server (e.g. host not available, DNS errors, etc). These are usually an HttpRequestException (or TaskCanceledException in the case of timeouts) thrown by HttpClient.SendAsync.

try
{
    return await _awesomeApi.GetFooAsync("bar");
}
catch (HttpRequestException ex)
{
    _logger.LogError(ex, "Unable to reach {Server}", "AwesomeApi");
}
catch (ApiException ex)
{
    _logger.LogError(ex, ex.Content);
}

When these exceptions are thrown, they're missing a lot of the detail about the request that Refit usually includes in its ApiException. If they are caught by a global exception handler (or some other catch block far away from the context of the call), then you lose out on the ability to include that context in your error logs or add generic handling based on the request info. See #273 and #719 for old issues on this topic.

These exceptions also aren't captured by ApiResponse.Error, requiring those who use ApiResponse to both have if statements for handling successful/failed responses, and try catch blocks if they wish to handle the above error cases too. See #1238 for an open issue on this topic.

try
{
    var response = await _awesomeApi.GetFooAsync("bar");

    if (response.IsSuccessful)
        return response.Content;

    _logger.LogError(response.Error, response.Error.Content);
}
catch (HttpRequestException ex)
{
    _logger.LogError(ex, "Unable to reach {Server}", "AwesomeApi");
}

What is the new behavior?

Part 1

Create a new ApiRequestException with all of the same request-related properties as ApiException (all the common properties are placed in a new ApiExceptionBase class that they both derive from). A new try/catch around HttpClient.SendAsync catches any exceptions, wraps them in the new ApiRequestException with all the additional context, and rethrows it. This is a breaking change, since existing code that catches the original exceptions won't work anymore.

Example usage:

try
{
   return await _awesomeApi.GetFooAsync("bar");
}
// If you need to distinguish between request/response errors
catch (ApiRequestException ex)
{
    _logger.LogError(ex, "Unable to reach {Server}", ex.RequestMessage.RequestUri?.Host);
}
catch (ApiException ex)
{
    _logger.LogError(ex, ex.Content);
}
// Or if you don't need to access any of the response-specific properties, you can have a single catch block:
catch (ApiExceptionBase ex)
{
    _logger.LogError(ex, "An error occurred while completing the request to {Server}.", ex.RequestMessage.RequestUri?.Host);
}

Part 2

For users who use the ApiResponse wrapper, the new exception won't be thrown, but instead placed in the ApiResponse.Error property and return early. This kicks off some more breaking changes:

  • ApiResponse is no longer guaranteed to contain a response from the server. All response-related properties are now nullable (e.g. status code, content, headers). IsSuccessful/IsSuccessStatusCode have compiler hints to mark them as not-null if true, and a new IsReceived property has been added to mark them as non-null but not distinguish between successful/unsuccessful responses from the server
  • The Error property's type has been changed from ApiException to ApiExceptionBase, so exception properties relating to responses aren't immediately accessible anymore. Two new methods have been added to cast to the actual exception type - bool HasRequestError(out ApiRequestException ex) (slightly redundant since all the properties are already in ApiExceptionBase) and bool HasResponseError(out ApiException ex) (where you can access all the previous exception properties).

Example usage:

var response = await _awesomeApi.GetFooAsync("bar");

// Original syntax works fine for success checking
if (response.IsSuccessful)
    return response.Content;

// New method for checking if there was a response, but not whether it was successful
if (response.IsReceived)
{
    // Compiler hint marks response properties as non-null
    _logger.LogInformation("Received {ResponseCode} response", response.StatusCode);
}
else
{
    _logger.LogError(response.Error, "Did not receive response from {Server}", response.Error.RequestMessage.RequestUri?.Host);
}

// New method for getting the full `ApiException` in case of a 4xx/5xx
if (response.HasResponseError(out var responseError))
{
    _logger.LogError(responseError, responseError.Content);
}

What might this PR break?

  • Part 1 of the solution breaks existing try/catch blocks for HttpRequestException and other exceptions thrown by HttpClient.SendAsync.
  • Prat 2 of the solution breaks many properties of IApiResponse by making them nullable, and also changes the base type of the Error property from ApiException to ApiBaseException (which leaves only the request-related properties immediately accessible)

If you want to keep the number of breaking changes to a minimum, you could:

A) Scrap Part 2 of the solution. I can't think of a way of adding request exceptions to ApiResponse without breaking all of the response-related properties

B) Keep Part 1, and add a new option into RefitSettings so that users can choose whether they'd like their exceptions wrapped or keep the legacy functionality. Could even have it default to false initially, and switch the default in a later major version.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants