-
Notifications
You must be signed in to change notification settings - Fork 18
Support for multiple firestore clients #89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
A big thanks for the PR! I'm really sorry to say we're pretty busy for the next month, so going to frankly say we're most likely not able to have a deeper look into the PR within the next month. |
joakimnordling
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I finally manage to get some time to start looking through this PR.
First of all a big thanks for taking the time to get into this! And sorry for the delay!
I think there's some things missing and some things where I've likely not managed to express my idea very clearly in the suggestion I made originally (in #86) or then we might have had just a bit of different visions. I'll try to list those things below.
- My idea was that we'd replace the
CONFIGURATIONSdictionary inconfigurations.pywith simply one instance of theConfigurationclass, by doing something similar toconfiguration = Configuration()and then use theget_***methods instead of directly using the dictionary. I think this would hide the actual way we store the data and especially theadd()method would be helping out a fair bit with making the configs easily either by providing the values for constructing the clients or then allowing you to pass in the pre-configured clients. - My idea was also that each model class would keep the info about which configuration it should be using, by referring to it by it's name, for example by using
__db_config__ = "billing"you'd tell the class to use the named configuration calledbillingand it could then be fetched from the config with something likeconfiguration.get_client("billing")where thatbillingwould be looked up from the__db_config__value of the class. This means you wouldn't need to pass in the name of the config to each of the methods that interact with the DB (save,reload,delete,findetc), which I think would sooner or later lead to bugs when you sometime accidentally forget to pass in the name (imagine reading aUserfrom one DB/collection and when updating it saving it to another one by accident). In the use cases I have in mind I think you'd most of the time want all your models of the same kind to be in the same database + collection, not scattered around in different databases or collections. If you still do need to have let's sayUser's in two different databases/collections, I think using subclasses with different__db_config__values would be a nicer solution and ensure you always load and save the same object from/to the same collection and database. Of course in some highly dymanic case where you'd have a lot of databases and collections, I can see that subclassing might not be an option; for example if you'd have dozens of customers and a software that would need to storeUserentries for each of those into different databases and you dynamically onboard new customers and thus create new databases on the fly. If you have a such need, I think firedantic has likely been more or less impossible to use so far. I was about to write that if there's a need for something like that, then I think adding theconfigparameter to the save/find/delete etc would be OK and that then the default value should not be set to(default), rather toNoneand if it'sNone, the value from__db_config__should be used. But after a second thought I'm not sure that would either be a good idea, simply because if you have such dynamic use, then giving each connection a name might not be that easy, or it could lead to a lot of connections being created etc. If you have some kind of such need, I'd value if you could share some level of insight into the actual need. - You mentioned you had issues with your changes being wiped out by running the tests the way recommended in the repo. I believe you've been developing your changes in the
_syncfolder primarily, as there's a lot of changes in themodel.pythere that are not present in the_asyncversion. The way the repo is set up and intended to be used is so that all changes are done in the_asyncfolder hierarchy (or then outside the_async/_sync) folders. There's some tooling in place that actually generates the_synccontent from the_asyncversion, with a fair bit of find and replace operations. This is described in the About sync and async versions of library section of the README. - This PR also seems to be missing all the things to make the multiple connections work together with the index/TTL policy creation, which will require admin clients.
- There seems to also be a fair bit of odd spacing (as you mentioned) that should likely be taken care of by the pre-commit hooks (which I guess you didn't want to run to not wipe out all the changes in the
_syncfolder, in which you should not do any manual changes). - Going to be honest and say I didn't really yet look into the changes in the README or tests due to all the issues mentioned above; I think it makes sense to first clear up the issues and only after that check those.
All in all, I'm sorry to say that it looks like there's a fair bit of work left still to get this finished. And this is a quite complex and big change to this library. I actually also had to spend quite a long time to read my (maybe too long) original suggestion to recap it etc. How do we proceed from this point forward? Do you think you have still time and energy to continue on this? I'll also need to check a bit with my colleagues how much time I could put on following up this or if I could offer to help out with some part of it or something such. But let's get back to that when I know a bit more about your status.
This pull request intends to resolve issue #86.
I have followed the discussion there and created what I believe to be a nice solution based on Option 3.
Primary things to note about this solution:
configure()method remains in tact.multiple_clients_examplesdirectory, and can be removed before merge.Other small things:
poetry run invoke testmy current changes were rolled back to the current firedantic version. I suppose this is because poetry is locked toversion = "0.11.0"in the pyproject.toml and so I used pytest instead.op.EQand"==". All work as expected.Now for the examples...
Example of client creation:
Under the hood of config.create():
configuration.pyfor more details) Created suggestedConfigurationclass for added support of multiple configurations/clients.configuremethod, it will be saved there as(default).For saving/finding entries:
A simplified example for the find() method:
Note here, the multi client method is using the (default) config, as no config was specified.