Firestore: Adding Collection Group queries #4652

schmidt-sebastian · 2019-03-06T18:27:17Z

CollectionGroup queries add support for queries to Firestore that query documents by collection ID rather than by collection path. With this feature, users can now query for documents in collections that share a name, no matter where these collection are located.

This the server port of firebase/firebase-android-sdk#233, meaning there is less input validation (since the backend does most of it for us) and there is no local logic to execute these queries.

Note: This PR is blocked on a backend release.

codecov · 2019-03-06T19:00:04Z

Codecov Report

Merging #4652 into master will increase coverage by <.01%.
The diff coverage is 93.46%.

@@             Coverage Diff              @@
##             master    #4652      +/-   ##
============================================
+ Coverage     49.67%   49.68%   +<.01%     
- Complexity    22300    22308       +8     
============================================
  Files          2238     2238              
  Lines        221344   221337       -7     
  Branches      24079    24069      -10     
============================================
+ Hits         109949   109967      +18     
+ Misses       102937   102924      -13     
+ Partials       8458     8446      -12

Impacted Files	Coverage Δ	Complexity Δ
...java/com/google/cloud/firestore/FirestoreImpl.java	`83.04% <100%> (+0.83%)`	`22 <4> (+3)`	⬆️
...rc/main/java/com/google/cloud/firestore/Watch.java	`91.81% <100%> (ø)`	`57 <0> (ø)`	⬇️
...om/google/cloud/firestore/CollectionReference.java	`48.78% <75%> (+1.28%)`	`10 <6> (+1)`	⬆️
...rc/main/java/com/google/cloud/firestore/Query.java	`92.53% <94.07%> (+4.8%)`	`90 <35> (+4)`	⬆️
...src/main/java/com/google/cloud/ServiceOptions.java	`41.4% <0%> (+0.88%)`	`26% <0%> (ø)`	⬇️
...om/google/cloud/logging/MonitoredResourceUtil.java	`50.92% <0%> (+0.92%)`	`8% <0%> (ø)`	⬇️
.../java/com/google/cloud/testing/CommandWrapper.java	`96.96% <0%> (+6.06%)`	`13% <0%> (ø)`	⬇️

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 d931fac...b873df9. Read the comment docs.

rsgowman

lgtm with a bunch of nits.

rsgowman · 2019-03-06T18:42:00Z

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


      QueryOptions that = (QueryOptions) o;

+      if (allDescendants != that.allDescendants) {


nit: Is there reasoning for this order? If not, then consider reordering to match the declaration order. (If certain variables are more likely to differ, that might be a legit reason to prioritize checking those ones in the presence of a tight loop.)

This is auto-generated code by IntelliJ (and not by AutoValue).

rsgowman · 2019-03-06T18:50:24Z

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

-      fieldFilters = new ArrayList<>();
-      fieldOrders = new ArrayList<>();
-      fieldProjections = new ArrayList<>();
+  protected static class QueryOptions {


Unless there's a reason not to, you should probably make this class final.

http://go/java-practices/final#classes

I updated this class to use AutoValue.

rsgowman · 2019-03-06T18:52:56Z

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


+      if (allDescendants != that.allDescendants) {
+        return false;
+      }


Consider refactoring to look like this: http://go/java-practices/equals#impl

Specifically:
a) optionally drop the this==o check unless you think it's useful.
b) use instanceof instead of getClass. (Less relevant if you mark this class final.)
c) Instead of using 3 lines per variable, just use ~1 per variable, i.e.:

return (foo == that.foo && bar == that.bar && ...);

Entirely optional.

As above, this is auto-generated and now replaced by AutoValue.

rsgowman · 2019-03-06T19:03:51Z

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

