feat(firestore): Added multiple firestore database support #719
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
lahirumaramba
changed the title
| self._client = firestore.Client(credentials=credentials, project=project) | ||
| def __init__(self, credentials, project, database_id=None): | ||
| if database_id: | ||
| self._client = firestore.Client(credentials=credentials, project=project, database=database_id) |
There was a problem hiding this comment.
Wasn't sure the best way to do this but I included a version check if attempting to use older that v2.12.0
There was a problem hiding this comment.
I think we can update the firestore version to the latest in the next release so this check probably won't be necessary (we don't check the version in other SDKs). I understand this PR has been sitting here for a while so I will first merge this and then remove the version check in a follow up PR :)
firebase_admin/_utils.py
Outdated
| @@ -93,8 +93,10 @@ def _get_initialized_app(app): | |||
|
|
|||
|
|
|||
|
|
|||
| def get_app_service(app, name, initializer): | |||
| def get_app_service(app, options, name, initializer): | |||
There was a problem hiding this comment.
I see this is a private module, but adding a new positional argument into the middle of the list seems dangerous. Can this be an optional kwarg?
| self._client = firestore.Client(credentials=credentials, project=project, database=database_id) | ||
| else: | ||
| self._client = firestore.Client(credentials=credentials, project=project) | ||
|
|
There was a problem hiding this comment.
we should add some tests for this new functionality
There was a problem hiding this comment.
I added a test to cover when an explicit database id is given
lahirumaramba
requested review from
lahirumaramba
and removed request for
tom-andersen and
wu-hui
Check firestore ver is > 2.12.0 is using database id Added test for explicit database id
|
Any ETA on when this will be pushed? Urgently need this feature. |
| self._client = firestore.Client(credentials=credentials, project=project) | ||
| def __init__(self, credentials, project, database_id=None): | ||
| if database_id: | ||
| self._client = firestore.Client(credentials=credentials, project=project, database=database_id) |
There was a problem hiding this comment.
I think we can update the firestore version to the latest in the next release so this check probably won't be necessary (we don't check the version in other SDKs). I understand this PR has been sitting here for a while so I will first merge this and then remove the version check in a follow up PR :)
|
Hello @lahirumaramba, is there any news on this PR or is there another way to do it that have been implemented already ? Thanks 🙂 |
|
Hey folks, want to provide an update on this. We are current going through the API approval process since this alters the public API surface. While doing this we decided to go with a different implementation to fully support caching of the Firestore clients and avoids modifying the app options. With that we will be closing this PR and will complete this change in a separate PR. |
#718 Added support for multiple Firestore databases within a single app