firestore: allow StartAt/EndBefore on direct children at any depth
In firestore, it is legal to construct a query with a StartAt or EndBefore
filter. These accept document snapshots (snaps) as input parameters. If a snap
is passed, that snap must be a direct child of the location being queried.
However, the current implementation has a bug that only allows children of
documents at depth=1 to be specified: an extra /documents/ gets added to the
collectionPath of each collection: e.g. querying /c1/d1/c2 with StartAt at
/c1/d1/c2/d2 resulted in an incorrect path
/database/some-db/c1/d1/c2/documents/d2. It should be
/database/some-db/documents/c1/d1/c2/d2.
So, despite being a valid StartAt value, the user experiences an error. This CL
fixes that issue.
Finally, some documentation was updated to make all this behavior more clear.
Fixes #1225
Change-Id: I7f467701d5ccf4b5a77bb3c4b1960e6ef173dfb3
Reviewed-on: https://code-review.googlesource.com/c/35790
Reviewed-by: Eno Compton <enocom@google.com>
diff --git a/firestore/client_test.go b/firestore/client_test.go
index 92ff877..332c1b2 100644
--- a/firestore/client_test.go
+++ b/firestore/client_test.go
@@ -35,19 +35,26 @@
wantc1 := &CollectionRef{
c: testClient,
parentPath: db,
+ selfPath: "X",
Parent: nil,
ID: "X",
Path: "projects/projectID/databases/(default)/documents/X",
- Query: Query{c: testClient, collectionID: "X", parentPath: db},
+ Query: Query{
+ c: testClient,
+ collectionID: "X",
+ path: "projects/projectID/databases/(default)/documents/X",
+ parentPath: db,
+ },
}
if !testEqual(coll1, wantc1) {
t.Fatalf("got\n%+v\nwant\n%+v", coll1, wantc1)
}
doc1 := testClient.Doc("X/a")
wantd1 := &DocumentRef{
- Parent: coll1,
- ID: "a",
- Path: "projects/projectID/databases/(default)/documents/X/a",
+ Parent: coll1,
+ ID: "a",
+ Path: "projects/projectID/databases/(default)/documents/X/a",
+ shortPath: "X/a",
}
if !testEqual(doc1, wantd1) {
@@ -58,19 +65,26 @@
wantc2 := &CollectionRef{
c: testClient,
parentPath: parentPath,
+ selfPath: "X/a/Y",
Parent: doc1,
ID: "Y",
Path: "projects/projectID/databases/(default)/documents/X/a/Y",
- Query: Query{c: testClient, collectionID: "Y", parentPath: parentPath},
+ Query: Query{
+ c: testClient,
+ collectionID: "Y",
+ parentPath: parentPath,
+ path: "projects/projectID/databases/(default)/documents/X/a/Y",
+ },
}
if !testEqual(coll2, wantc2) {
t.Fatalf("\ngot %+v\nwant %+v", coll2, wantc2)
}
doc2 := testClient.Doc("X/a/Y/b")
wantd2 := &DocumentRef{
- Parent: coll2,
- ID: "b",
- Path: "projects/projectID/databases/(default)/documents/X/a/Y/b",
+ Parent: coll2,
+ ID: "b",
+ Path: "projects/projectID/databases/(default)/documents/X/a/Y/b",
+ shortPath: "X/a/Y/b",
}
if !testEqual(doc2, wantd2) {
t.Fatalf("got %+v, want %+v", doc2, wantd2)
diff --git a/firestore/collref.go b/firestore/collref.go
index ff445d1..4f9f365 100644
--- a/firestore/collref.go
+++ b/firestore/collref.go
@@ -26,11 +26,17 @@
type CollectionRef struct {
c *Client
- // Typically Parent.Path, or c.path if Parent is nil.
- // May be different if this CollectionRef was created from a stored reference
- // to a different project/DB.
+ // The full resource path of the collection's parent. Typically Parent.Path,
+ // or c.path if Parent is nil. May be different if this CollectionRef was
+ // created from a stored reference to a different project/DB.
+ //
+ // For example, "projects/P/databases/D/documents/coll-1/doc-1".
parentPath string
+ // The shorter resource path of the collection. A collection "coll-2" in
+ // document "doc-1" in collection "coll-1" would be: "coll-1/doc-1/coll-2".
+ selfPath string
+
// Parent is the document of which this collection is a part. It is
// nil for top-level collections.
Parent *DocumentRef
@@ -50,19 +56,32 @@
c: c,
ID: id,
parentPath: dbPath,
+ selfPath: id,
Path: dbPath + "/documents/" + id,
- Query: Query{c: c, collectionID: id, parentPath: dbPath},
+ Query: Query{
+ c: c,
+ collectionID: id,
+ path: dbPath + "/documents/" + id,
+ parentPath: dbPath,
+ },
}
}
func newCollRefWithParent(c *Client, parent *DocumentRef, id string) *CollectionRef {
+ selfPath := parent.shortPath + "/" + id
return &CollectionRef{
c: c,
Parent: parent,
ID: id,
parentPath: parent.Path,
+ selfPath: selfPath,
Path: parent.Path + "/" + id,
- Query: Query{c: c, collectionID: id, parentPath: parent.Path},
+ Query: Query{
+ c: c,
+ collectionID: id,
+ path: parent.Path + "/" + id,
+ parentPath: parent.Path,
+ },
}
}
diff --git a/firestore/collref_test.go b/firestore/collref_test.go
index e77c0db..b4c0aad 100644
--- a/firestore/collref_test.go
+++ b/firestore/collref_test.go
@@ -26,9 +26,10 @@
coll := testClient.Collection("C")
got := coll.Doc("d")
want := &DocumentRef{
- Parent: coll,
- ID: "d",
- Path: "projects/projectID/databases/(default)/documents/C/d",
+ Parent: coll,
+ ID: "d",
+ Path: "projects/projectID/databases/(default)/documents/C/d",
+ shortPath: "C/d",
}
if !testEqual(got, want) {
t.Errorf("got %+v, want %+v", got, want)
diff --git a/firestore/conformance_test.go b/firestore/conformance_test.go
index 1fcbaaa..f7ee7d5 100644
--- a/firestore/conformance_test.go
+++ b/firestore/conformance_test.go
@@ -75,10 +75,10 @@
func runTest(t *testing.T, msg string, test *pb.Test) {
check := func(gotErr error, wantErr bool) bool {
if wantErr && gotErr == nil {
- t.Errorf("%s: got nil, want error", msg)
+ t.Fatalf("%s: got nil, want error", msg)
return false
} else if !wantErr && gotErr != nil {
- t.Errorf("%s: %v", msg, gotErr)
+ t.Fatalf("%s: %v", msg, gotErr)
return false
}
return true
@@ -105,7 +105,7 @@
ref := docRefFromPath(tt.Get.DocRefPath, c)
_, err := ref.Get(ctx)
if err != nil {
- t.Errorf("%s: %v", msg, err)
+ t.Fatalf("%s: %v", msg, err)
return
}
// Checking response would just be testing the function converting a Document
@@ -116,7 +116,7 @@
ref := docRefFromPath(tt.Create.DocRefPath, c)
data, err := convertData(tt.Create.JsonData)
if err != nil {
- t.Errorf("%s: %v", msg, err)
+ t.Fatalf("%s: %v", msg, err)
return
}
_, err = ref.Create(ctx, data)
@@ -127,7 +127,7 @@
ref := docRefFromPath(tt.Set.DocRefPath, c)
data, err := convertData(tt.Set.JsonData)
if err != nil {
- t.Errorf("%s: %v", msg, err)
+ t.Fatalf("%s: %v", msg, err)
return
}
var opts []SetOption
@@ -172,7 +172,7 @@
got, err := q.toProto()
if check(err, tt.Query.IsError) && err == nil {
if want := tt.Query.Query; !proto.Equal(got, want) {
- t.Errorf("%s\ngot: %s\nwant: %s", msg, proto.MarshalTextString(got), proto.MarshalTextString(want))
+ t.Fatalf("%s\ngot: %s\nwant: %s", msg, proto.MarshalTextString(got), proto.MarshalTextString(want))
}
}
@@ -190,14 +190,14 @@
}, rs)
got, err := nSnapshots(iter, len(tt.Listen.Snapshots))
if err != nil {
- t.Errorf("%s: %v", msg, err)
+ t.Fatalf("%s: %v", msg, err)
} else if diff := cmp.Diff(got, tt.Listen.Snapshots); diff != "" {
- t.Errorf("%s:\n%s", msg, diff)
+ t.Fatalf("%s:\n%s", msg, diff)
}
if tt.Listen.IsError {
_, err := iter.Next()
if err == nil {
- t.Errorf("%s: got nil, want error", msg)
+ t.Fatalf("%s: got nil, want error", msg)
}
}
@@ -359,6 +359,7 @@
q := Query{
parentPath: strings.Join(parts[:len(parts)-2], "/"),
collectionID: parts[len(parts)-1],
+ path: qt.CollPath,
}
for _, c := range qt.Clauses {
switch c := c.Clause.(type) {
diff --git a/firestore/docref.go b/firestore/docref.go
index 422b6bd..159600d 100644
--- a/firestore/docref.go
+++ b/firestore/docref.go
@@ -36,18 +36,24 @@
// The CollectionRef that this document is a part of. Never nil.
Parent *CollectionRef
- // The full resource path of the document: "projects/P/databases/D/documents..."
+ // The full resource path of the document. A document "doc-1" in collection
+ // "coll-1" would be: "projects/P/databases/D/documents/coll-1/doc-1".
Path string
+ // The shorter resource path of the document. A document "doc-1" in
+ // collection "coll-1" would be: "coll-1/doc-1".
+ shortPath string
+
// The ID of the document: the last component of the resource path.
ID string
}
func newDocRef(parent *CollectionRef, id string) *DocumentRef {
return &DocumentRef{
- Parent: parent,
- ID: id,
- Path: parent.Path + "/" + id,
+ Parent: parent,
+ ID: id,
+ Path: parent.Path + "/" + id,
+ shortPath: parent.selfPath + "/" + id,
}
}
diff --git a/firestore/from_value_test.go b/firestore/from_value_test.go
index 0093795..bee7a4a 100644
--- a/firestore/from_value_test.go
+++ b/firestore/from_value_test.go
@@ -73,20 +73,26 @@
{
in: refval("projects/P/databases/D/documents/c/d"),
want: &DocumentRef{
- ID: "d",
+ ID: "d",
+ Path: "projects/P/databases/D/documents/c/d",
+ shortPath: "c/d",
Parent: &CollectionRef{
ID: "c",
parentPath: "projects/P/databases/D",
+ selfPath: "c",
Path: "projects/P/databases/D/documents/c",
- Query: Query{collectionID: "c", parentPath: "projects/P/databases/D"},
+ Query: Query{
+ collectionID: "c",
+ parentPath: "projects/P/databases/D",
+ path: "projects/P/databases/D/documents/c",
+ },
},
- Path: "projects/P/databases/D/documents/c/d",
},
},
} {
got, err := createFromProtoValue(test.in, nil)
if err != nil {
- t.Errorf("%v: %v", test.in, err)
+ t.Errorf("%+v: %+v", test.in, err)
continue
}
if !testEqual(got, test.want) {
@@ -497,30 +503,49 @@
t.Fatal(err)
}
want := &DocumentRef{
- ID: "d2",
- Path: "projects/P/databases/D/documents/c1/d1/c2/d2",
+ ID: "d2",
+ Path: "projects/P/databases/D/documents/c1/d1/c2/d2",
+ shortPath: "c1/d1/c2/d2",
Parent: &CollectionRef{
ID: "c2",
parentPath: "projects/P/databases/D/documents/c1/d1",
Path: "projects/P/databases/D/documents/c1/d1/c2",
+ selfPath: "c1/d1/c2",
c: c,
- Query: Query{c: c, collectionID: "c2", parentPath: "projects/P/databases/D/documents/c1/d1"},
+ Query: Query{
+ c: c,
+ collectionID: "c2",
+ parentPath: "projects/P/databases/D/documents/c1/d1",
+ path: "projects/P/databases/D/documents/c1/d1/c2",
+ },
Parent: &DocumentRef{
- ID: "d1",
- Path: "projects/P/databases/D/documents/c1/d1",
+ ID: "d1",
+ Path: "projects/P/databases/D/documents/c1/d1",
+ shortPath: "c1/d1",
Parent: &CollectionRef{
ID: "c1",
c: c,
parentPath: "projects/P/databases/D",
Path: "projects/P/databases/D/documents/c1",
+ selfPath: "c1",
Parent: nil,
- Query: Query{c: c, collectionID: "c1", parentPath: "projects/P/databases/D"},
+ Query: Query{
+ c: c,
+ collectionID: "c1",
+ parentPath: "projects/P/databases/D",
+ path: "projects/P/databases/D/documents/c1",
+ },
},
},
},
}
if !testEqual(got, want) {
t.Errorf("\ngot %+v\nwant %+v", got, want)
+ t.Logf("\ngot.Parent %+v\nwant.Parent %+v", got.Parent, want.Parent)
+ t.Logf("\ngot.Parent.Query %+v\nwant.Parent.Query %+v", got.Parent.Query, want.Parent.Query)
+ t.Logf("\ngot.Parent.Parent %+v\nwant.Parent.Parent %+v", got.Parent.Parent, want.Parent.Parent)
+ t.Logf("\ngot.Parent.Parent.Parent %+v\nwant.Parent.Parent.Parent %+v", got.Parent.Parent.Parent, want.Parent.Parent.Parent)
+ t.Logf("\ngot.Parent.Parent.Parent.Query %+v\nwant.Parent.Parent.Parent.Query %+v", got.Parent.Parent.Parent.Query, want.Parent.Parent.Parent.Query)
}
}
diff --git a/firestore/query.go b/firestore/query.go
index 6470439..c4a638d 100644
--- a/firestore/query.go
+++ b/firestore/query.go
@@ -35,7 +35,8 @@
// a new Query; it does not modify the old.
type Query struct {
c *Client
- parentPath string // path of the collection's parent
+ path string // path to query (collection)
+ parentPath string // path of the collection's parent (document)
collectionID string
selection []FieldPath
filters []filter
@@ -48,10 +49,6 @@
err error
}
-func (q *Query) collectionPath() string {
- return q.parentPath + "/documents/" + q.collectionID
-}
-
// DocumentID is the special field name representing the ID of a document
// in queries.
const DocumentID = "__name__"
@@ -169,8 +166,10 @@
// StartAt returns a new Query that specifies that results should start at
// the document with the given field values.
//
-// If StartAt is called with a single DocumentSnapshot, its field values are used.
-// The DocumentSnapshot must have all the fields mentioned in the OrderBy clauses.
+// StartAt may be called with a single DocumentSnapshot, representing an
+// existing document within the query. The document must be a direct child of
+// the location being queried (not a parent document, or document in a
+// different collection, or a grandchild document, for example).
//
// Otherwise, StartAt should be called with one field value for each OrderBy clause,
// in the order that they appear. For example, in
@@ -375,7 +374,7 @@
if !ok {
return nil, fmt.Errorf("firestore: expected doc ID for DocumentID field, got %T", fval)
}
- vals[i] = &pb.Value{ValueType: &pb.Value_ReferenceValue{q.collectionPath() + "/" + docID}}
+ vals[i] = &pb.Value{ValueType: &pb.Value_ReferenceValue{q.path + "/" + docID}}
} else {
var sawTransform bool
vals[i], sawTransform, err = toProtoValue(reflect.ValueOf(fval))
@@ -395,7 +394,7 @@
vals := make([]*pb.Value, len(orders))
for i, ord := range orders {
if ord.isDocumentID() {
- dp, qp := ds.Ref.Parent.Path, q.collectionPath()
+ dp, qp := ds.Ref.Parent.Path, q.path
if dp != qp {
return nil, fmt.Errorf("firestore: document snapshot for %s passed to query on %s", dp, qp)
}
diff --git a/firestore/query_test.go b/firestore/query_test.go
index 3ed6307..848c647 100644
--- a/firestore/query_test.go
+++ b/firestore/query_test.go
@@ -545,18 +545,19 @@
}
func TestQueryFromCollectionRef(t *testing.T) {
- c := &Client{}
+ c := &Client{projectID: "P", databaseID: "D"}
coll := c.Collection("C")
got := coll.Select("x").Offset(8)
want := Query{
c: c,
parentPath: c.path(),
+ path: "projects/P/databases/D/documents/C",
collectionID: "C",
selection: []FieldPath{{"x"}},
offset: 8,
}
if !testEqual(got, want) {
- t.Fatalf("got %+v, want %+v", got, want)
+ t.Fatalf("\ngot %+v, \nwant %+v", got, want)
}
}
@@ -701,6 +702,138 @@
}
}
+func TestQuerySubCollections(t *testing.T) {
+ c := &Client{projectID: "P", databaseID: "DB"}
+
+ /*
+ parent-collection
+ +---------+ +---------+
+ | |
+ | |
+ parent-doc some-other-parent-doc
+ |
+ |
+ sub-collection
+ |
+ |
+ sub-doc
+ |
+ |
+ sub-sub-collection
+ |
+ |
+ sub-sub-doc
+ */
+ parentColl := c.Collection("parent-collection")
+ parentDoc := parentColl.Doc("parent-doc")
+ someOtherParentDoc := parentColl.Doc("some-other-parent-doc")
+ subColl := parentDoc.Collection("sub-collection")
+ subDoc := subColl.Doc("sub-doc")
+ subSubColl := subDoc.Collection("sub-sub-collection")
+ subSubDoc := subSubColl.Doc("sub-sub-doc")
+
+ testCases := []struct {
+ queryColl *CollectionRef
+ queryFilterDoc *DocumentRef // startAt or endBefore
+ wantColl string
+ wantRef string
+ wantErr bool
+ }{
+ // Queries are allowed at depth 0.
+ {parentColl, parentDoc, "parent-collection", "projects/P/databases/DB/documents/parent-collection/parent-doc", false},
+ // Queries are allowed at any depth.
+ {subColl, subDoc, "sub-collection", "projects/P/databases/DB/documents/parent-collection/parent-doc/sub-collection/sub-doc", false},
+ // Queries must be on immediate children (not allowed on grandchildren).
+ {subColl, someOtherParentDoc, "", "", true},
+ // Queries must be on immediate children (not allowed on siblings).
+ {subColl, subSubDoc, "", "", true},
+ }
+
+ // startAt
+ for _, testCase := range testCases {
+ // Query a child within the document.
+ q := testCase.queryColl.StartAt(&DocumentSnapshot{
+ Ref: testCase.queryFilterDoc,
+ proto: &pb.Document{
+ Fields: map[string]*pb.Value{"a": intval(7)},
+ },
+ }).OrderBy("a", Asc)
+ got, err := q.toProto()
+ if testCase.wantErr {
+ if err == nil {
+ t.Fatal("expected err, got nil")
+ }
+ continue
+ }
+
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := &pb.StructuredQuery{
+ From: []*pb.StructuredQuery_CollectionSelector{
+ {CollectionId: testCase.wantColl},
+ },
+ OrderBy: []*pb.StructuredQuery_Order{
+ {Field: fref1("a"), Direction: pb.StructuredQuery_ASCENDING},
+ {Field: fref1("__name__"), Direction: pb.StructuredQuery_ASCENDING},
+ },
+ StartAt: &pb.Cursor{
+ Values: []*pb.Value{
+ intval(7),
+ // This is the only part of the assertion we really care about.
+ refval(testCase.wantRef),
+ },
+ Before: true,
+ },
+ }
+ if !testEqual(got, want) {
+ t.Fatalf("got\n%v\nwant\n%v", pretty.Value(got), pretty.Value(want))
+ }
+ }
+
+ // endBefore
+ for _, testCase := range testCases {
+ // Query a child within the document.
+ q := testCase.queryColl.EndBefore(&DocumentSnapshot{
+ Ref: testCase.queryFilterDoc,
+ proto: &pb.Document{
+ Fields: map[string]*pb.Value{"a": intval(7)},
+ },
+ }).OrderBy("a", Asc)
+ got, err := q.toProto()
+ if testCase.wantErr {
+ if err == nil {
+ t.Fatal("expected err, got nil")
+ }
+ continue
+ }
+
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := &pb.StructuredQuery{
+ From: []*pb.StructuredQuery_CollectionSelector{
+ {CollectionId: testCase.wantColl},
+ },
+ OrderBy: []*pb.StructuredQuery_Order{
+ {Field: fref1("a"), Direction: pb.StructuredQuery_ASCENDING},
+ {Field: fref1("__name__"), Direction: pb.StructuredQuery_ASCENDING},
+ },
+ EndAt: &pb.Cursor{
+ Values: []*pb.Value{
+ intval(7),
+ // This is the only part of the assertion we really care about.
+ refval(testCase.wantRef),
+ },
+ Before: true,
+ },
+ }
+ if !testEqual(got, want) {
+ t.Fatalf("got\n%v\nwant\n%v", pretty.Value(got), pretty.Value(want))
+ }
+ }
+}
+
// Stop should be callable on an uninitialized QuerySnapshotIterator.
func TestStop_Uninitialized(t *testing.T) {
i := &QuerySnapshotIterator{}
