[realppl 5] serverTimestamp and integration tests #7353
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Firebase AI Mock Responses Check
|
Generated by 🚫 Danger |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wu-hui, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the Firestore client library by introducing a robust real-time data processing pipeline. It allows developers to define complex data transformations and aggregations that execute continuously, providing live updates as underlying data changes. The changes also expand the available expression functions for more powerful data manipulation and refine how server-generated timestamps are handled, ensuring greater control and accuracy in real-time applications.
Highlights
- Realtime Pipeline Enhancements: Introduced a new
RealtimePipelineAPI with support for real-time snapshot listeners and Kotlin Flows, enabling continuous data updates based on defined pipeline stages. - Advanced Expression Functions: Added a suite of new expression functions, including mathematical operations (absolute value, exponentiation, logarithms, square root, rounding, ceiling, floor) and string manipulations (substring), significantly expanding the query capabilities within pipelines.
- Server Timestamp Behavior Control: Implemented explicit control over server timestamp behavior (estimate, previous, none) within
RealtimePipelinesnapshots, providing more flexibility in how timestamp fields are represented in client-side data. - Improved Query-to-Pipeline Conversion: Refined the conversion of traditional Firestore queries into pipeline stages, including a new requirement for
limitToLast()queries to specify anorderBy()clause for predictable results.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: Realtime Pipelines. This allows for listening to live updates on pipeline queries. The changes are extensive, touching API definitions, core logic, evaluation engines, and adding a large suite of integration tests.
The implementation looks solid. Key points:
- The new
RealtimePipelineAPI is well-designed, with support for snapshot listeners and coroutine Flows. - Server timestamp handling (
estimate,previous,none) is correctly implemented in the evaluation logic, plumbed through newRealtimePipelineOptions. - The expression engine has been expanded with many new mathematical and string functions, and their evaluation logic correctly handles edge cases like
InfinityandNaN. - Existing tests have been improved for determinism by adding explicit sorting, and several test-only bugs have been fixed.
- The new integration tests for
RealtimePipelineare comprehensive and cover a wide range of scenarios. - Code has been refactored for better organization, such as moving
RealtimePipelineclasses to a new file and centralizingchangesFromSnapshotlogic.
I have one suggestion for improving robustness in RealtimePipeline.kt.
Comment on lines
+190
to
+215
| fun addSnapshotListener( | ||
| executor: Executor, | ||
| options: RealtimePipelineOptions, | ||
| listener: EventListener<RealtimePipelineSnapshot> | ||
| ): ListenerRegistration { | ||
| val userListener = | ||
| EventListener<ViewSnapshot> { snapshot, error -> | ||
| val realtimeSnapshot = snapshot?.let { RealtimePipelineSnapshot(it, firestore!!, options) } | ||
| listener.onEvent(realtimeSnapshot, error) | ||
| } | ||
|
|
||
| val asyncListener = AsyncEventListener(executor, userListener) | ||
|
|
||
| return firestore!!.callClient { client -> | ||
| val listener: QueryListener = | ||
| client!!.listen( | ||
| QueryOrPipeline.PipelineWrapper(this), | ||
| options.toListenOptions(), | ||
| asyncListener | ||
| ) | ||
| ListenerRegistration { | ||
| asyncListener.mute() | ||
| client!!.stopListening(listener) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The firestore property can be null for RealtimePipeline instances that are deserialized from the cache, and the client in callClient can also be null. Using the non-null assertion operator (!!) could lead to a NullPointerException if this method is ever called on such an instance or if the client is not available.
While the current code flow seems to prevent this, adding explicit checks would make the code more robust and clearly state the preconditions for this method.
fun addSnapshotListener(
executor: Executor,
options: RealtimePipelineOptions,
listener: EventListener<RealtimePipelineSnapshot>
): ListenerRegistration {
checkNotNull(firestore) {
"This RealtimePipeline instance cannot be used to listen to snapshots. It may have been deserialized from persistence."
}
val userListener =
EventListener<ViewSnapshot> { snapshot, error ->
val realtimeSnapshot = snapshot?.let { RealtimePipelineSnapshot(it, firestore, options) }
listener.onEvent(realtimeSnapshot, error)
}
val asyncListener = AsyncEventListener(executor, userListener)
return firestore.callClient { client ->
checkNotNull(client) { "FirestoreClient should not be null." }
val listener: QueryListener =
client.listen(
QueryOrPipeline.PipelineWrapper(this),
options.toListenOptions(),
asyncListener
)
ListenerRegistration {
asyncListener.mute()
client.stopListening(listener)
}
}
}
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected Products
Test Logs |
wu-hui
force-pushed
the
wuandy/RealPpl_4
branch
from
3187cd1 to
748f6cf
Compare
wu-hui
force-pushed
the
wuandy/RealPpl_5
branch
from
d013c6d to
164fbde
Compare
wu-hui
force-pushed
the
wuandy/RealPpl_5
branch
from
164fbde to
2110f36
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.