GODRIVER-1831 Avoid accidental extra getMore when using limit on Find #562

benjirewis · 2021-01-19T18:40:18Z

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: when numReturned == limit and batchSize is not set on a batchCursor an extra getMore is run when the cursor should have just been closed

Adds a test to make sure that when batchSize is not set on a batchCursor, a set limit will not be surpassed accidentally.

kevinAlbs · 2021-01-20T16:42:57Z

mongo/integration/collection_test.go

+
+				assert.Equal(mt, int(limit), len(docs), "expected number of docs to be %v, got %v", limit, len(docs))
+			}
+		})


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, and batchSize which can check for an expected result count. That will make it easy to add future tests as well.

Added both!

kevinAlbs · 2021-01-20T17:20:36Z

x/mongo/driver/batch_cursor.go

@@ -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 {


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.

I'm not sure either, but the tests now query a collection containing more than 101 documents and pass...

Great, the new tests cover the case of a getMore being run.

iwysiu

The code change looks good to me, just an additional test case request.

mongo/integration/collection_test.go

divjotarora · 2021-01-21T18:27:25Z

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.

kevinAlbs

I have a couple nits about test case names and error messages. Otherwise LGTM!

kevinAlbs · 2021-01-27T19:24:48Z

mongo/integration/collection_test.go

+
+				assert.Equal(mt, int(limit), len(docs), "expected number of docs to be %v, got %v", limit, len(docs))
+			}
+		})


kevinAlbs · 2021-01-27T19:55:18Z

mongo/integration/collection_test.go

+				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))


Suggested change

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))

Good catch! Changed.

kevinAlbs · 2021-01-27T20:05:45Z

x/mongo/driver/batch_cursor.go

@@ -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 {


Great, the new tests cover the case of a getMore being run.

kevinAlbs · 2021-01-27T20:09:18Z

mongo/integration/collection_test.go

+				},
+			}
+
+			for _, tc := range testCases {


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.

Ah good idea! Used "case 1", etc. since they were difficult to describe succinctly.

kevinAlbs · 2021-01-27T20:11:31Z

mongo/integration/collection_test.go

+			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)


Similarly, I suggest adding a name to the test cases. Otherwise, it'd be harder to track down which case failed if L949 errors.

Sounds good! Used the limit number as the name for each case.

iwysiu

lgtm!

…#562)

GODRIVER-1831 Add find limit test with unset batch size

d5740e6

benjirewis requested review from kevinAlbs, iwysiu and divjotarora January 19, 2021 18:40

kevinAlbs reviewed Jan 20, 2021

View reviewed changes

Add more test coverage

1bae72e

kevinAlbs self-requested a review January 21, 2021 13:50

iwysiu approved these changes Jan 21, 2021

View reviewed changes

mongo/integration/collection_test.go Outdated Show resolved Hide resolved

Test limit < expected docs

d1d7aee

benjirewis requested a review from iwysiu January 21, 2021 18:22

divjotarora removed their request for review January 21, 2021 18:26

benjirewis changed the title ~~GODRIVER-1831 Add find limit test with unset batch size~~ GODRIVER-1831 Avoid accidental extra getMore when using limit on Find Jan 21, 2021

kevinAlbs approved these changes Jan 27, 2021

View reviewed changes

Use names for testCases

42fb8ee

iwysiu approved these changes Jan 27, 2021

View reviewed changes

benjirewis merged commit f893899 into mongodb:master Jan 27, 2021

benjirewis deleted the addFindLimitTests.1831 branch January 27, 2021 22:33

benjirewis added a commit that referenced this pull request Jan 27, 2021

GODRIVER-1831 Avoid accidental extra getMore when using limit on Find (…

f537927

…#562)

benjirewis added a commit that referenced this pull request Jan 27, 2021

GODRIVER-1831 Avoid accidental extra getMore when using limit on Find (…

3ffdf76

…#562)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-1831 Avoid accidental extra getMore when using limit on Find #562

GODRIVER-1831 Avoid accidental extra getMore when using limit on Find #562

benjirewis commented Jan 19, 2021

kevinAlbs Jan 20, 2021

benjirewis Jan 21, 2021

kevinAlbs Jan 27, 2021

kevinAlbs Jan 20, 2021

benjirewis Jan 21, 2021

kevinAlbs Jan 27, 2021

iwysiu left a comment

divjotarora commented Jan 21, 2021

kevinAlbs left a comment

kevinAlbs Jan 27, 2021

kevinAlbs Jan 27, 2021

benjirewis Jan 27, 2021

kevinAlbs Jan 27, 2021

kevinAlbs Jan 27, 2021

benjirewis Jan 27, 2021

kevinAlbs Jan 27, 2021

benjirewis Jan 27, 2021

iwysiu left a 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))

GODRIVER-1831 Avoid accidental extra getMore when using limit on Find #562

GODRIVER-1831 Avoid accidental extra getMore when using limit on Find #562

Conversation

benjirewis commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu left a comment

Choose a reason for hiding this comment

divjotarora commented Jan 21, 2021

kevinAlbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu left a comment

Choose a reason for hiding this comment