-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Feature FirestoreItemKeyedDatasource #1599
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
Changes from all commits
075227c
177d9b2
53ace3b
36a291f
75129af
41b98b0
3b67104
cae4d35
c343620
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,263 @@ | ||
package com.firebase.ui.firestore.paging | ||
|
||
|
||
import android.arch.paging.DataSource | ||
import android.arch.paging.ItemKeyedDataSource | ||
import android.arch.paging.PagedList | ||
import com.google.android.gms.tasks.Task | ||
import com.google.android.gms.tasks.TaskCompletionSource | ||
import com.google.android.gms.tasks.Tasks | ||
import com.google.firebase.firestore.* | ||
import com.google.firebase.firestore.Source.CACHE | ||
import java.util.concurrent.atomic.AtomicBoolean | ||
|
||
/** | ||
* Implementation of [ItemKeyedDataSource] for Firebase Firestore. | ||
* | ||
* [errorCallback] is optional and if not provided the [defaultErrorCallback] will be used. | ||
* | ||
* Note: If user provide [errorCallback] he should control data source invalidation manually. | ||
* | ||
* @property directQuery The direct query. | ||
* @property reverseQuery The reverse query. | ||
* @property errorCallback The error callback. | ||
*/ | ||
@Suppress("NOTHING_TO_INLINE") | ||
class FirestoreItemKeyedDataSource( | ||
private val directQuery: Query, | ||
private val reverseQuery: Query, | ||
private val errorCallback: ErrorCallback? | ||
) : ItemKeyedDataSource<DocumentSnapshot, DocumentSnapshot>() { | ||
|
||
private val registrations = mutableListOf<ListenerRegistration>() | ||
|
||
/** | ||
* Factory for [FirestoreItemKeyedDataSource]. | ||
* | ||
* @property queryFactory The query factory. | ||
* @property errorCallback The error callback. | ||
*/ | ||
class Factory @JvmOverloads constructor( | ||
private val queryFactory: QueryFactory, | ||
private val errorCallback: ErrorCallback? = null | ||
) : DataSource.Factory<DocumentSnapshot, DocumentSnapshot>() { | ||
|
||
/** | ||
* Creates and returns [FirestoreItemKeyedDataSource]. | ||
* | ||
* @return The created data source. | ||
*/ | ||
override fun create(): DataSource<DocumentSnapshot, DocumentSnapshot> = | ||
FirestoreItemKeyedDataSource(queryFactory.create(), queryFactory.createReverse(), errorCallback) | ||
} | ||
|
||
override fun loadInitial( | ||
params: LoadInitialParams<DocumentSnapshot>, | ||
callback: LoadInitialCallback<DocumentSnapshot> | ||
) { | ||
val lastSeenDocument = params.requestedInitialKey | ||
val requestedLoadSize = params.requestedLoadSize.toLong() | ||
|
||
if (lastSeenDocument == null) { // Initial data load could be asynchronous. | ||
loadFromBeginning(requestedLoadSize, callback) | ||
} else { // Stale data refresh. Should be synchronous to prevent RecyclerView items disappearing and loss of current position. | ||
loadAround(lastSeenDocument, requestedLoadSize, callback).await() | ||
} | ||
} | ||
|
||
/** | ||
* Tries to load half of [loadSize] documents before [document] and half of [loadSize] documents starting at provided [document]. | ||
* | ||
* @param document The document to load around. | ||
* @param loadSize Requested number of items to load. | ||
* | ||
* @return A Task that will be resolved with the results of the Query. | ||
*/ | ||
private fun loadAround( | ||
document: DocumentSnapshot, | ||
loadSize: Long, | ||
callback: LoadInitialCallback<DocumentSnapshot> | ||
) = tryGetDocumentPrior(document, loadSize / 2).continueWithTask { previousTask -> | ||
loadStartAt(previousTask.result!!, loadSize, callback) | ||
} | ||
|
||
/** | ||
* Returns document before [document] maximum by [preloadSize] or [document] if there is no documents before. | ||
* | ||
* @param document The document to load before. | ||
* @param preloadSize Requested number of items to preload. | ||
* | ||
* @return A Task that will be resolved with the results of the Query. | ||
*/ | ||
private fun tryGetDocumentPrior( | ||
document: DocumentSnapshot, | ||
preloadSize: Long | ||
): Task<DocumentSnapshot> = getItemsBefore(document, preloadSize).continueWith { task -> | ||
if (task.isSuccessful) { | ||
task.result!!.documents.lastOrNull() ?: document | ||
} else { | ||
document | ||
} | ||
} | ||
|
||
private fun getItemsBefore( | ||
document: DocumentSnapshot, | ||
loadSize: Long | ||
) = queryEndAt(document, loadSize).get(CACHE) | ||
|
||
private fun loadStartAt( | ||
document: DocumentSnapshot, | ||
loadSize: Long, | ||
callback: LoadInitialCallback<DocumentSnapshot> | ||
) = PageLoader(callback).also { pageLoader -> | ||
addPageLoader(queryStartAt(document, loadSize), pageLoader) | ||
}.task | ||
|
||
private fun loadFromBeginning( | ||
loadSize: Long, | ||
callback: LoadInitialCallback<DocumentSnapshot> | ||
) { | ||
addPageLoader( | ||
queryStartFromBeginning(loadSize), | ||
PageLoader(callback) | ||
) | ||
} | ||
|
||
override fun loadAfter( | ||
params: LoadParams<DocumentSnapshot>, | ||
callback: LoadCallback<DocumentSnapshot> | ||
) { | ||
addPageLoader( | ||
queryStartAfter(params.key, params.requestedLoadSize.toLong()), | ||
PageLoader(callback) | ||
) | ||
} | ||
|
||
override fun loadBefore( | ||
params: LoadParams<DocumentSnapshot>, | ||
callback: LoadCallback<DocumentSnapshot> | ||
) { | ||
addPageLoader( | ||
queryEndBefore(params.key, params.requestedLoadSize.toLong()), | ||
PageLoader(callback.reversedResults()) | ||
) | ||
} | ||
|
||
override fun getKey(item: DocumentSnapshot) = item | ||
|
||
override fun invalidate() { | ||
super.invalidate() | ||
removeAllPageLoaders() | ||
} | ||
|
||
private fun removeAllPageLoaders() { | ||
registrations.forEach { it.remove() } | ||
registrations.clear() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe one of the reasons this hasn't been done yet is because of how difficult it is to properly subscribe and unsub from these listeners. Can you explain a bit more how and when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I just forget to unsubscribe. I wrote it previously. By design of Paging library to refresh data in UI you should invalidate DS.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think I understand what's going on now. 👍 I'm a little concerned about cost. Correct me if I'm wrong, but here's what I think is going on: when a page is being loaded, it adds a snapshot listener which submits the data the first time it gets triggered. Then, on the second trigger, it invalidates the entire data set and removes all the listeners. This causes the adapter to requery everything from scratch and start the process anew. Is that correct? If so, this feature would be extremely expensive to users. They wouldn't be paying for the diff—every single update would cost them the entire query. Coming from an official Firebase library, that seems a little abusive. @samtstern WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you right that it requery data on every change, but not all the data only recent pageSize*3 items by default or can be specified by PagedList.Config.initialLoadSizeHint. What is more when you finish #1388 there will be PagedList.Config.maxSize which limits number of item currently kept in memory and which will be involved in old with new data comparison. What about requerying - this 3 resent pages most likely will be loaded from cache if I understand correctly how firebase works. upd: Here is quote from firestore pricing page:
According to it only if some new documents will be queried after data change detection there will be extra charges. As I understand even if we requery whole data from the beginning there will be no extra charge during immideate requery. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After some testing I come to conclusion that I was wrong - every requery leeds to extra charges. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you right, but problem is that exactly same means with the same limit and start/end conditions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assume that the query result is identical
Is this still true? @samtstern There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's true, identical queries issued within 30m of each other don't double-charge unless there are new results. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if we have 1 new document in the query result, for example, the first query return 3 documents, the second query return the same 3 documents + 1 new document. In this case, how many documents read will be charged? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case 4 total reads. 3 for the initial result. 1 for the update later. |
||
} | ||
|
||
private fun addPageLoader( | ||
query: Query, | ||
pageLoader: PageLoader | ||
) { | ||
registrations.add(query.addSnapshotListener(pageLoader)) | ||
} | ||
|
||
private fun queryStartFromBeginning(loadSize: Long) = directQuery.limit(loadSize) | ||
|
||
private fun queryStartAt(document: DocumentSnapshot, loadSize: Long) = directQuery.startAt(document).limit(loadSize) | ||
|
||
private fun queryStartAfter(document: DocumentSnapshot, loadSize: Long) = directQuery.startAfter(document).limit(loadSize) | ||
|
||
private fun queryEndAt(document: DocumentSnapshot, loadSize: Long) = reverseQuery.startAt(document).limit(loadSize) | ||
|
||
private fun queryEndBefore(document: DocumentSnapshot, loadSize: Long) = reverseQuery.startAfter(document).limit(loadSize) | ||
|
||
/** | ||
* Implementation of [EventListener]. | ||
* | ||
* Listen for loading of the requested page. | ||
* | ||
* The first received result is treated as page data. The subsequent results treated as data change and invoke | ||
* data source invalidation. | ||
* | ||
* @property callback The callback to pass data to [PagedList]. | ||
*/ | ||
private inner class PageLoader( | ||
private val callback: LoadCallback<DocumentSnapshot> | ||
) : EventListener<QuerySnapshot> { | ||
private val isWaitingForPage = AtomicBoolean(true) | ||
private val taskCompletionSource = TaskCompletionSource<MutableList<DocumentSnapshot>>() | ||
|
||
/** | ||
* A [Task] that will be resolved when this page loader loads page or fails. | ||
* Used to load initial data synchronously on stale data refresh. | ||
*/ | ||
val task get() = taskCompletionSource.task | ||
|
||
override fun onEvent(snapshot: QuerySnapshot?, exception: FirebaseFirestoreException?) { | ||
if (isWaitingForPage()) { | ||
|
||
snapshot?.documents?.also { result -> | ||
submit(result) | ||
} ?: submit(exception!!) | ||
|
||
} else { | ||
invalidate() | ||
} | ||
} | ||
|
||
private fun submit(result: MutableList<DocumentSnapshot>) { | ||
callback.onResult(result) | ||
taskCompletionSource.setResult(result) | ||
} | ||
|
||
private fun submit(exception: FirebaseFirestoreException) { | ||
notifyError(exception) | ||
taskCompletionSource.setException(exception) | ||
} | ||
|
||
private inline fun isWaitingForPage() = isWaitingForPage.compareAndSet(true, false) | ||
} | ||
|
||
private fun notifyError(exception: FirebaseFirestoreException) { | ||
getErrorCallback().onError(exception) | ||
} | ||
|
||
private inline fun getErrorCallback() = errorCallback ?: defaultErrorCallback | ||
|
||
/** | ||
* Callback to listen to data loading errors. | ||
*/ | ||
interface ErrorCallback { | ||
fun onError(exception: FirebaseFirestoreException) | ||
} | ||
|
||
/** | ||
* Wraps [LoadCallback] with new one that reverses the results list. | ||
*/ | ||
private fun LoadCallback<DocumentSnapshot>.reversedResults() = object : LoadCallback<DocumentSnapshot>() { | ||
override fun onResult(data: MutableList<DocumentSnapshot>) { | ||
[email protected](data.reversed()) | ||
} | ||
} | ||
|
||
/** | ||
* Waits for [Task] to resolve ignoring whether it succeeded or failed. | ||
* Used to load initial page synchronously on stale data refresh. | ||
*/ | ||
private inline fun <TResult> Task<TResult>.await() { | ||
try { | ||
Tasks.await(this) | ||
} catch (e: Throwable) { } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What sorts of errors are you ignoring here and why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It waits for completion of the Task that tries to get key befor one that is provided on DS recreation after invalidation(that happens after any document change, creadion or delition, if this change is in the range of existing listener thar previously load page). If it fails DS just start load new portion of documents beginning from provided key. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry! It's not true right now. Described in the previous comment is how it was working at first. Now it wait for whole initial data load finished. And error handling occurred in ResultsListener.onResults method. This code only needed because if we don't wait for this load synchronously Page library erase Recyclerview by submitting empty results. |
||
} | ||
|
||
/** | ||
* Used if user do not provide [errorCallback]. | ||
* Simply invalidates this data source if loading error occurred. | ||
*/ | ||
private val defaultErrorCallback = object : ErrorCallback { | ||
override fun onError(exception: FirebaseFirestoreException) { | ||
invalidate() | ||
} | ||
} | ||
} |
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.
Whoa there! @samtstern Is FUI accepting Kotlin now? From the FAQ:

Typically, libs will be written entirely in Kotlin (not postfixed like this) and only if it offers some sort of syntactic benefit that isn't available in Java (like coroutines or multi-platform stuff, see https://github.com/square/okio). Here, I have trouble seeing what's worth the 1MB and cognitive overhead of ensuring our APIs stay idiomatic in both Java and Kotlin.
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.
Yeah I am pushing the Kotlin question to the end. If the PR is worth having we can translate it back to Java at the last step.
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.
SGTM 👍