Add 'toFlow()' extensions to DocumentSnapshot and Query #1252
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
Hi @michgauz. Thanks for your PR. I'm waiting for a firebase member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
/ok-to-test |
|
@michgauz thanks for sending this! I saw your discussion about this on Android Study Group. Just to set some expectation, all public API changes go through our internal API review process, so if @vkryachko or one of our other Kotlin enthusiasts else decides to push this forward with this it could take some time for them to write up the internal document and schedule the review meeting. |
|
The public api surface has changed for the subproject firebase-firestore_ktx: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
| if (value != null && value.exists()) { | ||
| offer(value) | ||
| } else if (error != null) { | ||
| Logger.warn("DocumentReference:flow", error.message) |
There was a problem hiding this comment.
It's not clear to me what the best course of action here would be:
- i.e. just logging the error is not general enough, as developers may want to react to them and don't want them to be swallowed
- otoh
close()ing the flow is not consistent with current Firestore's behavior, as it looks like listeners are not "de-registered" upon exceptions
cc @schmidt-sebastian any thoughts?
There was a problem hiding this comment.
If the listen stream encounters an error we do currently close the stream. For example, if you have data-dependent rules in effect on the server like a group membership test and a write is performed that removes you from the group, this error is the one that will tell you that permission is denied and that should terminate the stream.
The flow should have the same behavior so this should propagate the error, not swallow it.
Note that ListenerRegistrations are unaffected by this. We track the listeners internally, and the ListenerRegistration is just a user-owned handle by which to request closing the stream. The stream can close out of band and ListenerRegistration handles this.
There was a problem hiding this comment.
Thanks Gil,
@michgauz then we definitely want to call close(error) here.
Also, as @samtstern mentioned, we will have to have an internal API review for this before it can be merged.
There was a problem hiding this comment.
Thanks for your review.
I've just taken into account your feedbacks and close the channel when receiving an error 👍
| import com.google.firebase.firestore.QueryDocumentSnapshot | ||
| import com.google.firebase.firestore.QuerySnapshot | ||
| import com.google.firebase.firestore.FirebaseFirestoreSettings | ||
| import com.google.firebase.firestore.* |
There was a problem hiding this comment.
We generally avoid collapsing imports.
There was a problem hiding this comment.
I have reverted the collapsing imports
There was a problem hiding this comment.
@schmidt-sebastian could that configuration be commited in the repo (.idea folder or something else?)
There was a problem hiding this comment.
nit: this is not fixed apparently
There was a problem hiding this comment.
I think it got fixed initially, but reverted in a follow-up commit.
There was a problem hiding this comment.
Oh gosh, it's back... I just removed it again
There was a problem hiding this comment.
Yea, I'll do a follow up PR to commit the codeStyle so that AS stops doing this automagically. I "just" need to figure out how the .gitignore syntax works...
|
@wu-hui Would you be willing to draft the API proposal document for this change? |
michgauz
force-pushed
the
add-to-flow-extension
branch
from
88f6d68 to
5549d17
Compare
|
The public api surface has changed for the subproject firebase-firestore_ktx: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
1 similar comment
|
The public api surface has changed for the subproject firebase-firestore_ktx: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
Codecov Report
@@ Coverage Diff @@
## master #1252 +/- ##
============================================
- Coverage 29.22% 21.21% -8.01%
============================================
Files 203 1 -202
Lines 8333 33 -8300
Branches 787 4 -783
============================================
- Hits 2435 7 -2428
+ Misses 5714 26 -5688
+ Partials 184 0 -184
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
I have just been aware of this:Kotlin/kotlinx.coroutines#974 Long story short, channel.offer might throw and we might need to catch any exception thrown. |
| * Attach a snapshotListener to a DocumentReference and use it as a coroutine flow | ||
| */ | ||
| fun DocumentReference.toFlow() = callbackFlow { | ||
| val listener = addSnapshotListener { value, error -> |
There was a problem hiding this comment.
We should allow the user to specify the metadataChangesparameter:
fun DocumentReference.toFlow(metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE) = callbackFlow {
val listener = addSnapshotListener(metadataChanges) { value, error ->I'm less sure about the executor parameter. This would typically be handled by coroutine Dispatchers but maybe there's a use case ?
There was a problem hiding this comment.
I have just added the MetadataChanges parameter.
Thanks 👍
| * Attach a snapshotListener to a Query and use it as a coroutine flow | ||
| */ | ||
| fun Query.toFlow() = callbackFlow { | ||
| val listener = addSnapshotListener { value, error -> |
There was a problem hiding this comment.
Same as above, we should allow to pass the metadataChanges parameter
+1 The comments on that Issue make a very compelling case for dealing with the exception. |
|
@thatfiredev I think it's all good now. That PR uses coroutines The only thing that fails now is smoke tests with: I'm not sure how to fix this? |
There was a problem hiding this comment.
@martinbonnin Thanks for rebasing!
Don't worry about the smoke-tests, we'll look into it when we leave a final review.
[ @vkryachko I wonder why the experimental GH Actions are not running here. Are they not enabled for ktx modules? Or do we need to merge the main branch into this one? (instead of a rebase) ]
Next Steps:
Last week I brought back the internal proposal for this and I'm waiting for a final review. It usually takes 2-3 weeks. I'll come back with an update once it's approved.
It's been 2 years since this was first opened but we're finally close to getting it merged 🤞
| * @param metadataChanges controls metadata-only changes. Default: [MetadataChanges.EXCLUDE] | ||
| * @param bufferCapacity the buffer capacity as in [Flow.buffer] or null to not buffer at all | ||
| */ | ||
| fun DocumentReference.toFlow( |
There was a problem hiding this comment.
We are considering calling this DocumentReference.documentSnapshot instead, to better describe the return type:
docRef.documentSnapshot.collect { snapshot ->
// handle returned document
}I would like to hear y'all thoughts.
There was a problem hiding this comment.
Not sure what the best practices are. It'd be worth investigating what other libs are doing. In all cases, we need to account for metadataChanges and bufferCapacity parameters so it can't really be a val. Maybe this instead?
docRef.snapshots().collect { snapshot ->
// do something with snapshot
}And if customization is required, this becomes:
docRef.snapshots(metadataChanges, bufferCapacity).collect { snapshot ->
// do something with snapshot
}
There was a problem hiding this comment.
I'm with @martinbonnin here. It should be snapshots() or documentSnapshots(). Plural, because a Flow usually returns multiple values. The document in documentSnapshots() is redundant because it should be clear that a DocumentReference returns DocumentSnapshot. So snapshots() is my favorite candidate.
There was a problem hiding this comment.
I agree it should be plural. 👍
Sorry for the val confusion, that was a mistake on my side, I forgot to add parenthesis.
| * @param metadataChanges controls metadata-only changes. Default: [MetadataChanges.EXCLUDE] | ||
| * @param bufferCapacity the buffer capacity as in [Flow.buffer] or null to not buffer at all | ||
| */ | ||
| fun Query.toFlow( |
There was a problem hiding this comment.
Same comment as above: we're considering calling this Query.querySnapshot.
| fun Query.toFlow( | ||
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE, | ||
| bufferCapacity: Int? = Channel.CONFLATED | ||
| ): Flow<QuerySnapshot?> { |
There was a problem hiding this comment.
| ): Flow<QuerySnapshot?> { | |
| ): Flow<QuerySnapshot> { |
Does it really need to be nullable? At the moment, trySend() is only called with non-null snapshots.
There was a problem hiding this comment.
Good point 👍 . Doc says
It's guaranteed that exactly one of value or error will be non-{@code null}.
So yay for making everything non-nullable, I'll push the change in a bit.
|
@thatfiredev I think it had to do with the following limitation https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks , I just approved the workflows. |
| @@ -144,7 +144,7 @@ dependencies { | |||
| implementation "io.grpc:grpc-protobuf-lite:$grpcVersion" | |||
| implementation "io.grpc:grpc-okhttp:$grpcVersion" | |||
| implementation "io.grpc:grpc-android:$grpcVersion" | |||
| implementation 'com.google.android.gms:play-services-basement:18.0.0' | |||
| api 'com.google.android.gms:play-services-basement:18.0.0' | |||
There was a problem hiding this comment.
Sorry, I didn't understand the reason for this change and how it relates to the Flows. Are you hoping to expose FirebaseException?
| * @param metadataChanges controls metadata-only changes. Default: [MetadataChanges.EXCLUDE] | ||
| * @param bufferCapacity the buffer capacity as in [Flow.buffer] or null to not buffer at all | ||
| */ | ||
| fun DocumentReference.toFlow( |
There was a problem hiding this comment.
| fun DocumentReference.toFlow( | |
| fun DocumentReference.documentSnapshots( |
There was a problem hiding this comment.
I went with just snapshots() in this commit. As @svenjacobs said, documentSnapshots() is more verbose and the "document" part is already in the "documentRef". Let me know if you want me to change it or feel free to amend.
| * @param metadataChanges controls metadata-only changes. Default: [MetadataChanges.EXCLUDE] | ||
| * @param bufferCapacity the buffer capacity as in [Flow.buffer] or null to not buffer at all | ||
| */ | ||
| fun Query.toFlow( |
There was a problem hiding this comment.
| fun Query.toFlow( | |
| fun Query.querySnapshots( |
There was a problem hiding this comment.
Don't forget to run ./gradlew :firebase-firestore:ktx:generateApiTxtFile after making these changes. That's the gradle task that generates our api.txt file.
firebase-firestore/ktx/src/main/kotlin/com/google/firebase/firestore/ktx/Firestore.kt
Outdated
Show resolved
Hide resolved
firebase-firestore/ktx/src/main/kotlin/com/google/firebase/firestore/ktx/Firestore.kt
Outdated
Show resolved
Hide resolved
| @@ -14,18 +14,28 @@ | |||
|
|
|||
| package com.google.firebase.firestore.ktx | |||
|
|
|||
| import android.support.multidex.BuildConfig | |||
There was a problem hiding this comment.
I understand you needed this change to build it locally, but its not the correct BuildConfig class. If I'm not wrong we rely on a BuildConfig class generated at build time under the same package name (com.google.firebase.firestore.ktx) which is why we don't have an import for it.
Can you please remove this import line?
There was a problem hiding this comment.
Good catch, that's done ✅ Turns out it wasn't even required for building.
…estore/ktx/Firestore.kt Co-authored-by: Rosário Pereira Fernandes <[email protected]>
…estore/ktx/Firestore.kt Co-authored-by: Rosário Pereira Fernandes <[email protected]>
There was a problem hiding this comment.
@martinbonnin Thank you very much for being so patient during this review process.
The code changes look good to me. We'll just need a final review from an engineer + one from a tech writer and then we'll be ready to merge this.
@vkryachko can you please review this and approve the CI workflows?
| if (exception != null) { | ||
| cancel(message = "Error getting DocumentReference snapshot", cause = exception) | ||
| } else if (snapshot != null) { | ||
| trySend(snapshot) |
There was a problem hiding this comment.
shouldn't it be trySendBlocking instead? here and below
There was a problem hiding this comment.
Along with buffer this should be safe and not block the UI thread. Please see my comment here.
There was a problem hiding this comment.
see my response re buffer, as for the comment you linked:
@martinbonnin Why not just use trySend instead of risking blocking the thread, which is likely to be the main one?
imo the fact that Firestore delivers events on the UI thread is an unfortunate(but necessary) historic artifact as it optimized for devEx before coroutines were available and switching to the UI thread was non-trivial, and most devs want to render UI when data changes so UI thread was chosen. However Firestore provides overloads for addSnapshotListener that takes an executor. Now in the coroutine and flow world, devs have explicit control of what thread they are collecting on, so we can afford to deliver events to the flow on any thread we like.
So I think it's a safer choice to be as unopinionated as possible and use a safe executor + trySendBlocking, i.e. AsyncTask.THREAD_POOL_EXECUTOR or com.google.firebase.firestore.util.Executors.BACKGROUND_EXECUTOR(which is more limited).
This will make our code safe by default, i.e. we will not "drop" events with CONFLATED, and given direct control to developers. wdyt?
There was a problem hiding this comment.
I've pushed a change to use BACKGROUND_EXECUTOR.
2 questions though:
- Isn't this the equivalent of
buffer(UNLIMITED)in practice? It's not using the coroutinesbuffer()but I think it will enqueueRunnables in the executors until the consumer can consume them? So all in all it's also doing a choice for the user. - Is there any executor that could avoid a thread switch (immediate maybe?) ? I'm not familiar with the Firestore internals but if the code is running in a background thread already, feels like there's no need to dispatch?
There was a problem hiding this comment.
- Yeah, I guess you're right, it may be suboptimal in this case. so direct executor sounds more appealing.
- That's a good question, on the one hand I like the idea of using direct executor but I am not sure where the execution happen in that case. grpc io pool maybe? what is the risk of starving those threads in that case? it may not actually be an issue since the developer have full control and can avoid congestion of that pool.
Would love to hear thoughts from the firestore folks on this.
There was a problem hiding this comment.
I chatted with Firestore folks offline, given that Firestore is single-threaded internally, they really don't want to run arbitrary user code on their internal thread, so BACKGROUND_EXECUTOR is preferred.
@martinbonnin thanks for accommodating all the changes
| } | ||
|
|
||
| return if (bufferCapacity != null) { | ||
| flow.buffer(bufferCapacity) |
There was a problem hiding this comment.
Do we need to have an opinion on this or should we just let the developer do their own buffering?
There was a problem hiding this comment.
Please see my comment here and previous comments. buffer is essential for back-pressure handling.
There was a problem hiding this comment.
While it's not unreasonable to do it, I just don't think firestore itself is in a position to choose a default buffering strategy and instead it should be the responsibility of the developer
i.e. I am not convinced that CONFLATED is a good default, what if the developer can't afford to lose any events and wants to get them all even if it means congesting the producer. Point being we don't know the use case, so we should not be opinionated about buffering and let the developer explicitly decide by using buffer().
imo it's safer to just add kdoc that recommends using buffering if needed.
There was a problem hiding this comment.
I'm fine with this if it is documented as you suggested.
|
@michgauz: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| * @param metadataChanges controls metadata-only changes. Default: [MetadataChanges.EXCLUDE] | ||
| */ | ||
| fun DocumentReference.snapshots( | ||
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE, |
There was a problem hiding this comment.
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE, | |
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE |
There was a problem hiding this comment.
Yup, sorry, just realized that. It's fixed in 692114c.
There was a problem hiding this comment.
Also interestingly this seemed to be a lint-only issue as Kotlin 1.4 is perfectly happy with the trailing coma
There was a problem hiding this comment.
yeah, I think our version of ktlint is pretty ancient, we are planning to upgrade it eventually
There was a problem hiding this comment.
@vkryachko I think all comments have been addressed. Can you please approve the pending CI workflows and review the PR once again?
| * @param metadataChanges controls metadata-only changes. Default: [MetadataChanges.EXCLUDE] | ||
| */ | ||
| fun Query.snapshots( | ||
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE, |
There was a problem hiding this comment.
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE, | |
| metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE |
|
Thanks all for your contribution to this feature! |
|
Amazing! Thanks to all contributors, especially @martinbonnin who did all the hard work in the end 👏🏼 |
|
Thank you very much @michgauz @martinbonnin @svenjacobs @LouisCAD for all the work done here! ❤️ I can't promise any dates, but this is going to be released on the next version of |
|
someone shud update Flow docs on android as they reference fbase https://developer.android.com/kotlin/flow#callback |
|
This feature has been released in firestore-ktx:24.3.0. 🎉 Thanks for the heads up @ColtonIdle :) I'll get that page updated. [internal bug: b/244705771] |
This PR intends to satisfy this Feature Request :
#1251