GODRIVER-1831 Avoid accidental extra getMore when using limit on Find #562
Conversation
|
|
||
| assert.Equal(mt, int(limit), len(docs), "expected number of docs to be %v, got %v", limit, len(docs)) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Given that this is a bit tricky to reason about, I suggest:
- Insert more than 101 documents in the collection. That is the default batch size for find, so we can test what happens on a getMore when using the default batch size.
- Adding a table test parameterized on:
skip,limit, andbatchSizewhich can check for an expected result count. That will make it easy to add future tests as well.
| @@ -241,7 +241,7 @@ func (bc *BatchCursor) getMore(ctx context.Context) { | |||
|
|
|||
| // Required for legacy operations which don't support limit. | |||
| numToReturn := bc.batchSize | |||
| if bc.limit != 0 && bc.numReturned+bc.batchSize > bc.limit { | |||
| if bc.limit != 0 && bc.numReturned+bc.batchSize >= bc.limit { | |||
There was a problem hiding this comment.
Based on my reading of https://github.com/mongodb/specifications/blob/master/source/crud/crud.rst#combining-limit-and-batch-size-for-the-wire-protocol I believe it is ok to use batchSize=0 when a limit is set. So I think this might be ok? I'm still not entirely sure where the limit is enforced after, when batchSize=0 and more than the limit documents are returned. I think tests which query a collection containing more than 101 documents may verify that behavior.
There was a problem hiding this comment.
I'm not sure either, but the tests now query a collection containing more than 101 documents and pass...
There was a problem hiding this comment.
Great, the new tests cover the case of a getMore being run.
|
I'm removing myself as a required reviewer here, but once this is approved by other reviewers, the title and final commit message should be changed as this PR actually includes a code change, not just a new test. |
benjirewis
changed the title
|
|
||
| assert.Equal(mt, int(limit), len(docs), "expected number of docs to be %v, got %v", limit, len(docs)) | ||
| } | ||
| }) |
mongo/integration/collection_test.go
Outdated
| err = cursor.All(mtest.Background, &docs) | ||
| assert.Nil(mt, err, "All error: %v", err) | ||
| if (201 - tc.skip) < tc.limit { | ||
| assert.Equal(mt, int(201-tc.skip), len(docs), "expected number of docs to be %v, got %v", tc.limit, len(docs)) |
There was a problem hiding this comment.
| assert.Equal(mt, int(201-tc.skip), len(docs), "expected number of docs to be %v, got %v", tc.limit, len(docs)) | |
| assert.Equal(mt, int(201-tc.skip), len(docs), "expected number of docs to be %v, got %v", int(201-tc.skip), len(docs)) |
There was a problem hiding this comment.
Good catch! Changed.
| @@ -241,7 +241,7 @@ func (bc *BatchCursor) getMore(ctx context.Context) { | |||
|
|
|||
| // Required for legacy operations which don't support limit. | |||
| numToReturn := bc.batchSize | |||
| if bc.limit != 0 && bc.numReturned+bc.batchSize > bc.limit { | |||
| if bc.limit != 0 && bc.numReturned+bc.batchSize >= bc.limit { | |||
There was a problem hiding this comment.
Great, the new tests cover the case of a getMore being run.
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { |
There was a problem hiding this comment.
I suggest adding a name to the test cases and having the loop use mt.Run:
mt.Run(tc.name, func(mt *mtest.T) { ... }
It will make it easier to identify which test case failed if they are split into cases. Even names as simple as "case 1", "case 2", "case 3", etc. will produce separate output for each test case.
There was a problem hiding this comment.
Ah good idea! Used "case 1", etc. since they were difficult to describe succinctly.
mongo/integration/collection_test.go
Outdated
| assert.Nil(mt, err, "InsertMany error for initial data: %v", err) | ||
|
|
||
| for _, limit := range []int64{99, 100, 101, 200} { | ||
| opts := options.Find().SetSkip(0).SetLimit(limit) |
There was a problem hiding this comment.
Similarly, I suggest adding a name to the test cases. Otherwise, it'd be harder to track down which case failed if L949 errors.
There was a problem hiding this comment.
Sounds good! Used the limit number as the name for each case.
GODRIVER-1831
Includes change from this PR to legacy
batch_cursor#getMore()to fix a bug found by an external user on MDB versioned 2.6.12: whennumReturned == limitandbatchSizeis not set on abatchCursoran extragetMoreis run when the cursor should have just been closedAdds a test to make sure that when
batchSizeis not set on abatchCursor, a set limit will not be surpassed accidentally.