Collection Group Queries #233

mikelehen · 2019-02-05T00:30:32Z

mikelehen · 2019-02-05T00:53:26Z

/retest

var-const

LGTM with small nitpicks. I din't read through the spec test file on the assumption that it's generated from TypeScript code that was reviewed before.

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java

firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java

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

var-const · 2019-02-06T19:56:18Z

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

                  + methodName
-                  + "().");
+                  + "() must result in a valid document path, but '"
+                  + path


Optional nit: the wording of this exception is different from the one above, even though they communicate the same thing. Would it be possible to add a helper method like validateDocumentId and move the checks there? Might not be a good idea if it requires too much configurability.

So these errors are actually redundant. I could get rid of the first one, but I think it will be hit more often, and by handling it separately we can make the error message a little clearer. That said, I did go back and forth on this. I'll think about it a bit more as I do the iOS port.

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java

var-const · 2019-02-06T20:44:51Z

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,6 @@
 # Unreleased
+- [feature] You can now query across all collections in your database with a


AFAIU, it's only possible to query the most nested collection (e.g., in col1/doc/col2, the only suitable collection is col2). Is it something that should be clear without additional explanation, or should we call it out somehow?

I think this is basically a Firestore-wide subtlety that permeates throughout the API. You can write to col1/doc1/col2/doc2 without col1/doc existing... In which case there isn't a document in "col1" and so doing a collection or collection group query on "col1" won't return anything. But if it DOES exist (because you specifically wrote col1/doc1 at some point), then you can query for "doc1" by doing a collection group query on "col1"

Anyway, I'm inclined to say this is just something people need to be familiar with and not give it extra mention here in the changelog. But if you had some specific callout in mind, let me know. I'm happy to tweak / expand the text here (or in the doc comment on the method).

mikelehen

Thanks for the speedy review!

mikelehen · 2019-02-07T00:29:15Z

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,6 @@
 # Unreleased
+- [feature] You can now query across all collections in your database with a


I think this is basically a Firestore-wide subtlety that permeates throughout the API. You can write to col1/doc1/col2/doc2 without col1/doc existing... In which case there isn't a document in "col1" and so doing a collection or collection group query on "col1" won't return anything. But if it DOES exist (because you specifically wrote col1/doc1 at some point), then you can query for "doc1" by doing a collection group query on "col1"

Anyway, I'm inclined to say this is just something people need to be familiar with and not give it extra mention here in the changelog. But if you had some specific callout in mind, let me know. I'm happy to tweak / expand the text here (or in the doc comment on the method).

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java

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

mikelehen · 2019-02-07T01:37:41Z

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

                  + methodName
-                  + "().");
+                  + "() must result in a valid document path, but '"
+                  + path


So these errors are actually redundant. I could get rid of the first one, but I think it will be hit more often, and by handling it separately we can make the error message a little clearer. That said, I did go back and forth on this. I'll think about it a bit more as I do the iOS port.

firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java

mikelehen · 2019-02-07T17:06:22Z

/retest

mikelehen · 2019-02-07T17:21:55Z

/retest

google-oss-bot · 2019-02-14T18:51:49Z