ioxiocom
Added support for Firestore transactions.
This PR includes support for Firebase transactions. Specifically, the following methods now take an optional transaction argument.
Model.delete(transaction=transaction)Model.find_one(transaction=transaction)Model.find(transaction=transaction)Model.get_by_doc_id(transaction=transaction)Model.get_by_id(transaction=transaction)Model.reload(transaction=transaction)Model.save(transaction=transaction)SubModel.get_by_id(transaction=transaction)
I've included a new section in the README with an example of using async transactions and there is a unit test that covers the same case as the README.
Let me know if you have any questions and thanks for your consideration!
Hi @lukwam, thanks for your contribution! Just want to inform you that we're pretty busy in the upcoming weeks and the review process might be a bit long. But we will definitely find time for that!
Hi @lukwam, thanks for your contribution! Just want to inform you that we're pretty busy in the upcoming weeks and the review process might be a bit long. But we will definitely find time for that!
No problem. I'm installing it from git+https://github.com/lukwam/firedantic.git@transactions#egg=firedantic for now.
Yeah, sorry. I wanted to push up some changes and reply to some of your comments but I didn't get it fully working yet. I still have more to do, when I have time, but I think this gives you the ability to review the responses to many of the comments that I did resolve while I continue to complete the rest of the pr and get the tests working.
@lukwam, we actually realized we would in one of our internal projects, where we use firedantic, see some benefit of using transactions (not required, but would be nice). And I think it wouldn't require too much work to finish this, so I got an OK to spend the time necessary to complete this in order to be able to use it in that project. But I don't either want to step on your toes with it in case you want to complete it. Thus I'd want to check that would you be OK with it, if I'd continue from this step to complete it? I would of course keep the commit history etc, so it'll be visible that you've done most of it.
I won't be offended if you push this through. I am excited about continuing to contribute to this project, but I don't have time at the moment to wrap up this PR so you can merge it. If you are able to do it, I would be grateful. I have a dire need for transactions in my current project, so however it gets done I will be happy about it.
The other thing I want to work on when I have some cycles is to add support for multiple firestore databases. Currently, the single client configuration makes that impossible. I also have a project that I want to move from sync to async and I want to be able to do it one collection at a time, and so being able to have some models use an async client and some use a sync client would help with that.
Thanks for reaching out to me! 🤝
I won't be offended if you push this through. I am excited about continuing to contribute to this project, but I don't have time at the moment to wrap up this PR so you can merge it. If you are able to do it, I would be grateful. I have a dire need for transactions in my current project, so however it gets done I will be happy about it.
The other thing I want to work on when I have some cycles is to add support for multiple firestore databases. Currently, the single client configuration makes that impossible. I also have a project that I want to move from sync to async and I want to be able to do it one collection at a time, and so being able to have some models use an async client and some use a sync client would help with that.
Thanks for reaching out to me! 🤝
Great, I'll try to get it done pretty soon, actually planning to get started on it later today.
When it comes to supporting multiple databases, I need to admit I've not looked into it basically at all; it was a feature added pretty recently. When firedantic was originally created it wasn't supported. And we actually ended up using collection name prefixes for different pieces of software that stored data in the same database and that has worked pretty well. But indeed it would be nice to have support for multiple databases. Would be nice to hear that would you want to be able to configure the database then per model or how.
When it comes to using some models with sync and some with async, I don't think there should really be any issue; should be a matter just of subclassing the models from the right models. I could also mention that I've had a need to use some async models in sync code (in practice CLI tools) where I just made a simple @async_to_sync decorator that wraps the function and runs it with asyncio.run().
When looking deeper into the codebase, I realized I was incorrect... Due to the way the configuration is stored, it is indeed not possible (at least easily/conveniently) use both async and sync models at the same time, because you have either a Client or an AsyncClient configured. Sorry about that mistake. Some sort of refactoring of the configuration would make sense. Not sure exactly how yet. And I also encountered a bit of an issue with typing when making the wrapper for getting a new transaction due to the fact that the config can either have a Client or an AsyncClient configured. Unless I find some other way than what I'm currently working on, I won't be 100% happy with it, but I don't on the other hand right now want to also refactor the configuration. But I think you could open an issue here on GitHub about refactoring the configuration so that we could support both multiple DBs and at the same time sync/async models in projects using the library.