From e6fd59176fb552e7c441693afc82828fc8ae9522 Mon Sep 17 00:00:00 2001 From: italo jose Date: Fri, 11 Apr 2025 12:29:37 -0300 Subject: [PATCH 1/8] fix(CursorResponse): update emptyGetMore to use serialization and improve cursor rewind handling - Refactored emptyGetMore to return a serialized CursorResponse instance. - Enhanced the id getter to handle cases where the cursor id may not be present, defaulting to 0. - Added integration tests to verify cursor rewind functionality after applying the emptyGetMore optimization, ensuring correct behavior when the cursor is exhausted. --- src/cmap/wire_protocol/responses.ts | 12 +-- test/integration/crud/find.test.js | 118 ++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 6 deletions(-) diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index 1d20566e2d5..c9dfabbb25c 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -8,6 +8,7 @@ import { parseToElementsToArray, parseUtf8ValidationOption, pluckBSONSerializeOptions, + serialize, type Timestamp } from '../../bson'; import { MONGODB_ERROR_CODES, MongoUnexpectedServerResponseError } from '../../error'; @@ -230,11 +231,9 @@ export class CursorResponse extends MongoDBResponse { * This supports a feature of the FindCursor. * It is an optimization to avoid an extra getMore when the limit has been reached */ - static emptyGetMore: CursorResponse = { - id: new Long(0), - length: 0, - shift: () => null - } as unknown as CursorResponse; + static get emptyGetMore(): CursorResponse { + return new CursorResponse(serialize({ ok: 1, cursor: { id: 0, nextBatch: [] } })); + } static override is(value: unknown): value is CursorResponse { return value instanceof CursorResponse || value === CursorResponse.emptyGetMore; @@ -249,7 +248,8 @@ export class CursorResponse extends MongoDBResponse { public get id(): Long { try { - return Long.fromBigInt(this.cursor.get('id', BSONType.long, true)); + const cursorId = this.cursor.get('id', BSONType.long, false); + return cursorId ? Long.fromBigInt(cursorId) : Long.fromBigInt(0n); } catch (cause) { throw new MongoUnexpectedServerResponseError(cause.message, { cause }); } diff --git a/test/integration/crud/find.test.js b/test/integration/crud/find.test.js index ed098d31c0a..da351abb79f 100644 --- a/test/integration/crud/find.test.js +++ b/test/integration/crud/find.test.js @@ -2388,4 +2388,122 @@ describe('Find', function () { }); }); }); + + it('should correctly rewind cursor after emptyGetMore optimization', { + metadata: { + requires: { topology: ['sharded'] } + }, + + test: async function () { + const configuration = this.configuration; + const client = configuration.newClient(configuration.writeConcernMax(), { maxPoolSize: 1 }); + await client.connect(); + this.defer(async () => await client.close()); + + const db = client.db(configuration.db); + const collectionName = 'test_rewind_emptygetmore'; + await db.dropCollection(collectionName).catch(() => null); + const collection = db.collection(collectionName); + + // Insert 10 documents + const docsToInsert = Array.from({ length: 10 }, (_, i) => ({ x: i })); + await collection.insertMany(docsToInsert, configuration.writeConcernMax()); + + // Create a cursor with batch size = 2 and limit = 4 (a multiple of batch size) + // This configuration is important to trigger the emptyGetMore optimization + const cursor = collection.find({}, { batchSize: 2, limit: 4 }); + + // Consume all documents (4 due to limit) + const documents = []; + for (let i = 0; i < 5; i++) { + // The 5th iteration should return null as the cursor is exhausted + const doc = await cursor.next(); + if (doc !== null) { + documents.push(doc); + } + } + + // Verify we got the correct number of documents (based on limit) + expect(documents).to.have.length(4); + + // Prior to the fix, this rewind() call would throw + // "TypeError: this.documents?.clear is not a function" + // because the emptyGetMore optimization sets documents to an object without a clear method + try { + cursor.rewind(); + + // Verify we can iterate the cursor again after rewind + const documentsAfterRewind = []; + for (let i = 0; i < 4; i++) { + const doc = await cursor.next(); + if (doc !== null) { + documentsAfterRewind.push(doc); + } + } + + // Verify we got the same documents again + expect(documentsAfterRewind).to.have.length(4); + for (let i = 0; i < 4; i++) { + expect(documentsAfterRewind[i].x).to.equal(documents[i].x); + } + } catch (error) { + // If the rewind() operation fails, the test should fail + expect.fail(`Rewind operation failed: ${error.message}`); + } + } + }); + + it('should handle rewind after emptyGetMore optimization (using repro.js scenario)', { + metadata: { + requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] } + }, + + test: async function () { + const configuration = this.configuration; + const client = configuration.newClient(configuration.writeConcernMax(), { maxPoolSize: 1 }); + await client.connect(); + this.defer(async () => await client.close()); + + const db = client.db(configuration.db); + const collectionName = 'test_rewind_repro'; + await db.dropCollection(collectionName).catch(() => null); + const collection = db.collection(collectionName); + + // Insert 100 documents (like in repro.js) + const documents = Array.from({ length: 100 }, (_, i) => ({ + _id: i, + value: `Document ${i}` + })); + await collection.insertMany(documents, configuration.writeConcernMax()); + + // Create a cursor with a small batch size to force multiple getMore operations + const cursor = collection.find({}).batchSize(10); + + // Consume the cursor until it's exhausted (like in repro.js) + while (await cursor.hasNext()) { + await cursor.next(); + } + + // At this point, cursor should be fully consumed and potentially + // optimized with emptyGetMore which lacks clear() method + + // Now try to rewind the cursor and fetch documents again + try { + // This would throw if documents.clear is not a function and fix isn't applied + cursor.rewind(); + + // If we got here, rewind succeeded - now try to use the cursor again + const results = await cursor.toArray(); + + // Verify the results + expect(results).to.have.length(100); + for (let i = 0; i < 100; i++) { + expect(results[i]).to.have.property('_id', i); + expect(results[i]).to.have.property('value', `Document ${i}`); + } + } catch (error) { + expect.fail(`Error during rewind or subsequent fetch: ${error.message}`); + } + } + }); }); From 324e793670b4a2220174cd94b4ea008e09d3c629 Mon Sep 17 00:00:00 2001 From: italo jose Date: Mon, 14 Apr 2025 10:36:49 -0300 Subject: [PATCH 2/8] fix(CursorResponse): update id getter to ensure cursorId is always retrieved - Modified the id getter to require the cursor id to be present, removing the defaulting to 0. - This change enhances the reliability of cursor handling by ensuring that a valid cursor id is always returned, improving overall response accuracy. --- src/cmap/wire_protocol/responses.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index c9dfabbb25c..39747f2a7e9 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -248,8 +248,8 @@ export class CursorResponse extends MongoDBResponse { public get id(): Long { try { - const cursorId = this.cursor.get('id', BSONType.long, false); - return cursorId ? Long.fromBigInt(cursorId) : Long.fromBigInt(0n); + const cursorId = this.cursor.get('id', BSONType.long, true); + return Long.fromBigInt(cursorId); } catch (cause) { throw new MongoUnexpectedServerResponseError(cause.message, { cause }); } From 12b6c30572dba764649b1edcde29a33ae39ee91f Mon Sep 17 00:00:00 2001 From: italo jose Date: Mon, 14 Apr 2025 11:49:49 -0300 Subject: [PATCH 3/8] fix(CursorResponse): enhance id getter to handle missing cursor id - Updated the id getter to throw an error if the cursor id is missing, ensuring more robust error handling. - Adjusted the retrieval of cursor id to return 0 when it is null, improving the reliability of cursor operations. - This change aims to enhance the accuracy of responses by enforcing the presence of a valid cursor id. --- src/cmap/wire_protocol/responses.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index 39747f2a7e9..4deb711eea3 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -248,9 +248,15 @@ export class CursorResponse extends MongoDBResponse { public get id(): Long { try { - const cursorId = this.cursor.get('id', BSONType.long, true); - return Long.fromBigInt(cursorId); + if (!this.cursor.has('id')) { + throw new MongoUnexpectedServerResponseError('"id" is missing'); + } + const cursorId = this.cursor.get('id', BSONType.long, false); + return cursorId != null ? Long.fromBigInt(cursorId) : Long.fromBigInt(0n); } catch (cause) { + if (cause instanceof MongoUnexpectedServerResponseError) { + throw cause; + } throw new MongoUnexpectedServerResponseError(cause.message, { cause }); } } From e9b652f013edfcffebaf4f15370dd503bb0d79e9 Mon Sep 17 00:00:00 2001 From: italo jose Date: Mon, 14 Apr 2025 11:52:43 -0300 Subject: [PATCH 4/8] fix(CursorResponse): enhance id getter to handle missing cursor id - Updated the id getter to throw an error if the cursor id is missing, ensuring more robust error handling. - Adjusted the retrieval of cursor id to return 0 when it is null, improving the reliability of cursor operations. - This change aims to enhance the accuracy of responses by enforcing the presence of a valid cursor id. --- src/cmap/wire_protocol/responses.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index 4deb711eea3..e8e873a3719 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -247,16 +247,13 @@ export class CursorResponse extends MongoDBResponse { } public get id(): Long { + if (!this.cursor.has('id')) { + throw new MongoUnexpectedServerResponseError('"id" is missing'); + } try { - if (!this.cursor.has('id')) { - throw new MongoUnexpectedServerResponseError('"id" is missing'); - } const cursorId = this.cursor.get('id', BSONType.long, false); return cursorId != null ? Long.fromBigInt(cursorId) : Long.fromBigInt(0n); } catch (cause) { - if (cause instanceof MongoUnexpectedServerResponseError) { - throw cause; - } throw new MongoUnexpectedServerResponseError(cause.message, { cause }); } } From 92e8598836d23144df3c276ef88eb75bd65ec58b Mon Sep 17 00:00:00 2001 From: bailey Date: Tue, 15 Apr 2025 07:33:12 -0600 Subject: [PATCH 5/8] use bigint for cursor id --- src/cmap/wire_protocol/responses.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index e8e873a3719..bacf6d648be 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -232,7 +232,7 @@ export class CursorResponse extends MongoDBResponse { * It is an optimization to avoid an extra getMore when the limit has been reached */ static get emptyGetMore(): CursorResponse { - return new CursorResponse(serialize({ ok: 1, cursor: { id: 0, nextBatch: [] } })); + return new CursorResponse(serialize({ ok: 1, cursor: { id: 0n, nextBatch: [] } })); } static override is(value: unknown): value is CursorResponse { @@ -247,12 +247,8 @@ export class CursorResponse extends MongoDBResponse { } public get id(): Long { - if (!this.cursor.has('id')) { - throw new MongoUnexpectedServerResponseError('"id" is missing'); - } try { - const cursorId = this.cursor.get('id', BSONType.long, false); - return cursorId != null ? Long.fromBigInt(cursorId) : Long.fromBigInt(0n); + return Long.fromBigInt(this.cursor.get('id', BSONType.long, true)); } catch (cause) { throw new MongoUnexpectedServerResponseError(cause.message, { cause }); } From e0b033c198bd1dc4466a67da7c5e2d9f10990eca Mon Sep 17 00:00:00 2001 From: bailey Date: Tue, 15 Apr 2025 07:55:31 -0600 Subject: [PATCH 6/8] tests --- test/integration/crud/find.test.js | 124 +++-------------------------- 1 file changed, 12 insertions(+), 112 deletions(-) diff --git a/test/integration/crud/find.test.js b/test/integration/crud/find.test.js index da351abb79f..278b7e60e9e 100644 --- a/test/integration/crud/find.test.js +++ b/test/integration/crud/find.test.js @@ -2389,121 +2389,21 @@ describe('Find', function () { }); }); - it('should correctly rewind cursor after emptyGetMore optimization', { - metadata: { - requires: { topology: ['sharded'] } - }, - - test: async function () { - const configuration = this.configuration; - const client = configuration.newClient(configuration.writeConcernMax(), { maxPoolSize: 1 }); - await client.connect(); - this.defer(async () => await client.close()); - - const db = client.db(configuration.db); - const collectionName = 'test_rewind_emptygetmore'; - await db.dropCollection(collectionName).catch(() => null); - const collection = db.collection(collectionName); - - // Insert 10 documents - const docsToInsert = Array.from({ length: 10 }, (_, i) => ({ x: i })); - await collection.insertMany(docsToInsert, configuration.writeConcernMax()); - - // Create a cursor with batch size = 2 and limit = 4 (a multiple of batch size) - // This configuration is important to trigger the emptyGetMore optimization - const cursor = collection.find({}, { batchSize: 2, limit: 4 }); - - // Consume all documents (4 due to limit) - const documents = []; - for (let i = 0; i < 5; i++) { - // The 5th iteration should return null as the cursor is exhausted - const doc = await cursor.next(); - if (doc !== null) { - documents.push(doc); - } - } - - // Verify we got the correct number of documents (based on limit) - expect(documents).to.have.length(4); - - // Prior to the fix, this rewind() call would throw - // "TypeError: this.documents?.clear is not a function" - // because the emptyGetMore optimization sets documents to an object without a clear method - try { - cursor.rewind(); - - // Verify we can iterate the cursor again after rewind - const documentsAfterRewind = []; - for (let i = 0; i < 4; i++) { - const doc = await cursor.next(); - if (doc !== null) { - documentsAfterRewind.push(doc); - } - } - - // Verify we got the same documents again - expect(documentsAfterRewind).to.have.length(4); - for (let i = 0; i < 4; i++) { - expect(documentsAfterRewind[i].x).to.equal(documents[i].x); - } - } catch (error) { - // If the rewind() operation fails, the test should fail - expect.fail(`Rewind operation failed: ${error.message}`); - } - } - }); - - it('should handle rewind after emptyGetMore optimization (using repro.js scenario)', { - metadata: { - requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] } - }, - - test: async function () { - const configuration = this.configuration; - const client = configuration.newClient(configuration.writeConcernMax(), { maxPoolSize: 1 }); - await client.connect(); - this.defer(async () => await client.close()); + it('regression test (NODE-6878): CursorResponse.emptyGetMore contains all CursorResponse fields', async function () { + const collection = client.db('rewind-regression').collection('bar'); - const db = client.db(configuration.db); - const collectionName = 'test_rewind_repro'; - await db.dropCollection(collectionName).catch(() => null); - const collection = db.collection(collectionName); - - // Insert 100 documents (like in repro.js) - const documents = Array.from({ length: 100 }, (_, i) => ({ - _id: i, - value: `Document ${i}` - })); - await collection.insertMany(documents, configuration.writeConcernMax()); - - // Create a cursor with a small batch size to force multiple getMore operations - const cursor = collection.find({}).batchSize(10); - - // Consume the cursor until it's exhausted (like in repro.js) - while (await cursor.hasNext()) { - await cursor.next(); - } + await collection.deleteMany({}); + await collection.insertMany(Array.from({ length: 4 }, (_, i) => ({ x: i }))); - // At this point, cursor should be fully consumed and potentially - // optimized with emptyGetMore which lacks clear() method + const cursor = collection.find({}, { batchSize: 1, limit: 3 }); + // emptyGetMore is used internally after limit + 1 documents have been iterated + await cursor.next(); + await cursor.next(); + await cursor.next(); + await cursor.next(); - // Now try to rewind the cursor and fetch documents again - try { - // This would throw if documents.clear is not a function and fix isn't applied - cursor.rewind(); + cursor.rewind(); - // If we got here, rewind succeeded - now try to use the cursor again - const results = await cursor.toArray(); - - // Verify the results - expect(results).to.have.length(100); - for (let i = 0; i < 100; i++) { - expect(results[i]).to.have.property('_id', i); - expect(results[i]).to.have.property('value', `Document ${i}`); - } - } catch (error) { - expect.fail(`Error during rewind or subsequent fetch: ${error.message}`); - } - } + await cursor.toArray(); }); }); From 3a46f6866f32e15f04ce8c284a45f182a079462b Mon Sep 17 00:00:00 2001 From: bailey Date: Fri, 18 Apr 2025 09:04:46 -0600 Subject: [PATCH 7/8] add sinon assertion --- test/integration/crud/find.test.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/integration/crud/find.test.js b/test/integration/crud/find.test.js index 278b7e60e9e..47a19766948 100644 --- a/test/integration/crud/find.test.js +++ b/test/integration/crud/find.test.js @@ -3,7 +3,7 @@ const { assert: test } = require('../shared'); const { expect } = require('chai'); const sinon = require('sinon'); const { setTimeout } = require('timers'); -const { Code, ObjectId, Long, Binary, ReturnDocument } = require('../../mongodb'); +const { Code, ObjectId, Long, Binary, ReturnDocument, CursorResponse } = require('../../mongodb'); describe('Find', function () { let client; @@ -2395,6 +2395,8 @@ describe('Find', function () { await collection.deleteMany({}); await collection.insertMany(Array.from({ length: 4 }, (_, i) => ({ x: i }))); + const getMoreSpy = sinon.spy(CursorResponse, 'emptyGetMore', ['get']); + const cursor = collection.find({}, { batchSize: 1, limit: 3 }); // emptyGetMore is used internally after limit + 1 documents have been iterated await cursor.next(); @@ -2402,6 +2404,10 @@ describe('Find', function () { await cursor.next(); await cursor.next(); + // assert that `emptyGetMore` is called. if it is not, this test + // always passes, even without the fix in NODE-6878. + expect(getMoreSpy.get).to.have.been.called; + cursor.rewind(); await cursor.toArray(); From dafb1d553b2916d21d538374066453c8ee19c739 Mon Sep 17 00:00:00 2001 From: bailey Date: Fri, 18 Apr 2025 09:34:45 -0600 Subject: [PATCH 8/8] only run test on sharded clusters --- test/integration/crud/find.test.js | 38 +++++++++++++++++------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/test/integration/crud/find.test.js b/test/integration/crud/find.test.js index 47a19766948..c6d5b2fdf6e 100644 --- a/test/integration/crud/find.test.js +++ b/test/integration/crud/find.test.js @@ -2389,27 +2389,31 @@ describe('Find', function () { }); }); - it('regression test (NODE-6878): CursorResponse.emptyGetMore contains all CursorResponse fields', async function () { - const collection = client.db('rewind-regression').collection('bar'); + it( + 'regression test (NODE-6878): CursorResponse.emptyGetMore contains all CursorResponse fields', + { requires: { topology: 'sharded' } }, + async function () { + const collection = client.db('rewind-regression').collection('bar'); - await collection.deleteMany({}); - await collection.insertMany(Array.from({ length: 4 }, (_, i) => ({ x: i }))); + await collection.deleteMany({}); + await collection.insertMany(Array.from({ length: 4 }, (_, i) => ({ x: i }))); - const getMoreSpy = sinon.spy(CursorResponse, 'emptyGetMore', ['get']); + const getMoreSpy = sinon.spy(CursorResponse, 'emptyGetMore', ['get']); - const cursor = collection.find({}, { batchSize: 1, limit: 3 }); - // emptyGetMore is used internally after limit + 1 documents have been iterated - await cursor.next(); - await cursor.next(); - await cursor.next(); - await cursor.next(); + const cursor = collection.find({}, { batchSize: 1, limit: 3 }); + // emptyGetMore is used internally after limit + 1 documents have been iterated + await cursor.next(); + await cursor.next(); + await cursor.next(); + await cursor.next(); - // assert that `emptyGetMore` is called. if it is not, this test - // always passes, even without the fix in NODE-6878. - expect(getMoreSpy.get).to.have.been.called; + // assert that `emptyGetMore` is called. if it is not, this test + // always passes, even without the fix in NODE-6878. + expect(getMoreSpy.get).to.have.been.called; - cursor.rewind(); + cursor.rewind(); - await cursor.toArray(); - }); + await cursor.toArray(); + } + ); });