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>
googleapis · Dec 6, 2018 · 3cd5a7b · 3cd5a7b
1 parent 6d96a60
commit 3cd5a7b
Show file tree

Hide file tree

Showing 8 changed files with 248 additions and 50 deletions.
diff --git a/firestore/client_test.go b/firestore/client_test.go
@@ -35,19 +35,26 @@ func TestClientCollectionAndDoc(t *testing.T) {
 	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 @@ func TestClientCollectionAndDoc(t *testing.T) {
 	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
@@ -26,11 +26,17 @@ import (
 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 @@ func newTopLevelCollRef(c *Client, dbPath, id string) *CollectionRef {
 		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
@@ -26,9 +26,10 @@ func TestDoc(t *testing.T) {
 	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
@@ -75,10 +75,10 @@ func runTestFromFile(t *testing.T, filename string) {
 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 @@ func runTest(t *testing.T, msg string, test *pb.Test) {
 		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 @@ func runTest(t *testing.T, msg string, test *pb.Test) {
 		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 @@ func runTest(t *testing.T, msg string, test *pb.Test) {
 		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 @@ func runTest(t *testing.T, msg string, test *pb.Test) {
 		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 @@ func runTest(t *testing.T, msg string, test *pb.Test) {
 		}, 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 @@ func convertQuery(t *testing.T, qt *pb.QueryTest) Query {
 	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
@@ -36,18 +36,24 @@ type DocumentRef struct {
 	// 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
@@ -73,20 +73,26 @@ func TestCreateFromProtoValue(t *testing.T) {
 		{
 			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 @@ func TestPathToDoc(t *testing.T) {
 		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
@@ -35,7 +35,8 @@ import (
 // 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 @@ type Query struct {
 	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 @@ func (q Query) Limit(n int) Query {
 // 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 @@ func (q *Query) fieldValuesToCursorValues(fieldValues []interface{}) ([]*pb.Valu
 			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 @@ func (q *Query) docSnapshotToCursorValues(ds *DocumentSnapshot, orders []order)
 	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)
 			}