-
Notifications
You must be signed in to change notification settings - Fork 65
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
feat: add transactions #146
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Alejandro Santiago <[email protected]>
Any predictions for this merge? We are really looking forward to this feature |
Hi @evandrobubiak, sorry for allowing this to stale, I must've missed the original PR email. I'll review this now. |
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.
Looks great!
I would just like to discuss the use of recursion - if you agree that it should be removed or have good arguments to keep it.
@@ -150,6 +160,49 @@ class FirestoreGateway { | |||
.toList(); | |||
} | |||
|
|||
Future<T> runTransaction<T>( |
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.
I appreciate recursiveness as much as anyone but here it feels a bit unwarranted, especially since it exposes attempt
and maxAttemps
in a "public" method (the class isn't supposed to be called directly but still).
Since you have the transaction itself being run in a private method _runTransaction
, would it make sense to wrap the retries in a loop instead of having the function calling itself?
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.
Your arguments make a lot of sense. @jsgalarraga , could you review this?
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.
Thank you @cachapa for the review. It's been some months since I implemented the solution, so I don't remember why I didn't use a loop instead of recursion.
From a quick review at the code, I believe that the catching the aborted
error in the loop and continue with it would be the hard part. However, I will think about it and try to refactor to use a loop or come up with strong reasons on why recursion might be a better solution.
Unfortunately I won't be able to get into this next week, but I will work on it as soon as I can.
PS. @evandrobubiak feel free to also contribute with code. I appreciate you reviving this PR, but I would kindly ask to avoid posting a message a couple hours after the review just to put pressure on getting the feature out.
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.
@jsgalarraga thank you for getting back to me, and once again sorry for allowing this to go unreviewed for so long.
I appreciate you trying out a loop-based solution but if you think that doesn't work out I'm happy accepting the patch as-is.
Also, would you mind writing a few tests? |
Oh, sure! Will also work on the tests |
Hi @jsgalarraga , I was trying to implement this add transactions pr in my project for running transactions but I am facing issues in it regarding permission denied. The error that I am facing: This error hits as soon as the beginTransaction request is hit What I tried:
The reason why I can't move forward with just cloud_firestore is that it doesn't support windows and I plan to give support for windows too. It would be really helpful if you or @cachapa could help me figure out the issue |
This PR introduces Firestore transactions to the package.
Why
Transactions are a powerful feature to have in the backend. This is an effort to have a more complete feature set for a dart backend.
It has been requested by users in #63 and #126.
Inspiration
The work here has been inspired by:
Transaction
class that exposes the methods that can be performed.Implementation notes
Transaction
class, does not extend the existingReference
even though it needs some of its methods because aTransaction
does not have a predefinedpath
. The_fullPath
and_encodeMap
methods have been adapted.mutations
in aTransaction
have to be exposed for theFirestoreGateway
to have access to them and run the transactioncommit
. To safely do this, it is kept as anUnmodifiableListView
to prevent the end user from modifying it.GrpcError
withStatusCode.aborted
is thrown. In this PR, the error handling is implemented to retry the transaction up to 5 times by default (as the other firestore SDKs do).document
attribute inFirestoreGateway
had the route to the/documents
path, which is needed for most common operations in the database. However, for the transactions, the raw database route is needed, therefore this has been updated to have bothdatabase
pointing to the root anddocumentDatabase
which adds the/documents
path.Examples
To ensure transactions are working and keeping the database consistent with all the write operations performed in parallel, the following test has been run: Increase a value multiple times both with and without transactions. As we can see, when running with transactions, the numbers are increased correctly 3 times, but the number is only increased by 1 when ran without transactions.
transactions-example.mov
With transactions
Without transactions