Skip to content
This repository was archived by the owner on Mar 17, 2025. It is now read-only.

Refactored Firebase library #42

Merged
merged 19 commits into from
Jan 29, 2016
Merged

Refactored Firebase library #42

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
0a613cd
Refactor Firebase module, split into more focues subclasses, change r…
ed7coyne Jan 12, 2016
d805dc6
removed FirebaseResultWithMessage consolidated into one for simplicity
ed7coyne Jan 12, 2016
217d58f
Added comments for clarify
ed7coyne Jan 12, 2016
b2438cf
removed old reference to FirebaseResultWithMessage
ed7coyne Jan 13, 2016
d5dd303
Refactor out FirebaseCall
ed7coyne Jan 14, 2016
23fbbed
possibly fix streaming example
ed7coyne Jan 14, 2016
2bf3a4f
Move helper classes below Firebase in Firebase.h
ed7coyne Jan 14, 2016
2df5bd6
Added error message back to streaming imp
ed7coyne Jan 14, 2016
077e060
Add call ids
ed7coyne Jan 26, 2016
6f65250
Revert "Add call ids"
ed7coyne Jan 27, 2016
33f81ef
Added independant opaque return types for all calls.
ed7coyne Jan 28, 2016
2ac7ab3
Remove unused method declaration.
ed7coyne Jan 28, 2016
357a1d0
firebase: refactor the refactoring
proppy Jan 28, 2016
c65fef7
Merge pull request #2 from proppy/refactor-12
ed7coyne Jan 28, 2016
b286b19
firebase: fix stream example
proppy Jan 29, 2016
34357a9
Merge pull request #3 from proppy/refactor
ed7coyne Jan 29, 2016
f38602f
merge
proppy Jan 29, 2016
e8ac1e9
examples/stream: fix error handling
proppy Jan 29, 2016
caa70e9
Merge pull request #4 from proppy/refactor-merge-3
ed7coyne Jan 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
186 changes: 114 additions & 72 deletions Firebase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,116 +15,158 @@
//
#include "Firebase.h"

const char* firebaseFingerprint = "7A 54 06 9B DC 7A 25 B3 86 8D 66 53 48 2C 0B 96 42 C7 B3 0A";
const uint16_t firebasePort = 443;
namespace {
const char* kFirebaseFingerprint = "7A 54 06 9B DC 7A 25 B3 86 8D 66 53 48 2C 0B 96 42 C7 B3 0A";
const uint16_t kFirebasePort = 443;
} // namespace

