[Bug]: IsSuccessStatusCode is true, but Error is non-null

[hartmark]

Describe the bug 🐞

We noticed that if you get any serialization errors IsSuccessStatusCode is still true for ApiResponse, but Error is not-null because of error in serialization.

We have this reproducing code. Using xunit and RichardSzalay.MockHttp.

Step to reproduce

Create a class using this code and run the unit-test:

using System;
using System.Net;
using System.Net.Http;
using System.Threading.Tasks;
using Refit;
using RichardSzalay.MockHttp;
using Xunit;
using Xunit.Abstractions;

namespace Tests.UnitTests;

public class DeserializeRefitTest
{
    private readonly ITestOutputHelper testOutputHelper;
    private readonly MockHttpMessageHandler mockHttp;
    private readonly IRefitClient refitClient;

    public DeserializeRefitTest(ITestOutputHelper testOutputHelper)
    {
        this.testOutputHelper = testOutputHelper;
        mockHttp = new MockHttpMessageHandler();
        var client = new HttpClient(mockHttp);
        client.BaseAddress = new Uri("https://example.com");
        
        refitClient = RestService.For<IRefitClient>(client);
    }
    
    [Fact]
    public async Task DeserializeResponse()
    {
        // If we change to use this JSON it all works
        // const string jsonString =
        //     """
        //     {
        //       "value": "bar"
        //     }
        //     """;

        const string jsonString = "broken json";

        const HttpStatusCode httpStatusCode = HttpStatusCode.OK;
        mockHttp.Fallback.Respond(httpStatusCode, "application/json", jsonString);

        IApiResponse<FooResponse> response =
            await refitClient.GetFoo();

        Assert.NotNull(response);

        if (!response.IsSuccessStatusCode)
        {
            testOutputHelper.WriteLine(response.Error!.Content);
        }

        Assert.Equal(HttpStatusCode.OK, response.StatusCode);

        Assert.Null(response.Error);

        Assert.NotNull(response.Content);
        Assert.Equal("bar", response.Content.Value);
    }
}

public interface IRefitClient
{
    [Get("/api/foo")]
    Task<IApiResponse<FooResponse>> GetFoo();
}

public class FooResponse
{
    public string Value { get; set; }
}

Reproduction repository

No response

Expected behavior

response.Error should not be null when IsSuccessStatusCode is true

Screenshots 🖼️

No response

IDE

No response

Operating system

No response

Version

No response

Device

No response

Refit Version

7.0.0

Additional information ℹ️

No response

[hartmark]

We added this workaround extension method so we catch all errors when checking IApiResponse:

public static class ApiResponseExtensions
{
    public static bool IsReallySuccessful<T>(this IApiResponse<T> response)
    {
        return response.IsSuccessStatusCode && response.Error == null;
    }
    
    public static bool IsReallySuccessful(this IApiResponse response)
    {
        return response.IsSuccessStatusCode && response.Error == null;
    }
}

[dtcopcp]

found the same too
#1303

[marcominerva]

Many customers have reported this problem. I would be happy to help solving the issue.

[TimothyMakkison]

I think this is intentional, no? It would be strange if ApiResponse.IsSuccessStatusCode returned false when the actual HttpResponseMessage was successful.

Instead I think ApiResponse should have the property IsSuccess. This will only return true when there are no errros and the status is successful. This is similar to other libraries like RestSharp who use both IsSuccessStatusCode and IsSuccess.

public interface IApiResponse : IDisposable
{
...
    public bool IsSuccess { get; }
...
}

public sealed class ApiResponse<T>
{
...
    public bool IsSuccess => IsSuccessStatusCode && Error is null;
}

[marcominerva]

Good idea @TimothyMakkison! If necessary, I can take care of such a PR.

[TimothyMakkison]

Thanks 😊, you're welcome to make a pr. Otherwise I'll get around to it tomorrow 😅

Might be worth updating the NotNullWhen attributes to use it, see #1303

[mikedepetris]

I'm sure @marcominerva will have care of the issue and submit the best PR to fix it, please Marco do it!

[marcominerva]

@TimothyMakkison I have made the PR: #1891.

I'm wondering if it's worth updating the documentation at https://github.com/reactiveui/refit#handling-exceptions.

[TimothyMakkison]

I'm wondering if it's worth updating the documentation at https://github.com/reactiveui/refit#handling-exceptions

Yeah, good idea. I feel like most usage of IsSuccessStatusCode could be replaced with IsSuccess.

On an related note do you prefer, IsSuccess or IsSuccessful, noticed that RestSharp uses IsSuccessful?

[marcominerva]

Yeah, good idea. I feel like most usage of IsSuccessStatusCode could be replaced with IsSuccess.

I agree with you, so I have update the documentation.

On an related note do you prefer, IsSuccess or IsSuccessful, noticed that RestSharp uses IsSuccessful?

After thinking about, probably IsSuccessful is more appropriate in this context, so I have renamed the property.