Skip to content

feat(NODE-6472): findOne and find no longer keep open cursors #4580

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
19 changes: 12 additions & 7 deletions src/collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import {
type EstimatedDocumentCountOptions
} from './operations/estimated_document_count';
import { executeOperation } from './operations/execute_operation';
import type { FindOptions } from './operations/find';
import { type FindOneOptions, type FindOptions } from './operations/find';
import {
FindOneAndDeleteOperation,
type FindOneAndDeleteOptions,
Expand Down Expand Up @@ -507,25 +507,30 @@ export class Collection<TSchema extends Document = Document> {
async findOne(filter: Filter<TSchema>): Promise<WithId<TSchema> | null>;
async findOne(
filter: Filter<TSchema>,
options: Omit<FindOptions, 'timeoutMode'> & Abortable
options: Omit<FindOneOptions, 'timeoutMode'> & Abortable
): Promise<WithId<TSchema> | null>;

// allow an override of the schema.
async findOne<T = TSchema>(): Promise<T | null>;
async findOne<T = TSchema>(filter: Filter<TSchema>): Promise<T | null>;
async findOne<T = TSchema>(
filter: Filter<TSchema>,
options?: Omit<FindOptions, 'timeoutMode'> & Abortable
options?: Omit<FindOneOptions, 'timeoutMode'> & Abortable
): Promise<T | null>;

async findOne(
filter: Filter<TSchema> = {},
options: FindOptions & Abortable = {}
options: Omit<FindOneOptions, 'timeoutMode'> & Abortable = {}
): Promise<WithId<TSchema> | null> {
const cursor = this.find(filter, options).limit(-1).batchSize(1);
const res = await cursor.next();
// Explicitly set the limit to 1 and singleBatch to true for all commands, per the spec.
// noCursorTimeout must be unset as well as batchSize.
// See: https://github.com/mongodb/specifications/blob/master/source/crud/crud.md#findone-api-details
const { batchSize: _batchSize, noCursorTimeout: _noCursorTimeout, ...opts } = options;
opts.singleBatch = true;
const cursor = this.find(filter, opts).limit(1);
const result = await cursor.next();
await cursor.close();
return res;
return result;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ export type { DeleteOptions, DeleteResult, DeleteStatement } from './operations/
export type { DistinctOptions } from './operations/distinct';
export type { DropCollectionOptions, DropDatabaseOptions } from './operations/drop';
export type { EstimatedDocumentCountOptions } from './operations/estimated_document_count';
export type { FindOptions } from './operations/find';
export type { FindOneOptions, FindOptions } from './operations/find';
export type {
FindOneAndDeleteOptions,
FindOneAndReplaceOptions,
Expand Down
3 changes: 2 additions & 1 deletion src/operations/execute_operation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,8 +248,9 @@ async function tryOperation<
if (hasWriteAspect && !isRetryableWriteError(previousOperationError))
throw previousOperationError;

if (hasReadAspect && !isRetryableReadError(previousOperationError))
if (hasReadAspect && !isRetryableReadError(previousOperationError)) {
throw previousOperationError;
}

if (
previousOperationError instanceof MongoNetworkError &&
Expand Down
28 changes: 18 additions & 10 deletions src/operations/find.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,16 @@ export interface FindOptions<TSchema extends Document = Document>
timeoutMode?: CursorTimeoutMode;
}

/** @public */
export interface FindOneOptions extends FindOptions {
/** @deprecated Will be removed in the next major version. User provided value will be ignored. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a ticket to remove these fields from FindOneOptions in v7?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd need to add this to the bucket deprecations ticket in V7 or file a new ticket in V7 if the change goes through. (Let's add AC to the ticket to remember to do this). But why are we introducing exposing a brand new interface with deprecated options at all? If we remove these options from the interface, we are going to be back at FindOneOptions being the same as FindOptions - is the intent to replace the definition of this interface in V7 with a copy of FindOptions minus these three?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect that once they're removed, we will have:

/** @public */
export type FindOneOptions = Omit<FindOptions, ....>;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think if we're doing more than just removing deprecations, we should do it in a separate ticket from the bucket work (to streamline the bucketed option removal).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So a separate ticket to NODE-5545? I had added these removals there but can create a another ticket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a very strong preference as far as using the bucket ticket or filing a separate one if the team feels like it's not necessary, but we do want to capture that this won't be strictly deleting the deprecation lines, because I can see us moving quickly down that list in the bucket and inadvertently ending up back at FindOneOptions = FindOptions

batchSize?: number;
/** @deprecated Will be removed in the next major version. User provided value will be ignored. */
limit?: number;
/** @deprecated Will be removed in the next major version. User provided value will be ignored. */
noCursorTimeout?: boolean;
}

/** @internal */
export class FindOperation extends CommandOperation<CursorResponse> {
/**
Expand Down Expand Up @@ -185,17 +195,15 @@ function makeFindCommand(ns: MongoDBNamespace, filter: Document, options: FindOp

if (typeof options.batchSize === 'number') {
if (options.batchSize < 0) {
if (
options.limit &&
options.limit !== 0 &&
Math.abs(options.batchSize) < Math.abs(options.limit)
) {
findCommand.limit = -options.batchSize;
}

findCommand.singleBatch = true;
findCommand.limit = -options.batchSize;
} else {
findCommand.batchSize = options.batchSize;
if (options.batchSize === options.limit) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the if statement above, which set singleBatch: true was only used for findOne I think. Is that dead code we can remove now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I've removed it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed two if-statements above:

  if (typeof options.limit === 'number') {
    if (options.limit < 0) {
      findCommand.limit = -options.limit;
      findCommand.singleBatch = true;
    } else {
      findCommand.limit = options.limit;
    }
  }

We can remove the first nested if-statement there, right? That seems to have been used for find one with the old findOne implementation:

    const cursor = this.find(filter, options).limit(-1).batchSize(1);

// Spec dictates that if these are equal the batchSize should be one more than the
// limit to avoid leaving the cursor open.
findCommand.batchSize = options.batchSize + 1;
} else {
findCommand.batchSize = options.batchSize;
}
}
}

Expand Down
62 changes: 62 additions & 0 deletions test/spec/crud/unified/find.json
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,68 @@
]
}
]
},
{
"description": "Find with batchSize equal to limit",
"operations": [
{
"object": "collection0",
"name": "find",
"arguments": {
"filter": {
"_id": {
"$gt": 1
}
},
"sort": {
"_id": 1
},
"limit": 4,
"batchSize": 4
},
"expectResult": [
{
"_id": 2,
"x": 22
},
{
"_id": 3,
"x": 33
},
{
"_id": 4,
"x": 44
},
{
"_id": 5,
"x": 55
}
]
}
],
"expectEvents": [
{
"client": "client0",
"events": [
{
"commandStartedEvent": {
"command": {
"find": "coll0",
"filter": {
"_id": {
"$gt": 1
}
},
"limit": 4,
"batchSize": 5
},
"commandName": "find",
"databaseName": "find-tests"
}
}
]
}
]
}
]
}
28 changes: 28 additions & 0 deletions test/spec/crud/unified/find.yml
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,31 @@ tests:
- { _id: 2, x: 22 }
- { _id: 3, x: 33 }
- { _id: 4, x: 44 }
-
description: 'Find with batchSize equal to limit'
operations:
-
object: *collection0
name: find
arguments:
filter: { _id: { $gt: 1 } }
sort: { _id: 1 }
limit: 4
batchSize: 4
expectResult:
- { _id: 2, x: 22 }
- { _id: 3, x: 33 }
- { _id: 4, x: 44 }
- { _id: 5, x: 55 }
expectEvents:
- client: *client0
events:
- commandStartedEvent:
command:
find: *collection0Name
filter: { _id: { $gt: 1 } }
limit: 4
# Drivers use limit + 1 for batchSize to ensure the server closes the cursor
batchSize: 5
commandName: find
databaseName: *database0Name
158 changes: 158 additions & 0 deletions test/spec/crud/unified/findOne.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
{
"description": "findOne",
"schemaVersion": "1.0",
"createEntities": [
{
"client": {
"id": "client0",
"observeEvents": [
"commandStartedEvent"
]
}
},
{
"database": {
"id": "database0",
"client": "client0",
"databaseName": "find-tests"
}
},
{
"collection": {
"id": "collection0",
"database": "database0",
"collectionName": "coll0"
}
}
],
"initialData": [
{
"collectionName": "coll0",
"databaseName": "find-tests",
"documents": [
{
"_id": 1,
"x": 11
},
{
"_id": 2,
"x": 22
},
{
"_id": 3,
"x": 33
},
{
"_id": 4,
"x": 44
},
{
"_id": 5,
"x": 55
},
{
"_id": 6,
"x": 66
}
]
}
],
"tests": [
{
"description": "FindOne with filter",
"operations": [
{
"object": "collection0",
"name": "findOne",
"arguments": {
"filter": {
"_id": 1
}
},
"expectResult": {
"_id": 1,
"x": 11
}
}
],
"expectEvents": [
{
"client": "client0",
"events": [
{
"commandStartedEvent": {
"command": {
"find": "coll0",
"filter": {
"_id": 1
},
"batchSize": {
"$$exists": false
},
"limit": 1,
"singleBatch": true
},
"commandName": "find",
"databaseName": "find-tests"
}
}
]
}
]
},
{
"description": "FindOne with filter, sort, and skip",
"operations": [
{
"object": "collection0",
"name": "findOne",
"arguments": {
"filter": {
"_id": {
"$gt": 2
}
},
"sort": {
"_id": 1
},
"skip": 2
},
"expectResult": {
"_id": 5,
"x": 55
}
}
],
"expectEvents": [
{
"client": "client0",
"events": [
{
"commandStartedEvent": {
"command": {
"find": "coll0",
"filter": {
"_id": {
"$gt": 2
}
},
"sort": {
"_id": 1
},
"skip": 2,
"batchSize": {
"$$exists": false
},
"limit": 1,
"singleBatch": true
},
"commandName": "find",
"databaseName": "find-tests"
}
}
]
}
]
}
]
}
Loading
Loading