-      fieldFilters = new ArrayList<>();
-      fieldOrders = new ArrayList<>();
-      fieldProjections = new ArrayList<>();
+  protected static class QueryOptions {


Can we just use go/autovalue here? (public link: https://github.com/google/auto/blob/master/value/userguide/index.md)

If we can, it'd cut this down by quite a lot.

I changed this to use AutoValue as suggested. I personally don't consider this an improvement, as it adds a lot of indirection. QueryOptions was treated by me like a value struct, but its immutability was merely by design. With AutoValue and with the migration to ImmutableList, this is now enforced in code.

rsgowman · 2019-03-06T19:09:33Z

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

-      fieldProjections = new ArrayList<>();
+  protected static class QueryOptions {
+
+    ResourcePath parentPath;


These instance variables should all be final too. http://go/java-practices/final#fields.

(Or at least final where possible. I'm haven't checked to see if these are reassigned or not. Hopefully mostly not.)

They were reassigned as part of my "builder-light" pattern:

Options newOptions = new Options(oldOptions); newOptions.foo = bar;

This is no longer applicable.

rsgowman · 2019-03-06T19:15:46Z

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

  Query(FirestoreImpl firestore, ResourcePath path) {
-    this(firestore, path, new QueryOptions());
+    this.firestore = firestore;
+    this.options = new QueryOptions(path.getParent(), path.getId());


Seems reasonable, but consider delegating to the private ctor anyway, i.e.:

Query(firestore, path) { this(firestore, new QueryOptoins(path.getParent(), path.getId()); }

rsgowman · 2019-03-06T19:16:10Z

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


    this.firestore = firestore;
-    this.path = path;
+    this.options = queryOptions;


Similarly, consider delegating to the primary ctor. Optional.

Also note that this ctor (and the other one above) could also be rephrased as static factory methods. (Which would allow you to rename them to something other than 'Query'. Whether that's useful or not is up to you.)

I made this change, which is now possible thanks to the QueryOptions.Builder. Without it, I had no way to initialize allDescendants.

I opted not to make these factory methods since CollectionReference inherits from Query and I like the consistency of having two constructors rather than one constructor and one factory method.

rsgowman · 2019-03-06T21:39:11Z

...cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java

+      Query query = firestoreMock.collectionGroup("foo/bar");
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertEquals(


This is fine. Personally, I find it slightly over-specified. (I'd just remove the assertEquals entirely.) Feel free to leave as is.

Acknowledged.

rsgowman · 2019-03-06T21:40:37Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java


    assertEquals(begin(), requests.get(0));
-    assertEquals(query(TRANSACTION_ID), requests.get(1));
+    assertEquals(query(TRANSACTION_ID, false), requests.get(1));


I've forgotten what the second parameter is (and am too lazy to look it up.) Consider one of the options here:
http://go/java-practices/boolean-parameters

Added /* allDescendants= */ as a comment. Thanks

schmidt-sebastian

I addressed your feedback and bit the bullet by moving to AutoValue.

schmidt-sebastian · 2019-03-07T00:29:06Z

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

-      fieldFilters = new ArrayList<>();
-      fieldOrders = new ArrayList<>();
-      fieldProjections = new ArrayList<>();
+  protected static class QueryOptions {


I updated this class to use AutoValue.

schmidt-sebastian · 2019-03-07T00:30:53Z

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

-      fieldFilters = new ArrayList<>();
-      fieldOrders = new ArrayList<>();
-      fieldProjections = new ArrayList<>();
+  protected static class QueryOptions {


I changed this to use AutoValue as suggested. I personally don't consider this an improvement, as it adds a lot of indirection. QueryOptions was treated by me like a value struct, but its immutability was merely by design. With AutoValue and with the migration to ImmutableList, this is now enforced in code.

schmidt-sebastian · 2019-03-07T00:31:57Z

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

-      fieldProjections = new ArrayList<>();
+  protected static class QueryOptions {
+
+    ResourcePath parentPath;


They were reassigned as part of my "builder-light" pattern:

Options newOptions = new Options(oldOptions); newOptions.foo = bar;

This is no longer applicable.

schmidt-sebastian · 2019-03-07T00:32:23Z

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


      QueryOptions that = (QueryOptions) o;

+      if (allDescendants != that.allDescendants) {


This is auto-generated code by IntelliJ (and not by AutoValue).

schmidt-sebastian · 2019-03-07T00:32:42Z

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


+      if (allDescendants != that.allDescendants) {
+        return false;
+      }


As above, this is auto-generated and now replaced by AutoValue.

schmidt-sebastian · 2019-03-07T00:33:01Z

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

  Query(FirestoreImpl firestore, ResourcePath path) {
-    this(firestore, path, new QueryOptions());
+    this.firestore = firestore;
+    this.options = new QueryOptions(path.getParent(), path.getId());


schmidt-sebastian · 2019-03-07T00:34:34Z

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


    this.firestore = firestore;
-    this.path = path;
+    this.options = queryOptions;


I made this change, which is now possible thanks to the QueryOptions.Builder. Without it, I had no way to initialize allDescendants.

I opted not to make these factory methods since CollectionReference inherits from Query and I like the consistency of having two constructors rather than one constructor and one factory method.

schmidt-sebastian · 2019-03-07T00:34:56Z

...cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java

+      Query query = firestoreMock.collectionGroup("foo/bar");
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertEquals(


Acknowledged.

schmidt-sebastian · 2019-03-07T00:36:10Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java


    assertEquals(begin(), requests.get(0));
-    assertEquals(query(TRANSACTION_ID), requests.get(1));
+    assertEquals(query(TRANSACTION_ID, false), requests.get(1));


Added /* allDescendants= */ as a comment. Thanks

schmidt-sebastian · 2019-04-26T21:26:39Z

This is no longer blocked and needs sign off from the Yoshi team.

...ts/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java

chingor13

Just a question on whether or not to add a getter method. Otherwise, LGTM.

chingor13 · 2019-04-30T22:33:13Z

google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java

        QueryTarget.newBuilder()
            .setStructuredQuery(query.buildQuery())
-            .setParent(query.getResourcePath().getParent().getName())
+            .setParent(query.options.getParentPath().getName())


Any particular reason for direct field access vs. a getter? We provide getFirestore() on query, but not getOptions().

options is purely an internal field that is only used inside the client. We could still use a getOptions() here, but I felt that making it package-private was safe enough.

Firestore: Adding Collection Group queries

a22b227

schmidt-sebastian requested a review from a team as a code owner March 6, 2019 18:27

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

schmidt-sebastian added the status: blocked Resolving the issue is dependent on other work. label Mar 6, 2019

schmidt-sebastian changed the title ~~Firestore: Adding Collection Group queries~~ Firestore: Adding Collection Group queries (DO NOT MERGE) Mar 6, 2019

rsgowman approved these changes Mar 6, 2019

View reviewed changes

schmidt-sebastian added 3 commits March 6, 2019 16:28

Using AutoValue

10d379c

Adding comment

98aabe0

Add append() helper

a6d948b

schmidt-sebastian commented Mar 7, 2019

View reviewed changes

schmidt-sebastian mentioned this pull request Mar 11, 2019

feature: Adding CollectionGroup queries googleapis/nodejs-firestore#578

Merged

schmidt-sebastian force-pushed the mrschmidt-collectiongroup branch from a9946bb to a6d948b Compare March 14, 2019 01:45

sduskis added the api: bigquery Issues related to the BigQuery API. label Mar 19, 2019

sduskis added api: firestore Issues related to the Firestore API. and removed api: bigquery Issues related to the BigQuery API. labels Apr 11, 2019

Merge

b873df9

crwilcox mentioned this pull request Apr 19, 2019

Firestore: add support for CollectionGroup queries. googleapis/google-cloud-python#7758

Merged

schmidt-sebastian requested a review from pongad April 26, 2019 21:26

schmidt-sebastian assigned pongad Apr 26, 2019

schmidt-sebastian removed the status: blocked Resolving the issue is dependent on other work. label Apr 26, 2019

yoshi-automation added the 🚨 This issue needs some love. label Apr 26, 2019

schmidt-sebastian changed the title ~~Firestore: Adding Collection Group queries (DO NOT MERGE)~~ Firestore: Adding Collection Group queries Apr 27, 2019

kolea2 suggested changes Apr 30, 2019

View reviewed changes

...ts/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java Show resolved Hide resolved

BenWhitehead mentioned this pull request Apr 30, 2019

Add snippet for Collection Group Query GoogleCloudPlatform/java-docs-samples#1399

Merged

chingor13 approved these changes Apr 30, 2019

View reviewed changes

kolea2 approved these changes May 1, 2019

View reviewed changes

kolea2 merged commit 7f6dc84 into master May 1, 2019

sduskis deleted the mrschmidt-collectiongroup branch May 1, 2019 17:39

JesseLovelace mentioned this pull request May 1, 2019

Release v0.90.0 #5047

Merged

quartzmo mentioned this pull request May 24, 2019

Firestore Collection Group queries googleapis/google-cloud-ruby#3416

Merged


		QueryOptions that = (QueryOptions) o;

		if (allDescendants != that.allDescendants) {

Firestore: Adding Collection Group queries #4652

Firestore: Adding Collection Group queries #4652

Uh oh!

Conversation

schmidt-sebastian commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

rsgowman left a comment

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!

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!

schmidt-sebastian left a comment

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!

schmidt-sebastian commented Apr 26, 2019

Uh oh!

Uh oh!

chingor13 left a comment

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

schmidt-sebastian commented Mar 6, 2019 •

edited

Loading

codecov bot commented Mar 6, 2019 •

edited

Loading