Firebase::Firebase(const String& host) : _host(host) {
_http.setReuse(true);
Firebase::Firebase(const String& host) : connection_(host) {
}

Firebase& Firebase::auth(const String& auth) {
_auth = auth;
connection_.auth(auth);
return *this;
}

String Firebase::get(const String& path) {
return sendRequestGetBody("GET", path);
FirebaseResult Firebase::get(const String& path) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of making reading/decoding optional.

fbase.post("/foo", "bar"); // just post the body
auto& result = fbase.post("/foo", "bar").json(); // get the result and decode it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While these examples flow very well and look great the problem is this style assumes the calls succeed, which isn't great for remote services. I think the resulting use will be uglier but safer if we encourage checking for errors.

That said I agree whole heartedly with making reading/decoding optional, the return value for a lot of these calls seems unnecessary (like echoing back the value on a put)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is this style assumes the calls succeed, which isn't great for remote services.

I tried to add explicit error checking after each call in every sample, but I agree with the sentiment.

An intermediate result object make things more discoverable (they have to lookup the doc for the result, and then it's easier for them to find the error methods here, rather than when it's hidden inside the main class), but they still need to remember to call isError() on it, and a good way to "learn" that pattern is to repeat it across sample.

return connection_.sendRequestGetBody("GET", path);
}

String Firebase::push(const String& path, const String& value) {
return sendRequestGetBody("POST", path, value);
FirebaseResult Firebase::push(const String& path, const String& value) {
return connection_.sendRequestGetBody("POST", path, value);
}

bool Firebase::remove(const String& path) {
int status = sendRequest("DELETE", path);
return status == HTTP_CODE_OK;
FirebaseResult Firebase::remove(const String& path) {
return connection_.sendRequest("DELETE", path);
}

Firebase& Firebase::stream(const String& path) {
_error.reset();
String url = makeURL(path);
/* FirebaseEventStream */

FirebaseEventStream::FirebaseEventStream(const String& host) : connection_(host) {
}

FirebaseEventStream& FirebaseEventStream::auth(const String& auth) {
connection_.auth(auth);
return *this;
}

FirebaseResult FirebaseEventStream::connect(const String& path) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think of moving this to the connection class, it's low level enough and that would allow you to get rid of the httpClient, host and makeURL methods.

String url = connection_.makeURL(path);
auto& http = connection_.httpClient();
http.setReuse(true);
http.begin(connection_.host().c_str(), kFirebasePort, url.c_str(), true,
kFirebaseFingerprint);
const char* headers[] = {"Location"};
_http.setReuse(true);
_http.begin(_host.c_str(), firebasePort, url.c_str(), true, firebaseFingerprint);
_http.collectHeaders(headers, 1);
_http.addHeader("Accept", "text/event-stream");
int statusCode = _http.sendRequest("GET", (uint8_t*)NULL, 0);
http.collectHeaders(headers, 1);
http.addHeader("Accept", "text/event-stream");
int statusCode = http.sendRequest("GET", (uint8_t*)NULL, 0);

String location;
// TODO(proppy): Add a max redirect check
while (statusCode == 307) {
location = _http.header("Location");
_http.setReuse(false);
_http.end();
_http.setReuse(true);
_http.begin(location, firebaseFingerprint);
statusCode = _http.sendRequest("GET", (uint8_t*)NULL, 0);
while (statusCode == HTTP_CODE_TEMPORARY_REDIRECT) {
location = http.header("Location");
http.setReuse(false);
http.end();
http.setReuse(true);
http.begin(location, kFirebaseFingerprint);
statusCode = http.sendRequest("GET", (uint8_t*)NULL, 0);
}
if (statusCode != 200) {
_error.set(statusCode,
"stream " + location + ": "
+ HTTPClient::errorToString(statusCode));
return FirebaseResult(statusCode);
}

bool FirebaseEventStream::connected() {
return connection_.httpClient().connected();
}

bool FirebaseEventStream::available() {
return connection_.httpClient().getStreamPtr()->available();
}

FirebaseEventStream::Event FirebaseEventStream::read(String& event) {
auto client = connection_.httpClient().getStreamPtr();
Event type;
String typeStr = client->readStringUntil('\n').substring(7);
if (typeStr == "put") {
type = FirebaseEventStream::Event::PUT;
} else if (typeStr == "patch") {
type = FirebaseEventStream::Event::PATCH;
} else {
type = FirebaseEventStream::Event::UNKNOWN;
}
event = client->readStringUntil('\n').substring(6);
client->readStringUntil('\n'); // consume separator
return type;
}

/* FirebaseConnection */

FirebaseConnection::FirebaseConnection(const String& host) : host_(host) {
http_.setReuse(true);
}

FirebaseConnection& FirebaseConnection::auth(const String& auth) {
auth_ = auth;
return *this;
}

String Firebase::makeURL(const String& path) {
FirebaseResult FirebaseConnection::sendRequest(const char* method, const String& path) {
return sendRequest(method, path, "");
}

FirebaseResult FirebaseConnection::sendRequest(const char* method, const String& path, const String& value) {
const String url = makeURL(path);
http_.begin(host_.c_str(), kFirebasePort, url.c_str(), true, kFirebaseFingerprint);
int statusCode = http_.sendRequest(method, (uint8_t*)value.c_str(), value.length());
return FirebaseResult(statusCode);
}

FirebaseResult FirebaseConnection::sendRequestGetBody(const char* method, const String& path) {
return sendRequestGetBody(method, path, "");
}

FirebaseResult FirebaseConnection::sendRequestGetBody(const char* method, const String& path, const String& value) {
FirebaseResult result = sendRequest(method, path, value);
return FirebaseResult(result.httpStatus(), http_.getString());
}

String FirebaseConnection::makeURL(const String& path) {
String url;
if (path[0] != '/') {
url = "/";
}
url += path + ".json";
if (_auth.length() > 0) {
url += "?auth=" + _auth;
if (auth_.length() > 0) {
url += "?auth=" + auth_;
}
return url;
}

int Firebase::sendRequest(const char* method, const String& path, const String& value) {
String url = makeURL(path);
_http.begin(_host.c_str(), firebasePort, url.c_str(), true, firebaseFingerprint);
int statusCode = _http.sendRequest(method, (uint8_t*)value.c_str(), value.length());
setError(method, url, statusCode);
return statusCode;
/* FirebaseResult */

FirebaseResult::FirebaseResult(int status) : status_(status) {
}

String Firebase::sendRequestGetBody(const char* method, const String& path, const String& value) {
sendRequest(method, path, value);
if (_error.code() != 0) {
return "";
}
// no _http.end() because of connection reuse.
return _http.getString();
FirebaseResult::FirebaseResult(int status, const String& response)
: status_(status), response_(response) {
}

void Firebase::setError(const char* method, const String& url, int statusCode) {
_error.reset();
if (statusCode < 0) {
_error.set(statusCode,
String(method) + " " + url + ": "
+ HTTPClient::errorToString(statusCode));
}
FirebaseResult::FirebaseResult(const FirebaseResult& other) {
status_ = other.status_;
response_ = other.response_;
}

bool Firebase::connected() {
return _http.connected();
bool FirebaseResult::isOk() const {
return status_ == HTTP_CODE_OK;
}

bool Firebase::available() {
return _http.getStreamPtr()->available();
bool FirebaseResult::isError() const {
return status_ < 0;
}

Firebase::Event Firebase::read(String& event) {
auto client = _http.getStreamPtr();
Event type;;
String typeStr = client->readStringUntil('\n').substring(7);
if (typeStr == "put") {
type = Firebase::Event::PUT;
} else if (typeStr == "patch") {
type = Firebase::Event::PATCH;
} else {
type = Firebase::Event::UNKNOWN;
}
event = client->readStringUntil('\n').substring(6);
client->readStringUntil('\n'); // consume separator
return type;
String FirebaseResult::errorMessage() const {
return HTTPClient::errorToString(status_);
}

const String& FirebaseResult::response() const {
return response_;
}
117 changes: 86 additions & 31 deletions Firebase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,53 +25,108 @@
#include <WiFiClientSecure.h>
#include <ESP8266HTTPClient.h>

// FirebaseError represents a Firebase API error with a code and a
// message.
class FirebaseError {
//TODO(edcoyne) split these into multiple files.

// Result from call to Firebase backend. ALWAYS check isError() before
// expecting any data.
class FirebaseResult {
public:
operator bool() const { return _code < 0; }
int code() const { return _code; }
const String& message() const { return _message; }
void reset() { set(0, ""); }
void set(int code, const String& message) {
_code = code;
_message = message;
FirebaseResult(int status);
FirebaseResult(int status, const String& response);
FirebaseResult(const FirebaseResult& result);

// True if there was an error completeing call.
bool isError() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked the idea to have a different error object you should check like a bool with fbase.error(), even if we introduce a separate result object maybe we could reuse that concept, ex:

auto& result = fbase.get();
if (result) {
  Serial.println(result.string());
} else {
  Serial.println(result.error());
  Serial.println(result.status());
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should keep the separate FirebaseError type, with a message() method, etc, and have API method specifics result types compose the generic error and the method specific result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

String errorMessage() const;

// True if http status code is 200(OK).
bool isOk() const;
// Message sent back from Firebase backend.
const String& response() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of exposing different result types to the user (maybe in a different PR), but I wanted to discuss that here since this is introducing a new result class.

Maybe thru read(int&), read(float&), read(String&) or int(), float(), string() on the result object?.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a good idea. So this would integrate parsing the json response into our library instead of asking the client to do it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for direct key access of leaf value, we wouldn't even need json, because we can just atoi/atof.

But for non-leaf value, we have the choice of either a/ returning the raw body and letting the user decode or b/ wrap the decoding.

As discussed in #43, b/ is not recommended by ArduinoJson and Arduino IDE has poor (read no) library dependencies management (yet), I heard it is coming soon though.


int httpStatus() const {
return status_;
}

private:
int _code = 0;
String _message = "";
int status_;
String response_;
};

// Firebase is the connection to firebase.
// Low level connection to Firebase backend, you probably want the
// Firebase class below.
class FirebaseConnection {
public:
FirebaseConnection(const String& host);
FirebaseConnection& auth(const String& auth);

const String& host() {
return host_;
}

HTTPClient& httpClient(){
return http_;
}

String makeURL(const String& path);

FirebaseResult sendRequest(const char* method, const String& path, const String& value);
FirebaseResult sendRequest(const char* method, const String& path);

FirebaseResult sendRequestGetBody(const char* method, const String& path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than having separate sendRequest* method, maybe we could make getting the body optional?

So you would do something like fbase.post().body() if you care about the body or fbase.delete() if you don't.

FirebaseResult sendRequestGetBody(const char* method, const String& path, const String& value);

private:
HTTPClient http_;
const String host_;
String auth_;
};

// Primary client to the Firebase backend.
class Firebase {
public:
Firebase(const String& host);
Firebase& auth(const String& auth);
const FirebaseError& error() const {
return _error;
}
String get(const String& path);
String push(const String& path, const String& value);
bool remove(const String& path);
bool connected();
Firebase& stream(const String& path);
bool available();

// Fetch result at "path" to a local variable. If the value is too large you will exceed
// local memory.
FirebaseResult get(const String& path);

// Add new value to list at "path", will return child name of new item.
FirebaseResult push(const String& path, const String& value);

// Deletes value at "path" from server.
FirebaseResult remove(const String& path);

private:
FirebaseConnection connection_;
};

// Listens on a stream of events from Firebase backend.
class FirebaseEventStream {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love if we could keep the stream method attached to the main Firebase class (even if the implementation is decoupled).

If we decouple the method for making request and getting result it seems that most of the stream interface could apply to regular request, and the real different would be that you can read() multiple data from a stream.

ex: available() could also be useful for regular get request to know if there is a body to read.

public:
enum Event {
UNKNOWN,
PUT,
PATCH
};

FirebaseEventStream(const String& host);
FirebaseEventStream& auth(const String& auth);

// Connect to backend and start receiving events.
FirebaseResult connect(const String& path);
// Read next event in stream.
Event read(String& event);

// True if connected to backend.
bool connected();

// True if there is an event available.
bool available();

private:
String makeURL(const String& path);
int sendRequest(const char* method, const String& path, const String& value = "");
String sendRequestGetBody(const char* method, const String& path, const String& value = "");
void setError(const char* method, const String& url, int status_code);

HTTPClient _http;
String _host;
String _auth;
FirebaseError _error;
FirebaseConnection connection_;
};

#endif // firebase_h
Loading