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 Query.limitToLast() #151
feat: add Query.limitToLast() #151
Conversation
Codecov Report
@@ Coverage Diff @@
## master #151 +/- ##
============================================
+ Coverage 71.62% 71.69% +0.07%
- Complexity 969 975 +6
============================================
Files 62 62
Lines 5222 5257 +35
Branches 579 588 +9
============================================
+ Hits 3740 3769 +29
Misses 1302 1302
- Partials 180 186 +6
Continue to review full report at Codecov.
|
e621584
to
c4d0881
Compare
/** | ||
* Creates and returns a new Query that only returns the last matching documents. | ||
* | ||
* <p>You must specify at least one orderBy clause for limitToLast queries, otherwise an exception |
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.
. Otherwise
which exception type specifically? This might use a throws clause for that info.
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.
Mentioned this is an IllegalStateException.
* <p>Results for limitToLast queries cannot be streamed via the {@link | ||
* #stream(ApiStreamObserver)} API. | ||
* | ||
* @param limit The maximum number of items to return. |
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.
nit: the (no caps) and no period at end since this is not a complete sentence
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 left it as is for now to be consistent with the rest of the code. I will send a PR once this is merged to clean this up everywhere.
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.
Google Java practices explicitly rule out consistency with existing code as a justification for violating style guidelines in newly added code:
https://engdoc.corp.google.com/eng/doc/devguide/java/style/index.md?cl=head#s8.5.2-newly-added-code
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.
Updated
* #stream(ApiStreamObserver)} API. | ||
* | ||
* @param limit The maximum number of items to return. | ||
* @return The created Query. |
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.
ditto
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.
Done
listenerAssertions.addedIdsIsAnyOf(emptySet(), newHashSet("doc")); | ||
listenerAssertions.modifiedIdsIsAnyOf(emptySet()); | ||
listenerAssertions.removedIdsIsAnyOf(emptySet()); | ||
listenerAssertions.addedIdsIsAnyOf(emptyList(), singletonList("doc")); |
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.
This is surprising. Shouldn't it be one or the other deterministically?
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.
Yes. I fixed this instance, I think there are a couple other tests that could use tighter assertions. @BenWhitehead did go out of his way to make sure that these tests are not flaky, so I suspect some of these are intended.
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.
After thousands of iterations of these tests there is a fairly broad amount of wiggle room from the backend around event delivery and potential collection. There are a number of internal SLOs that are monitored by the backend, but no SLA related to the events being delivered in a certain amount of time. Due to these observations, I added the any-of checks in several locations to reduce the flakiness of these tests. If there were publicly available SLAs for the event listener we could tighten up the tests bounds.
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.
Based on @BenWhitehead comment, I reverted this change.
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.
LGTM, FWIW..
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.
A couple nits and one recommendation.
Let me know your decision then I can approve and merge.
google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java
Outdated
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java
Show resolved
Hide resolved
/** | ||
* Creates and returns a new Query that only returns the last matching documents. | ||
* | ||
* <p>You must specify at least one orderBy clause for limitToLast queries, otherwise an {@link |
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.
, otherwise --> . Otherwise
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.
Done
* Creates and returns a new Query that only returns the last matching documents. | ||
* | ||
* <p>You must specify at least one orderBy clause for limitToLast queries, otherwise an {@link | ||
* java.lang.IllegalStateException} will be thrown during execution. |
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.
will be --> is
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.
Done
* <p>Results for limitToLast queries cannot be streamed via the {@link | ||
* #stream(ApiStreamObserver)} API. | ||
* | ||
* @param limit The maximum number of items to return. |
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.
Google Java practices explicitly rule out consistency with existing code as a justification for violating style guidelines in newly added code:
https://engdoc.corp.google.com/eng/doc/devguide/java/style/index.md?cl=head#s8.5.2-newly-added-code
Adds limitToLast() support to Java Firestore.
Port of https://github.com/googleapis/nodejs-firestore/pull/954/files
Also changes the ITWatchTests to use Lists instead of Sets, which allows us to verify ordering