Skip to content

Commit 2090651

Browse files
bhshkhbhshkh
authored
fix(firestore): Correct the cursors when LimitToLast is used (#9413)
1 parent 9fe6061 commit 2090651

2 files changed

Lines changed: 104 additions & 47 deletions

File tree

‎firestore/integration_test.go‎

Lines changed: 58 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1020,13 +1020,22 @@ func TestIntegration_QueryDocuments_WhereEntity(t *testing.T) {
10201020
})
10211021
}
10221022

1023+
func reverseSlice(s []map[string]interface{}) []map[string]interface{} {
1024+
reversed := make([]map[string]interface{}, len(s))
1025+
for i, j := 0, len(s)-1; i <= j; i, j = i+1, j-1 {
1026+
reversed[i] = s[j]
1027+
reversed[j] = s[i]
1028+
}
1029+
return reversed
1030+
}
1031+
10231032
func TestIntegration_QueryDocuments(t *testing.T) {
10241033
ctx := context.Background()
10251034
coll := integrationColl(t)
10261035
h := testHelper{t}
10271036
var wants []map[string]interface{}
10281037
var createdDocRefs []*DocumentRef
1029-
for i := 0; i < 3; i++ {
1038+
for i := 0; i < 8; i++ {
10301039
doc := coll.NewDoc()
10311040
createdDocRefs = append(createdDocRefs, doc)
10321041

@@ -1037,46 +1046,69 @@ func TestIntegration_QueryDocuments(t *testing.T) {
10371046
}
10381047
q := coll.Select("q")
10391048
for i, test := range []struct {
1040-
q Query
1041-
want []map[string]interface{}
1042-
orderBy bool // Some query types do not allow ordering.
1049+
desc string
1050+
q Query
1051+
want []map[string]interface{}
1052+
orderBy bool // Some query types do not allow ordering.
1053+
orderByDir Direction
10431054
}{
1044-
{q, wants, true},
1045-
{q.Where("q", ">", 1), wants[2:], true},
1046-
{q.Where("q", "<", 1), wants[:1], true},
1047-
{q.Where("q", "==", 1), wants[1:2], false},
1048-
{q.Where("q", "!=", 0), wants[1:], true},
1049-
{q.Where("q", ">=", 1), wants[1:], true},
1050-
{q.Where("q", "<=", 1), wants[:2], true},
1051-
{q.Where("q", "in", []int{0}), wants[:1], false},
1052-
{q.Where("q", "not-in", []int{0, 1}), wants[2:], true},
1053-
{q.WherePath([]string{"q"}, ">", 1), wants[2:], true},
1054-
{q.Offset(1).Limit(1), wants[1:2], true},
1055-
{q.StartAt(1), wants[1:], true},
1056-
{q.StartAfter(1), wants[2:], true},
1057-
{q.EndAt(1), wants[:2], true},
1058-
{q.EndBefore(1), wants[:1], true},
1059-
{q.LimitToLast(2), wants[1:], true},
1060-
{q.EndBefore(2).LimitToLast(2), wants[:2], true},
1061-
{q.StartAt(1).EndBefore(2).LimitToLast(3), wants[1:2], true},
1055+
{"Without filters", q, wants, true, 0},
1056+
{"> filter", q.Where("q", ">", 1), wants[2:], true, Asc},
1057+
{"< filter", q.Where("q", "<", 1), wants[:1], true, Asc},
1058+
{"== filter", q.Where("q", "==", 1), wants[1:2], false, 0},
1059+
{"!= filter", q.Where("q", "!=", 0), wants[1:], true, Asc},
1060+
{">= filter", q.Where("q", ">=", 1), wants[1:], true, Asc},
1061+
{"<= filter", q.Where("q", "<=", 1), wants[:2], true, Asc},
1062+
{"in filter", q.Where("q", "in", []int{0}), wants[:1], false, 0},
1063+
{"not-in filter", q.Where("q", "not-in", []int{0, 1}), wants[2:], true, Asc},
1064+
{"WherePath", q.WherePath([]string{"q"}, ">", 1), wants[2:], true, Asc},
1065+
{"Offset with Limit", q.Offset(1).Limit(1), wants[1:2], true, Asc},
1066+
{"StartAt", q.StartAt(1), wants[1:], true, Asc},
1067+
{"StartAfter", q.StartAfter(1), wants[2:], true, Asc},
1068+
{"EndAt", q.EndAt(1), wants[:2], true, Asc},
1069+
{"EndBefore", q.EndBefore(1), wants[:1], true, Asc},
1070+
{"Open range with DESC order", q.StartAfter(6).EndBefore(2), reverseSlice(wants[3:6]), true, Desc},
1071+
{"LimitToLast", q.LimitToLast(2), wants[len(wants)-2:], true, Asc},
1072+
{"StartAfter with LimitToLast", q.StartAfter(2).LimitToLast(2), wants[len(wants)-2:], true, Asc},
1073+
{"StartAt with LimitToLast", q.StartAt(2).LimitToLast(2), wants[len(wants)-2:], true, Asc},
1074+
{"EndBefore with LimitToLast", q.EndBefore(7).LimitToLast(2), wants[5:7], true, Asc},
1075+
{"EndAt with LimitToLast", q.EndAt(7).LimitToLast(2), wants[6:8], true, Asc},
1076+
{"LimitToLast greater than no. of results", q.StartAt(1).EndBefore(2).LimitToLast(3), wants[1:2], true, Asc},
1077+
{"Closed range with LimitToLast ASC order", q.StartAt(2).EndAt(6).LimitToLast(2), wants[5:7], true, Asc},
1078+
{"Left closed right open range with LimitToLast ASC order", q.StartAt(2).EndBefore(6).LimitToLast(2), wants[4:6], true, Asc},
1079+
{"Left open right closed with LimitToLast ASC order", q.StartAfter(2).EndAt(6).LimitToLast(2), wants[5:7], true, Asc},
1080+
{"Open range with LimitToLast ASC order", q.StartAfter(2).EndBefore(6).LimitToLast(2), wants[4:6], true, Asc},
1081+
{"Closed range with LimitToLast DESC order", q.StartAt(6).EndAt(2).LimitToLast(2), reverseSlice(wants[2:4]), true, Desc},
1082+
{"Left closed right open range with LimitToLast DESC order", q.StartAt(6).EndBefore(2).LimitToLast(2), reverseSlice(wants[3:5]), true, Desc},
1083+
{"Left open right closed with LimitToLast DESC order", q.StartAfter(6).EndAt(2).LimitToLast(2), reverseSlice(wants[2:4]), true, Desc},
1084+
{"Open range with LimitToLast DESC order", q.StartAfter(6).EndBefore(2).LimitToLast(2), reverseSlice(wants[3:5]), true, Desc},
10621085
} {
10631086
if test.orderBy {
1064-
test.q = test.q.OrderBy("q", Asc)
1087+
test.q = test.q.OrderBy("q", test.orderByDir)
10651088
}
10661089
gotDocs, err := test.q.Documents(ctx).GetAll()
10671090
if err != nil {
1068-
t.Errorf("#%d: %+v: %v", i, test.q, err)
1091+
t.Errorf("#%d %v: %+v: %v", i, test.desc, test.q, err)
10691092
continue
10701093
}
10711094
if len(gotDocs) != len(test.want) {
1072-
t.Errorf("#%d: %+v: got %d docs, want %d", i, test.q, len(gotDocs), len(test.want))
1095+
t.Errorf("#%d %v: %+v: got %d docs, want %d", i, test.desc, test.q, len(gotDocs), len(test.want))
10731096
continue
10741097
}
1098+
1099+
fmt.Printf("test.want: %+v\n", test.want)
1100+
1101+
docsEqual := true
1102+
docsNotEqualErr := ""
10751103
for j, g := range gotDocs {
10761104
if got, want := g.Data(), test.want[j]; !testEqual(got, want) {
1077-
t.Errorf("#%d: %+v, #%d: got\n%+v\nwant\n%+v", i, test.q, j, got, want)
1105+
docsNotEqualErr += fmt.Sprintf("\n\t#%d: got %+v want %+v", j, got, want)
1106+
docsEqual = false
10781107
}
10791108
}
1109+
if !docsEqual {
1110+
t.Errorf("#%d %v: %+v %v", i, test.desc, test.q, docsNotEqualErr)
1111+
}
10801112
}
10811113
_, err := coll.Select("q").Where("x", "==", 1).OrderBy("q", Asc).Documents(ctx).GetAll()
10821114
codeEq(t, "Where and OrderBy on different fields without an index", codes.FailedPrecondition, err)

‎firestore/query.go‎

Lines changed: 46 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,21 @@ import (
3636
// Query values are immutable. Each Query method creates
3737
// a new Query; it does not modify the old.
3838
type Query struct {
39-
c *Client
40-
path string // path to query (collection)
41-
parentPath string // path of the collection's parent (document)
42-
collectionID string
43-
selection []*pb.StructuredQuery_FieldReference
44-
filters []*pb.StructuredQuery_Filter
45-
orders []order
46-
offset int32
47-
limit *wrappers.Int32Value
48-
limitToLast bool
49-
startVals, endVals []interface{}
50-
startDoc, endDoc *DocumentSnapshot
39+
c *Client
40+
path string // path to query (collection)
41+
parentPath string // path of the collection's parent (document)
42+
collectionID string
43+
selection []*pb.StructuredQuery_FieldReference
44+
filters []*pb.StructuredQuery_Filter
45+
orders []order
46+
offset int32
47+
limit *wrappers.Int32Value
48+
limitToLast bool
49+
startVals, endVals []interface{}
50+
startDoc, endDoc *DocumentSnapshot
51+
52+
// Set startBefore to true when doc in startVals needs to be included in result
53+
// Set endBefore to false when doc in endVals needs to be included in result
5154
startBefore, endBefore bool
5255
err error
5356

@@ -293,23 +296,37 @@ func (q *Query) processCursorArg(name string, docSnapshotOrFieldValues []interfa
293296

294297
func (q *Query) processLimitToLast() {
295298
if q.limitToLast {
296-
// Flip order statements before posting a request.
299+
// Firestore service does not provide limit to last behaviour out of the box. This is a client-side concept
300+
// So, flip order statements and cursors before posting a request. The response is flipped by other methods before returning to user
301+
// E.g.
302+
// If id of documents is 1, 2, 3, 4, 5, 6, 7 and query is (OrderBy(id, ASC), StartAt(2), EndAt(6), LimitToLast(3))
303+
// request sent to server is (OrderBy(id, DESC), StartAt(6), EndAt(2), Limit(3))
297304
for i := range q.orders {
298305
if q.orders[i].dir == Asc {
299306
q.orders[i].dir = Desc
300307
} else {
301308
q.orders[i].dir = Asc
302309
}
303310
}
304-
// Swap cursors.
305-
q.startVals, q.endVals = q.endVals, q.startVals
306-
q.startDoc, q.endDoc = q.endDoc, q.startDoc
307-
if q.endBefore {
311+
312+
if q.startBefore == q.endBefore && q.startCursorSpecified() && q.endCursorSpecified() {
313+
// E.g. query.StartAt(2).EndBefore(6).LimitToLast(3).OrderBy(Asc) i.e. cursors are [2, 6)
314+
// E.g. query.StartAfter(2).EndAt(6).LimitToLast(3).OrderBy(Asc) i.e. cursors are (2, 6]
315+
q.startBefore, q.endBefore = !q.startBefore, !q.endBefore
316+
} else if !q.startCursorSpecified() && q.endCursorSpecified() {
317+
// E.g. query.EndAt(6).LimitToLast(3).OrderBy(Asc) i.e. cursors are (-inf, 6]
318+
q.startBefore = !q.endBefore
308319
q.endBefore = false
320+
} else if q.startCursorSpecified() && !q.endCursorSpecified() {
321+
// E.g. query.StartAt(2).LimitToLast(3).OrderBy(Asc) i.e. cursors are [2, inf)
322+
q.endBefore = !q.startBefore
309323
q.startBefore = false
310-
} else {
311-
q.startBefore, q.endBefore = q.endBefore, q.startBefore
312324
}
325+
326+
// Swap cursors.
327+
q.startVals, q.endVals = q.endVals, q.startVals
328+
q.startDoc, q.endDoc = q.endDoc, q.startDoc
329+
313330
q.limitToLast = false
314331
}
315332
}
@@ -463,6 +480,14 @@ func (q Query) fromProto(pbQuery *pb.RunQueryRequest) (Query, error) {
463480
return q, q.err
464481
}
465482

483+
func (q Query) startCursorSpecified() bool {
484+
return len(q.startVals) != 0 || q.startDoc != nil
485+
}
486+
487+
func (q Query) endCursorSpecified() bool {
488+
return len(q.endVals) != 0 || q.endDoc != nil
489+
}
490+
466491
func (q Query) toProto() (*pb.StructuredQuery, error) {
467492
if q.err != nil {
468493
return nil, q.err
@@ -471,12 +496,12 @@ func (q Query) toProto() (*pb.StructuredQuery, error) {
471496
return nil, errors.New("firestore: query created without CollectionRef")
472497
}
473498
if q.startBefore {
474-
if len(q.startVals) == 0 && q.startDoc == nil {
499+
if !q.startCursorSpecified() {
475500
return nil, errors.New("firestore: StartAt/StartAfter must be called with at least one value")
476501
}
477502
}
478503
if q.endBefore {
479-
if len(q.endVals) == 0 && q.endDoc == nil {
504+
if !q.endCursorSpecified() {
480505
return nil, errors.New("firestore: EndAt/EndBefore must be called with at least one value")
481506
}
482507
}

0 commit comments

Comments
 (0)
⚡