Exceptions don't display default exception messages when ApiError.Message is empty

[SeanKilleen]

Currently, many exceptions take ApiErrorMessageSafe and display the default text if it's null.

However, this should probably also apply if ApiErrorMessageSafe is empty so that the user gets something.

I came across this while attempting to write out tests for #1528. I didn't want to change too many things in that PR, so figured I'd create this as an off-shoot.

A failing test

Removed this from my previous PR. Expected the default error message (e.g. generic "Request Forbidden") -- actual was an empty string. This was part of ConnectionTests.cs in #1528 before I ripped it out:

[Fact]
public async Task ThrowsAbuseExceptionWithDefaultMessageForUnsafeAbuseResponse()
{
	string messageText = string.Empty;

	var httpClient = Substitute.For<IHttpClient>();
	IResponse response = new Response(
		HttpStatusCode.Forbidden,
		 "{\"message\":\"" + messageText + "\"," +
		"\"documentation_url\":\"https://developer.github.com/v3/#abuse-rate-limits\"}",
	   new Dictionary<string, string>(),
		"application/json");
	httpClient.Send(Args.Request, Args.CancellationToken).Returns(Task.FromResult(response));
	var connection = new Connection(new ProductHeaderValue("OctokitTests"),
		_exampleUri,
		Substitute.For<ICredentialStore>(),
		httpClient,
		Substitute.For<IJsonSerializer>());

	var exception = await Assert.ThrowsAsync<AbuseException>(
		() => connection.GetResponse<string>(new Uri("endpoint", UriKind.Relative)));

	Assert.Equal("Request Forbidden - Abuse Detection", exception.Message);
}

Potential Fixes

• The fix for this would likely be updating the Message property of the affected exceptions to do an IsNullOrWhitespace check on ApiErrorMessageSafe.
• Or, ApiErrorMessageSafe could be changed to return null if the Message string is empty or whitepsace, rather than just returning the message property itself.

[ryangribble]

Is this related to something I found when working on a PR here: #1415 ?

If you see the 2nd bullet point where I talk about the attempted ApiError deserialization never returning a null and thus never falling back to the text payload...

[ryangribble]

Just refreshing my memory with this stuff and yes I agree with your suggestion.

Specifically I think the 2nd suggested fix would be the way to go, since we can "fix" it in one place, rather than needing to fix it in every place that currently has ApiErrorMessageSafe ?? "default error text"

And this is a different situation to what I commented about above - in that case I found that a compeltely invalid json structure in the error message was still returning a default/empty ApiError object because the deserializer creates an object then tries to populate any fields that match the setter properties. This led to code that was never really getting what it expected, since ApiError itself could never be null. Your case is slightly different since it would also happen if there was valid json error payload but the message field was empty string or not present.

[SeanKilleen]

👍 Got it, makes sense. I'll put together a PR to un-skip this test and fix it via the decision here.