[BUG] Unable to throw a ValidationApiException exception

[marcominerva]

I have a Web API that returns a ProblemDetails with the application/problem+json content type:

Then, I have the following Refit interface:

public interface IPeopleServiceApi
{
    [Post("/people")]
    Task<ApiResponse<Person>> GetAsync(Person person);
}

If I try to call this API from Refit, when there is a validation problem, in the Error property of the ApiResponse I see a ValidationApiException object:

However, if I call EnsureSuccessStatusCodeAsync, the exception that is raised is ApiException, not ValidationApiException. Moreover, the HasContent property of the exception is false:

If, instead, I change the interface to directly return a Person instead of an ApiResponse<Person>, when I call the API, a ValidationApiException is thrown, with the Content correctly set.

Steps To Reproduce

1. Invoke an API that return a problem details with the application/problem+json content type
2. Define a method in the interface for Refit that returns an ApiReponse<T> for this API
3. Invoke the API from the client
4. Call the ApiResponse.EnsureSuccessStatusCodeAsync method
5. An ApiException is thrown

Expected behavior A ValidationApiException should be thrown

Environment

• OS: Windows 11
• Device: Desktop
• Version: 6.3.2

[marcominerva]

After some digging, it seems that the issue depends on this method: https://github.com/reactiveui/refit/blob/246ee8d9989c29092fdaba6821d5b3098a5ccf9e/Refit/ApiResponse.cs#L85

public async Task<ApiResponse<T>> EnsureSuccessStatusCodeAsync()
{
    if (!IsSuccessStatusCode)
    {
        var exception = await ApiException.Create(response.RequestMessage!, response.RequestMessage!.Method, response, Settings).ConfigureAwait(false);

        Dispose();

        throw exception;
    }

    return this;
}

If there was an error in the response, the ApiException.Create() method is called to create the Exception object. However, as this point response.Content has already been read and disposed, because the ApiException.Create() method has already been called when the ApiResponse<T> object has been created (to populate its Error property). In fact, we already have the correct Exception in the ApiResponse<T>.Error property, so why don't just return it?

In other words, we can think about changing the EnsureSuccessStatusCodeAsync method in this way:

public async Task<ApiResponse<T>> EnsureSuccessStatusCodeAsync()
{
    if (!IsSuccessStatusCode)
    {
        var exception = Error ?? await ApiException.Create(response.RequestMessage!, response.RequestMessage!.Method, response, Settings).ConfigureAwait(false);

        Dispose();

        throw exception;
    }

    return this;
}

So, if the Error property isn't null, we just use it, instead of recreating the exception at all.

[sciocoder]

I agree with @marcominerva

Also, with the default ExceptionFactory the exception is created if the response does not have a success status code (https://github.com/reactiveui/refit/blob/main/Refit/RefitSettings.cs#L223) and this is good for the EnsureSuccessStatusCodeAsync method of ApiResponse but if a custom is provided this is not guaranteed and the disposal problem may recur

https://github.com/reactiveui/refit/blob/main/Refit/RequestBuilderImplementation.cs#L261

    if (typeof(T) != typeof(HttpResponseMessage))
    {
        e = await settings.ExceptionFactory(resp).ConfigureAwait(false);
[+]     disposeResponse = false; // caller has to dispose
    }
...    
    else if (e != null)
    {
[-]     disposeResponse = false; // caller has to dispose
        throw e;
    }

this update would solve the disposal problem but move to the stream which is already consumed https://github.com/reactiveui/refit/blob/main/Refit/RequestBuilderImplementation.cs#L274 (e is null)

    // Only attempt to deserialize content if no error present for backward-compatibility
[~] body = resp.IsSuccessStatusCode // it's right ???
        ? await DeserializeContentAsync<TBody>(resp, content, ct).ConfigureAwait(false)
        : default;

...for ApiResponse, custom ExceptionFactory doesn't risk losing control?

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.