EnsureSuccessStatusCode hides some details

[toryalsip]

I've noticed that when calling EnsureSuccessStatusCode() after making a web request, we're giving control over the Framework to make the message that is included in the exception that is thrown and what it will do is give a fairly generic error message. If the web server is giving back some important detail in the error message, it gets lost. Additionally, a consumer would also need to parse out the StatusCode from the message to at least get an idea why the call failed, and then subsequently return back a more user-friendly message to the user.

I would like to propose that as an alternative is something that looks like this

if (!response.IsSuccessStatusCode)
{
    var responseBody = await response.Content.ReadAsStringAsync();
    throw new MastodonAPIException(response.StatusCode, responseBody);
}

// Constructor for MastodonAPIException

MastodonAPIException(HttpStatusCode statusCode, string responseBody)
{
    StatusCode = statusCode;
    ResponseBody = responseBody;
    // Possibly have an additional more generic Message property
}

[53ningen]

I'm mostly in favor of your proposal.

I've defined Error entity as MastodonApi error response, but i could not implement on ApiBase yet.

If you are ok, I would like to change the handling of non-success status code response as following:

if (!response.IsSuccessStatusCode)
{
    var error = await response.Content.ReadAsStringAsync()
        .ContinueWith(x => JsonConvertDeserializeObject<Error>(x.Result));
    throw new MastodonAPIException(response.StatusCode, error);
}

// Constructor for MastodonAPIException

MastodonAPIException(HttpStatusCode statusCode, Error error)
{
    StatusCode = statusCode;
    Error = error;
    // Possibly have an additional more generic Message property
}

What do you think of this?

[toryalsip]

I think that does sound like a better approach. I do wonder, though, if it is possible for the result to come back in a format that doesn't deserialize nicely into the Error object. Most of the time it should, unless something is going very wrong on the server.

[toryalsip]

Additionally, I wanted to share with you an approach to mocking the HttpClient to enable placing unit testing around some of these method calls to assure they're throwing the correct exception. I was working on this last night just to try it out. It overrides the handler with one that simply returns a predefined response object instead of making the actual http call. https://github.com/jbalsip/mastodon-api-cs/tree/refactor/error-codes/Mastodon.API.Tests/Mocks

So then a unit test could look like this. I'm fairly certain it could be expanded to check that the properties of the exception are populated correctly too.

[Test]
public void GetAsyncWithBadRequest()
{
        var message = "This was bad";
        var statusCode = HttpStatusCode.BadRequest;
        var mockHttp = MockHttpClient.Create(message, statusCode);
        var apiClient = new ApiClientBase(new Uri("http://example.com/"), mockHttp);

        Assert.Throws<MastodonAPIException>(async () => await apiClient.GetAsync("/test"));
}

[toryalsip]

I've been thinking about this through the day, and I think before we go too far with this we should stop and give careful consideration to this approach compared to alternatives. From an API development standpoint, simply throwing exceptions certainly looks convenient, but using them for control flow can be a dangerous practice. Typically they should be reserved for when something has gone wrong or is unexpected, which differs from some HTTP status codes that are normal and expected even though they also indicate the call wasn't successful. This is certainly true in typical REST APIs, where for example 404 is used to indicate a resource hasn't been found. So the consumer is going to then be required to wrap their calls with try / catch statements and then handle the errors from there. They could choose not to do so, of course, but then that could lead to a bad app experience down the road the first time an API call doesn't succeed and an uncaught exception brings that app down.

Another approach could be to have all the API calls return a standard interface, and then the consumer could just check the result of that interface to see if they can retrieve the object they are expecting, and if not they have to then check error conditions. It does have its trade-offs, but maybe worth considering as a perhaps a gentler way of indicating common error conditions.

[toryalsip]

One obvious downside I can think of to returning an interface is that it's a breaking change, and that alone may be far more disruptive than to ask to catch a different kind of exception. Just brainstorming ideas at this point, though, so I hope you don't mind.

[53ningen]

@jbalsip Sorry for the late reply. (I was working away outside the office for the last few days...)

I'll comment as soon as I finish reviewing issue comments and pull request. Thank you.