Skip to content

Add 'toFlow()' extensions to DocumentSnapshot and Query #1252

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

Merged
merged 23 commits into from
Aug 12, 2022
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
af0f469
Add toFlow extensions to DocumentSnapshot and Query
michgauz Feb 18, 2020
931e73f
Add toFlow extensions to DocumentSnapshot and Query
michgauz Feb 19, 2020
57fbe74
Update API Txt file
michgauz Feb 19, 2020
c87ea63
Implement 'com.google.android.gms:play-services-base' to retrieve Fir…
michgauz Feb 19, 2020
fe0916c
Add MetadataChanges optional parameter
michgauz Feb 25, 2020
15bb8b9
Wrap 'offer()' in runCatching
michgauz Feb 26, 2020
fcfb4b1
Use 'flow' and 'Channel' instead of unstable 'callbackFlow'
michgauz Jun 22, 2020
ad9db4d
pull the version from @svenjacobs
martinbonnin Jun 2, 2022
4d7237d
add "bufferCapacity" as a parameter
martinbonnin Jun 2, 2022
de32290
remove wildcard imports
martinbonnin Jun 2, 2022
cccf86f
use cancel(message, cause)
martinbonnin Jun 6, 2022
c91731a
make compile
martinbonnin Aug 4, 2022
a3d04ac
update public API
martinbonnin Aug 4, 2022
31d1553
fix lint
martinbonnin Aug 4, 2022
2e51a63
return non nullable snapshots
martinbonnin Aug 4, 2022
288ae9a
toFlow -> snapshots
martinbonnin Aug 9, 2022
bcb69e5
remove wrong BuildConfig
martinbonnin Aug 9, 2022
f6e1aeb
add play-services-basement to ktx
martinbonnin Aug 10, 2022
fbd4864
Update firebase-firestore/ktx/src/main/kotlin/com/google/firebase/fir…
martinbonnin Aug 10, 2022
31d3b61
Update firebase-firestore/ktx/src/main/kotlin/com/google/firebase/fir…
martinbonnin Aug 10, 2022
8cd52cd
let the user decide the buffering
martinbonnin Aug 10, 2022
253fbca
remove wildcard import
martinbonnin Aug 11, 2022
692114c
make lint happy
martinbonnin Aug 11, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion firebase-firestore/firebase-firestore.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is moved to api because FirebaseFirestoreException exposes its supertype FirebaseException in public API

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't understand the reason for this change and how it relates to the Flows. Are you hoping to expose FirebaseException?

Copy link
Contributor

@martinbonnin martinbonnin Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The firebase-firestore module exposes FirebaseException publicly from EventListener:

public interface EventListener<T> {

  /**
   * {@code onEvent} will be called with the new value or the error if an error occurred. It's
   * guaranteed that exactly one of value or error will be non-{@code null}.
   *
   * @param value The value of the event. {@code null} if there was an error.
   * @param error The error if there was error. {@code null} otherwise.
   */
  void onEvent(@Nullable T value, @Nullable FirebaseFirestoreException error);
}

Without this, I'm getting this error:

Screenshot 2022-08-09 at 20 18 52

We could also add implementation 'com.google.android.gms:play-services-basement:18.0.0' to the :firebase-firestore:ktx module but I believe the proposed change above is better as it will fix it for all consumers, even the non-ktx ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinbonnin Ah, I see. Thanks for explaining.
I would prefer adding that implementation to firebase-firestore-ktx instead of exposing it on the base module.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in f6e1aeb

implementation 'com.google.android.gms:play-services-tasks:18.0.1'
implementation 'com.google.android.gms:play-services-base:18.0.1'

