feat: add Query.limitToLast() by schmidt-sebastian · Pull Request #151 · googleapis/java-firestore

schmidt-sebastian · 2020-03-25T19:22:24Z

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

Update Fork

codecov · 2020-03-25T19:25:11Z

Codecov Report

Merging #151 into master will increase coverage by 0.07%.
The diff coverage is 85.71%.

@@             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

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/com/google/cloud/firestore/Query.java	`90.99% <85.71%> (-0.74%)`	`100.00 <4.00> (+6.00)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ccdf21...0a5deea. Read the comment docs.

elharo · 2020-03-26T12:10:17Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

+  /**
+   * 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


. Otherwise
which exception type specifically? This might use a throws clause for that info.

Mentioned this is an IllegalStateException.

elharo · 2020-03-26T12:10:40Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

+   * <p>Results for limitToLast queries cannot be streamed via the {@link
+   * #stream(ApiStreamObserver)} API.
+   *
+   * @param limit The maximum number of items to return.


nit: the (no caps) and no period at end since this is not a complete sentence

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.

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

elharo · 2020-03-26T12:10:46Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

+   * #stream(ApiStreamObserver)} API.
+   *
+   * @param limit The maximum number of items to return.
+   * @return The created Query.


elharo · 2020-03-26T12:11:59Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

-    listenerAssertions.addedIdsIsAnyOf(emptySet(), newHashSet("doc"));
-    listenerAssertions.modifiedIdsIsAnyOf(emptySet());
-    listenerAssertions.removedIdsIsAnyOf(emptySet());
+    listenerAssertions.addedIdsIsAnyOf(emptyList(), singletonList("doc"));


This is surprising. Shouldn't it be one or the other deterministically?

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.

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.

Based on @BenWhitehead comment, I reverted this change.

wu-hui

LGTM, FWIW..

BenWhitehead

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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

elharo · 2020-03-26T22:15:24Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

+  /**
+   * 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


, otherwise --> . Otherwise

elharo · 2020-03-26T22:15:36Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

+   * 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.


will be --> is

elharo · 2020-03-26T22:18:06Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

+   * <p>Results for limitToLast queries cannot be streamed via the {@link
+   * #stream(ApiStreamObserver)} API.
+   *
+   * @param limit The maximum number of items to return.


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

schmidt-sebastian added 2 commits March 10, 2020 14:08

Merge pull request #2 from googleapis/master

fa8ddc1

Update Fork

Merge github.com:googleapis/java-firestore

79b5f51

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 25, 2020

schmidt-sebastian changed the title ~~Mrschmidt/limittolast~~ feat: add Query.limitToLast() Mar 25, 2020

feat: add Query.limitToLast()

c4d0881

schmidt-sebastian force-pushed the mrschmidt/limittolast branch from e621584 to c4d0881 Compare March 26, 2020 01:07

elharo reviewed Mar 26, 2020

View reviewed changes

Review

fe20e96

wu-hui approved these changes Mar 26, 2020

View reviewed changes

BenWhitehead reviewed Mar 26, 2020

View reviewed changes

elharo reviewed Mar 26, 2020

View reviewed changes

Review

0a5deea

BenWhitehead approved these changes Mar 27, 2020

View reviewed changes

BenWhitehead merged commit c104615 into googleapis:master Mar 27, 2020

dmitry-fa mentioned this pull request May 13, 2020

Add snippet for Query.limitToLast() GoogleCloudPlatform/java-docs-samples#2873

Closed

IlyaFaer mentioned this pull request Jun 10, 2020

firestore: implement "limit to last" feature googleapis/google-cloud-go#2419

Closed

quartzmo mentioned this pull request Aug 14, 2020

Add support for Firestore Query#limit_to_last googleapis/google-cloud-ruby#7572

Closed

Conversation

schmidt-sebastian commented Mar 25, 2020

Uh oh!

codecov bot commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

BenWhitehead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Mar 25, 2020 •

edited

Loading