[Bug]: new keyword for properties on generic IApiResponse breaks inheritance

[timosnel]

trafficstars

Describe the bug 🐞

Version 8 introduced a change in #1879 where the IApiResponse<out T> interface applies the new keyword on certain properties like StatusCode and ContentHeaders.

If you have a method that uses the non-generic IApiResponse as parameter, the base interface properties will be used which will then not be set. This is especially difficult when mocking these interfaces where we now have to setup both the properties of the generic and non-generic interface.

This is a breaking change which is not mentioned in the release notes of release 8.0.0. This works perfectly in version 7.2.2

Step to reproduce

Have a method that uses the non-generic IApiResponse:

public static bool IsProblemDetails(this IApiResponse apiResponse)
{
    Guard.Against.Null(apiResponse);

    var contentType = apiResponse.ContentHeaders?.ContentType?.MediaType;

    return
        apiResponse.StatusCode == HttpStatusCode.UnprocessableEntity &&
        contentType?.Equals(MediaTypeNames.Application.ProblemJson) == true;
}

Create a unit test that mocks the generic IApiResponse<out T> interface and calls the method:

[Fact]
public void IsProblemDetails_should_return_true_for_unprocessable_entity_with_content_type_problem_details()
{
    // Arrange
    var headers = new StringContent(string.Empty).Headers;
    headers.ContentType = new MediaTypeHeaderValue(MediaTypeNames.Application.ProblemJson);
    var mock = new Mock<IApiResponse<string>>();
    mock.SetupGet(x => x.IsSuccessStatusCode).Returns(false);
    mock.SetupGet(x => x.StatusCode).Returns(HttpStatusCode.UnprocessableEntity);
    mock.SetupGet(x => x.ContentHeaders).Returns(headers);
    var apiResponse = mock.Object;

    // Act
    var result = apiResponse.IsProblemDetails();

    // Assert
    Assert.True(result);
}

This results in null values for the non-generic interface, making the test fail:

Reproduction repository

No response

Expected behavior

Setting properties on the generic interface should result in being able to use it as the non-generic interface (not breaking inheritance)

Screenshots 🖼️

No response

IDE

No response

Operating system

No response

Version

.NET >6

Device

No response

Refit Version

8.0.0

Additional information ℹ️

No response

[sguryev]

Hi @timosnel, nice catch!

Yes this is one of the expected problems with the new keyword. Moreover Moq mixes inheritor and base properties showing them on the same level (1) and knows about the "new" ones as well (2)

It's a kind of breaking the Liskov Substitution Principle (LSP) in terms of interfaces but not in terms of normal class usage Sharplab (you can see that compiler is smart enough to map both interfaces to the same property)

I was not able to come up with the easy solution here. It would be great if you can mock the exact interface you are going to test and others will be able to avoid checking Content, ContentHeaders and Error for null after checking IsSuccessful.

Here is one of my product where we have to check Content for null in addition to the IsSuccessStatusCode

[sguryev]

Another idea is to use custom class TestApiResponse<T> implementation and mock this class instead