Firestore: Adding Collection Group queries by schmidt-sebastian · Pull Request #4652 · googleapis/google-cloud-java · GitHub

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

lgtm with a bunch of nits.

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).

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.

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.

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.

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.

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

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

Done.

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.

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

Acknowledged.

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

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

I updated this class to use AutoValue.

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.

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

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

This is no longer applicable.

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

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

Done.

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.

Acknowledged.

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

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

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

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 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 🚨 critical P0 critical issue. Requires immediate fix 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

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

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

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

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

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

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

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


		QueryOptions that = (QueryOptions) o;

		if (allDescendants != that.allDescendants) {

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

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

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

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

Firestore: Adding Collection Group queries by schmidt-sebastian · Pull Request #4652 · googleapis/google-cloud-java · GitHub

Uh oh!

Firestore: Adding Collection Group queries - #4652

Firestore: Adding Collection Group queries#4652kolea2 merged 5 commits intomastergoogleapis/google-cloud-java:masterfrom mrschmidt-collectiongroupgoogleapis/google-cloud-java:mrschmidt-collectiongroupCopy head branch name to clipboard

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!

Firestore: Adding Collection Group queries#4652
kolea2 merged 5 commits into
masterfrom
mrschmidt-collectiongroup

schmidt-sebastian commented Mar 6, 2019 •
edited
Loading

codecov Bot commented Mar 6, 2019 •
edited
Loading