Implementing a Database API for Python Admin SDK #31
Conversation
|
The API of this module is still under review, but I think we can go ahead and start reviewing the implementation. I can update the PR if and when any changes are required as a result of the API review process. |
firebase_admin/__init__.py
Outdated
| def get_token(self): | ||
| """Returns an OAuth2 bearer token. | ||
|
|
||
| This method may return a cached token. But it handles cache invalidation, and therefore |
There was a problem hiding this comment.
You indentation here is inconsistent with the rest of the file (shift left by three spaces).
There was a problem hiding this comment.
Fixed indentation.
| skewed_expiry = self._token.expiry - datetime.timedelta(seconds=_CLOCK_SKEW_SECONDS) | ||
| return _clock() < skewed_expiry | ||
|
|
||
| def _get_service(self, name, initializer): |
There was a problem hiding this comment.
What's a service, and why would you want to perform this kind of registration?
There was a problem hiding this comment.
A service is something we expose from the SDK (e.g. auth, database). By attaching each service to an App instance, we can gracefully terminate/stop them when the App is deleted. Here's the equivalent implementation from Java SDK: https://github.com/firebase/firebase-admin-java/blob/master/src/main/java/com/google/firebase/internal/FirebaseService.java
I've added docstrings to further explain this in the source.
firebase_admin/__init__.py
Outdated
| with self._lock: | ||
| for service in self._services: | ||
| if hasattr(service, 'close'): | ||
| service.close() |
There was a problem hiding this comment.
It's possible for this to match non-method attributes and then fail with a TypeError when calling them. Depending on whether or not these are user-supplied services you may want to check if close is callable.
There was a problem hiding this comment.
It looks like callable() is not available in Python 3.0 and 3.1. They dropped it and added it back in 3.2: https://docs.python.org/3/library/functions.html#callable
So I implemented this extra check by looking for the __call__ property on the function itself.
Also found a bug in the logic while testing it. I've fixed it and updated the tests to check for that.
firebase_admin/db.py
Outdated
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Firebase Database module. |
There was a problem hiding this comment.
In other repos when referring to the product it's "Firebase Realtime Database" or "Realtime Database". "Firebase Database" is not a thing and is a term we avoid because of potential confusion.
Note that when referring to a user's project-specific storage area "database" is fine.
firebase_admin/db.py
Outdated
|
|
||
| """Firebase Database module. | ||
|
|
||
| This module contains functions and classes that facilitate interacting with the Firebase database. |
There was a problem hiding this comment.
s/Firebase database/Firebase Realtime Database/
firebase_admin/db.py
Outdated
| return _Client('https://{0}'.format(parsed.netloc), _OAuth(app), requests.Session()) | ||
|
|
||
| def request(self, method, urlpath, **kwargs): | ||
| return self._do_request(method, urlpath, **kwargs).json() |
There was a problem hiding this comment.
Some comments about what these do would be helpful, especially for what are valid values of method and what kinds of keyword arguments are taken here.
| """ | ||
| cert_path = _get_cert_path(request) | ||
| with open(cert_path) as cert: | ||
| project_id = json.load(cert).get('project_id') |
There was a problem hiding this comment.
Is this really how this works? Isn't project_id the Google Cloud Project ID? Or are those the same as the realtime database name?
Aren't there are group of realtime databases that have different URLs than their project IDs?
There was a problem hiding this comment.
It seems the project_id field in the service account key file we download from Firebase console is same as the realtime database name of the project. We have been using this convention in other 2 Admin SDKs too for testing. I'll try to get it confirmed from the database server team.
There was a problem hiding this comment.
There are legacy projects that came over from pre-cloud Firebase where these values do not match. In new projects they match.
There was a problem hiding this comment.
Interesting. I think what we have here is good enough for integration tests. It seems it should work fine for any project we create today or in the future.
| assert ref.get_value() == value | ||
| assert ref.get_priority() == 2 | ||
|
|
||
| def test_update_children(self, testref): |
There was a problem hiding this comment.
You might consider a test for update_children that updates an existing value, preserving attributes not in the value you pass.
tests/test_db.py
Outdated
| firebase_admin.delete_app(app) | ||
| with pytest.raises(ValueError): | ||
| db.get_reference() | ||
|
|
tests/test_db.py
Outdated
| assert isinstance(ordered, list) | ||
| assert ordered == expected | ||
|
|
||
| @pytest.mark.parametrize('value', [None, False, True, 0, 1, "foo"]) |
|
Made the suggested changes. Sending back for a quick sanity check. |
This PR implements a database API for the Admin SDK, based on the Firebase Database REST API. Related to #28
Summary of changes:
get_token()method toAppclass. This can be used in the database module (or any other module) to obtain an OAuth2 token.dbmodule with the single methodget_reference(), which returns adb.Reference.db.Referenceanddb.Query, which enable running queries and updates against the Firebase database._servicescollection in theAppclass. Each service module (e.g.auth,db) get registered with the app as a service. This enables gracefully terminating services when anAppis deleted.utilsmodule, where it can be reused easily.integration/directory. We can continue to flesh it out in the future.What's supported?
What's not supported and other limitations?