FirebaseClient does not verify response status codes

[pjhuck]

The FirebaseClient never verifies that it receives a 200 or 204 response code for all of its requests. Instead, it assumes that any response is successful. Even worse, the status code is never exposed to the caller's, so it's impossible to verify that the response was successful unless you parse the FirebaseResponse.Body, which is not very practical.

This can be verified by doing the following:

 var response = await _client.UpdateAsync("todos", true);

The response is a 400 Bad Request with the following body:

{
  "error" : "Invalid data; couldn't parse JSON object. Are you sending a JSON object with valid key names?"
}

I'm happy to do a pull request which fixes this, but I'm not sure of your error handling strategy. It seems as if all of the methods in IFirebaseClient are never supposed to throw, and instead all errors are returned via the FirebaseResult.Exception property. I'd suggest removing the Exception property and just throwing a FirebaseException for all errors. All of these methods return a Task which already has an Exception property. There's no there's no real point in hiding the exception in Task.Result.Exception when you could just fault the task instead.

Throwing exceptions (and faulting the task) would make life much easier on the caller. As it stands now the caller needs to examine the Response.Exception property for each request. Throwing an exception allows them to just put their operations in a try/catch. For example:

try  {
    _client.PushAsync("todos", todo);
    _client.Update("todos/id", todo);
} catch (FirebaseException e) {
    // handle failure ...
}

as opposed to

var response = _client.PushAsync("todos", todo);
if (response.Exception != null)
     // handle failure
var response2 = _client.UpdateAsync("todos/id", todo);
if (response2.Exception != null)
     // handle failure

[bugthesystem]

Hi @pjhuck , Thanks for your feedback and contributions. I know there are some issues about verify response status codes and exception handling strategy. I was very busy to refactor them. You right; I prefer Throwing an exception allows them to just put their operations in a try/catch. So, I'm happy to see a pull request which fixes this :+1:

try  {
    _client.PushAsync("todos", todo);
    _client.Update("todos/id", todo);
} catch (FirebaseException e) {
    // handle failure ...
}

[bugthesystem]

Closed with 8