Expand Down
2 changes: 2 additions & 0 deletions firebase-firestore/ktx/api.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ package com.google.firebase.firestore.ktx {
method @Nullable public static inline <reified T> T getField(@NonNull com.google.firebase.firestore.DocumentSnapshot, @NonNull com.google.firebase.firestore.FieldPath fieldPath);
method @Nullable public static inline <reified T> T getField(@NonNull com.google.firebase.firestore.DocumentSnapshot, @NonNull com.google.firebase.firestore.FieldPath fieldPath, @NonNull com.google.firebase.firestore.DocumentSnapshot.ServerTimestampBehavior serverTimestampBehavior);
method @NonNull public static com.google.firebase.firestore.FirebaseFirestore getFirestore(@NonNull com.google.firebase.ktx.Firebase);
method @NonNull public static kotlinx.coroutines.flow.Flow<com.google.firebase.firestore.DocumentSnapshot> toFlow(@NonNull com.google.firebase.firestore.DocumentReference, @NonNull com.google.firebase.firestore.MetadataChanges metadataChanges = com.google.firebase.firestore.MetadataChanges.EXCLUDE, @Nullable Integer bufferCapacity = -1);
method @NonNull public static kotlinx.coroutines.flow.Flow<com.google.firebase.firestore.QuerySnapshot> toFlow(@NonNull com.google.firebase.firestore.Query, @NonNull com.google.firebase.firestore.MetadataChanges metadataChanges = com.google.firebase.firestore.MetadataChanges.EXCLUDE, @Nullable Integer bufferCapacity = -1);
method @Nullable public static inline <reified T> T toObject(@NonNull com.google.firebase.firestore.DocumentSnapshot);
method @Nullable public static inline <reified T> T toObject(@NonNull com.google.firebase.firestore.DocumentSnapshot, @NonNull com.google.firebase.firestore.DocumentSnapshot.ServerTimestampBehavior serverTimestampBehavior);
method @NonNull public static inline <reified T> T toObject(@NonNull com.google.firebase.firestore.QueryDocumentSnapshot);
Expand Down
1 change: 1 addition & 0 deletions firebase-firestore/ktx/ktx.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ dependencies {
implementation project(':firebase-common:ktx')
implementation project(':firebase-firestore')
implementation 'androidx.annotation:annotation:1.1.0'
implementation 'org.jetbrains.kotlinx:kotlinx-coroutines-core:1.5.2'
testImplementation project(':firebase-database-collection')
testImplementation 'org.mockito:mockito-core:2.25.0'
testImplementation 'com.fasterxml.jackson.core:jackson-databind:2.9.8'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,28 @@

package com.google.firebase.firestore.ktx

import android.support.multidex.BuildConfig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, that's done ✅ Turns out it wasn't even required for building.

import androidx.annotation.Keep
import com.google.firebase.FirebaseApp
import com.google.firebase.components.Component
import com.google.firebase.components.ComponentRegistrar
import com.google.firebase.firestore.DocumentReference
import com.google.firebase.firestore.DocumentSnapshot
import com.google.firebase.firestore.FieldPath
import com.google.firebase.firestore.FirebaseFirestore
import com.google.firebase.firestore.FirebaseFirestoreSettings
import com.google.firebase.firestore.MetadataChanges
import com.google.firebase.firestore.Query
import com.google.firebase.firestore.QueryDocumentSnapshot
import com.google.firebase.firestore.QuerySnapshot
import com.google.firebase.ktx.Firebase
import com.google.firebase.platforminfo.LibraryVersionComponent
import kotlinx.coroutines.cancel
import kotlinx.coroutines.channels.Channel
import kotlinx.coroutines.channels.awaitClose
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.buffer
import kotlinx.coroutines.flow.callbackFlow

/** Returns the [FirebaseFirestore] instance of the default [FirebaseApp]. */
val Firebase.firestore: FirebaseFirestore
Expand Down Expand Up @@ -162,3 +172,67 @@ class FirebaseFirestoreKtxRegistrar : ComponentRegistrar {
override fun getComponents(): List<Component<*>> =
listOf(LibraryVersionComponent.create(LIBRARY_NAME, BuildConfig.VERSION_NAME))
}

/**
* Transforms a [DocumentReference] into a coroutine [Flow]
*
* **Backpressure handling**: by default this method conflates items. If the consumer isn't fast enough,
* it might miss some values but is always guaranteed to get the latest value. Use [bufferCapacity]
* to change that behaviour
*
* @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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it should be plural. 👍
Sorry for the val confusion, that was a mistake on my side, I forgot to add parenthesis.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fun DocumentReference.toFlow(
fun DocumentReference.documentSnapshots(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE,
metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, sorry, just realized that. It's fixed in 692114c.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also interestingly this seemed to be a lint-only issue as Kotlin 1.4 is perfectly happy with the trailing coma

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think our version of ktlint is pretty ancient, we are planning to upgrade it eventually

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vkryachko I think all comments have been addressed. Can you please approve the pending CI workflows and review the PR once again?

bufferCapacity: Int? = Channel.CONFLATED
): Flow<DocumentSnapshot> {
val flow = callbackFlow {
val registration = addSnapshotListener(metadataChanges) { snapshot, exception ->
if (exception != null) {
cancel(message = "Error getting DocumentReference snapshot", cause = exception)
} else if (snapshot != null) {
trySend(snapshot)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be trySendBlocking instead? here and below

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with buffer this should be safe and not block the UI thread. Please see my comment here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

cc @wu-hui @ehsannas

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

Copy link
Contributor

@martinbonnin martinbonnin Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a change to use BACKGROUND_EXECUTOR.

2 questions though:

  1. Isn't this the equivalent of buffer(UNLIMITED) in practice? It's not using the coroutines buffer() but I think it will enqueue Runnables in the executors until the consumer can consume them? So all in all it's also doing a choice for the user.
  2. 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yeah, I guess you're right, it may be suboptimal in this case. so direct executor sounds more appealing.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good 👍

}
}
awaitClose { registration.remove() }
}

return if (bufferCapacity != null) {
flow.buffer(bufferCapacity)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to have an opinion on this or should we just let the developer do their own buffering?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment here and previous comments. buffer is essential for back-pressure handling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@svenjacobs svenjacobs Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this if it is documented as you suggested.

} else {
flow
}
}

/**
* Transforms a [Query] into a coroutine [Flow]
*
* **Backpressure handling**: by default this method conflates items. If the consumer isn't fast enough,
* it might miss some values but is always guaranteed to get the latest value. Use [bufferCapacity]
* to change that behaviour
*
* @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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above: we're considering calling this Query.querySnapshot.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as here. I'm for snapshots().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fun Query.toFlow(
fun Query.querySnapshots(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE,
metadataChanges: MetadataChanges = MetadataChanges.EXCLUDE

bufferCapacity: Int? = Channel.CONFLATED
): Flow<QuerySnapshot> {
val flow = callbackFlow {
val registration = addSnapshotListener(metadataChanges) { snapshot, exception ->
if (exception != null) {
cancel(message = "Error getting Query snapshot", cause = exception)
} else if (snapshot != null) {
trySend(snapshot)
}
}
awaitClose { registration.remove() }
}

return if (bufferCapacity != null) {
flow.buffer(bufferCapacity)
} else {
flow
}
}