SrcResource.GetFileContent(string) return an error message instead of null or an exception when the file do not exists about sharpbucket

[mnivet]

Here an example of the json I get:
404 NOT FOUND
{"type":"error","error":{"message":"No such file or directory: path/To/NonExistingFile.txt"}}

[mnivet]

I've push 3b43cfd to show that in API V1 we were returning null

4f68640 show that there is also some methods that were returning empty objects and not null when an error 404 was encountered

We can also remarks that there is that code in SharpBucket.cs that catch WebException to return default(T) (which means null for POCOs since they are reference types)

So what error handling strategy should we apply ?

1. Returning null like in V1 ?
2. Returning empty POCOs like some methods were doing in V2 ?
3. Let the exceptions propagate ?

Simply returning null or an empty POCO can't be the right solution for all types of HTTP errors. It may be the right one for errors 404, but for other types of HTTP errors the caller must be able to retrieve a precise information about the error.
And writing the error on the console is not a good solution. The caller may not have access to a console. Maybe the caller is an application that log elsewhere and the console is not visible to anyone (like in some unit test client interfaces).
So I think that letting the exceptions propagate is the right solution at least for errors other than 404.

So I think we can split the main question like that:

1. How should we do to raise exceptions for general HTTP errors ?
   1. Break actual behavior and let WebExceptions propagate ?
   2. Break actual behavior for V2 but not for V1 ?
   3. Provide a way to choose what to to (through an option or an injection point) ?
2. What error handling strategy should we apply for Error 404 ?
   1. Always return null and break existing behaviors in V2 ?
   2. Always return empty POCOs and break existing behaviors in V1 ?
   3. Returns null in V1 and empty POCOs in V2 ?
   4. Always throw exceptions and break all existing behaviors ?
   5. Returns null in V1 and throw exceptions in V2 and break only V2 existing behavior ?

[mnivet]

For question 1, my choice would go to "Break actual behavior and let WebExceptions propagate"
Maybe we can improve the code to handle some special errors like HTTP 429 (see #108), but while an error is not specifically managed by our code, it should be propage to the caller to let him a chance to to implements it's own management of the error

For question 2, I'm more hesitating between:

• "Always throw exceptions and break all existing behaviors", because its simple since we manage all errors (404 and others) the same way
• "Always return null and break existing behaviors in V2", because it breaks less things, it's more simple to manage that common type of error for callers than catching an exception, and there is not so much code to write too achieve that.

All solutions that would try to limit breaking change just for keeping some backward compatibility (1.ii, 1.iii, 2.iii, 2.v) seems clunky for me and I would be really favorable to reject them. I've write them mostly to be exhaustive, but frankly it would generate more garbage code that improving what we have.

So there is still one solution for which I've not give my opinion which is "2.ii Always return empty POCOs"
I think that it will need to do much code than other solution to go there, because we will need to create empty instance of various types, so it will probably need to add some where T : new() constraints, so why should we choose a solution more complex to implements than returning null ?
Furthermore it's less simple for caller to manage that case. As a caller I prefer testing the nullity of the returned POCO, than the nullity of some random (or all) properties of the POCO to determine that we are in the case of a 404 error
And finally for the case of the SrcResource.GetFileContent(string) method, retuning an empty string (which is what seems the most near to an empty POCO) is not viable since it won't allow to distinct an empty file from a non existing file.

[mnivet]

I've just think about one think : What happens if I call SrcResource.GetFileContent(string), but that in fact it's not just the file that do not exists, but the reference I give to SrcResource when I built the instance ?
I suppose that bitbucket will tell me in the 404 error response.
But If we choose to simply return null for every 404 response the caller won't be able to guess if it's the file or another parameter which is invalid...

So maybe if we chose to go in the direction of returning null, we should also pay attention to return null only if it concern the last parameter give by the caller ?
I think I will do some try around that question, but it may quickly be really complex, and may be an argument to chose the solution to always let the exceptions propagate.

[mnivet]

I've done few tests and be able to get three different types of 404 error message on response to that URL pattern: /2.0/repositories/{account}/{repository}/src/{version}/AnyFile.txt

When path do not exists:

{"type":"error","error":{"message":"No such file or directory: NotExistingFile.txt"}}

When revision do not exists:

{"type":"error","error":{"message":"Changeset not found."}}

When repo or account name do not exists:

{"type":"error","error":{"message":"Repository sharpbucket-mnivet/not_existing_repository not found"}}

And We can also add the case of error 403 when I try to hit the URL of a file that exists in a location where I don't have the rights which also return null in the naive implementation started in 4f68640

So definitively I would go to the solution that consist to Always throw exceptions and break all existing behaviors for all type of HTTP error, since it the only solution I see that will allow to be transparent with the API we expose in C#.