Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-4609): allow mapping to falsey non-null values in cursors #3452

Merged
52 changes: 44 additions & 8 deletions src/cursor/abstract_cursor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { promisify } from 'util';
import { BSONSerializeOptions, Document, Long, pluckBSONSerializeOptions } from '../bson';
import {
AnyError,
MongoAPIError,
MongoCursorExhaustedError,
MongoCursorInUseError,
MongoInvalidArgumentError,
Expand Down Expand Up @@ -305,7 +306,18 @@ export abstract class AbstractCursor<
while (true) {
const document = await this.next();

if (document == null) {
// Intentional strict null check, because users can map cursors to falsey values.
// We allow mapping to all values except for null.
// eslint-disable-next-line no-restricted-syntax
if (document === null) {
if (!this.closed) {
const message =
'Cursor returned a `null` document, but the cursor is not exhausted. Mapping documents to `null` is not supported in the cursor transform.';

await cleanupCursorAsync(this, { needsToEmitClosed: true }).catch(() => null);

throw new MongoAPIError(message);
}
break;
}

Expand Down Expand Up @@ -504,6 +516,29 @@ export abstract class AbstractCursor<
* this function's transform.
*
* @remarks
*
* **Note** Cursors use `null` internally to indicate that there are no more documents in the cursor. Providing a mapping
* function that maps values to `null` will result in the cursor closing itself before it has finished iterating
* all documents. This will **not** result in a memory leak, just surprising behavior. For example:
*
* ```typescript
* const cursor = collection.find({});
* cursor.map(() => null);
*
* const documents = await cursor.toArray();
* // documents is always [], regardless of how many documents are in the collection.
* ```
*
* Other falsey values are allowed:
*
* ```typescript
* const cursor = collection.find({});
* cursor.map(() => '');
*
* const documents = await cursor.toArray();
* // documents is now an array of empty strings
* ```
*
* **Note for Typescript Users:** adding a transform changes the return type of the iteration of this cursor,
* it **does not** return a new instance of a cursor. This means when calling map,
* you should always assign the result to a new variable in order to get a correctly typed cursor variable.
Expand Down Expand Up @@ -657,7 +692,7 @@ export abstract class AbstractCursor<
* a significant refactor.
*/
[kInit](callback: Callback<TSchema | null>): void {
this._initialize(this[kSession], (err, state) => {
this._initialize(this[kSession], (error, state) => {
if (state) {
const response = state.response;
this[kServer] = state.server;
Expand Down Expand Up @@ -689,8 +724,12 @@ export abstract class AbstractCursor<
// the cursor is now initialized, even if an error occurred or it is dead
this[kInitialized] = true;

if (err || cursorIsDead(this)) {
return cleanupCursor(this, { error: err }, () => callback(err, nextDocument(this)));
if (error) {
return cleanupCursor(this, { error }, () => callback(error, undefined));
}

if (cursorIsDead(this)) {
return cleanupCursor(this, undefined, () => callback());
}

callback();
Expand Down Expand Up @@ -743,11 +782,8 @@ export function next<T>(

if (cursorId == null) {
// All cursors must operate within a session, one must be made implicitly if not explicitly provided
cursor[kInit]((err, value) => {
cursor[kInit](err => {
if (err) return callback(err);
if (value) {
return callback(undefined, value);
}
return next(cursor, blocking, callback);
});

Expand Down
114 changes: 114 additions & 0 deletions test/integration/node-specific/abstract_cursor.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import { expect } from 'chai';
import { inspect } from 'util';

import { Collection, MongoAPIError, MongoClient } from '../../../src';

const falseyValues = [0, 0n, NaN, '', false, undefined];

describe('class AbstractCursor', function () {
let client: MongoClient;

let collection: Collection;
beforeEach(async function () {
client = this.configuration.newClient();

collection = client.db('abstract_cursor_integration').collection('test');

await collection.insertMany(Array.from({ length: 5 }, (_, index) => ({ index })));
});

afterEach(async function () {
await collection.deleteMany({});
await client.close();
});

context('toArray() with custom transforms', function () {
for (const value of falseyValues) {
it(`supports mapping to falsey value '${inspect(value)}'`, async function () {
const cursor = collection.find();
cursor.map(() => value);

const result = await cursor.toArray();

const expected = Array.from({ length: 5 }, () => value);
expect(result).to.deep.equal(expected);
});
}

it('throws when mapping to `null` and cleans up cursor', async function () {
const cursor = collection.find();
cursor.map(() => null);

const error = await cursor.toArray().catch(e => e);

expect(error).be.instanceOf(MongoAPIError);
expect(cursor.closed).to.be.true;
});
});

context('Symbol.asyncIterator() with custom transforms', function () {
for (const value of falseyValues) {
it(`supports mapping to falsey value '${inspect(value)}'`, async function () {
const cursor = collection.find();
cursor.map(() => value);

let count = 0;

for await (const document of cursor) {
expect(document).to.deep.equal(value);
count++;
}

expect(count).to.equal(5);
});
}

it('throws when mapping to `null` and cleans up cursor', async function () {
const cursor = collection.find();
cursor.map(() => null);

try {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
for await (const document of cursor) {
expect.fail('Expected error to be thrown');
}
} catch (error) {
expect(error).to.be.instanceOf(MongoAPIError);
expect(cursor.closed).to.be.true;
}
});
});

context('forEach() with custom transforms', function () {
for (const value of falseyValues) {
it(`supports mapping to falsey value '${inspect(value)}'`, async function () {
const cursor = collection.find();
cursor.map(() => value);

let count = 0;

function transform(value) {
expect(value).to.deep.equal(value);
count++;
}

await cursor.forEach(transform);

expect(count).to.equal(5);
});
}

it('throws when mapping to `null` and cleans up cursor', async function () {
const cursor = collection.find();
cursor.map(() => null);

function iterator() {
expect.fail('Expected no documents from cursor, received at least one.');
}

const error = await cursor.forEach(iterator).catch(e => e);
expect(error).to.be.instanceOf(MongoAPIError);
expect(cursor.closed).to.be.true;
});
});
});