From c9fc7869c22d4139c66cbf4a2dfd933ba3426f8a Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 9 May 2024 16:12:40 -0400 Subject: [PATCH 01/29] chore: reuse parse work --- src/cmap/connection.ts | 59 +++++--------------- src/cmap/wire_protocol/on_demand/document.ts | 6 +- src/cmap/wire_protocol/responses.ts | 13 ++++- 3 files changed, 29 insertions(+), 49 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index c6420d8306e..27c200fabce 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -62,11 +62,7 @@ import type { ClientMetadata } from './handshake/client_metadata'; import { StreamDescription, type StreamDescriptionOptions } from './stream_description'; import { type CompressorName, decompressResponse } from './wire_protocol/compression'; import { onData } from './wire_protocol/on_data'; -import { - isErrorResponse, - MongoDBResponse, - type MongoDBResponseConstructor -} from './wire_protocol/responses'; +import { MongoDBResponse, type MongoDBResponseConstructor } from './wire_protocol/responses'; import { getReadPreference, isSharded } from './wire_protocol/shared'; /** @internal */ @@ -448,12 +444,7 @@ export class Connection extends TypedEventEmitter { this.socket.setTimeout(0); const bson = response.parse(); - const document = - responseType == null - ? new MongoDBResponse(bson) - : isErrorResponse(bson) - ? new MongoDBResponse(bson) - : new responseType(bson); + const document = (responseType ?? MongoDBResponse).make(bson); yield document; this.throwIfAborted(); @@ -517,11 +508,6 @@ export class Connection extends TypedEventEmitter { this.emit(Connection.CLUSTER_TIME_RECEIVED, document.$clusterTime); } - if (document.has('writeConcernError')) { - object ??= document.toObject(bsonOptions); - throw new MongoWriteConcernError(object.writeConcernError, object); - } - if (document.isError) { throw new MongoServerError((object ??= document.toObject(bsonOptions))); } @@ -552,35 +538,13 @@ export class Connection extends TypedEventEmitter { } } catch (error) { if (this.shouldEmitAndLogCommand) { - if (error.name === 'MongoWriteConcernError') { - this.emitAndLogCommand( - this.monitorCommands, - Connection.COMMAND_SUCCEEDED, - message.databaseName, - this.established, - new CommandSucceededEvent( - this, - message, - options.noResponse ? undefined : (object ??= document?.toObject(bsonOptions)), - started, - this.description.serverConnectionId - ) - ); - } else { - this.emitAndLogCommand( - this.monitorCommands, - Connection.COMMAND_FAILED, - message.databaseName, - this.established, - new CommandFailedEvent( - this, - message, - error, - started, - this.description.serverConnectionId - ) - ); - } + this.emitAndLogCommand( + this.monitorCommands, + Connection.COMMAND_FAILED, + message.databaseName, + this.established, + new CommandFailedEvent(this, message, error, started, this.description.serverConnectionId) + ); } throw error; } @@ -607,8 +571,13 @@ export class Connection extends TypedEventEmitter { ): Promise { this.throwIfAborted(); for await (const document of this.sendCommand(ns, command, options, responseType)) { + if (document.has('writeConcernError')) { + const object = MongoDBResponse.is(document) ? document.toObject(options) : document; + throw new MongoWriteConcernError(object.writeConcernError, object); + } return document; } + throw new MongoUnexpectedServerResponseError('Unable to get response from server'); } diff --git a/src/cmap/wire_protocol/on_demand/document.ts b/src/cmap/wire_protocol/on_demand/document.ts index 638946d647f..bb410627e51 100644 --- a/src/cmap/wire_protocol/on_demand/document.ts +++ b/src/cmap/wire_protocol/on_demand/document.ts @@ -66,9 +66,11 @@ export class OnDemandDocument { /** The start of the document */ private readonly offset = 0, /** If this is an embedded document, indicates if this was a BSON array */ - public readonly isArray = false + public readonly isArray = false, + /** If elements was already calculated */ + elements?: BSONElement[] ) { - this.elements = parseToElementsToArray(this.bson, offset); + this.elements = elements ?? parseToElementsToArray(this.bson, offset); } /** Only supports basic latin strings */ diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index 65515cbb316..956b9c78de2 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -1,4 +1,5 @@ import { + type BSONElement, type BSONSerializeOptions, BSONType, type Document, @@ -30,8 +31,7 @@ const enum BSONElementOffset { * * @param bytes - BSON document returned from the server */ -export function isErrorResponse(bson: Uint8Array): boolean { - const elements = parseToElementsToArray(bson, 0); +export function isErrorResponse(bson: Uint8Array, elements: BSONElement[]): boolean { for (let eIdx = 0; eIdx < elements.length; eIdx++) { const element = elements[eIdx]; @@ -60,6 +60,7 @@ export function isErrorResponse(bson: Uint8Array): boolean { /** @internal */ export type MongoDBResponseConstructor = { new (bson: Uint8Array, offset?: number, isArray?: boolean): MongoDBResponse; + make(bson: Uint8Array): MongoDBResponse; }; /** @internal */ @@ -68,6 +69,14 @@ export class MongoDBResponse extends OnDemandDocument { return value instanceof MongoDBResponse; } + static make(bson: Uint8Array) { + const elements = parseToElementsToArray(bson, 0); + const isError = isErrorResponse(bson, elements); + return isError + ? new MongoDBResponse(bson, 0, false, elements) + : new this(bson, 0, false, elements); + } + // {ok:1} static empty = new MongoDBResponse(new Uint8Array([13, 0, 0, 0, 16, 111, 107, 0, 1, 0, 0, 0, 0])); From 91418d8e750b77a8195f765e895d26c67c9d905a Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 9 May 2024 16:14:32 -0400 Subject: [PATCH 02/29] chore: turn back on cursorResponse --- src/cursor/find_cursor.ts | 2 +- src/operations/find.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cursor/find_cursor.ts b/src/cursor/find_cursor.ts index bb21d49fbed..94ec996b20a 100644 --- a/src/cursor/find_cursor.ts +++ b/src/cursor/find_cursor.ts @@ -117,7 +117,7 @@ export class FindCursor extends AbstractCursor { } } - const response = await super.getMore(batchSize, false); + const response = await super.getMore(batchSize, this.client.autoEncrypter ? false : true); // TODO: wrap this in some logic to prevent it from happening if we don't need this support if (CursorResponse.is(response)) { this[kNumReturned] = this[kNumReturned] + response.batchSize; diff --git a/src/operations/find.ts b/src/operations/find.ts index cdf1a711767..1c2ccdb1cac 100644 --- a/src/operations/find.ts +++ b/src/operations/find.ts @@ -1,4 +1,5 @@ import type { Document } from '../bson'; +import { CursorResponse } from '../cmap/wire_protocol/responses'; import { MongoInvalidArgumentError } from '../error'; import { ReadConcern } from '../read_concern'; import type { Server } from '../sdam/server'; @@ -114,7 +115,7 @@ export class FindOperation extends CommandOperation { documentsReturnedIn: 'firstBatch', session }, - undefined + this.explain ? undefined : CursorResponse ); } } From 414cdfa5137a2890b5650e0b7b7b7d0dd4256fc9 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 9 May 2024 16:24:48 -0400 Subject: [PATCH 03/29] wip --- src/cmap/connection.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 27c200fabce..d15655dc5ce 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -571,7 +571,10 @@ export class Connection extends TypedEventEmitter { ): Promise { this.throwIfAborted(); for await (const document of this.sendCommand(ns, command, options, responseType)) { - if (document.has('writeConcernError')) { + if ( + (MongoDBResponse.is(document) && document.has('writeConcernError')) || + (!MongoDBResponse.is(document) && document.writeConcernError) + ) { const object = MongoDBResponse.is(document) ? document.toObject(options) : document; throw new MongoWriteConcernError(object.writeConcernError, object); } From cd29ba1375c14d9954907163a09dffdb1aadb438 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 10 May 2024 12:06:03 -0400 Subject: [PATCH 04/29] wip --- src/cmap/wire_protocol/on_demand/document.ts | 11 +++-- src/cmap/wire_protocol/responses.ts | 50 ++++++++++---------- src/cursor/abstract_cursor.ts | 6 ++- test/benchmarks/driverBench/index.js | 20 ++++++-- test/benchmarks/mongoBench/runner.js | 7 +++ 5 files changed, 62 insertions(+), 32 deletions(-) diff --git a/src/cmap/wire_protocol/on_demand/document.ts b/src/cmap/wire_protocol/on_demand/document.ts index bb410627e51..247e8ca8579 100644 --- a/src/cmap/wire_protocol/on_demand/document.ts +++ b/src/cmap/wire_protocol/on_demand/document.ts @@ -80,8 +80,13 @@ export class OnDemandDocument { if (name.length !== nameLength) return false; - for (let i = 0; i < name.length; i++) { - if (this.bson[nameOffset + i] !== name.charCodeAt(i)) return false; + const nameEnd = nameOffset + nameLength; + for ( + let byteIndex = nameOffset, charIndex = 0; + charIndex < name.length && byteIndex < nameEnd; + charIndex++, byteIndex++ + ) { + if (this.bson[byteIndex] !== name.charCodeAt(charIndex)) return false; } return true; @@ -127,7 +132,7 @@ export class OnDemandDocument { const element = this.elements[index]; // skip this element if it has already been associated with a name - if (!this.indexFound[index] && this.isElementName(name, element)) { + if (!(index in this.indexFound) && this.isElementName(name, element)) { const cachedElement = { element, value: undefined }; this.cache[name] = cachedElement; this.indexFound[index] = true; diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index 956b9c78de2..3e091293d69 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -5,6 +5,7 @@ import { type Document, Long, parseToElementsToArray, + pluckBSONSerializeOptions, type Timestamp } from '../../bson'; import { MongoUnexpectedServerResponseError } from '../../error'; @@ -153,13 +154,7 @@ export class MongoDBResponse extends OnDemandDocument { public override toObject(options?: BSONSerializeOptions): Record { const exactBSONOptions = { - useBigInt64: options?.useBigInt64, - promoteLongs: options?.promoteLongs, - promoteValues: options?.promoteValues, - promoteBuffers: options?.promoteBuffers, - bsonRegExp: options?.bsonRegExp, - raw: options?.raw ?? false, - fieldsAsRaw: options?.fieldsAsRaw ?? {}, + ...pluckBSONSerializeOptions(options ?? {}), validation: this.parseBsonSerializationOptions(options) }; return super.toObject(exactBSONOptions); @@ -188,33 +183,38 @@ export class CursorResponse extends MongoDBResponse { return value instanceof CursorResponse || value === CursorResponse.emptyGetMore; } - public id: Long; - public ns: MongoDBNamespace | null = null; - public batchSize = 0; - - private batch: OnDemandDocument; + private _batch: OnDemandDocument | null = null; private iterated = 0; - constructor(bytes: Uint8Array, offset?: number, isArray?: boolean) { - super(bytes, offset, isArray); + get cursor() { + return this.get('cursor', BSONType.object, true); + } - const cursor = this.get('cursor', BSONType.object, true); + get id(): Long { + return Long.fromBigInt(this.cursor.get('id', BSONType.long, true)); + } - const id = cursor.get('id', BSONType.long, true); - this.id = new Long(Number(id & 0xffff_ffffn), Number((id >> 32n) & 0xffff_ffffn)); + get ns() { + const namespace = this.cursor.get('ns', BSONType.string); + if (namespace != null) return ns(namespace); + return null; + } - const namespace = cursor.get('ns', BSONType.string); - if (namespace != null) this.ns = ns(namespace); + get length() { + return Math.max(this.batchSize - this.iterated, 0); + } - if (cursor.has('firstBatch')) this.batch = cursor.get('firstBatch', BSONType.array, true); - else if (cursor.has('nextBatch')) this.batch = cursor.get('nextBatch', BSONType.array, true); + get batch() { + if (this._batch != null) return this._batch; + const cursor = this.cursor; + if (cursor.has('firstBatch')) this._batch = cursor.get('firstBatch', BSONType.array, true); + else if (cursor.has('nextBatch')) this._batch = cursor.get('nextBatch', BSONType.array, true); else throw new MongoUnexpectedServerResponseError('Cursor document did not contain a batch'); - - this.batchSize = this.batch.size(); + return this._batch; } - get length() { - return Math.max(this.batchSize - this.iterated, 0); + get batchSize() { + return this.batch?.size(); } shift(options?: BSONSerializeOptions): any { diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index c4f349500a1..d389178302d 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -302,7 +302,7 @@ export abstract class AbstractCursor< return bufferedDocs; } - async *[Symbol.asyncIterator](): AsyncGenerator { + private async *asyncIterator() { if (this.closed) { return; } @@ -350,6 +350,10 @@ export abstract class AbstractCursor< } } + async *[Symbol.asyncIterator](): AsyncGenerator { + yield* this.asyncIterator(); + } + stream(options?: CursorStreamOptions): Readable & AsyncIterable { if (options?.transform) { const transform = options.transform; diff --git a/test/benchmarks/driverBench/index.js b/test/benchmarks/driverBench/index.js index 0b1986455b9..378720c86a6 100644 --- a/test/benchmarks/driverBench/index.js +++ b/test/benchmarks/driverBench/index.js @@ -1,14 +1,26 @@ 'use strict'; const MongoBench = require('../mongoBench'); +const process = require('node:process'); const os = require('node:os'); +const util = require('node:util'); + +const args = util.parseArgs({ + args: process.argv.slice(2), + options: { + grep: { + short: 'g', + type: 'string', + required: false + } + } +}); const Runner = MongoBench.Runner; let bsonType = 'js-bson'; // TODO(NODE-4606): test against different driver configurations in CI -const { inspect } = require('util'); const { writeFile } = require('fs/promises'); const { makeParallelBenchmarks, makeSingleBench, makeMultiBench } = require('../mongoBench/suites'); @@ -30,7 +42,9 @@ function average(arr) { return arr.reduce((x, y) => x + y, 0) / arr.length; } -const benchmarkRunner = new Runner() +const benchmarkRunner = new Runner({ + grep: args.values.grep ?? null +}) .suite('singleBench', suite => makeSingleBench(suite)) .suite('multiBench', suite => makeMultiBench(suite)) .suite('parallel', suite => makeParallelBenchmarks(suite)); @@ -96,7 +110,7 @@ benchmarkRunner }) .then(data => { const results = JSON.stringify(data, undefined, 2); - console.log(inspect(data, { depth: Infinity, colors: true })); + console.log(util.inspect(data, { depth: Infinity, colors: true })); return writeFile('results.json', results); }) .catch(err => console.error(err)); diff --git a/test/benchmarks/mongoBench/runner.js b/test/benchmarks/mongoBench/runner.js index 064f51dec58..ca91e88cd33 100644 --- a/test/benchmarks/mongoBench/runner.js +++ b/test/benchmarks/mongoBench/runner.js @@ -63,6 +63,7 @@ class Runner { console.log.apply(console, arguments); }; this.children = {}; + this.grep = options.grep?.toLowerCase() ?? null; } /** @@ -124,6 +125,12 @@ class Runner { const result = {}; for (const [name, benchmark] of benchmarks) { + if (this.grep != null) { + if (!name.toLowerCase().includes(this.grep)) { + result[name] = 0; + continue; + } + } this.reporter(` Executing Benchmark "${name}"`); result[name] = await this._runBenchmark(benchmark); } From a3424122fe7e6f3e12b6289ddf90e4ea4c68851f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 15 May 2024 17:09:46 -0400 Subject: [PATCH 05/29] feat(NODE-6136): parse cursor responses on demand --- src/bson.ts | 1 + src/cmap/wire_protocol/responses.ts | 79 +++++-- src/cursor/abstract_cursor.ts | 68 ++---- src/cursor/aggregation_cursor.ts | 18 +- src/cursor/change_stream_cursor.ts | 44 ++-- src/cursor/find_cursor.ts | 35 ++- src/cursor/run_command_cursor.ts | 26 +-- src/error.ts | 4 +- src/index.ts | 7 +- src/operations/aggregate.ts | 17 +- src/operations/command.ts | 20 +- src/operations/count_documents.ts | 14 +- src/operations/execute_operation.ts | 3 +- src/operations/find.ts | 11 +- src/operations/get_more.ts | 6 +- src/operations/indexes.ts | 10 +- src/operations/list_collections.ts | 11 +- src/operations/run_command.ts | 22 +- .../collection-management/collection.test.ts | 199 ++++++++---------- .../crud/abstract_operation.test.ts | 6 +- test/integration/crud/explain.test.ts | 3 + 21 files changed, 294 insertions(+), 310 deletions(-) diff --git a/src/bson.ts b/src/bson.ts index 7938a2b173a..ce5bd486b78 100644 --- a/src/bson.ts +++ b/src/bson.ts @@ -27,6 +27,7 @@ export { UUID } from 'bson'; +/** @internal */ export type BSONElement = BSON.OnDemand['BSONElement']; export function parseToElementsToArray(bytes: Uint8Array, offset?: number): BSONElement[] { diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index 3e091293d69..1d7b0bd71f6 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -10,7 +10,7 @@ import { } from '../../bson'; import { MongoUnexpectedServerResponseError } from '../../error'; import { type ClusterTime } from '../../sdam/common'; -import { type MongoDBNamespace, ns } from '../../utils'; +import { ns } from '../../utils'; import { OnDemandDocument } from './on_demand/document'; // eslint-disable-next-line no-restricted-syntax @@ -171,13 +171,29 @@ export class MongoDBResponse extends OnDemandDocument { } } +// Here's a litle blast from the past. +// OLD style method definition so that I can override get without redefining ALL the fancy TS :/ +Object.defineProperty(MongoDBResponse.prototype, 'get', { + value: function get(name: any, as: any, required: any) { + try { + return OnDemandDocument.prototype.get.call(this, name, as, required); + } catch (cause) { + throw new MongoUnexpectedServerResponseError(cause.message, { cause }); + } + } +}); + /** @internal */ 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 = { id: new Long(0), length: 0, shift: () => null }; + static emptyGetMore: CursorResponse = { + id: new Long(0), + length: 0, + shift: () => null + } as unknown as CursorResponse; static override is(value: unknown): value is CursorResponse { return value instanceof CursorResponse || value === CursorResponse.emptyGetMore; @@ -186,25 +202,25 @@ export class CursorResponse extends MongoDBResponse { private _batch: OnDemandDocument | null = null; private iterated = 0; - get cursor() { + private get cursor() { return this.get('cursor', BSONType.object, true); } - get id(): Long { + public get id(): Long { return Long.fromBigInt(this.cursor.get('id', BSONType.long, true)); } - get ns() { + public get ns() { const namespace = this.cursor.get('ns', BSONType.string); if (namespace != null) return ns(namespace); return null; } - get length() { + public get length() { return Math.max(this.batchSize - this.iterated, 0); } - get batch() { + private get batch() { if (this._batch != null) return this._batch; const cursor = this.cursor; if (cursor.has('firstBatch')) this._batch = cursor.get('firstBatch', BSONType.array, true); @@ -213,11 +229,21 @@ export class CursorResponse extends MongoDBResponse { return this._batch; } - get batchSize() { + public get batchSize() { return this.batch?.size(); } - shift(options?: BSONSerializeOptions): any { + public get postBatchResumeToken() { + return ( + this.cursor.get('postBatchResumeToken', BSONType.object)?.toObject({ + promoteValues: false, + promoteLongs: false, + promoteBuffers: false + }) ?? null + ); + } + + public shift(options?: BSONSerializeOptions): any { if (this.iterated >= this.batchSize) { return null; } @@ -232,15 +258,40 @@ export class CursorResponse extends MongoDBResponse { } } - clear() { + public clear() { this.iterated = this.batchSize; } +} + +/** + * Explain responses have nothing to do with cursor responses + * This class serves to temporarily avoid refactoring how cursors handle + * explain responses which is to detect that the response is not cursor-like and return the explain + * result as the "first and only" document in the "batch" and end the "cursor" + */ +export class ExplainedCursorResponse extends CursorResponse { + isExplain = true; + + override get id(): Long { + return Long.fromBigInt(0n); + } + + override get batchSize() { + return 0; + } + + override get ns() { + return null; + } - pushMany() { - throw new Error('pushMany Unsupported method'); + _length = 1; + override get length(): number { + return this._length; } - push() { - throw new Error('push Unsupported method'); + override shift(options?: BSONSerializeOptions | undefined) { + if (this._length === 0) return null; + this._length -= 1; + return this.toObject(options); } } diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index d389178302d..1a0cb73242f 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -13,7 +13,7 @@ import { MongoTailableCursorError } from '../error'; import type { MongoClient } from '../mongo_client'; -import { type TODO_NODE_3286, TypedEventEmitter } from '../mongo_types'; +import { TypedEventEmitter } from '../mongo_types'; import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; import { GetMoreOperation } from '../operations/get_more'; import { KillCursorsOperation } from '../operations/kill_cursors'; @@ -21,7 +21,7 @@ import { ReadConcern, type ReadConcernLike } from '../read_concern'; import { ReadPreference, type ReadPreferenceLike } from '../read_preference'; import type { Server } from '../sdam/server'; import { ClientSession, maybeClearPinnedConnection } from '../sessions'; -import { List, type MongoDBNamespace, ns, squashError } from '../utils'; +import { type MongoDBNamespace, squashError } from '../utils'; /** @internal */ const kId = Symbol('id'); @@ -145,13 +145,7 @@ export abstract class AbstractCursor< /** @internal */ [kNamespace]: MongoDBNamespace; /** @internal */ - [kDocuments]: { - length: number; - shift(bsonOptions?: any): TSchema | null; - clear(): void; - pushMany(many: Iterable): void; - push(item: TSchema): void; - }; + [kDocuments]: CursorResponse = { length: 0 } as unknown as CursorResponse; /** @internal */ [kClient]: MongoClient; /** @internal */ @@ -182,7 +176,6 @@ export abstract class AbstractCursor< this[kClient] = client; this[kNamespace] = namespace; this[kId] = null; - this[kDocuments] = new List(); this[kInitialized] = false; this[kClosed] = false; this[kKilled] = false; @@ -637,13 +630,12 @@ export abstract class AbstractCursor< protected abstract _initialize(session: ClientSession | undefined): Promise; /** @internal */ - async getMore(batchSize: number, useCursorResponse = false): Promise { + async getMore(batchSize: number): Promise { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const getMoreOperation = new GetMoreOperation(this[kNamespace], this[kId]!, this[kServer]!, { ...this[kOptions], session: this[kSession], - batchSize, - useCursorResponse + batchSize }); return await executeOperation(this[kClient], getMoreOperation); @@ -661,37 +653,13 @@ export abstract class AbstractCursor< const state = await this._initialize(this[kSession]); const response = state.response; this[kServer] = state.server; - if (CursorResponse.is(response)) { - this[kId] = response.id; - if (response.ns) this[kNamespace] = response.ns; - this[kDocuments] = response; - } else if (response.cursor) { - // TODO(NODE-2674): Preserve int64 sent from MongoDB - this[kId] = - typeof response.cursor.id === 'number' - ? Long.fromNumber(response.cursor.id) - : typeof response.cursor.id === 'bigint' - ? Long.fromBigInt(response.cursor.id) - : response.cursor.id; - - if (response.cursor.ns) { - this[kNamespace] = ns(response.cursor.ns); - } - - this[kDocuments].pushMany(response.cursor.firstBatch); - } - // When server responses return without a cursor document, we close this cursor - // and return the raw server response. This is often the case for explain commands - // for example - if (this[kId] == null) { - this[kId] = Long.ZERO; - // TODO(NODE-3286): ExecutionResult needs to accept a generic parameter - this[kDocuments].push(state.response as TODO_NODE_3286); - } + if (!CursorResponse.is(response)) throw new Error('ah'); - // the cursor is now initialized, even if it is dead - this[kInitialized] = true; + this[kId] = response.id; + this[kNamespace] = response.ns ?? this[kNamespace]; + this[kDocuments] = response; + this[kInitialized] = true; // the cursor is now initialized, even if it is dead } catch (error) { // the cursor is now initialized, even if an error occurred this[kInitialized] = true; @@ -802,20 +770,8 @@ async function next( try { const response = await cursor.getMore(batchSize); - if (CursorResponse.is(response)) { - cursor[kId] = response.id; - cursor[kDocuments] = response; - } else if (response) { - const cursorId = - typeof response.cursor.id === 'number' - ? Long.fromNumber(response.cursor.id) - : typeof response.cursor.id === 'bigint' - ? Long.fromBigInt(response.cursor.id) - : response.cursor.id; - - cursor[kDocuments].pushMany(response.cursor.nextBatch); - cursor[kId] = cursorId; - } + cursor[kId] = response.id; + cursor[kDocuments] = response; } catch (error) { try { await cleanupCursor(cursor, { error, needsToEmitClosed: true }); diff --git a/src/cursor/aggregation_cursor.ts b/src/cursor/aggregation_cursor.ts index cba77e9b52f..73ddcba40c7 100644 --- a/src/cursor/aggregation_cursor.ts +++ b/src/cursor/aggregation_cursor.ts @@ -76,14 +76,16 @@ export class AggregationCursor extends AbstractCursor { /** Execute the explain for the cursor */ async explain(verbosity?: ExplainVerbosityLike): Promise { - return await executeOperation( - this.client, - new AggregateOperation(this.namespace, this[kPipeline], { - ...this[kOptions], // NOTE: order matters here, we may need to refine this - ...this.cursorOptions, - explain: verbosity ?? true - }) - ); + return ( + await executeOperation( + this.client, + new AggregateOperation(this.namespace, this[kPipeline], { + ...this[kOptions], // NOTE: order matters here, we may need to refine this + ...this.cursorOptions, + explain: verbosity ?? true + }) + ) + ).shift(this[kOptions]); } /** Add a stage to the aggregation pipeline diff --git a/src/cursor/change_stream_cursor.ts b/src/cursor/change_stream_cursor.ts index 31fda3308b4..7bebcc2d92b 100644 --- a/src/cursor/change_stream_cursor.ts +++ b/src/cursor/change_stream_cursor.ts @@ -1,4 +1,4 @@ -import type { Document, Long, Timestamp } from '../bson'; +import type { Document } from '../bson'; import { ChangeStream, type ChangeStreamDocument, @@ -6,9 +6,9 @@ import { type OperationTime, type ResumeToken } from '../change_stream'; +import { type CursorResponse } from '../cmap/wire_protocol/responses'; import { INIT, RESPONSE } from '../constants'; import type { MongoClient } from '../mongo_client'; -import type { TODO_NODE_3286 } from '../mongo_types'; import { AggregateOperation } from '../operations/aggregate'; import type { CollationOptions } from '../operations/command'; import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; @@ -26,25 +26,13 @@ export interface ChangeStreamCursorOptions extends AbstractCursorOptions { fullDocument?: string; } -/** @internal */ -export type ChangeStreamAggregateRawResult = { - $clusterTime: { clusterTime: Timestamp }; - cursor: { - postBatchResumeToken: ResumeToken; - ns: string; - id: number | Long; - } & ({ firstBatch: TChange[] } | { nextBatch: TChange[] }); - ok: 1; - operationTime: Timestamp; -}; - /** @internal */ export class ChangeStreamCursor< TSchema extends Document = Document, TChange extends Document = ChangeStreamDocument > extends AbstractCursor { _resumeToken: ResumeToken; - startAtOperationTime?: OperationTime; + startAtOperationTime: OperationTime | null = null; hasReceived?: boolean; resumeAfter: ResumeToken; startAfter: ResumeToken; @@ -71,7 +59,7 @@ export class ChangeStreamCursor< this.pipeline = pipeline; this.options = options; this._resumeToken = null; - this.startAtOperationTime = options.startAtOperationTime; + this.startAtOperationTime = options.startAtOperationTime ?? null; if (options.startAfter) { this.resumeToken = options.startAfter; @@ -120,15 +108,13 @@ export class ChangeStreamCursor< this.hasReceived = true; } - _processBatch(response: ChangeStreamAggregateRawResult): void { - const cursor = response.cursor; - if (cursor.postBatchResumeToken) { - this.postBatchResumeToken = response.cursor.postBatchResumeToken; + _processBatch(response: CursorResponse): void { + const { postBatchResumeToken } = response; + if (postBatchResumeToken) { + this.postBatchResumeToken = postBatchResumeToken; - const batch = - 'firstBatch' in response.cursor ? response.cursor.firstBatch : response.cursor.nextBatch; - if (batch.length === 0) { - this.resumeToken = cursor.postBatchResumeToken; + if (response.batchSize === 0) { + this.resumeToken = postBatchResumeToken; } } } @@ -146,10 +132,7 @@ export class ChangeStreamCursor< session }); - const response = await executeOperation< - TODO_NODE_3286, - ChangeStreamAggregateRawResult - >(session.client, aggregateOperation); + const response = await executeOperation(session.client, aggregateOperation); const server = aggregateOperation.server; this.maxWireVersion = maxWireVersion(server); @@ -172,11 +155,12 @@ export class ChangeStreamCursor< return { server, session, response }; } - override async getMore(batchSize: number): Promise { + override async getMore(batchSize: number): Promise { const response = await super.getMore(batchSize); this.maxWireVersion = maxWireVersion(this.server); - this._processBatch(response as ChangeStreamAggregateRawResult); + // as ChangeStreamAggregateRawResult + this._processBatch(response); this.emit(ChangeStream.MORE, response); this.emit(ChangeStream.RESPONSE); diff --git a/src/cursor/find_cursor.ts b/src/cursor/find_cursor.ts index 94ec996b20a..2966cfd4dbf 100644 --- a/src/cursor/find_cursor.ts +++ b/src/cursor/find_cursor.ts @@ -79,19 +79,14 @@ export class FindCursor extends AbstractCursor { const response = await executeOperation(this.client, findOperation); // the response is not a cursor when `explain` is enabled - if (CursorResponse.is(response)) { - this[kNumReturned] = response.batchSize; - } else { - // Can be an explain response, hence the ?. on everything - this[kNumReturned] = this[kNumReturned] + (response?.cursor?.firstBatch?.length ?? 0); - } + this[kNumReturned] = response.batchSize; // TODO: NODE-2882 return { server: findOperation.server, session, response }; } /** @internal */ - override async getMore(batchSize: number): Promise { + override async getMore(batchSize: number): Promise { const numReturned = this[kNumReturned]; if (numReturned) { // TODO(DRIVERS-1448): Remove logic to enforce `limit` in the driver @@ -117,13 +112,9 @@ export class FindCursor extends AbstractCursor { } } - const response = await super.getMore(batchSize, this.client.autoEncrypter ? false : true); + const response = await super.getMore(batchSize); // TODO: wrap this in some logic to prevent it from happening if we don't need this support - if (CursorResponse.is(response)) { - this[kNumReturned] = this[kNumReturned] + response.batchSize; - } else { - this[kNumReturned] = this[kNumReturned] + (response?.cursor?.nextBatch?.length ?? 0); - } + this[kNumReturned] = this[kNumReturned] + response.batchSize; return response; } @@ -151,14 +142,16 @@ export class FindCursor extends AbstractCursor { /** Execute the explain for the cursor */ async explain(verbosity?: ExplainVerbosityLike): Promise { - return await executeOperation( - this.client, - new FindOperation(this.namespace, this[kFilter], { - ...this[kBuiltOptions], // NOTE: order matters here, we may need to refine this - ...this.cursorOptions, - explain: verbosity ?? true - }) - ); + return ( + await executeOperation( + this.client, + new FindOperation(this.namespace, this[kFilter], { + ...this[kBuiltOptions], // NOTE: order matters here, we may need to refine this + ...this.cursorOptions, + explain: verbosity ?? true + }) + ) + ).shift(this[kBuiltOptions]); } /** Set the cursor query */ diff --git a/src/cursor/run_command_cursor.ts b/src/cursor/run_command_cursor.ts index 553041492f4..10ca22f7564 100644 --- a/src/cursor/run_command_cursor.ts +++ b/src/cursor/run_command_cursor.ts @@ -1,6 +1,7 @@ -import type { BSONSerializeOptions, Document, Long } from '../bson'; +import type { BSONSerializeOptions, Document } from '../bson'; +import { CursorResponse } from '../cmap/wire_protocol/responses'; import type { Db } from '../db'; -import { MongoAPIError, MongoUnexpectedServerResponseError } from '../error'; +import { MongoAPIError } from '../error'; import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; import { GetMoreOperation } from '../operations/get_more'; import { RunCommandOperation } from '../operations/run_command'; @@ -16,12 +17,6 @@ export type RunCursorCommandOptions = { session?: ClientSession; } & BSONSerializeOptions; -/** @internal */ -type RunCursorCommandResponse = { - cursor: { id: bigint | Long | number; ns: string; firstBatch: Document[] }; - ok: 1; -}; - /** @public */ export class RunCommandCursor extends AbstractCursor { public readonly command: Readonly>; @@ -103,15 +98,15 @@ export class RunCommandCursor extends AbstractCursor { /** @internal */ protected async _initialize(session: ClientSession): Promise { - const operation = new RunCommandOperation(this.db, this.command, { + const operation = new RunCommandOperation(this.db, this.command, { ...this.cursorOptions, session: session, - readPreference: this.cursorOptions.readPreference + readPreference: this.cursorOptions.readPreference, + responseType: CursorResponse }); + const response = await executeOperation(this.client, operation); - if (response.cursor == null) { - throw new MongoUnexpectedServerResponseError('Expected server to respond with cursor'); - } + return { server: operation.server, session, @@ -120,13 +115,12 @@ export class RunCommandCursor extends AbstractCursor { } /** @internal */ - override async getMore(_batchSize: number): Promise { + override async getMore(_batchSize: number): Promise { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const getMoreOperation = new GetMoreOperation(this.namespace, this.id!, this.server!, { ...this.cursorOptions, session: this.session, - ...this.getMoreOptions, - useCursorResponse: false + ...this.getMoreOptions }); return await executeOperation(this.client, getMoreOperation); diff --git a/src/error.ts b/src/error.ts index 294062e3d1c..cee15493b28 100644 --- a/src/error.ts +++ b/src/error.ts @@ -750,8 +750,8 @@ export class MongoUnexpectedServerResponseError extends MongoRuntimeError { * * @public **/ - constructor(message: string) { - super(message); + constructor(message: string, options?: { cause?: Error }) { + super(message, options); } override get name(): string { diff --git a/src/index.ts b/src/index.ts index 7c0bfdf841d..8546a30de05 100644 --- a/src/index.ts +++ b/src/index.ts @@ -160,7 +160,7 @@ export { SrvPollingEvent } from './sdam/srv_polling'; // type only exports below, these are removed from emitted JS export type { AdminPrivate } from './admin'; -export type { BSONSerializeOptions, Document } from './bson'; +export type { BSONElement, BSONSerializeOptions, Document } from './bson'; export type { deserialize, serialize } from './bson'; export type { BulkResult, @@ -338,10 +338,7 @@ export type { } from './cursor/abstract_cursor'; export type { InternalAbstractCursorOptions } from './cursor/abstract_cursor'; export type { AggregationCursorOptions } from './cursor/aggregation_cursor'; -export type { - ChangeStreamAggregateRawResult, - ChangeStreamCursorOptions -} from './cursor/change_stream_cursor'; +export type { ChangeStreamCursorOptions } from './cursor/change_stream_cursor'; export type { ListSearchIndexesCursor, ListSearchIndexesOptions diff --git a/src/operations/aggregate.ts b/src/operations/aggregate.ts index a3a9349c273..63a057e0872 100644 --- a/src/operations/aggregate.ts +++ b/src/operations/aggregate.ts @@ -1,6 +1,6 @@ import type { Document } from '../bson'; +import { CursorResponse, ExplainedCursorResponse } from '../cmap/wire_protocol/responses'; import { MongoInvalidArgumentError } from '../error'; -import { type TODO_NODE_3286 } from '../mongo_types'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; import { maxWireVersion, type MongoDBNamespace } from '../utils'; @@ -37,7 +37,7 @@ export interface AggregateOptions extends CommandOperationOptions { } /** @internal */ -export class AggregateOperation extends CommandOperation { +export class AggregateOperation extends CommandOperation { override options: AggregateOptions; target: string | typeof DB_AGGREGATE_COLLECTION; pipeline: Document[]; @@ -94,7 +94,10 @@ export class AggregateOperation extends CommandOperation { this.pipeline.push(stage); } - override async execute(server: Server, session: ClientSession | undefined): Promise { + override async execute( + server: Server, + session: ClientSession | undefined + ): Promise { const options: AggregateOptions = this.options; const serverWireVersion = maxWireVersion(server); const command: Document = { aggregate: this.target, pipeline: this.pipeline }; @@ -134,8 +137,12 @@ export class AggregateOperation extends CommandOperation { command.cursor.batchSize = options.batchSize; } - const res: TODO_NODE_3286 = await super.executeCommand(server, session, command); - return res; + return await super.executeCommand( + server, + session, + command, + this.explain ? ExplainedCursorResponse : CursorResponse + ); } } diff --git a/src/operations/command.ts b/src/operations/command.ts index fbcafe3f8b5..033ec8aa943 100644 --- a/src/operations/command.ts +++ b/src/operations/command.ts @@ -1,4 +1,5 @@ import type { BSONSerializeOptions, Document } from '../bson'; +import { type MongoDBResponseConstructor } from '../cmap/wire_protocol/responses'; import { MongoInvalidArgumentError } from '../error'; import { Explain, type ExplainOptions } from '../explain'; import { ReadConcern } from '../read_concern'; @@ -106,12 +107,25 @@ export abstract class CommandOperation extends AbstractOperation { return true; } - async executeCommand( + public async executeCommand( + server: Server, + session: ClientSession | undefined, + cmd: Document, + responseType: T | undefined + ): Promise>; + + public async executeCommand( server: Server, session: ClientSession | undefined, cmd: Document + ): Promise; + + async executeCommand( + server: Server, + session: ClientSession | undefined, + cmd: Document, + responseType?: MongoDBResponseConstructor ): Promise { - // TODO: consider making this a non-enumerable property this.server = server; const options = { @@ -152,6 +166,6 @@ export abstract class CommandOperation extends AbstractOperation { cmd = decorateWithExplain(cmd, this.explain); } - return await server.command(this.ns, cmd, options); + return await server.command(this.ns, cmd, options, responseType); } } diff --git a/src/operations/count_documents.ts b/src/operations/count_documents.ts index 11fb0c3db69..a7037a64ba4 100644 --- a/src/operations/count_documents.ts +++ b/src/operations/count_documents.ts @@ -13,7 +13,7 @@ export interface CountDocumentsOptions extends AggregateOptions { } /** @internal */ -export class CountDocumentsOperation extends AggregateOperation { +export class CountDocumentsOperation extends AggregateOperation { constructor(collection: Collection, query: Document, options: CountDocumentsOptions) { const pipeline = []; pipeline.push({ $match: query }); @@ -31,16 +31,8 @@ export class CountDocumentsOperation extends AggregateOperation { super(collection.s.namespace, pipeline, options); } - override async execute(server: Server, session: ClientSession | undefined): Promise { + override async execute(server: Server, session: ClientSession | undefined): Promise { const result = await super.execute(server, session); - - // NOTE: We're avoiding creating a cursor here to reduce the callstack. - const response = result as unknown as Document; - if (response.cursor == null || response.cursor.firstBatch == null) { - return 0; - } - - const docs = response.cursor.firstBatch; - return docs.length ? docs[0].n : 0; + return result.shift()?.n ?? 0; } } diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 4faf4fd95ad..6b453a60926 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -1,4 +1,3 @@ -import type { Document } from '../bson'; import { type CursorResponse } from '../cmap/wire_protocol/responses'; import { isRetryableReadError, @@ -45,7 +44,7 @@ export interface ExecutionResult { /** The session used for this operation, may be implicitly created */ session?: ClientSession; /** The raw server response for the operation */ - response: Document | CursorResponse; + response: CursorResponse; } /** diff --git a/src/operations/find.ts b/src/operations/find.ts index 1c2ccdb1cac..a040af73bc6 100644 --- a/src/operations/find.ts +++ b/src/operations/find.ts @@ -1,5 +1,5 @@ import type { Document } from '../bson'; -import { CursorResponse } from '../cmap/wire_protocol/responses'; +import { CursorResponse, ExplainedCursorResponse } from '../cmap/wire_protocol/responses'; import { MongoInvalidArgumentError } from '../error'; import { ReadConcern } from '../read_concern'; import type { Server } from '../sdam/server'; @@ -66,7 +66,7 @@ export interface FindOptions } /** @internal */ -export class FindOperation extends CommandOperation { +export class FindOperation extends CommandOperation { /** * @remarks WriteConcern can still be present on the options because * we inherit options from the client/db/collection. The @@ -96,7 +96,10 @@ export class FindOperation extends CommandOperation { return 'find' as const; } - override async execute(server: Server, session: ClientSession | undefined): Promise { + override async execute( + server: Server, + session: ClientSession | undefined + ): Promise { this.server = server; const options = this.options; @@ -115,7 +118,7 @@ export class FindOperation extends CommandOperation { documentsReturnedIn: 'firstBatch', session }, - this.explain ? undefined : CursorResponse + this.explain ? ExplainedCursorResponse : CursorResponse ); } } diff --git a/src/operations/get_more.ts b/src/operations/get_more.ts index 05f54b0b57c..1688d919834 100644 --- a/src/operations/get_more.ts +++ b/src/operations/get_more.ts @@ -20,8 +20,6 @@ export interface GetMoreOptions extends OperationOptions { maxTimeMS?: number; /** TODO(NODE-4413): Address bug with maxAwaitTimeMS not being passed in from the cursor correctly */ maxAwaitTimeMS?: number; - - useCursorResponse: boolean; } /** @@ -58,7 +56,7 @@ export class GetMoreOperation extends AbstractOperation { * Although there is a server already associated with the get more operation, the signature * for execute passes a server so we will just use that one. */ - override async execute(server: Server, _session: ClientSession | undefined): Promise { + override async execute(server: Server, _session: ClientSession | undefined): Promise { if (server !== this.server) { throw new MongoRuntimeError('Getmore must run on the same server operation began on'); } @@ -103,7 +101,7 @@ export class GetMoreOperation extends AbstractOperation { this.ns, getMoreCmd, commandOptions, - this.options.useCursorResponse ? CursorResponse : undefined + CursorResponse ); } } diff --git a/src/operations/indexes.ts b/src/operations/indexes.ts index 8bf390f28b7..fda3fa80dd6 100644 --- a/src/operations/indexes.ts +++ b/src/operations/indexes.ts @@ -1,4 +1,5 @@ import type { Document } from '../bson'; +import { CursorResponse } from '../cmap/wire_protocol/responses'; import type { Collection } from '../collection'; import { type AbstractCursorOptions } from '../cursor/abstract_cursor'; import { MongoCompatibilityError } from '../error'; @@ -353,7 +354,7 @@ export class DropIndexOperation extends CommandOperation { export type ListIndexesOptions = AbstractCursorOptions; /** @internal */ -export class ListIndexesOperation extends CommandOperation { +export class ListIndexesOperation extends CommandOperation { /** * @remarks WriteConcern can still be present on the options because * we inherit options from the client/db/collection. The @@ -376,7 +377,10 @@ export class ListIndexesOperation extends CommandOperation { return 'listIndexes' as const; } - override async execute(server: Server, session: ClientSession | undefined): Promise { + override async execute( + server: Server, + session: ClientSession | undefined + ): Promise { const serverWireVersion = maxWireVersion(server); const cursor = this.options.batchSize ? { batchSize: this.options.batchSize } : {}; @@ -389,7 +393,7 @@ export class ListIndexesOperation extends CommandOperation { command.comment = this.options.comment; } - return await super.executeCommand(server, session, command); + return await super.executeCommand(server, session, command, CursorResponse); } } diff --git a/src/operations/list_collections.ts b/src/operations/list_collections.ts index 2d3819cac95..e94300f1205 100644 --- a/src/operations/list_collections.ts +++ b/src/operations/list_collections.ts @@ -1,4 +1,5 @@ import type { Binary, Document } from '../bson'; +import { CursorResponse } from '../cmap/wire_protocol/responses'; import type { Db } from '../db'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; @@ -17,7 +18,7 @@ export interface ListCollectionsOptions extends Omit { +export class ListCollectionsOperation extends CommandOperation { /** * @remarks WriteConcern can still be present on the options because * we inherit options from the client/db/collection. The @@ -51,11 +52,15 @@ export class ListCollectionsOperation extends CommandOperation { return 'listCollections' as const; } - override async execute(server: Server, session: ClientSession | undefined): Promise { + override async execute( + server: Server, + session: ClientSession | undefined + ): Promise { return await super.executeCommand( server, session, - this.generateCommand(maxWireVersion(server)) + this.generateCommand(maxWireVersion(server)), + CursorResponse ); } diff --git a/src/operations/run_command.ts b/src/operations/run_command.ts index 9042407b3e2..ad7d02c044f 100644 --- a/src/operations/run_command.ts +++ b/src/operations/run_command.ts @@ -1,4 +1,5 @@ import type { BSONSerializeOptions, Document } from '../bson'; +import { type MongoDBResponseConstructor } from '../cmap/wire_protocol/responses'; import { type Db } from '../db'; import { type TODO_NODE_3286 } from '../mongo_types'; import type { ReadPreferenceLike } from '../read_preference'; @@ -17,7 +18,11 @@ export type RunCommandOptions = { /** @internal */ export class RunCommandOperation extends AbstractOperation { - constructor(parent: Db, public command: Document, public override options: RunCommandOptions) { + constructor( + parent: Db, + public command: Document, + public override options: RunCommandOptions & { responseType?: MongoDBResponseConstructor } + ) { super(options); this.ns = parent.s.namespace.withCollection('$cmd'); } @@ -28,11 +33,16 @@ export class RunCommandOperation extends AbstractOperation { override async execute(server: Server, session: ClientSession | undefined): Promise { this.server = server; - const res: TODO_NODE_3286 = await server.command(this.ns, this.command, { - ...this.options, - readPreference: this.readPreference, - session - }); + const res: TODO_NODE_3286 = await server.command( + this.ns, + this.command, + { + ...this.options, + readPreference: this.readPreference, + session + }, + this.options.responseType + ); return res; } } diff --git a/test/integration/collection-management/collection.test.ts b/test/integration/collection-management/collection.test.ts index 9db80c43ebd..3077d93dbbe 100644 --- a/test/integration/collection-management/collection.test.ts +++ b/test/integration/collection-management/collection.test.ts @@ -2,6 +2,7 @@ import { expect } from 'chai'; import { Collection, type Db, isHello, type MongoClient, MongoServerError } from '../../mongodb'; import * as mock from '../../tools/mongodb-mock/index'; +import { type FailPoint } from '../../tools/utils'; import { setupDatabase } from '../shared'; describe('Collection', function () { @@ -422,144 +423,110 @@ describe('Collection', function () { }); }); - describe('#countDocuments', function () { - let client; - let db; - let collection; + describe('countDocuments()', function () { + let client: MongoClient; + let collection: Collection<{ test: string }>; + let aggCommands; beforeEach(async function () { - client = configuration.newClient({ w: 1 }); - await client.connect(); - db = client.db(configuration.db); - collection = db.collection('test_coll'); - await collection.insertOne({ a: 'c' }); + client = this.configuration.newClient({ monitorCommands: true }); + collection = client.db('test').collection('countDocuments'); + await collection.insertMany( + Array.from({ length: 100 }, (_, i) => ({ test: i < 50 ? 'a' : 'b' })) + ); + aggCommands = []; + client.on('commandStarted', ev => { + if (ev.commandName === 'aggregate') aggCommands.push(ev.command); + }); }); afterEach(async function () { - await collection.drop(); + await collection.deleteMany({}); await client.close(); }); - context('when passing a non-matching query', function () { - it('returns 0', async function () { - const result = await collection.countDocuments({ a: 'b' }); - expect(result).to.equal(0); - }); - }); + it('returns the correct count as a js number', async () => { + const count = await collection.countDocuments({}); + expect(count).to.be.a('number').that.equals(100); - it('returns a promise', function () { - const docsPromise = collection.countDocuments(); - expect(docsPromise).to.exist.and.to.be.an.instanceof(Promise); - return docsPromise.then(result => expect(result).to.equal(1)); - }); - }); + const countDefault = await collection.countDocuments(); + expect(countDefault).to.be.a('number').that.equals(100); - describe('countDocuments with mock server', function () { - let server; + const countA = await collection.countDocuments({ test: 'a' }); + expect(countA).to.be.a('number').that.equals(50); - beforeEach(() => { - return mock.createServer().then(s => { - server = s; - }); + const countC = await collection.countDocuments({ test: 'c' }); + expect(countC).to.be.a('number').that.equals(0); }); - afterEach(() => mock.cleanup()); - - function testCountDocMock(testConfiguration, config, done) { - const client = testConfiguration.newClient(`mongodb://${server.uri()}/test`, { - serverApi: null // TODO(NODE-3807): remove resetting serverApi when the usage of mongodb mock server is removed + context('when a filter is applied', () => { + it('adds a $match pipeline', async () => { + await collection.countDocuments({ test: 'a' }); + expect(aggCommands[0]) + .to.have.property('pipeline') + .that.deep.equals([{ $match: { test: 'a' } }, { $group: { _id: 1, n: { $sum: 1 } } }]); }); - const close = e => client.close(() => done(e)); - - server.setMessageHandler(request => { - const doc = request.document; - if (doc.aggregate) { - try { - config.replyHandler(doc); - request.reply(config.reply); - } catch (e) { - close(e); - } - } + }); - if (isHello(doc)) { - request.reply(Object.assign({}, mock.HELLO)); - } else if (doc.endSessions) { - request.reply({ ok: 1 }); - } + context('when aggregation fails', () => { + beforeEach(async function () { + await client + .db() + .admin() + .command({ + configureFailPoint: 'failCommand', + mode: 'alwaysOn', + data: { failCommands: ['aggregate'], errorCode: 1 } + } as FailPoint); }); - const db = client.db('test'); - const collection = db.collection('countDoc_mock'); - - config.executeCountDocuments(collection, close); - } - - it('countDocuments should return appropriate error if aggregation fails with callback given', function (done) { - const replyHandler = () => null; - const executeCountDocuments = (collection, close) => { - collection.countDocuments(err => { - expect(err).to.exist; - expect(err.errmsg).to.equal('aggregation error - callback'); - close(); - }); - }; + afterEach(async function () { + await client + .db() + .admin() + .command({ + configureFailPoint: 'failCommand', + mode: 'off', + data: { failCommands: ['aggregate'] } + } as FailPoint); + }); - testCountDocMock( - configuration, - { - replyHandler, - executeCountDocuments, - reply: { ok: 0, errmsg: 'aggregation error - callback' } - }, - done - ); + it('rejects the countDocuments API', async () => { + const error = await collection.countDocuments().catch(error => error); + expect(error).to.be.instanceOf(MongoServerError); + }); }); - it('countDocuments should error if aggregation fails using Promises', function (done) { - const replyHandler = () => null; - const executeCountDocuments = (collection, close) => { - collection - .countDocuments() - .then(() => expect(false).to.equal(true)) // should never get here; error should be caught - .catch(e => { - expect(e.errmsg).to.equal('aggregation error - promise'); - close(); - }); - }; - - testCountDocMock( - configuration, - { - replyHandler, - executeCountDocuments, - reply: { ok: 0, errmsg: 'aggregation error - promise' } - }, - done - ); - }); + context('when provided with options', () => { + it('adds $skip stage to the pipeline', async () => { + await collection.countDocuments({}, { skip: 1 }); + expect(aggCommands[0]) + .to.have.property('pipeline') + .that.deep.equals([{ $match: {} }, { $skip: 1 }, { $group: { _id: 1, n: { $sum: 1 } } }]); + }); - it('countDocuments pipeline should be correct with skip and limit applied', function (done) { - const replyHandler = doc => { - expect(doc.pipeline).to.deep.include({ $skip: 1 }); - expect(doc.pipeline).to.deep.include({ $limit: 1 }); - }; - const executeCountDocuments = (collection, close) => { - collection.countDocuments({}, { limit: 1, skip: 1 }, err => { - expect(err).to.not.exist; - close(); - }); - }; + it('adds $limit stage to the pipeline', async () => { + await collection.countDocuments({}, { limit: 1 }); + expect(aggCommands[0]) + .to.have.property('pipeline') + .that.deep.equals([ + { $match: {} }, + { $limit: 1 }, + { $group: { _id: 1, n: { $sum: 1 } } } + ]); + }); - testCountDocMock( - configuration, - { - replyHandler, - executeCountDocuments, - reply: { ok: 1 } - }, - done - ); + it('adds $skip and $limit stages to the pipeline', async () => { + await collection.countDocuments({}, { skip: 1, limit: 1 }); + expect(aggCommands[0]) + .to.have.property('pipeline') + .that.deep.equals([ + { $match: {} }, + { $skip: 1 }, + { $limit: 1 }, + { $group: { _id: 1, n: { $sum: 1 } } } + ]); + }); }); }); diff --git a/test/integration/crud/abstract_operation.test.ts b/test/integration/crud/abstract_operation.test.ts index 77c45890e26..dd972cd316f 100644 --- a/test/integration/crud/abstract_operation.test.ts +++ b/test/integration/crud/abstract_operation.test.ts @@ -322,7 +322,11 @@ describe('abstract operation', function () { it(`operation.commandName equals key in command document`, async function () { const subclassInstance = subclassCreator(); const yieldDoc = - subclassType.name === 'ProfilingLevelOperation' ? { ok: 1, was: 1 } : { ok: 1 }; + subclassType.name === 'ProfilingLevelOperation' + ? { ok: 1, was: 1 } + : subclassType.name === 'CountDocumentsOperation' + ? { shift: () => ({ n: 1 }) } + : { ok: 1 }; const cmdCallerStub = sinon.stub(Server.prototype, 'command').resolves(yieldDoc); if (sameServerOnlyOperationSubclasses.includes(subclassType.name.toString())) { await subclassInstance.execute(constructorServer, client.session); diff --git a/test/integration/crud/explain.test.ts b/test/integration/crud/explain.test.ts index c99e4f1709d..189a1ca3878 100644 --- a/test/integration/crud/explain.test.ts +++ b/test/integration/crud/explain.test.ts @@ -88,6 +88,9 @@ describe('CRUD API explain option', function () { context(`When explain is ${explainValue}, operation ${name}`, function () { it(`sets command verbosity to ${explainValue} and includes ${explainValueToExpectation(explainValue)} in the return response`, async function () { const response = await op.op(explainValue).catch(error => error); + if (response instanceof Error && !(response instanceof MongoServerError)) { + throw response; + } const commandStartedEvent = await commandStartedPromise; const explainJson = JSON.stringify(response); switch (explainValue) { From 9eff48ae4bcd216eb55df6520e68682e43ccbbae Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 10 May 2024 12:06:03 -0400 Subject: [PATCH 06/29] wip --- src/cmap/wire_protocol/responses.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index 1d7b0bd71f6..b9f230abe90 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -202,7 +202,7 @@ export class CursorResponse extends MongoDBResponse { private _batch: OnDemandDocument | null = null; private iterated = 0; - private get cursor() { + get cursor() { return this.get('cursor', BSONType.object, true); } From 4534adac39a713d56598a81dc96d8fe8764d383f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 17 May 2024 14:44:00 -0400 Subject: [PATCH 07/29] fix: FLE --- src/client-side-encryption/auto_encrypter.ts | 79 +++---------------- .../client_encryption.ts | 10 +-- src/client-side-encryption/state_machine.ts | 14 ++-- src/cmap/connection.ts | 31 ++++++-- src/cmap/wire_protocol/responses.ts | 28 ++++++- src/constants.ts | 9 +++ src/cursor/abstract_cursor.ts | 3 - src/utils.ts | 54 ++++++++++++- 8 files changed, 133 insertions(+), 95 deletions(-) diff --git a/src/client-side-encryption/auto_encrypter.ts b/src/client-side-encryption/auto_encrypter.ts index e6334dc57de..65b5bf71664 100644 --- a/src/client-side-encryption/auto_encrypter.ts +++ b/src/client-side-encryption/auto_encrypter.ts @@ -6,6 +6,7 @@ import { import { deserialize, type Document, serialize } from '../bson'; import { type CommandOptions, type ProxyOptions } from '../cmap/connection'; +import { kDecorateResult } from '../constants'; import { getMongoDBClientEncryption } from '../deps'; import { MongoRuntimeError } from '../error'; import { MongoClient, type MongoClientOptions } from '../mongo_client'; @@ -212,15 +213,6 @@ export const AutoEncryptionLoggerLevel = Object.freeze({ export type AutoEncryptionLoggerLevel = (typeof AutoEncryptionLoggerLevel)[keyof typeof AutoEncryptionLoggerLevel]; -// Typescript errors if we index objects with `Symbol.for(...)`, so -// to avoid TS errors we pull them out into variables. Then we can type -// the objects (and class) that we expect to see them on and prevent TS -// errors. -/** @internal */ -const kDecorateResult = Symbol.for('@@mdb.decorateDecryptionResult'); -/** @internal */ -const kDecoratedKeys = Symbol.for('@@mdb.decryptedKeys'); - /** * @internal An internal class to be used by the driver for auto encryption * **NOTE**: Not meant to be instantiated directly, this is for internal use only. @@ -467,16 +459,18 @@ export class AutoEncrypter { proxyOptions: this._proxyOptions, tlsOptions: this._tlsOptions }); - return await stateMachine.execute(this, context); + + return deserialize(await stateMachine.execute(this, context), { + promoteValues: false, + promoteLongs: false + }); } /** * Decrypt a command response */ - async decrypt(response: Uint8Array | Document, options: CommandOptions = {}): Promise { - const buffer = Buffer.isBuffer(response) ? response : serialize(response, options); - - const context = this._mongocrypt.makeDecryptionContext(buffer); + async decrypt(response: Uint8Array, options: CommandOptions = {}): Promise { + const context = this._mongocrypt.makeDecryptionContext(response); context.id = this._contextCounter++; @@ -486,12 +480,7 @@ export class AutoEncrypter { tlsOptions: this._tlsOptions }); - const decorateResult = this[kDecorateResult]; - const result = await stateMachine.execute(this, context); - if (decorateResult) { - decorateDecryptionResult(result, response); - } - return result; + return await stateMachine.execute(this, context); } /** @@ -518,53 +507,3 @@ export class AutoEncrypter { return AutoEncrypter.getMongoCrypt().libmongocryptVersion; } } - -/** - * Recurse through the (identically-shaped) `decrypted` and `original` - * objects and attach a `decryptedKeys` property on each sub-object that - * contained encrypted fields. Because we only call this on BSON responses, - * we do not need to worry about circular references. - * - * @internal - */ -function decorateDecryptionResult( - decrypted: Document & { [kDecoratedKeys]?: Array }, - original: Document, - isTopLevelDecorateCall = true -): void { - if (isTopLevelDecorateCall) { - // The original value could have been either a JS object or a BSON buffer - if (Buffer.isBuffer(original)) { - original = deserialize(original); - } - if (Buffer.isBuffer(decrypted)) { - throw new MongoRuntimeError('Expected result of decryption to be deserialized BSON object'); - } - } - - if (!decrypted || typeof decrypted !== 'object') return; - for (const k of Object.keys(decrypted)) { - const originalValue = original[k]; - - // An object was decrypted by libmongocrypt if and only if it was - // a BSON Binary object with subtype 6. - if (originalValue && originalValue._bsontype === 'Binary' && originalValue.sub_type === 6) { - if (!decrypted[kDecoratedKeys]) { - Object.defineProperty(decrypted, kDecoratedKeys, { - value: [], - configurable: true, - enumerable: false, - writable: false - }); - } - // this is defined in the preceding if-statement - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - decrypted[kDecoratedKeys]!.push(k); - // Do not recurse into this decrypted value. It could be a sub-document/array, - // in which case there is no original value associated with its subfields. - continue; - } - - decorateDecryptionResult(decrypted[k], originalValue, false); - } -} diff --git a/src/client-side-encryption/client_encryption.ts b/src/client-side-encryption/client_encryption.ts index a9e77ed191e..6d2ff784ee6 100644 --- a/src/client-side-encryption/client_encryption.ts +++ b/src/client-side-encryption/client_encryption.ts @@ -5,7 +5,7 @@ import type { MongoCryptOptions } from 'mongodb-client-encryption'; -import { type Binary, type Document, type Long, serialize, type UUID } from '../bson'; +import { type Binary, deserialize, type Document, type Long, serialize, type UUID } from '../bson'; import { type AnyBulkWriteOperation, type BulkWriteResult } from '../bulk/common'; import { type ProxyOptions } from '../cmap/connection'; import { type Collection } from '../collection'; @@ -202,7 +202,7 @@ export class ClientEncryption { tlsOptions: this._tlsOptions }); - const dataKey = await stateMachine.execute(this, context); + const dataKey = deserialize(await stateMachine.execute(this, context)) as DataKey; const { db: dbName, collection: collectionName } = MongoDBCollectionNamespace.fromString( this._keyVaultNamespace @@ -259,7 +259,7 @@ export class ClientEncryption { tlsOptions: this._tlsOptions }); - const { v: dataKeys } = await stateMachine.execute<{ v: DataKey[] }>(this, context); + const { v: dataKeys } = deserialize(await stateMachine.execute(this, context)); if (dataKeys.length === 0) { return {}; } @@ -640,7 +640,7 @@ export class ClientEncryption { tlsOptions: this._tlsOptions }); - const { v } = await stateMachine.execute<{ v: T }>(this, context); + const { v } = deserialize(await stateMachine.execute(this, context)); return v; } @@ -719,7 +719,7 @@ export class ClientEncryption { }); const context = this._mongoCrypt.makeExplicitEncryptionContext(valueBuffer, contextOptions); - const result = await stateMachine.execute<{ v: Binary }>(this, context); + const result = deserialize(await stateMachine.execute(this, context)); return result.v; } } diff --git a/src/client-side-encryption/state_machine.ts b/src/client-side-encryption/state_machine.ts index a4b2379fb51..94df0c728d3 100644 --- a/src/client-side-encryption/state_machine.ts +++ b/src/client-side-encryption/state_machine.ts @@ -112,6 +112,9 @@ export type CSFLEKMSTlsOptions = { azure?: ClientEncryptionTlsOptions; }; +/** `{ v: [] }` */ +const EMPTY_V = Uint8Array.from([13, 0, 0, 0, 4, 118, 0, 5, 0, 0, 0, 0, 0]); + /** * @internal * @@ -154,16 +157,13 @@ export class StateMachine { /** * Executes the state machine according to the specification */ - async execute( - executor: StateMachineExecutable, - context: MongoCryptContext - ): Promise { + async execute(executor: StateMachineExecutable, context: MongoCryptContext): Promise { const keyVaultNamespace = executor._keyVaultNamespace; const keyVaultClient = executor._keyVaultClient; const metaDataClient = executor._metaDataClient; const mongocryptdClient = executor._mongocryptdClient; const mongocryptdManager = executor._mongocryptdManager; - let result: T | null = null; + let result: Uint8Array | null = null; while (context.state !== MONGOCRYPT_CTX_DONE && context.state !== MONGOCRYPT_CTX_ERROR) { debug(`[context#${context.id}] ${stateToString.get(context.state) || context.state}`); @@ -220,7 +220,7 @@ export class StateMachine { // do not. We set the result manually here, and let the state machine continue. `libmongocrypt` // will inform us if we need to error by setting the state to `MONGOCRYPT_CTX_ERROR` but // otherwise we'll return `{ v: [] }`. - result = { v: [] } as any as T; + result = EMPTY_V; } for await (const key of keys) { context.addMongoOperationResponse(serialize(key)); @@ -252,7 +252,7 @@ export class StateMachine { const message = context.status.message || 'Finalization error'; throw new MongoCryptError(message); } - result = deserialize(finalizedContext, this.options) as T; + result = finalizedContext; break; } diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index d15655dc5ce..ee512123d65 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -1,14 +1,15 @@ import { type Readable, Transform, type TransformCallback } from 'stream'; import { clearTimeout, setTimeout } from 'timers'; -import type { BSONSerializeOptions, Document, ObjectId } from '../bson'; -import type { AutoEncrypter } from '../client-side-encryption/auto_encrypter'; +import { type BSONSerializeOptions, deserialize, type Document, type ObjectId } from '../bson'; +import { type AutoEncrypter } from '../client-side-encryption/auto_encrypter'; import { CLOSE, CLUSTER_TIME_RECEIVED, COMMAND_FAILED, COMMAND_STARTED, COMMAND_SUCCEEDED, + kDecorateResult, PINNED, UNPINNED } from '../constants'; @@ -33,6 +34,7 @@ import { BufferPool, calculateDurationInMs, type Callback, + decorateDecryptionResult, HostAddress, maxWireVersion, type MongoDBNamespace, @@ -721,7 +723,7 @@ export class CryptoConnection extends Connection { ns: MongoDBNamespace, cmd: Document, options?: CommandOptions, - _responseType?: T | undefined + responseType?: T | undefined ): Promise { const { autoEncrypter } = this; if (!autoEncrypter) { @@ -735,7 +737,7 @@ export class CryptoConnection extends Connection { const serverWireVersion = maxWireVersion(this); if (serverWireVersion === 0) { // This means the initial handshake hasn't happened yet - return await super.command(ns, cmd, options, undefined); + return await super.command(ns, cmd, options, responseType); } if (serverWireVersion < 8) { @@ -769,8 +771,25 @@ export class CryptoConnection extends Connection { } } - const response = await super.command(ns, encrypted, options, undefined); + const encryptedResponse: MongoDBResponse = (await super.command( + ns, + encrypted, + options, + (responseType ?? MongoDBResponse) as any + )) as unknown as MongoDBResponse; - return await autoEncrypter.decrypt(response, options); + const result = await autoEncrypter.decrypt(encryptedResponse.toBytes(), options); + + const decryptedResponse = responseType?.make(result) ?? deserialize(result, options); + + if (autoEncrypter[kDecorateResult]) { + if (responseType == null) { + decorateDecryptionResult(decryptedResponse, encryptedResponse.toObject(), true); + } else { + decryptedResponse.encryptedResponse = encryptedResponse; + } + } + + return decryptedResponse; } } diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index b9f230abe90..793cb99b996 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -10,7 +10,7 @@ import { } from '../../bson'; import { MongoUnexpectedServerResponseError } from '../../error'; import { type ClusterTime } from '../../sdam/common'; -import { ns } from '../../utils'; +import { decorateDecryptionResult, ns } from '../../utils'; import { OnDemandDocument } from './on_demand/document'; // eslint-disable-next-line no-restricted-syntax @@ -169,6 +169,9 @@ export class MongoDBResponse extends OnDemandDocument { } return { utf8: { writeErrors: false } }; } + + // TODO: Supports decorating result + encryptedResponse?: MongoDBResponse; } // Here's a litle blast from the past. @@ -220,6 +223,21 @@ export class CursorResponse extends MongoDBResponse { return Math.max(this.batchSize - this.iterated, 0); } + private _encryptedBatch: OnDemandDocument | null = null; + get encryptedBatch() { + if (this.encryptedResponse == null) return null; + if (this._encryptedBatch != null) return this._encryptedBatch; + + const cursor = this.encryptedResponse?.get('cursor', BSONType.object); + if (cursor?.has('firstBatch')) + this._encryptedBatch = cursor.get('firstBatch', BSONType.array, true); + else if (cursor?.has('nextBatch')) + this._encryptedBatch = cursor.get('nextBatch', BSONType.array, true); + else throw new MongoUnexpectedServerResponseError('Cursor document did not contain a batch'); + + return this._encryptedBatch; + } + private get batch() { if (this._batch != null) return this._batch; const cursor = this.cursor; @@ -249,12 +267,18 @@ export class CursorResponse extends MongoDBResponse { } const result = this.batch.get(this.iterated, BSONType.object, true) ?? null; + const encryptedResult = this.encryptedBatch?.get(this.iterated, BSONType.object, true) ?? null; + this.iterated += 1; if (options?.raw) { return result.toBytes(); } else { - return result.toObject(options); + const object = result.toObject(options); + if (encryptedResult) { + decorateDecryptionResult(object, encryptedResult.toObject(options), true); + } + return object; } } diff --git a/src/constants.ts b/src/constants.ts index 293749cad89..abb69509f94 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -165,3 +165,12 @@ export const LEGACY_HELLO_COMMAND = 'ismaster'; * The legacy hello command that was deprecated in MongoDB 5.0. */ export const LEGACY_HELLO_COMMAND_CAMEL_CASE = 'isMaster'; + +// Typescript errors if we index objects with `Symbol.for(...)`, so +// to avoid TS errors we pull them out into variables. Then we can type +// the objects (and class) that we expect to see them on and prevent TS +// errors. +/** @internal */ +export const kDecorateResult = Symbol.for('@@mdb.decorateDecryptionResult'); +/** @internal */ +export const kDecoratedKeys = Symbol.for('@@mdb.decryptedKeys'); diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index 1a0cb73242f..0ae55f81b34 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -653,9 +653,6 @@ export abstract class AbstractCursor< const state = await this._initialize(this[kSession]); const response = state.response; this[kServer] = state.server; - - if (!CursorResponse.is(response)) throw new Error('ah'); - this[kId] = response.id; this[kNamespace] = response.ns ?? this[kNamespace]; this[kDocuments] = response; diff --git a/src/utils.ts b/src/utils.ts index 2ede778258d..cebb81e0a0d 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -8,11 +8,11 @@ import * as url from 'url'; import { URL } from 'url'; import { promisify } from 'util'; -import { type Document, ObjectId, resolveBSONOptions } from './bson'; +import { deserialize, type Document, ObjectId, resolveBSONOptions } from './bson'; import type { Connection } from './cmap/connection'; import { MAX_SUPPORTED_WIRE_VERSION } from './cmap/wire_protocol/constants'; import type { Collection } from './collection'; -import { LEGACY_HELLO_COMMAND } from './constants'; +import { kDecoratedKeys, LEGACY_HELLO_COMMAND } from './constants'; import type { AbstractCursor } from './cursor/abstract_cursor'; import type { FindCursor } from './cursor/find_cursor'; import type { Db } from './db'; @@ -1366,3 +1366,53 @@ export async function fileIsAccessible(fileName: string, mode?: number) { export function noop() { return; } + +/** + * Recurse through the (identically-shaped) `decrypted` and `original` + * objects and attach a `decryptedKeys` property on each sub-object that + * contained encrypted fields. Because we only call this on BSON responses, + * we do not need to worry about circular references. + * + * @internal + */ +export function decorateDecryptionResult( + decrypted: Document & { [kDecoratedKeys]?: Array }, + original: Document, + isTopLevelDecorateCall = true +): void { + if (isTopLevelDecorateCall) { + // The original value could have been either a JS object or a BSON buffer + if (Buffer.isBuffer(original)) { + original = deserialize(original); + } + if (Buffer.isBuffer(decrypted)) { + throw new MongoRuntimeError('Expected result of decryption to be deserialized BSON object'); + } + } + + if (!decrypted || typeof decrypted !== 'object') return; + for (const k of Object.keys(decrypted)) { + const originalValue = original[k]; + + // An object was decrypted by libmongocrypt if and only if it was + // a BSON Binary object with subtype 6. + if (originalValue && originalValue._bsontype === 'Binary' && originalValue.sub_type === 6) { + if (!decrypted[kDecoratedKeys]) { + Object.defineProperty(decrypted, kDecoratedKeys, { + value: [], + configurable: true, + enumerable: false, + writable: false + }); + } + // this is defined in the preceding if-statement + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + decrypted[kDecoratedKeys]!.push(k); + // Do not recurse into this decrypted value. It could be a sub-document/array, + // in which case there is no original value associated with its subfields. + continue; + } + + decorateDecryptionResult(decrypted[k], originalValue, false); + } +} From 007572966fbd8f3282a10a908fd01ddecf1d0f3f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 17 May 2024 15:10:30 -0400 Subject: [PATCH 08/29] test: needs fail points --- test/integration/collection-management/collection.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/collection-management/collection.test.ts b/test/integration/collection-management/collection.test.ts index 3077d93dbbe..698d96667bd 100644 --- a/test/integration/collection-management/collection.test.ts +++ b/test/integration/collection-management/collection.test.ts @@ -468,7 +468,7 @@ describe('Collection', function () { }); }); - context('when aggregation fails', () => { + describe('when aggregation fails', { requires: { mongodb: '>=4.4' } }, () => { beforeEach(async function () { await client .db() From 79acc71ef66a227973b47b4946dbb2f3a50280dd Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 17 May 2024 15:20:18 -0400 Subject: [PATCH 09/29] test: undo bench changes --- test/benchmarks/driverBench/index.js | 20 +++----------------- test/benchmarks/mongoBench/runner.js | 7 ------- 2 files changed, 3 insertions(+), 24 deletions(-) diff --git a/test/benchmarks/driverBench/index.js b/test/benchmarks/driverBench/index.js index 378720c86a6..0b1986455b9 100644 --- a/test/benchmarks/driverBench/index.js +++ b/test/benchmarks/driverBench/index.js @@ -1,26 +1,14 @@ 'use strict'; const MongoBench = require('../mongoBench'); -const process = require('node:process'); const os = require('node:os'); -const util = require('node:util'); - -const args = util.parseArgs({ - args: process.argv.slice(2), - options: { - grep: { - short: 'g', - type: 'string', - required: false - } - } -}); const Runner = MongoBench.Runner; let bsonType = 'js-bson'; // TODO(NODE-4606): test against different driver configurations in CI +const { inspect } = require('util'); const { writeFile } = require('fs/promises'); const { makeParallelBenchmarks, makeSingleBench, makeMultiBench } = require('../mongoBench/suites'); @@ -42,9 +30,7 @@ function average(arr) { return arr.reduce((x, y) => x + y, 0) / arr.length; } -const benchmarkRunner = new Runner({ - grep: args.values.grep ?? null -}) +const benchmarkRunner = new Runner() .suite('singleBench', suite => makeSingleBench(suite)) .suite('multiBench', suite => makeMultiBench(suite)) .suite('parallel', suite => makeParallelBenchmarks(suite)); @@ -110,7 +96,7 @@ benchmarkRunner }) .then(data => { const results = JSON.stringify(data, undefined, 2); - console.log(util.inspect(data, { depth: Infinity, colors: true })); + console.log(inspect(data, { depth: Infinity, colors: true })); return writeFile('results.json', results); }) .catch(err => console.error(err)); diff --git a/test/benchmarks/mongoBench/runner.js b/test/benchmarks/mongoBench/runner.js index ca91e88cd33..064f51dec58 100644 --- a/test/benchmarks/mongoBench/runner.js +++ b/test/benchmarks/mongoBench/runner.js @@ -63,7 +63,6 @@ class Runner { console.log.apply(console, arguments); }; this.children = {}; - this.grep = options.grep?.toLowerCase() ?? null; } /** @@ -125,12 +124,6 @@ class Runner { const result = {}; for (const [name, benchmark] of benchmarks) { - if (this.grep != null) { - if (!name.toLowerCase().includes(this.grep)) { - result[name] = 0; - continue; - } - } this.reporter(` Executing Benchmark "${name}"`); result[name] = await this._runBenchmark(benchmark); } From dde8250ff0112d5ba868c99cb78564b29cf8ae3d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 17 May 2024 15:38:02 -0400 Subject: [PATCH 10/29] chore: cleanup --- src/cursor/abstract_cursor.ts | 2 +- src/operations/get_more.ts | 14 ++++++-------- .../collection-management/collection.test.ts | 3 +-- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index 0ae55f81b34..826594a55dc 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -1,7 +1,7 @@ import { Readable, Transform } from 'stream'; import { type BSONSerializeOptions, type Document, Long, pluckBSONSerializeOptions } from '../bson'; -import { CursorResponse } from '../cmap/wire_protocol/responses'; +import { type CursorResponse } from '../cmap/wire_protocol/responses'; import { type AnyError, MongoAPIError, diff --git a/src/operations/get_more.ts b/src/operations/get_more.ts index 1688d919834..aa550721b6f 100644 --- a/src/operations/get_more.ts +++ b/src/operations/get_more.ts @@ -1,4 +1,4 @@ -import type { Document, Long } from '../bson'; +import type { Long } from '../bson'; import { CursorResponse } from '../cmap/wire_protocol/responses'; import { MongoRuntimeError } from '../error'; import type { Server } from '../sdam/server'; @@ -56,7 +56,10 @@ export class GetMoreOperation extends AbstractOperation { * Although there is a server already associated with the get more operation, the signature * for execute passes a server so we will just use that one. */ - override async execute(server: Server, _session: ClientSession | undefined): Promise { + override async execute( + server: Server, + _session: ClientSession | undefined + ): Promise { if (server !== this.server) { throw new MongoRuntimeError('Getmore must run on the same server operation began on'); } @@ -97,12 +100,7 @@ export class GetMoreOperation extends AbstractOperation { ...this.options }; - return await server.command( - this.ns, - getMoreCmd, - commandOptions, - CursorResponse - ); + return await server.command(this.ns, getMoreCmd, commandOptions, CursorResponse); } } diff --git a/test/integration/collection-management/collection.test.ts b/test/integration/collection-management/collection.test.ts index 698d96667bd..92183df7ff5 100644 --- a/test/integration/collection-management/collection.test.ts +++ b/test/integration/collection-management/collection.test.ts @@ -1,7 +1,6 @@ import { expect } from 'chai'; -import { Collection, type Db, isHello, type MongoClient, MongoServerError } from '../../mongodb'; -import * as mock from '../../tools/mongodb-mock/index'; +import { Collection, type Db, type MongoClient, MongoServerError } from '../../mongodb'; import { type FailPoint } from '../../tools/utils'; import { setupDatabase } from '../shared'; From 0b825f1dc4bbb9a41f3607d2b7331a6a6bd631aa Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 3 Jun 2024 16:30:13 -0400 Subject: [PATCH 11/29] wip --- src/cmap/connection.ts | 13 +---- src/cmap/wire_protocol/responses.ts | 18 +++--- .../collection-management/collection.test.ts | 7 +++ .../unit/assorted/sessions_collection.test.js | 22 ------- .../auto_encrypter.test.ts | 6 +- .../unit/cmap/wire_protocol/responses.test.ts | 58 +++++++++++++------ test/unit/error.test.ts | 6 +- 7 files changed, 61 insertions(+), 69 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index ee512123d65..c37785414bc 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -20,8 +20,7 @@ import { MongoNetworkTimeoutError, MongoParseError, MongoServerError, - MongoUnexpectedServerResponseError, - MongoWriteConcernError + MongoUnexpectedServerResponseError } from '../error'; import type { ServerApi, SupportedNodeConnectionOptions } from '../mongo_client'; import { type MongoClientAuthProviders } from '../mongo_client_auth_providers'; @@ -510,7 +509,7 @@ export class Connection extends TypedEventEmitter { this.emit(Connection.CLUSTER_TIME_RECEIVED, document.$clusterTime); } - if (document.isError) { + if (document.ok === 0) { throw new MongoServerError((object ??= document.toObject(bsonOptions))); } @@ -573,16 +572,8 @@ export class Connection extends TypedEventEmitter { ): Promise { this.throwIfAborted(); for await (const document of this.sendCommand(ns, command, options, responseType)) { - if ( - (MongoDBResponse.is(document) && document.has('writeConcernError')) || - (!MongoDBResponse.is(document) && document.writeConcernError) - ) { - const object = MongoDBResponse.is(document) ? document.toObject(options) : document; - throw new MongoWriteConcernError(object.writeConcernError, object); - } return document; } - throw new MongoUnexpectedServerResponseError('Unable to get response from server'); } diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index 793cb99b996..fd32aa56790 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -81,15 +81,6 @@ export class MongoDBResponse extends OnDemandDocument { // {ok:1} static empty = new MongoDBResponse(new Uint8Array([13, 0, 0, 0, 16, 111, 107, 0, 1, 0, 0, 0, 0])); - /** Indicates this document is a server error */ - public get isError() { - let isError = this.ok === 0; - isError ||= this.has('errmsg'); - isError ||= this.has('code'); - isError ||= this.has('$err'); // The '$err' field is used in OP_REPLY responses - return isError; - } - /** * Drivers can safely assume that the `recoveryToken` field is always a BSON document but drivers MUST NOT modify the * contents of the document. @@ -120,6 +111,7 @@ export class MongoDBResponse extends OnDemandDocument { return this.get('operationTime', BSONType.timestamp); } + /** Normalizes whatever BSON value is "ok" to a JS number 1 or 0. */ public get ok(): 0 | 1 { return this.getNumber('ok') ? 1 : 0; } @@ -174,7 +166,7 @@ export class MongoDBResponse extends OnDemandDocument { encryptedResponse?: MongoDBResponse; } -// Here's a litle blast from the past. +// Here's a little blast from the past. // OLD style method definition so that I can override get without redefining ALL the fancy TS :/ Object.defineProperty(MongoDBResponse.prototype, 'get', { value: function get(name: any, as: any, required: any) { @@ -210,7 +202,11 @@ export class CursorResponse extends MongoDBResponse { } public get id(): Long { - return Long.fromBigInt(this.cursor.get('id', BSONType.long, true)); + try { + return Long.fromBigInt(this.cursor.get('id', BSONType.long, true)); + } catch (cause) { + throw new MongoUnexpectedServerResponseError(cause.message, { cause }); + } } public get ns() { diff --git a/test/integration/collection-management/collection.test.ts b/test/integration/collection-management/collection.test.ts index 92183df7ff5..809b4697dea 100644 --- a/test/integration/collection-management/collection.test.ts +++ b/test/integration/collection-management/collection.test.ts @@ -458,6 +458,13 @@ describe('Collection', function () { expect(countC).to.be.a('number').that.equals(0); }); + it('does not mutate options', async () => { + const options = Object.freeze(Object.create(null)); + const count = await collection.countDocuments({}, options); + expect(count).to.be.a('number').that.equals(100); + expect(options).to.deep.equal({}); + }); + context('when a filter is applied', () => { it('adds a $match pipeline', async () => { await collection.countDocuments({ test: 'a' }); diff --git a/test/unit/assorted/sessions_collection.test.js b/test/unit/assorted/sessions_collection.test.js index 409d818d136..e7711efb6b5 100644 --- a/test/unit/assorted/sessions_collection.test.js +++ b/test/unit/assorted/sessions_collection.test.js @@ -48,27 +48,5 @@ describe('Sessions - unit/sessions', function () { return client.close(); }); }); - - it('does not mutate command options', function () { - const options = Object.freeze({}); - test.server.setMessageHandler(request => { - const doc = request.document; - if (isHello(doc)) { - request.reply(mock.HELLO); - } else if (doc.count || doc.aggregate || doc.endSessions) { - request.reply({ ok: 1 }); - } - }); - - const client = new MongoClient(`mongodb://${test.server.uri()}/test`); - return client.connect().then(client => { - const coll = client.db('foo').collection('bar'); - - return coll.countDocuments({}, options).then(() => { - expect(options).to.deep.equal({}); - return client.close(); - }); - }); - }); }); }); diff --git a/test/unit/client-side-encryption/auto_encrypter.test.ts b/test/unit/client-side-encryption/auto_encrypter.test.ts index bd50961cdeb..95536181278 100644 --- a/test/unit/client-side-encryption/auto_encrypter.test.ts +++ b/test/unit/client-side-encryption/auto_encrypter.test.ts @@ -162,7 +162,7 @@ describe('AutoEncrypter', function () { local: { key: Buffer.alloc(96) } } }); - const decrypted = await mc.decrypt(input); + const decrypted = BSON.deserialize(await mc.decrypt(input)); expect(decrypted).to.eql({ filter: { find: 'test', ssn: '457-55-5462' } }); expect(decrypted).to.not.have.property(Symbol.for('@@mdb.decryptedKeys')); expect(decrypted.filter).to.not.have.property(Symbol.for('@@mdb.decryptedKeys')); @@ -186,7 +186,7 @@ describe('AutoEncrypter', function () { } }); mc[Symbol.for('@@mdb.decorateDecryptionResult')] = true; - let decrypted = await mc.decrypt(input); + let decrypted = BSON.deserialize(await mc.decrypt(input)); expect(decrypted).to.eql({ filter: { find: 'test', ssn: '457-55-5462' } }); expect(decrypted).to.not.have.property(Symbol.for('@@mdb.decryptedKeys')); expect(decrypted.filter[Symbol.for('@@mdb.decryptedKeys')]).to.eql(['ssn']); @@ -243,7 +243,7 @@ describe('AutoEncrypter', function () { aws: {} } }); - const decrypted = await mc.decrypt(input); + const decrypted = BSON.deserialize(await mc.decrypt(input)); expect(decrypted).to.eql({ filter: { find: 'test', ssn: '457-55-5462' } }); }); }); diff --git a/test/unit/cmap/wire_protocol/responses.test.ts b/test/unit/cmap/wire_protocol/responses.test.ts index 91e052da84f..0c520528c9c 100644 --- a/test/unit/cmap/wire_protocol/responses.test.ts +++ b/test/unit/cmap/wire_protocol/responses.test.ts @@ -86,30 +86,58 @@ describe('class MongoDBResponse', () => { }); describe('class CursorResponse', () => { - describe('constructor()', () => { + describe('get cursor()', () => { it('throws if input does not contain cursor embedded document', () => { - expect(() => new CursorResponse(BSON.serialize({ ok: 1 }))).to.throw(BSONError); + // @ts-expect-error: testing private getter + expect(() => new CursorResponse(BSON.serialize({ ok: 1 })).cursor).to.throw( + MongoUnexpectedServerResponseError + ); }); + }); + describe('get id()', () => { it('throws if input does not contain cursor.id int64', () => { - expect(() => new CursorResponse(BSON.serialize({ ok: 1, cursor: {} }))).to.throw(BSONError); + expect(() => new CursorResponse(BSON.serialize({ ok: 1, cursor: {} })).id).to.throw( + MongoUnexpectedServerResponseError + ); + }); + }); + + describe('get batch()', () => { + it('throws if input does not contain firstBatch nor nextBatch', () => { + expect( + // @ts-expect-error: testing private getter + () => new CursorResponse(BSON.serialize({ ok: 1, cursor: { id: 0n, batch: [] } })).batch + ).to.throw(MongoUnexpectedServerResponseError); }); + }); + describe('get ns()', () => { it('sets namespace to null if input does not contain cursor.ns', () => { expect(new CursorResponse(BSON.serialize({ ok: 1, cursor: { id: 0n, firstBatch: [] } })).ns) .to.be.null; }); + }); - it('throws if input does not contain firstBatch nor nextBatch', () => { - expect( - () => new CursorResponse(BSON.serialize({ ok: 1, cursor: { id: 0n, batch: [] } })) - ).to.throw(MongoUnexpectedServerResponseError); + describe('get batchSize()', () => { + it('reports the returned batch size', () => { + const response = new CursorResponse( + BSON.serialize({ ok: 1, cursor: { id: 0n, nextBatch: [{}, {}, {}] } }) + ); + expect(response.batchSize).to.equal(3); + expect(response.shift()).to.deep.equal({}); + expect(response.batchSize).to.equal(3); }); + }); - it('reports a length equal to the batch', () => { - expect( - new CursorResponse(BSON.serialize({ ok: 1, cursor: { id: 0n, nextBatch: [1, 2, 3] } })) - ).to.have.lengthOf(3); + describe('get length()', () => { + it('reports number of documents remaining in the batch', () => { + const response = new CursorResponse( + BSON.serialize({ ok: 1, cursor: { id: 0n, nextBatch: [{}, {}, {}] } }) + ); + expect(response).to.have.lengthOf(3); + expect(response.shift()).to.deep.equal({}); + expect(response).to.have.lengthOf(2); // length makes CursorResponse act like an array }); }); @@ -162,12 +190,4 @@ describe('class CursorResponse', () => { expect(response.shift()).to.be.null; }); }); - - describe('pushMany()', () => - it('throws unsupported error', () => - expect(CursorResponse.prototype.pushMany).to.throw(/Unsupported/i))); - - describe('push()', () => - it('throws unsupported error', () => - expect(CursorResponse.prototype.push).to.throw(/Unsupported/i))); }); diff --git a/test/unit/error.test.ts b/test/unit/error.test.ts index 298e1f2191f..90eeec7cb26 100644 --- a/test/unit/error.test.ts +++ b/test/unit/error.test.ts @@ -301,7 +301,7 @@ describe('MongoErrors', () => { }; const RAW_USER_WRITE_CONCERN_ERROR = { - ok: 0, + ok: 1, errmsg: 'waiting for replication timed out', code: 64, codeName: 'WriteConcernFailed', @@ -316,7 +316,7 @@ describe('MongoErrors', () => { }; const RAW_USER_WRITE_CONCERN_ERROR_INFO = { - ok: 0, + ok: 1, errmsg: 'waiting for replication timed out', code: 64, codeName: 'WriteConcernFailed', @@ -367,7 +367,7 @@ describe('MongoErrors', () => { replSet.connect(); } - it('should expose a user command writeConcern error like a normal WriteConcernError', function () { + it.only('should expose a user command writeConcern error like a normal WriteConcernError', function () { test.primaryServer.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { From e0c22501be305825b124827cf0d450db93ecc5c6 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 4 Jun 2024 11:24:29 -0400 Subject: [PATCH 12/29] wip --- src/error.ts | 42 ++++++++++++------------------- src/operations/bulk_write.ts | 2 ++ src/operations/delete.ts | 3 ++- src/operations/find_and_modify.ts | 8 +++++- src/operations/update.ts | 6 +++-- src/sdam/server.ts | 7 ++++-- src/utils.ts | 16 +++++++++++- 7 files changed, 51 insertions(+), 33 deletions(-) diff --git a/src/error.ts b/src/error.ts index cee15493b28..1a07ff4d760 100644 --- a/src/error.ts +++ b/src/error.ts @@ -1157,27 +1157,14 @@ export class MongoServerSelectionError extends MongoSystemError { } } -function makeWriteConcernResultObject(input: any) { - const output = Object.assign({}, input); - - if (output.ok === 0) { - output.ok = 1; - delete output.errmsg; - delete output.code; - delete output.codeName; - } - - return output; -} - /** * An error thrown when the server reports a writeConcernError * @public * @category Error */ export class MongoWriteConcernError extends MongoServerError { - /** The result document (provided if ok: 1) */ - result?: Document; + /** The result document */ + result: Document; /** * **Do not use this constructor!** @@ -1190,17 +1177,20 @@ export class MongoWriteConcernError extends MongoServerError { * * @public **/ - constructor(message: ErrorDescription, result?: Document) { - if (result && Array.isArray(result.errorLabels)) { - message.errorLabels = result.errorLabels; - } - - super(message); - this.errInfo = message.errInfo; - - if (result != null) { - this.result = makeWriteConcernResultObject(result); - } + constructor({ + writeConcernError, + ...result + }: { + writeConcernError: { + code: number; + errmsg: string; + codeName?: string; + errInfo?: Document; + }; + } & Document) { + super(writeConcernError); + this.errInfo = writeConcernError.errInfo; + this.result = result; } override get name(): string { diff --git a/src/operations/bulk_write.ts b/src/operations/bulk_write.ts index 58a143f2085..76dba810ee2 100644 --- a/src/operations/bulk_write.ts +++ b/src/operations/bulk_write.ts @@ -7,6 +7,7 @@ import type { import type { Collection } from '../collection'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; +import { throwIfWriteConcernError } from '../utils'; import { AbstractOperation, Aspect, defineAspects } from './operation'; /** @internal */ @@ -51,6 +52,7 @@ export class BulkWriteOperation extends AbstractOperation { // Execute the bulk const result = await bulk.execute({ ...options, session }); + throwIfWriteConcernError(result); return result; } } diff --git a/src/operations/delete.ts b/src/operations/delete.ts index e952554ff77..a601b7a5a43 100644 --- a/src/operations/delete.ts +++ b/src/operations/delete.ts @@ -4,7 +4,7 @@ import { MongoCompatibilityError, MongoServerError } from '../error'; import { type TODO_NODE_3286 } from '../mongo_types'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import type { MongoDBNamespace } from '../utils'; +import { type MongoDBNamespace, throwIfWriteConcernError } from '../utils'; import type { WriteConcernOptions } from '../write_concern'; import { type CollationOptions, CommandOperation, type CommandOperationOptions } from './command'; import { Aspect, defineAspects, type Hint } from './operation'; @@ -96,6 +96,7 @@ export class DeleteOperation extends CommandOperation { } const res: TODO_NODE_3286 = await super.executeCommand(server, session, command); + throwIfWriteConcernError(res); return res; } } diff --git a/src/operations/find_and_modify.ts b/src/operations/find_and_modify.ts index 68abe51a7e9..de107f4835b 100644 --- a/src/operations/find_and_modify.ts +++ b/src/operations/find_and_modify.ts @@ -5,7 +5,12 @@ import { ReadPreference } from '../read_preference'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; import { formatSort, type Sort, type SortForCmd } from '../sort'; -import { decorateWithCollation, hasAtomicOperators, maxWireVersion } from '../utils'; +import { + decorateWithCollation, + hasAtomicOperators, + maxWireVersion, + throwIfWriteConcernError +} from '../utils'; import type { WriteConcern, WriteConcernSettings } from '../write_concern'; import { CommandOperation, type CommandOperationOptions } from './command'; import { Aspect, defineAspects } from './operation'; @@ -214,6 +219,7 @@ export class FindAndModifyOperation extends CommandOperation { // Execute the command const result = await super.executeCommand(server, session, cmd); + throwIfWriteConcernError(result); return options.includeResultMetadata ? result : result.value ?? null; } } diff --git a/src/operations/update.ts b/src/operations/update.ts index a863afdc68e..85fbe0808ad 100644 --- a/src/operations/update.ts +++ b/src/operations/update.ts @@ -4,7 +4,7 @@ import { MongoCompatibilityError, MongoInvalidArgumentError, MongoServerError } import type { InferIdType, TODO_NODE_3286 } from '../mongo_types'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { hasAtomicOperators, type MongoDBNamespace } from '../utils'; +import { hasAtomicOperators, type MongoDBNamespace, throwIfWriteConcernError } from '../utils'; import { type CollationOptions, CommandOperation, type CommandOperationOptions } from './command'; import { Aspect, defineAspects, type Hint } from './operation'; @@ -122,7 +122,9 @@ export class UpdateOperation extends CommandOperation { } } - return await super.executeCommand(server, session, command); + const res = await super.executeCommand(server, session, command); + throwIfWriteConcernError(res); + return res; } } diff --git a/src/sdam/server.ts b/src/sdam/server.ts index 8ea91815c60..d0de87cedff 100644 --- a/src/sdam/server.ts +++ b/src/sdam/server.ts @@ -46,7 +46,8 @@ import { makeStateMachine, maxWireVersion, type MongoDBNamespace, - supportsRetryableWrites + supportsRetryableWrites, + throwIfWriteConcernError } from '../utils'; import { type ClusterTime, @@ -323,7 +324,9 @@ export class Server extends TypedEventEmitter { try { try { - return await conn.command(ns, cmd, finalOptions, responseType); + const res = await conn.command(ns, cmd, finalOptions, responseType); + throwIfWriteConcernError(res); + return res; } catch (commandError) { throw this.decorateCommandError(conn, cmd, finalOptions, commandError); } diff --git a/src/utils.ts b/src/utils.ts index cebb81e0a0d..7d3df75b5b5 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -11,6 +11,7 @@ import { promisify } from 'util'; import { deserialize, type Document, ObjectId, resolveBSONOptions } from './bson'; import type { Connection } from './cmap/connection'; import { MAX_SUPPORTED_WIRE_VERSION } from './cmap/wire_protocol/constants'; +import { MongoDBResponse } from './cmap/wire_protocol/responses'; import type { Collection } from './collection'; import { kDecoratedKeys, LEGACY_HELLO_COMMAND } from './constants'; import type { AbstractCursor } from './cursor/abstract_cursor'; @@ -23,7 +24,8 @@ import { MongoNetworkTimeoutError, MongoNotConnectedError, MongoParseError, - MongoRuntimeError + MongoRuntimeError, + MongoWriteConcernError } from './error'; import type { Explain } from './explain'; import type { MongoClient } from './mongo_client'; @@ -1416,3 +1418,15 @@ export function decorateDecryptionResult( decorateDecryptionResult(decrypted[k], originalValue, false); } } + +/** Called with either a plain object or MongoDBResponse */ +export function throwIfWriteConcernError(response: unknown): void { + if (typeof response === 'object' && response != null) { + if (MongoDBResponse.is(response) && response.has('writeConcernError')) { + const object = response.toObject(); + throw new MongoWriteConcernError(object.writeConcernError, object); + } else if ('writeConcernError' in response) { + throw new MongoWriteConcernError(response.writeConcernError as any, response); + } + } +} From 1c10a1f971f708331a3085624d0572fb69f4c99c Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 4 Jun 2024 14:41:46 -0400 Subject: [PATCH 13/29] chore: fix unit tests --- src/error.ts | 25 ++-- src/utils.ts | 14 ++- .../auto_encrypter.test.ts | 17 +-- .../unit/cmap/wire_protocol/responses.test.ts | 31 ----- test/unit/error.test.ts | 107 +++++++++--------- 5 files changed, 81 insertions(+), 113 deletions(-) diff --git a/src/error.ts b/src/error.ts index 1a07ff4d760..8ed8958d1e2 100644 --- a/src/error.ts +++ b/src/error.ts @@ -1177,19 +1177,18 @@ export class MongoWriteConcernError extends MongoServerError { * * @public **/ - constructor({ - writeConcernError, - ...result - }: { - writeConcernError: { - code: number; - errmsg: string; - codeName?: string; - errInfo?: Document; - }; - } & Document) { - super(writeConcernError); - this.errInfo = writeConcernError.errInfo; + constructor( + result: { + writeConcernError: { + code: number; + errmsg: string; + codeName?: string; + errInfo?: Document; + }; + } & Document + ) { + super(result.writeConcernError); + this.errInfo = result.writeConcernError.errInfo; this.result = result; } diff --git a/src/utils.ts b/src/utils.ts index 7d3df75b5b5..120af227380 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1422,11 +1422,15 @@ export function decorateDecryptionResult( /** Called with either a plain object or MongoDBResponse */ export function throwIfWriteConcernError(response: unknown): void { if (typeof response === 'object' && response != null) { - if (MongoDBResponse.is(response) && response.has('writeConcernError')) { - const object = response.toObject(); - throw new MongoWriteConcernError(object.writeConcernError, object); - } else if ('writeConcernError' in response) { - throw new MongoWriteConcernError(response.writeConcernError as any, response); + const writeConcernError: object | null = + MongoDBResponse.is(response) && response.has('writeConcernError') + ? response.toObject() + : !MongoDBResponse.is(response) && 'writeConcernError' in response + ? response + : null; + + if (writeConcernError != null) { + throw new MongoWriteConcernError(writeConcernError as any); } } } diff --git a/test/unit/client-side-encryption/auto_encrypter.test.ts b/test/unit/client-side-encryption/auto_encrypter.test.ts index 95536181278..08072512c70 100644 --- a/test/unit/client-side-encryption/auto_encrypter.test.ts +++ b/test/unit/client-side-encryption/auto_encrypter.test.ts @@ -185,25 +185,26 @@ describe('AutoEncrypter', function () { local: { key: Buffer.alloc(96) } } }); - mc[Symbol.for('@@mdb.decorateDecryptionResult')] = true; + let decrypted = BSON.deserialize(await mc.decrypt(input)); expect(decrypted).to.eql({ filter: { find: 'test', ssn: '457-55-5462' } }); - expect(decrypted).to.not.have.property(Symbol.for('@@mdb.decryptedKeys')); - expect(decrypted.filter[Symbol.for('@@mdb.decryptedKeys')]).to.eql(['ssn']); // The same, but with an object containing different data types as the input - decrypted = await mc.decrypt({ - a: [null, 1, { c: new bson.Binary(Buffer.from('foo', 'utf8'), 1) }] - }); + decrypted = BSON.deserialize( + await mc.decrypt( + BSON.serialize({ + a: [null, 1, { c: new bson.Binary(Buffer.from('foo', 'utf8'), 1) }] + }) + ) + ); expect(decrypted).to.eql({ a: [null, 1, { c: new bson.Binary(Buffer.from('foo', 'utf8'), 1) }] }); expect(decrypted).to.not.have.property(Symbol.for('@@mdb.decryptedKeys')); // The same, but with nested data inside the decrypted input - decrypted = await mc.decrypt(nestedInput); + decrypted = BSON.deserialize(await mc.decrypt(nestedInput)); expect(decrypted).to.eql({ nested: { x: { y: 1234 } } }); - expect(decrypted[Symbol.for('@@mdb.decryptedKeys')]).to.eql(['nested']); expect(decrypted.nested).to.not.have.property(Symbol.for('@@mdb.decryptedKeys')); expect(decrypted.nested.x).to.not.have.property(Symbol.for('@@mdb.decryptedKeys')); expect(decrypted.nested.x.y).to.not.have.property(Symbol.for('@@mdb.decryptedKeys')); diff --git a/test/unit/cmap/wire_protocol/responses.test.ts b/test/unit/cmap/wire_protocol/responses.test.ts index 0c520528c9c..a3338c77edc 100644 --- a/test/unit/cmap/wire_protocol/responses.test.ts +++ b/test/unit/cmap/wire_protocol/responses.test.ts @@ -3,7 +3,6 @@ import * as sinon from 'sinon'; import { BSON, - BSONError, CursorResponse, Int32, MongoDBResponse, @@ -16,36 +15,6 @@ describe('class MongoDBResponse', () => { expect(new MongoDBResponse(BSON.serialize({ ok: 1 }))).to.be.instanceOf(OnDemandDocument); }); - context('get isError', () => { - it('returns true when ok is 0', () => { - const doc = new MongoDBResponse(BSON.serialize({ ok: 0 })); - expect(doc.isError).to.be.true; - }); - - it('returns true when $err is defined', () => { - const doc = new MongoDBResponse(BSON.serialize({ $err: 0 })); - expect(doc.isError).to.be.true; - }); - - it('returns true when errmsg is defined', () => { - const doc = new MongoDBResponse(BSON.serialize({ errmsg: 0 })); - expect(doc.isError).to.be.true; - }); - - it('returns true when code is defined', () => { - const doc = new MongoDBResponse(BSON.serialize({ code: 0 })); - expect(doc.isError).to.be.true; - }); - - it('short circuits detection of $err, errmsg, code', () => { - const doc = new MongoDBResponse(BSON.serialize({ ok: 0 })); - expect(doc.isError).to.be.true; - expect(doc).to.not.have.property('cache.$err'); - expect(doc).to.not.have.property('cache.errmsg'); - expect(doc).to.not.have.property('cache.code'); - }); - }); - context('utf8 validation', () => { afterEach(() => sinon.restore()); diff --git a/test/unit/error.test.ts b/test/unit/error.test.ts index 90eeec7cb26..22ac271af88 100644 --- a/test/unit/error.test.ts +++ b/test/unit/error.test.ts @@ -65,20 +65,15 @@ describe('MongoErrors', () => { }); describe('error names should be read-only', () => { - for (const [errorName, errorClass] of Object.entries(errorClassesFromEntryPoint)) { + for (const [errorName, errorClass] of Object.entries<{ new (): Error }>( + errorClassesFromEntryPoint + )) { it(`${errorName} should be read-only`, () => { - // Dynamically create error class with message - const error = new (errorClass as any)('generated by test', { - cause: new Error('something went wrong') - }); - // expect name property to be class name - expect(error).to.have.property('name', errorName); - - try { - error.name = 'renamed by test'; - // eslint-disable-next-line no-empty - } catch (err) {} - expect(error).to.have.property('name', errorName); + const errorNameDescriptor = Object.getOwnPropertyDescriptor(errorClass.prototype, 'name'); + expect(errorNameDescriptor).to.have.property('set').that.does.not.exist; + expect(errorNameDescriptor).to.not.have.property('value'); + expect(errorNameDescriptor).to.have.property('get'); + expect(errorNameDescriptor.get.call(undefined)).to.equal(errorName); }); } }); @@ -367,7 +362,7 @@ describe('MongoErrors', () => { replSet.connect(); } - it.only('should expose a user command writeConcern error like a normal WriteConcernError', function () { + it('should expose a user command writeConcern error like a normal WriteConcernError', function () { test.primaryServer.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -393,9 +388,9 @@ describe('MongoErrors', () => { expect(err).to.be.an.instanceOf(MongoWriteConcernError); expect(err.result).to.exist; expect(err.result).to.have.property('ok', 1); - expect(err.result).to.not.have.property('errmsg'); - expect(err.result).to.not.have.property('code'); - expect(err.result).to.not.have.property('codeName'); + expect(err.result).to.have.property('errmsg'); + expect(err.result).to.have.property('code'); + expect(err.result).to.have.property('codeName'); expect(err.result).to.have.property('writeConcernError'); } ) @@ -480,45 +475,45 @@ describe('MongoErrors', () => { error: new MongoNetworkError('socket bad, try again'), maxWireVersion: BELOW_4_4 }, - { - description: 'a MongoWriteConcernError with no code nor label', - result: false, - error: new MongoWriteConcernError({ message: 'empty wc error' }), - maxWireVersion: BELOW_4_4 - }, - { - description: 'a MongoWriteConcernError with a random label', - result: false, - error: new MongoWriteConcernError( - { message: 'random label' }, - { errorLabels: ['myLabel'] } - ), - maxWireVersion: BELOW_4_4 - }, - { - description: 'a MongoWriteConcernError with a retryable code above server 4.4', - result: false, - error: new MongoWriteConcernError({}, { code: 262 }), - maxWireVersion: ABOVE_4_4 - }, - { - description: 'a MongoWriteConcernError with a retryable code below server 4.4', - result: true, - error: new MongoWriteConcernError({}, { code: 262 }), - maxWireVersion: BELOW_4_4 - }, - { - description: 'a MongoWriteConcernError with a RetryableWriteError label below server 4.4', - result: false, - error: new MongoWriteConcernError({}, { errorLabels: ['RetryableWriteError'] }), - maxWireVersion: BELOW_4_4 - }, - { - description: 'a MongoWriteConcernError with a RetryableWriteError label above server 4.4', - result: false, - error: new MongoWriteConcernError({}, { errorLabels: ['RetryableWriteError'] }), - maxWireVersion: ABOVE_4_4 - }, + // { + // description: 'a MongoWriteConcernError with no code nor label', + // result: false, + // error: new MongoWriteConcernError({ message: 'empty wc error' }), + // maxWireVersion: BELOW_4_4 + // }, + // { + // description: 'a MongoWriteConcernError with a random label', + // result: false, + // error: new MongoWriteConcernError( + // { message: 'random label' }, + // { errorLabels: ['myLabel'] } + // ), + // maxWireVersion: BELOW_4_4 + // }, + // { + // description: 'a MongoWriteConcernError with a retryable code above server 4.4', + // result: false, + // error: new MongoWriteConcernError({}, { code: 262 }), + // maxWireVersion: ABOVE_4_4 + // }, + // { + // description: 'a MongoWriteConcernError with a retryable code below server 4.4', + // result: true, + // error: new MongoWriteConcernError({}, { code: 262 }), + // maxWireVersion: BELOW_4_4 + // }, + // { + // description: 'a MongoWriteConcernError with a RetryableWriteError label below server 4.4', + // result: false, + // error: new MongoWriteConcernError({}, { errorLabels: ['RetryableWriteError'] }), + // maxWireVersion: BELOW_4_4 + // }, + // { + // description: 'a MongoWriteConcernError with a RetryableWriteError label above server 4.4', + // result: false, + // error: new MongoWriteConcernError({}, { errorLabels: ['RetryableWriteError'] }), + // maxWireVersion: ABOVE_4_4 + // }, { description: 'any MongoError with a RetryableWriteError label', result: false, From be871b8be23b4c724de3122f32b43dcb82604d59 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 4 Jun 2024 15:42:49 -0400 Subject: [PATCH 14/29] chore: fix wc throw location --- .../client_encryption.ts | 4 +-- src/client-side-encryption/state_machine.ts | 31 ++++++++++------- src/cmap/wire_protocol/responses.ts | 12 +++++-- src/cursor/abstract_cursor.ts | 7 +--- src/error.ts | 21 ++++++------ src/operations/bulk_write.ts | 5 +-- src/operations/delete.ts | 5 ++- src/operations/find_and_modify.ts | 10 ++---- src/operations/update.ts | 3 +- src/sdam/server.ts | 8 +++-- src/utils.ts | 20 +---------- src/write_concern.ts | 18 ++++++++++ .../non-server-retryable_writes.test.ts | 15 +++++---- .../retryable_writes.spec.prose.test.ts | 33 +++++++++---------- 14 files changed, 96 insertions(+), 96 deletions(-) diff --git a/src/client-side-encryption/client_encryption.ts b/src/client-side-encryption/client_encryption.ts index 6d2ff784ee6..44b2411bb94 100644 --- a/src/client-side-encryption/client_encryption.ts +++ b/src/client-side-encryption/client_encryption.ts @@ -719,8 +719,8 @@ export class ClientEncryption { }); const context = this._mongoCrypt.makeExplicitEncryptionContext(valueBuffer, contextOptions); - const result = deserialize(await stateMachine.execute(this, context)); - return result.v; + const { v } = deserialize(await stateMachine.execute(this, context)); + return v; } } diff --git a/src/client-side-encryption/state_machine.ts b/src/client-side-encryption/state_machine.ts index 94df0c728d3..8cd0e1fb5f7 100644 --- a/src/client-side-encryption/state_machine.ts +++ b/src/client-side-encryption/state_machine.ts @@ -112,8 +112,25 @@ export type CSFLEKMSTlsOptions = { azure?: ClientEncryptionTlsOptions; }; -/** `{ v: [] }` */ -const EMPTY_V = Uint8Array.from([13, 0, 0, 0, 4, 118, 0, 5, 0, 0, 0, 0, 0]); +/** + * This is kind of a hack. For `rewrapManyDataKey`, we have tests that + * guarantee that when there are no matching keys, `rewrapManyDataKey` returns + * nothing. We also have tests for auto encryption that guarantee for `encrypt` + * we return an error when there are no matching keys. This error is generated in + * subsequent iterations of the state machine. + * Some apis (`encrypt`) throw if there are no filter matches and others (`rewrapManyDataKey`) + * do not. We set the result manually here, and let the state machine continue. `libmongocrypt` + * will inform us if we need to error by setting the state to `MONGOCRYPT_CTX_ERROR` but + * otherwise we'll return `{ v: [] }`. + */ +const EMPTY_V = Uint8Array.from([ + ...[13, 0, 0, 0], // document size = 13 bytes + ...[ + ...[4, 118, 0], // array type (4), "v\x00" basic latin "v" + ...[5, 0, 0, 0, 0] // empty document (5 byte size, null terminator) + ], + 0 // null terminator +]); /** * @internal @@ -211,15 +228,7 @@ export class StateMachine { const keys = await this.fetchKeys(keyVaultClient, keyVaultNamespace, filter); if (keys.length === 0) { - // This is kind of a hack. For `rewrapManyDataKey`, we have tests that - // guarantee that when there are no matching keys, `rewrapManyDataKey` returns - // nothing. We also have tests for auto encryption that guarantee for `encrypt` - // we return an error when there are no matching keys. This error is generated in - // subsequent iterations of the state machine. - // Some apis (`encrypt`) throw if there are no filter matches and others (`rewrapManyDataKey`) - // do not. We set the result manually here, and let the state machine continue. `libmongocrypt` - // will inform us if we need to error by setting the state to `MONGOCRYPT_CTX_ERROR` but - // otherwise we'll return `{ v: [] }`. + // See docs on EMPTY_V result = EMPTY_V; } for await (const key of keys) { diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index fd32aa56790..cf65885e034 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -66,6 +66,14 @@ export type MongoDBResponseConstructor = { /** @internal */ export class MongoDBResponse extends OnDemandDocument { + /** + * Devtools need to know which keys were encrypted before the driver automatically decrypted them. + * If decorating is enabled (`Symbol.for('@@mdb.decorateDecryptionResult')`), this field will be set, + * storing the original encrypted response from the server, so that we can build an object that has + * the list of BSON keys that were encrypted stored at a well known symbol: `Symbol.for('@@mdb.decryptedKeys')`. + */ + encryptedResponse?: MongoDBResponse; + static is(value: unknown): value is MongoDBResponse { return value instanceof MongoDBResponse; } @@ -161,13 +169,11 @@ export class MongoDBResponse extends OnDemandDocument { } return { utf8: { writeErrors: false } }; } - - // TODO: Supports decorating result - encryptedResponse?: MongoDBResponse; } // Here's a little blast from the past. // OLD style method definition so that I can override get without redefining ALL the fancy TS :/ +// TODO there must be a better way... Object.defineProperty(MongoDBResponse.prototype, 'get', { value: function get(name: any, as: any, required: any) { try { diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index 826594a55dc..8faf23649a1 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -294,8 +294,7 @@ export abstract class AbstractCursor< return bufferedDocs; } - - private async *asyncIterator() { + async *[Symbol.asyncIterator](): AsyncGenerator { if (this.closed) { return; } @@ -343,10 +342,6 @@ export abstract class AbstractCursor< } } - async *[Symbol.asyncIterator](): AsyncGenerator { - yield* this.asyncIterator(); - } - stream(options?: CursorStreamOptions): Readable & AsyncIterable { if (options?.transform) { const transform = options.transform; diff --git a/src/error.ts b/src/error.ts index 8ed8958d1e2..066cc1f82a7 100644 --- a/src/error.ts +++ b/src/error.ts @@ -1177,17 +1177,16 @@ export class MongoWriteConcernError extends MongoServerError { * * @public **/ - constructor( - result: { - writeConcernError: { - code: number; - errmsg: string; - codeName?: string; - errInfo?: Document; - }; - } & Document - ) { - super(result.writeConcernError); + constructor(result: { + writeConcernError: { + code: number; + errmsg: string; + codeName?: string; + errInfo?: Document; + }; + errorLabels?: string[]; + }) { + super({ ...result, ...result.writeConcernError }); this.errInfo = result.writeConcernError.errInfo; this.result = result; } diff --git a/src/operations/bulk_write.ts b/src/operations/bulk_write.ts index 76dba810ee2..64c0664b5a1 100644 --- a/src/operations/bulk_write.ts +++ b/src/operations/bulk_write.ts @@ -7,7 +7,6 @@ import type { import type { Collection } from '../collection'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { throwIfWriteConcernError } from '../utils'; import { AbstractOperation, Aspect, defineAspects } from './operation'; /** @internal */ @@ -51,9 +50,7 @@ export class BulkWriteOperation extends AbstractOperation { } // Execute the bulk - const result = await bulk.execute({ ...options, session }); - throwIfWriteConcernError(result); - return result; + return await bulk.execute({ ...options, session }); } } diff --git a/src/operations/delete.ts b/src/operations/delete.ts index a601b7a5a43..f0ef61cb7b1 100644 --- a/src/operations/delete.ts +++ b/src/operations/delete.ts @@ -4,8 +4,8 @@ import { MongoCompatibilityError, MongoServerError } from '../error'; import { type TODO_NODE_3286 } from '../mongo_types'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { type MongoDBNamespace, throwIfWriteConcernError } from '../utils'; -import type { WriteConcernOptions } from '../write_concern'; +import { type MongoDBNamespace } from '../utils'; +import { type WriteConcernOptions } from '../write_concern'; import { type CollationOptions, CommandOperation, type CommandOperationOptions } from './command'; import { Aspect, defineAspects, type Hint } from './operation'; @@ -96,7 +96,6 @@ export class DeleteOperation extends CommandOperation { } const res: TODO_NODE_3286 = await super.executeCommand(server, session, command); - throwIfWriteConcernError(res); return res; } } diff --git a/src/operations/find_and_modify.ts b/src/operations/find_and_modify.ts index de107f4835b..c295fcb0a88 100644 --- a/src/operations/find_and_modify.ts +++ b/src/operations/find_and_modify.ts @@ -5,13 +5,8 @@ import { ReadPreference } from '../read_preference'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; import { formatSort, type Sort, type SortForCmd } from '../sort'; -import { - decorateWithCollation, - hasAtomicOperators, - maxWireVersion, - throwIfWriteConcernError -} from '../utils'; -import type { WriteConcern, WriteConcernSettings } from '../write_concern'; +import { decorateWithCollation, hasAtomicOperators, maxWireVersion } from '../utils'; +import { type WriteConcern, type WriteConcernSettings } from '../write_concern'; import { CommandOperation, type CommandOperationOptions } from './command'; import { Aspect, defineAspects } from './operation'; @@ -219,7 +214,6 @@ export class FindAndModifyOperation extends CommandOperation { // Execute the command const result = await super.executeCommand(server, session, cmd); - throwIfWriteConcernError(result); return options.includeResultMetadata ? result : result.value ?? null; } } diff --git a/src/operations/update.ts b/src/operations/update.ts index 85fbe0808ad..ba0ad6d95ff 100644 --- a/src/operations/update.ts +++ b/src/operations/update.ts @@ -4,7 +4,7 @@ import { MongoCompatibilityError, MongoInvalidArgumentError, MongoServerError } import type { InferIdType, TODO_NODE_3286 } from '../mongo_types'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { hasAtomicOperators, type MongoDBNamespace, throwIfWriteConcernError } from '../utils'; +import { hasAtomicOperators, type MongoDBNamespace } from '../utils'; import { type CollationOptions, CommandOperation, type CommandOperationOptions } from './command'; import { Aspect, defineAspects, type Hint } from './operation'; @@ -123,7 +123,6 @@ export class UpdateOperation extends CommandOperation { } const res = await super.executeCommand(server, session, command); - throwIfWriteConcernError(res); return res; } } diff --git a/src/sdam/server.ts b/src/sdam/server.ts index d0de87cedff..59a7231b4f4 100644 --- a/src/sdam/server.ts +++ b/src/sdam/server.ts @@ -46,9 +46,9 @@ import { makeStateMachine, maxWireVersion, type MongoDBNamespace, - supportsRetryableWrites, - throwIfWriteConcernError + supportsRetryableWrites } from '../utils'; +import { throwIfWriteConcernError } from '../write_concern'; import { type ClusterTime, STATE_CLOSED, @@ -337,7 +337,9 @@ export class Server extends TypedEventEmitter { ) { await this.pool.reauthenticate(conn); try { - return await conn.command(ns, cmd, finalOptions, responseType); + const res = await conn.command(ns, cmd, finalOptions, responseType); + throwIfWriteConcernError(res); + return res; } catch (commandError) { throw this.decorateCommandError(conn, cmd, finalOptions, commandError); } diff --git a/src/utils.ts b/src/utils.ts index 120af227380..cebb81e0a0d 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -11,7 +11,6 @@ import { promisify } from 'util'; import { deserialize, type Document, ObjectId, resolveBSONOptions } from './bson'; import type { Connection } from './cmap/connection'; import { MAX_SUPPORTED_WIRE_VERSION } from './cmap/wire_protocol/constants'; -import { MongoDBResponse } from './cmap/wire_protocol/responses'; import type { Collection } from './collection'; import { kDecoratedKeys, LEGACY_HELLO_COMMAND } from './constants'; import type { AbstractCursor } from './cursor/abstract_cursor'; @@ -24,8 +23,7 @@ import { MongoNetworkTimeoutError, MongoNotConnectedError, MongoParseError, - MongoRuntimeError, - MongoWriteConcernError + MongoRuntimeError } from './error'; import type { Explain } from './explain'; import type { MongoClient } from './mongo_client'; @@ -1418,19 +1416,3 @@ export function decorateDecryptionResult( decorateDecryptionResult(decrypted[k], originalValue, false); } } - -/** Called with either a plain object or MongoDBResponse */ -export function throwIfWriteConcernError(response: unknown): void { - if (typeof response === 'object' && response != null) { - const writeConcernError: object | null = - MongoDBResponse.is(response) && response.has('writeConcernError') - ? response.toObject() - : !MongoDBResponse.is(response) && 'writeConcernError' in response - ? response - : null; - - if (writeConcernError != null) { - throw new MongoWriteConcernError(writeConcernError as any); - } - } -} diff --git a/src/write_concern.ts b/src/write_concern.ts index e3a3f5510e3..b315798fd43 100644 --- a/src/write_concern.ts +++ b/src/write_concern.ts @@ -1,4 +1,6 @@ import { type Document } from './bson'; +import { MongoDBResponse } from './cmap/wire_protocol/responses'; +import { MongoWriteConcernError } from './error'; /** @public */ export type W = number | 'majority'; @@ -159,3 +161,19 @@ export class WriteConcern { return undefined; } } + +/** Called with either a plain object or MongoDBResponse */ +export function throwIfWriteConcernError(response: unknown): void { + if (typeof response === 'object' && response != null) { + const writeConcernError: object | null = + MongoDBResponse.is(response) && response.has('writeConcernError') + ? response.toObject() + : !MongoDBResponse.is(response) && 'writeConcernError' in response + ? response + : null; + + if (writeConcernError != null) { + throw new MongoWriteConcernError(writeConcernError as any); + } + } +} diff --git a/test/integration/retryable-writes/non-server-retryable_writes.test.ts b/test/integration/retryable-writes/non-server-retryable_writes.test.ts index 8a4190615c2..40513134d5d 100644 --- a/test/integration/retryable-writes/non-server-retryable_writes.test.ts +++ b/test/integration/retryable-writes/non-server-retryable_writes.test.ts @@ -34,13 +34,14 @@ describe('Non Server Retryable Writes', function () { async () => { const serverCommandStub = sinon.stub(Server.prototype, 'command'); serverCommandStub.onCall(0).rejects(new PoolClearedError('error')); - serverCommandStub - .onCall(1) - .returns( - Promise.reject( - new MongoWriteConcernError({ errorLabels: ['NoWritesPerformed'], errorCode: 10107 }, {}) - ) - ); + serverCommandStub.onCall(1).returns( + Promise.reject( + new MongoWriteConcernError({ + errorLabels: ['NoWritesPerformed'], + writeConcernError: { errmsg: 'NotWritablePrimary error', errorCode: 10107 } + }) + ) + ); const insertResult = await collection.insertOne({ _id: 1 }).catch(error => error); sinon.restore(); diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index a5a53e7cf7f..d02540526a8 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -277,23 +277,22 @@ describe('Retryable Writes Spec Prose', () => { { requires: { topology: 'replicaset', mongodb: '>=4.2.9' } }, async () => { const serverCommandStub = sinon.stub(Server.prototype, 'command'); - serverCommandStub - .onCall(0) - .returns( - Promise.reject( - new MongoWriteConcernError({ errorLabels: ['RetryableWriteError'], code: 91 }, {}) - ) - ); - serverCommandStub - .onCall(1) - .returns( - Promise.reject( - new MongoWriteConcernError( - { errorLabels: ['RetryableWriteError', 'NoWritesPerformed'], errorCode: 10107 }, - {} - ) - ) - ); + serverCommandStub.onCall(0).returns( + Promise.reject( + new MongoWriteConcernError({ + errorLabels: ['RetryableWriteError'], + writeConcernError: { errmsg: 'ShutdownInProgress error', code: 91 } + }) + ) + ); + serverCommandStub.onCall(1).returns( + Promise.reject( + new MongoWriteConcernError({ + errorLabels: ['RetryableWriteError', 'NoWritesPerformed'], + writeConcernError: { errmsg: 'NotWritablePrimary error', errorCode: 10107 } + }) + ) + ); const insertResult = await collection.insertOne({ _id: 1 }).catch(error => error); sinon.restore(); From 6989a54dd5be174d4dbfae65e99fcc5722b1704f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 5 Jun 2024 12:34:17 -0400 Subject: [PATCH 15/29] fix: add get overload properly --- src/cmap/wire_protocol/on_demand/document.ts | 2 +- src/cmap/wire_protocol/responses.ts | 38 ++++++++++++-------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/cmap/wire_protocol/on_demand/document.ts b/src/cmap/wire_protocol/on_demand/document.ts index 247e8ca8579..6bdb8d7bcf9 100644 --- a/src/cmap/wire_protocol/on_demand/document.ts +++ b/src/cmap/wire_protocol/on_demand/document.ts @@ -254,7 +254,7 @@ export class OnDemandDocument { public get( name: string | number, as: T, - required?: false | undefined + required?: false | undefined | boolean ): JSTypeOf[T] | null; /** `required` will make `get` throw if name does not exist or is null/undefined */ diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index cf65885e034..0bfc87c011e 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -11,7 +11,7 @@ import { import { MongoUnexpectedServerResponseError } from '../../error'; import { type ClusterTime } from '../../sdam/common'; import { decorateDecryptionResult, ns } from '../../utils'; -import { OnDemandDocument } from './on_demand/document'; +import { type JSTypeOf, OnDemandDocument } from './on_demand/document'; // eslint-disable-next-line no-restricted-syntax const enum BSONElementOffset { @@ -74,6 +74,29 @@ export class MongoDBResponse extends OnDemandDocument { */ encryptedResponse?: MongoDBResponse; + // Wrap error thrown from BSON + public override get( + name: string | number, + as: T, + required?: false | undefined + ): JSTypeOf[T] | null; + public override get( + name: string | number, + as: T, + required: true + ): JSTypeOf[T]; + public override get( + name: string | number, + as: T, + required?: boolean | undefined + ): JSTypeOf[T] | null { + try { + return super.get(name, as, required); + } catch (cause) { + throw new MongoUnexpectedServerResponseError(cause.message, { cause }); + } + } + static is(value: unknown): value is MongoDBResponse { return value instanceof MongoDBResponse; } @@ -171,19 +194,6 @@ export class MongoDBResponse extends OnDemandDocument { } } -// Here's a little blast from the past. -// OLD style method definition so that I can override get without redefining ALL the fancy TS :/ -// TODO there must be a better way... -Object.defineProperty(MongoDBResponse.prototype, 'get', { - value: function get(name: any, as: any, required: any) { - try { - return OnDemandDocument.prototype.get.call(this, name, as, required); - } catch (cause) { - throw new MongoUnexpectedServerResponseError(cause.message, { cause }); - } - } -}); - /** @internal */ export class CursorResponse extends MongoDBResponse { /** From 9e07ea8bc1132019b467f6b26ac570baa87f0118 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 6 Jun 2024 15:07:47 -0400 Subject: [PATCH 16/29] chore: use serialize to make empty_v --- src/client-side-encryption/state_machine.ts | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/client-side-encryption/state_machine.ts b/src/client-side-encryption/state_machine.ts index 8cd0e1fb5f7..64a5c6061d3 100644 --- a/src/client-side-encryption/state_machine.ts +++ b/src/client-side-encryption/state_machine.ts @@ -123,14 +123,7 @@ export type CSFLEKMSTlsOptions = { * will inform us if we need to error by setting the state to `MONGOCRYPT_CTX_ERROR` but * otherwise we'll return `{ v: [] }`. */ -const EMPTY_V = Uint8Array.from([ - ...[13, 0, 0, 0], // document size = 13 bytes - ...[ - ...[4, 118, 0], // array type (4), "v\x00" basic latin "v" - ...[5, 0, 0, 0, 0] // empty document (5 byte size, null terminator) - ], - 0 // null terminator -]); +let EMPTY_V; /** * @internal @@ -229,7 +222,7 @@ export class StateMachine { if (keys.length === 0) { // See docs on EMPTY_V - result = EMPTY_V; + result = EMPTY_V ??= serialize({ v: [] }); } for await (const key of keys) { context.addMongoOperationResponse(serialize(key)); From 6d081f49bd1b1d61d5a91c5c7e33500b6440893b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 6 Jun 2024 15:13:32 -0400 Subject: [PATCH 17/29] docs: add comment about type crime --- src/cmap/connection.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index c37785414bc..fd23f0a53fe 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -766,6 +766,9 @@ export class CryptoConnection extends Connection { ns, encrypted, options, + // Eventually we want to require `responseType` which means we would satisfy `T` as the return type. + // In the meantime, we want encryptedResponse to always be _at least_ a MongoDBResponse if not a more specific subclass + // So that we can ensure we have access to the on-demand APIs for decorate response (responseType ?? MongoDBResponse) as any )) as unknown as MongoDBResponse; From 241a08e2808d4040fed06d6c21ebea40e8b356a1 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 6 Jun 2024 15:14:25 -0400 Subject: [PATCH 18/29] cruft --- src/cursor/change_stream_cursor.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cursor/change_stream_cursor.ts b/src/cursor/change_stream_cursor.ts index 7bebcc2d92b..bf0a55d848a 100644 --- a/src/cursor/change_stream_cursor.ts +++ b/src/cursor/change_stream_cursor.ts @@ -159,7 +159,6 @@ export class ChangeStreamCursor< const response = await super.getMore(batchSize); this.maxWireVersion = maxWireVersion(this.server); - // as ChangeStreamAggregateRawResult this._processBatch(response); this.emit(ChangeStream.MORE, response); From de3c2716a6472ea51dafd7331ca98092898782db Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 6 Jun 2024 15:20:54 -0400 Subject: [PATCH 19/29] chore: type annotation --- src/operations/count_documents.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/operations/count_documents.ts b/src/operations/count_documents.ts index a7037a64ba4..62273ad0228 100644 --- a/src/operations/count_documents.ts +++ b/src/operations/count_documents.ts @@ -1,5 +1,6 @@ import type { Document } from '../bson'; import type { Collection } from '../collection'; +import { type TODO_NODE_3286 } from '../mongo_types'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; import { AggregateOperation, type AggregateOptions } from './aggregate'; @@ -31,7 +32,10 @@ export class CountDocumentsOperation extends AggregateOperation { super(collection.s.namespace, pipeline, options); } - override async execute(server: Server, session: ClientSession | undefined): Promise { + override async execute( + server: Server, + session: ClientSession | undefined + ): Promise { const result = await super.execute(server, session); return result.shift()?.n ?? 0; } From 474369595b770a01eb959ca843384e6d8e34cfec Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 6 Jun 2024 15:30:12 -0400 Subject: [PATCH 20/29] chore: move ExecutionResult and document --- src/cursor/abstract_cursor.ts | 24 ++++++++++++++++++++++-- src/cursor/aggregation_cursor.ts | 7 +++---- src/cursor/change_stream_cursor.ts | 11 +++++++---- src/cursor/find_cursor.ts | 7 +++---- src/cursor/list_collections_cursor.ts | 7 +++---- src/cursor/list_indexes_cursor.ts | 7 +++---- src/cursor/run_command_cursor.ts | 6 +++--- src/index.ts | 6 ++++-- src/operations/execute_operation.ts | 12 ------------ 9 files changed, 48 insertions(+), 39 deletions(-) diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index 8faf23649a1..05de3b1af14 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -14,7 +14,7 @@ import { } from '../error'; import type { MongoClient } from '../mongo_client'; import { TypedEventEmitter } from '../mongo_types'; -import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; +import { executeOperation } from '../operations/execute_operation'; import { GetMoreOperation } from '../operations/get_more'; import { KillCursorsOperation } from '../operations/kill_cursors'; import { ReadConcern, type ReadConcernLike } from '../read_concern'; @@ -48,6 +48,24 @@ const kKilled = Symbol('killed'); /** @internal */ const kInit = Symbol('kInit'); +/** + * @internal + * TODO(NODE-2882): A cursor's getMore commands must be run on the same server it was started on + * and the same session must be used for the lifetime of the cursor. This object serves to get the + * server and session (along with the response) out of executeOperation back to the AbstractCursor. + * + * There may be a better design for communicating these values back to the cursor, currently an operation + * MUST store the selected server on itself so it can be read after executeOperation has returned. + */ +export interface InitialCursorResponse { + /** The server selected for the operation */ + server: Server; + /** The session used for this operation, may be implicitly created */ + session?: ClientSession; + /** The raw server response for the operation */ + response: CursorResponse; +} + /** @public */ export const CURSOR_FLAGS = [ 'tailable', @@ -622,7 +640,9 @@ export abstract class AbstractCursor< abstract clone(): AbstractCursor; /** @internal */ - protected abstract _initialize(session: ClientSession | undefined): Promise; + protected abstract _initialize( + session: ClientSession | undefined + ): Promise; /** @internal */ async getMore(batchSize: number): Promise { diff --git a/src/cursor/aggregation_cursor.ts b/src/cursor/aggregation_cursor.ts index 73ddcba40c7..d6c4338a07a 100644 --- a/src/cursor/aggregation_cursor.ts +++ b/src/cursor/aggregation_cursor.ts @@ -2,12 +2,12 @@ import type { Document } from '../bson'; import type { ExplainVerbosityLike } from '../explain'; import type { MongoClient } from '../mongo_client'; import { AggregateOperation, type AggregateOptions } from '../operations/aggregate'; -import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; +import { executeOperation } from '../operations/execute_operation'; import type { ClientSession } from '../sessions'; import type { Sort } from '../sort'; import type { MongoDBNamespace } from '../utils'; import { mergeOptions } from '../utils'; -import type { AbstractCursorOptions } from './abstract_cursor'; +import type { AbstractCursorOptions, InitialCursorResponse } from './abstract_cursor'; import { AbstractCursor, assertUninitialized } from './abstract_cursor'; /** @public */ @@ -61,7 +61,7 @@ export class AggregationCursor extends AbstractCursor { } /** @internal */ - async _initialize(session: ClientSession): Promise { + async _initialize(session: ClientSession): Promise { const aggregateOperation = new AggregateOperation(this.namespace, this[kPipeline], { ...this[kOptions], ...this.cursorOptions, @@ -70,7 +70,6 @@ export class AggregationCursor extends AbstractCursor { const response = await executeOperation(this.client, aggregateOperation); - // TODO: NODE-2882 return { server: aggregateOperation.server, session, response }; } diff --git a/src/cursor/change_stream_cursor.ts b/src/cursor/change_stream_cursor.ts index bf0a55d848a..b7c31c1b3f3 100644 --- a/src/cursor/change_stream_cursor.ts +++ b/src/cursor/change_stream_cursor.ts @@ -11,10 +11,14 @@ import { INIT, RESPONSE } from '../constants'; import type { MongoClient } from '../mongo_client'; import { AggregateOperation } from '../operations/aggregate'; import type { CollationOptions } from '../operations/command'; -import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; +import { executeOperation } from '../operations/execute_operation'; import type { ClientSession } from '../sessions'; import { maxWireVersion, type MongoDBNamespace } from '../utils'; -import { AbstractCursor, type AbstractCursorOptions } from './abstract_cursor'; +import { + AbstractCursor, + type AbstractCursorOptions, + type InitialCursorResponse +} from './abstract_cursor'; /** @internal */ export interface ChangeStreamCursorOptions extends AbstractCursorOptions { @@ -125,7 +129,7 @@ export class ChangeStreamCursor< }); } - async _initialize(session: ClientSession): Promise { + async _initialize(session: ClientSession): Promise { const aggregateOperation = new AggregateOperation(this.namespace, this.pipeline, { ...this.cursorOptions, ...this.options, @@ -151,7 +155,6 @@ export class ChangeStreamCursor< this.emit(INIT, response); this.emit(RESPONSE); - // TODO: NODE-2882 return { server, session, response }; } diff --git a/src/cursor/find_cursor.ts b/src/cursor/find_cursor.ts index 2966cfd4dbf..3469d8802d6 100644 --- a/src/cursor/find_cursor.ts +++ b/src/cursor/find_cursor.ts @@ -5,13 +5,13 @@ import { type ExplainVerbosityLike } from '../explain'; import type { MongoClient } from '../mongo_client'; import type { CollationOptions } from '../operations/command'; import { CountOperation, type CountOptions } from '../operations/count'; -import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; +import { executeOperation } from '../operations/execute_operation'; import { FindOperation, type FindOptions } from '../operations/find'; import type { Hint } from '../operations/operation'; import type { ClientSession } from '../sessions'; import { formatSort, type Sort, type SortDirection } from '../sort'; import { emitWarningOnce, mergeOptions, type MongoDBNamespace, squashError } from '../utils'; -import { AbstractCursor, assertUninitialized } from './abstract_cursor'; +import { AbstractCursor, assertUninitialized, type InitialCursorResponse } from './abstract_cursor'; /** @internal */ const kFilter = Symbol('filter'); @@ -69,7 +69,7 @@ export class FindCursor extends AbstractCursor { } /** @internal */ - async _initialize(session: ClientSession): Promise { + async _initialize(session: ClientSession): Promise { const findOperation = new FindOperation(this.namespace, this[kFilter], { ...this[kBuiltOptions], // NOTE: order matters here, we may need to refine this ...this.cursorOptions, @@ -81,7 +81,6 @@ export class FindCursor extends AbstractCursor { // the response is not a cursor when `explain` is enabled this[kNumReturned] = response.batchSize; - // TODO: NODE-2882 return { server: findOperation.server, session, response }; } diff --git a/src/cursor/list_collections_cursor.ts b/src/cursor/list_collections_cursor.ts index 6cefbd0d03d..a529709556d 100644 --- a/src/cursor/list_collections_cursor.ts +++ b/src/cursor/list_collections_cursor.ts @@ -1,13 +1,13 @@ import type { Document } from '../bson'; import type { Db } from '../db'; -import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; +import { executeOperation } from '../operations/execute_operation'; import { type CollectionInfo, ListCollectionsOperation, type ListCollectionsOptions } from '../operations/list_collections'; import type { ClientSession } from '../sessions'; -import { AbstractCursor } from './abstract_cursor'; +import { AbstractCursor, type InitialCursorResponse } from './abstract_cursor'; /** @public */ export class ListCollectionsCursor< @@ -34,7 +34,7 @@ export class ListCollectionsCursor< } /** @internal */ - async _initialize(session: ClientSession | undefined): Promise { + async _initialize(session: ClientSession | undefined): Promise { const operation = new ListCollectionsOperation(this.parent, this.filter, { ...this.cursorOptions, ...this.options, @@ -43,7 +43,6 @@ export class ListCollectionsCursor< const response = await executeOperation(this.parent.client, operation); - // TODO: NODE-2882 return { server: operation.server, session, response }; } } diff --git a/src/cursor/list_indexes_cursor.ts b/src/cursor/list_indexes_cursor.ts index 7d75edb9990..799ddf5bdb5 100644 --- a/src/cursor/list_indexes_cursor.ts +++ b/src/cursor/list_indexes_cursor.ts @@ -1,8 +1,8 @@ import type { Collection } from '../collection'; -import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; +import { executeOperation } from '../operations/execute_operation'; import { ListIndexesOperation, type ListIndexesOptions } from '../operations/indexes'; import type { ClientSession } from '../sessions'; -import { AbstractCursor } from './abstract_cursor'; +import { AbstractCursor, type InitialCursorResponse } from './abstract_cursor'; /** @public */ export class ListIndexesCursor extends AbstractCursor { @@ -23,7 +23,7 @@ export class ListIndexesCursor extends AbstractCursor { } /** @internal */ - async _initialize(session: ClientSession | undefined): Promise { + async _initialize(session: ClientSession | undefined): Promise { const operation = new ListIndexesOperation(this.parent, { ...this.cursorOptions, ...this.options, @@ -32,7 +32,6 @@ export class ListIndexesCursor extends AbstractCursor { const response = await executeOperation(this.parent.client, operation); - // TODO: NODE-2882 return { server: operation.server, session, response }; } } diff --git a/src/cursor/run_command_cursor.ts b/src/cursor/run_command_cursor.ts index 10ca22f7564..78b9826b9b1 100644 --- a/src/cursor/run_command_cursor.ts +++ b/src/cursor/run_command_cursor.ts @@ -2,14 +2,14 @@ import type { BSONSerializeOptions, Document } from '../bson'; import { CursorResponse } from '../cmap/wire_protocol/responses'; import type { Db } from '../db'; import { MongoAPIError } from '../error'; -import { executeOperation, type ExecutionResult } from '../operations/execute_operation'; +import { executeOperation } from '../operations/execute_operation'; import { GetMoreOperation } from '../operations/get_more'; import { RunCommandOperation } from '../operations/run_command'; import type { ReadConcernLike } from '../read_concern'; import type { ReadPreferenceLike } from '../read_preference'; import type { ClientSession } from '../sessions'; import { ns } from '../utils'; -import { AbstractCursor } from './abstract_cursor'; +import { AbstractCursor, type InitialCursorResponse } from './abstract_cursor'; /** @public */ export type RunCursorCommandOptions = { @@ -97,7 +97,7 @@ export class RunCommandCursor extends AbstractCursor { } /** @internal */ - protected async _initialize(session: ClientSession): Promise { + protected async _initialize(session: ClientSession): Promise { const operation = new RunCommandOperation(this.db, this.command, { ...this.cursorOptions, session: session, diff --git a/src/index.ts b/src/index.ts index 8546a30de05..d38f9db2650 100644 --- a/src/index.ts +++ b/src/index.ts @@ -336,7 +336,10 @@ export type { CursorFlag, CursorStreamOptions } from './cursor/abstract_cursor'; -export type { InternalAbstractCursorOptions } from './cursor/abstract_cursor'; +export type { + InitialCursorResponse, + InternalAbstractCursorOptions +} from './cursor/abstract_cursor'; export type { AggregationCursorOptions } from './cursor/aggregation_cursor'; export type { ChangeStreamCursorOptions } from './cursor/change_stream_cursor'; export type { @@ -464,7 +467,6 @@ 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 { ExecutionResult } from './operations/execute_operation'; export type { FindOptions } from './operations/find'; export type { FindOneAndDeleteOptions, diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 6b453a60926..829ea2ce6e2 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -1,4 +1,3 @@ -import { type CursorResponse } from '../cmap/wire_protocol/responses'; import { isRetryableReadError, isRetryableWriteError, @@ -17,7 +16,6 @@ import { } from '../error'; import type { MongoClient } from '../mongo_client'; import { ReadPreference } from '../read_preference'; -import type { Server } from '../sdam/server'; import type { ServerDescription } from '../sdam/server_description'; import { sameServerSelector, @@ -37,16 +35,6 @@ type ResultTypeFromOperation = TOperation extends AbstractOperation< ? K : never; -/** @internal */ -export interface ExecutionResult { - /** The server selected for the operation */ - server: Server; - /** The session used for this operation, may be implicitly created */ - session?: ClientSession; - /** The raw server response for the operation */ - response: CursorResponse; -} - /** * Executes the given operation with provided arguments. * @internal From cba9c49f45e48fe7da4c15dede3eea9321b61213 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 6 Jun 2024 15:36:21 -0400 Subject: [PATCH 21/29] test: add match to expected errors --- test/unit/cmap/wire_protocol/responses.test.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/unit/cmap/wire_protocol/responses.test.ts b/test/unit/cmap/wire_protocol/responses.test.ts index a3338c77edc..18497dc1579 100644 --- a/test/unit/cmap/wire_protocol/responses.test.ts +++ b/test/unit/cmap/wire_protocol/responses.test.ts @@ -54,12 +54,12 @@ describe('class MongoDBResponse', () => { }); }); -describe('class CursorResponse', () => { +describe.only('class CursorResponse', () => { describe('get cursor()', () => { it('throws if input does not contain cursor embedded document', () => { - // @ts-expect-error: testing private getter expect(() => new CursorResponse(BSON.serialize({ ok: 1 })).cursor).to.throw( - MongoUnexpectedServerResponseError + MongoUnexpectedServerResponseError, + /"cursor" is missing/ ); }); }); @@ -67,7 +67,8 @@ describe('class CursorResponse', () => { describe('get id()', () => { it('throws if input does not contain cursor.id int64', () => { expect(() => new CursorResponse(BSON.serialize({ ok: 1, cursor: {} })).id).to.throw( - MongoUnexpectedServerResponseError + MongoUnexpectedServerResponseError, + /"id" is missing/ ); }); }); @@ -77,7 +78,7 @@ describe('class CursorResponse', () => { expect( // @ts-expect-error: testing private getter () => new CursorResponse(BSON.serialize({ ok: 1, cursor: { id: 0n, batch: [] } })).batch - ).to.throw(MongoUnexpectedServerResponseError); + ).to.throw(MongoUnexpectedServerResponseError, /did not contain a batch/); }); }); From acbb323cc7172b2738fe4ef7ca5f019be83b688e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 6 Jun 2024 15:43:44 -0400 Subject: [PATCH 22/29] test: uncomment wc error ctor tests --- .../unit/cmap/wire_protocol/responses.test.ts | 2 +- test/unit/error.test.ts | 97 +++++++++++-------- 2 files changed, 59 insertions(+), 40 deletions(-) diff --git a/test/unit/cmap/wire_protocol/responses.test.ts b/test/unit/cmap/wire_protocol/responses.test.ts index 18497dc1579..9498765cf4f 100644 --- a/test/unit/cmap/wire_protocol/responses.test.ts +++ b/test/unit/cmap/wire_protocol/responses.test.ts @@ -54,7 +54,7 @@ describe('class MongoDBResponse', () => { }); }); -describe.only('class CursorResponse', () => { +describe('class CursorResponse', () => { describe('get cursor()', () => { it('throws if input does not contain cursor embedded document', () => { expect(() => new CursorResponse(BSON.serialize({ ok: 1 })).cursor).to.throw( diff --git a/test/unit/error.test.ts b/test/unit/error.test.ts index 22ac271af88..c57ee71da6f 100644 --- a/test/unit/error.test.ts +++ b/test/unit/error.test.ts @@ -475,45 +475,64 @@ describe('MongoErrors', () => { error: new MongoNetworkError('socket bad, try again'), maxWireVersion: BELOW_4_4 }, - // { - // description: 'a MongoWriteConcernError with no code nor label', - // result: false, - // error: new MongoWriteConcernError({ message: 'empty wc error' }), - // maxWireVersion: BELOW_4_4 - // }, - // { - // description: 'a MongoWriteConcernError with a random label', - // result: false, - // error: new MongoWriteConcernError( - // { message: 'random label' }, - // { errorLabels: ['myLabel'] } - // ), - // maxWireVersion: BELOW_4_4 - // }, - // { - // description: 'a MongoWriteConcernError with a retryable code above server 4.4', - // result: false, - // error: new MongoWriteConcernError({}, { code: 262 }), - // maxWireVersion: ABOVE_4_4 - // }, - // { - // description: 'a MongoWriteConcernError with a retryable code below server 4.4', - // result: true, - // error: new MongoWriteConcernError({}, { code: 262 }), - // maxWireVersion: BELOW_4_4 - // }, - // { - // description: 'a MongoWriteConcernError with a RetryableWriteError label below server 4.4', - // result: false, - // error: new MongoWriteConcernError({}, { errorLabels: ['RetryableWriteError'] }), - // maxWireVersion: BELOW_4_4 - // }, - // { - // description: 'a MongoWriteConcernError with a RetryableWriteError label above server 4.4', - // result: false, - // error: new MongoWriteConcernError({}, { errorLabels: ['RetryableWriteError'] }), - // maxWireVersion: ABOVE_4_4 - // }, + { + description: 'a MongoWriteConcernError with a random label', + result: false, + error: new MongoWriteConcernError({ + writeConcernError: { + errmsg: 'random label', + code: 1 + }, + errorLabels: ['myLabel'] + }), + maxWireVersion: BELOW_4_4 + }, + { + description: 'a MongoWriteConcernError with a retryable code above server 4.4', + result: false, + error: new MongoWriteConcernError({ + writeConcernError: { + errmsg: 'code 262', // ExceededTimeLimit, is retryable + code: 262 + } + }), + maxWireVersion: ABOVE_4_4 + }, + { + description: 'a MongoWriteConcernError with a retryable code below server 4.4', + result: true, + error: new MongoWriteConcernError({ + writeConcernError: { + errmsg: 'code 262', + code: 262 + } + }), + maxWireVersion: BELOW_4_4 + }, + { + description: 'a MongoWriteConcernError with a RetryableWriteError label below server 4.4', + result: false, + error: new MongoWriteConcernError({ + writeConcernError: { + errmsg: 'code 1', + code: 1 + }, + errorLabels: ['RetryableWriteError'] + }), + maxWireVersion: BELOW_4_4 + }, + { + description: 'a MongoWriteConcernError with a RetryableWriteError label above server 4.4', + result: false, + error: new MongoWriteConcernError({ + writeConcernError: { + errmsg: 'code 1', + code: 1 + }, + errorLabels: ['RetryableWriteError'] + }), + maxWireVersion: ABOVE_4_4 + }, { description: 'any MongoError with a RetryableWriteError label', result: false, From 1ffa7529d7aa03338982cae82a1a9d8fccee1237 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 7 Jun 2024 14:35:12 -0400 Subject: [PATCH 23/29] fix: super generic --- src/cmap/connection.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index fd23f0a53fe..b5964af11a2 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -551,6 +551,13 @@ export class Connection extends TypedEventEmitter { } } + public async command( + ns: MongoDBNamespace, + command: Document, + options: CommandOptions | undefined, + responseType: T + ): Promise>; + public async command( ns: MongoDBNamespace, command: Document, @@ -762,15 +769,15 @@ export class CryptoConnection extends Connection { } } - const encryptedResponse: MongoDBResponse = (await super.command( + const encryptedResponse = await super.command( ns, encrypted, options, // Eventually we want to require `responseType` which means we would satisfy `T` as the return type. // In the meantime, we want encryptedResponse to always be _at least_ a MongoDBResponse if not a more specific subclass // So that we can ensure we have access to the on-demand APIs for decorate response - (responseType ?? MongoDBResponse) as any - )) as unknown as MongoDBResponse; + responseType ?? MongoDBResponse + ); const result = await autoEncrypter.decrypt(encryptedResponse.toBytes(), options); From 827e3f725053ce0a8cb68f442003c0af5e70450d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 7 Jun 2024 15:15:54 -0400 Subject: [PATCH 24/29] chore: fix nullish documents --- src/cursor/abstract_cursor.ts | 42 +++++++++++++++-------------------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index d0d28de36bd..abe6e2ecb68 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -159,7 +159,7 @@ export abstract class AbstractCursor< /** @internal */ private [kNamespace]: MongoDBNamespace; /** @internal */ - [kDocuments]: CursorResponse = { length: 0 } as unknown as CursorResponse; + private [kDocuments]: CursorResponse | null = null; /** @internal */ private [kClient]: MongoClient; /** @internal */ @@ -291,16 +291,19 @@ export abstract class AbstractCursor< /** Returns current buffered documents length */ bufferedCount(): number { - return this[kDocuments].length; + return this[kDocuments]?.length ?? 0; } /** Returns current buffered documents */ readBufferedDocuments(number?: number): TSchema[] { const bufferedDocs: TSchema[] = []; - const documentsToRead = Math.min(number ?? this[kDocuments].length, this[kDocuments].length); + const documentsToRead = Math.min( + number ?? this[kDocuments]?.length ?? 0, + this[kDocuments]?.length ?? 0 + ); for (let count = 0; count < documentsToRead; count++) { - const document = this[kDocuments].shift(this[kOptions]); + const document = this[kDocuments]?.shift(this[kOptions]); if (document != null) { bufferedDocs.push(document); } @@ -319,11 +322,11 @@ export abstract class AbstractCursor< return; } - if (this.closed && this[kDocuments].length === 0) { + if (this.closed && (this[kDocuments]?.length ?? 0) === 0) { return; } - if (this[kId] != null && this.isDead && this[kDocuments].length === 0) { + if (this[kId] != null && this.isDead && (this[kDocuments]?.length ?? 0) === 0) { return; } @@ -385,11 +388,11 @@ export abstract class AbstractCursor< } do { - if (this[kDocuments].length !== 0) { + if ((this[kDocuments]?.length ?? 0) !== 0) { return true; } await this.fetchBatch(); - } while (!this.isDead || this[kDocuments].length !== 0); + } while (!this.isDead || (this[kDocuments]?.length ?? 0) !== 0); return false; } @@ -401,13 +404,13 @@ export abstract class AbstractCursor< } do { - const doc = this[kDocuments].shift(); + const doc = this[kDocuments]?.shift(); if (doc != null) { if (this[kTransform] != null) return await this.transformDocument(doc); return doc; } await this.fetchBatch(); - } while (!this.isDead || this[kDocuments].length !== 0); + } while (!this.isDead || (this[kDocuments]?.length ?? 0) !== 0); return null; } @@ -420,7 +423,7 @@ export abstract class AbstractCursor< throw new MongoCursorExhaustedError(); } - let doc = this[kDocuments].shift(); + let doc = this[kDocuments]?.shift(); if (doc != null) { if (this[kTransform] != null) return await this.transformDocument(doc); return doc; @@ -428,7 +431,7 @@ export abstract class AbstractCursor< await this.fetchBatch(); - doc = this[kDocuments].shift(); + doc = this[kDocuments]?.shift(); if (doc != null) { if (this[kTransform] != null) return await this.transformDocument(doc); return doc; @@ -629,7 +632,7 @@ export abstract class AbstractCursor< } this[kId] = null; - this[kDocuments].clear(); + this[kDocuments]?.clear(); this[kClosed] = false; this[kKilled] = false; this[kInitialized] = false; @@ -716,7 +719,7 @@ export abstract class AbstractCursor< if (this[kId] == null) { await this.cursorInit(); // If the cursor died or returned documents, return - if (this[kDocuments].length !== 0 || this.isDead) return; + if ((this[kDocuments]?.length ?? 0) !== 0 || this.isDead) return; // Otherwise, run a getMore } @@ -787,7 +790,7 @@ export abstract class AbstractCursor< /** @internal */ private emitClose() { try { - if (!this.hasEmittedClose && (this[kDocuments].length === 0 || this[kClosed])) { + if (!this.hasEmittedClose && ((this[kDocuments]?.length ?? 0) === 0 || this[kClosed])) { // @ts-expect-error: CursorEvents is generic so Parameters may not be assignable to `[]`. Not sure how to require extenders do not add parameters. this.emit('close'); } @@ -826,15 +829,6 @@ export abstract class AbstractCursor< } } -/** A temporary helper to box up the many possible type issue of cursor ids */ -function getCursorId(response: Document) { - return typeof response.cursor.id === 'number' - ? Long.fromNumber(response.cursor.id) - : typeof response.cursor.id === 'bigint' - ? Long.fromBigInt(response.cursor.id) - : response.cursor.id; -} - class ReadableCursorStream extends Readable { private _cursor: AbstractCursor; private _readInProgress = false; From 00b90eaaca82296957d0eb93e3518cf0ea4d39ea Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 7 Jun 2024 15:36:45 -0400 Subject: [PATCH 25/29] fix: pass through options --- src/cursor/abstract_cursor.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index abe6e2ecb68..7705bf5d0be 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -404,7 +404,7 @@ export abstract class AbstractCursor< } do { - const doc = this[kDocuments]?.shift(); + const doc = this[kDocuments]?.shift(this[kOptions]); if (doc != null) { if (this[kTransform] != null) return await this.transformDocument(doc); return doc; @@ -423,7 +423,7 @@ export abstract class AbstractCursor< throw new MongoCursorExhaustedError(); } - let doc = this[kDocuments]?.shift(); + let doc = this[kDocuments]?.shift(this[kOptions]); if (doc != null) { if (this[kTransform] != null) return await this.transformDocument(doc); return doc; @@ -431,7 +431,7 @@ export abstract class AbstractCursor< await this.fetchBatch(); - doc = this[kDocuments]?.shift(); + doc = this[kDocuments]?.shift(this[kOptions]); if (doc != null) { if (this[kTransform] != null) return await this.transformDocument(doc); return doc; From e517ab39fbfe84777799160e6211dd972f4010fd Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 10 Jun 2024 12:00:26 -0400 Subject: [PATCH 26/29] refactor: move CountDocument logic into collection API (#4138) --- src/collection.ts | 30 +++++- src/index.ts | 8 +- src/operations/count_documents.ts | 42 -------- .../crud/abstract_operation.test.ts | 5 - test/integration/crud/crud_api.test.ts | 95 +++++++++++++++++-- test/mongodb.ts | 1 - 6 files changed, 119 insertions(+), 62 deletions(-) delete mode 100644 src/operations/count_documents.ts diff --git a/src/collection.ts b/src/collection.ts index 459ecd34226..9b87429d6a7 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -25,7 +25,6 @@ import type { import type { AggregateOptions } from './operations/aggregate'; import { BulkWriteOperation } from './operations/bulk_write'; import { CountOperation, type CountOptions } from './operations/count'; -import { CountDocumentsOperation, type CountDocumentsOptions } from './operations/count_documents'; import { DeleteManyOperation, DeleteOneOperation, @@ -94,6 +93,14 @@ import { } from './utils'; import { WriteConcern, type WriteConcernOptions } from './write_concern'; +/** @public */ +export interface CountDocumentsOptions extends AggregateOptions { + /** The number of documents to skip. */ + skip?: number; + /** The maximum amounts to count before aborting. */ + limit?: number; +} + /** @public */ export interface ModifyResult { value: WithId | null; @@ -764,10 +771,23 @@ export class Collection { filter: Filter = {}, options: CountDocumentsOptions = {} ): Promise { - return await executeOperation( - this.client, - new CountDocumentsOperation(this as TODO_NODE_3286, filter, resolveOptions(this, options)) - ); + const pipeline = []; + pipeline.push({ $match: filter }); + + if (typeof options.skip === 'number') { + pipeline.push({ $skip: options.skip }); + } + + if (typeof options.limit === 'number') { + pipeline.push({ $limit: options.limit }); + } + + pipeline.push({ $group: { _id: 1, n: { $sum: 1 } } }); + + const cursor = this.aggregate<{ n: number }>(pipeline, options); + const doc = await cursor.next(); + await cursor.close(); + return doc?.n ?? 0; } /** diff --git a/src/index.ts b/src/index.ts index 9c4e3d4697c..ae5288a4856 100644 --- a/src/index.ts +++ b/src/index.ts @@ -300,7 +300,12 @@ export type { MongoDBResponse, MongoDBResponseConstructor } from './cmap/wire_protocol/responses'; -export type { CollectionOptions, CollectionPrivate, ModifyResult } from './collection'; +export type { + CollectionOptions, + CollectionPrivate, + CountDocumentsOptions, + ModifyResult +} from './collection'; export type { COMMAND_FAILED, COMMAND_STARTED, @@ -458,7 +463,6 @@ export type { OperationParent } from './operations/command'; export type { CountOptions } from './operations/count'; -export type { CountDocumentsOptions } from './operations/count_documents'; export type { ClusteredCollectionOptions, CreateCollectionOptions, diff --git a/src/operations/count_documents.ts b/src/operations/count_documents.ts deleted file mode 100644 index 62273ad0228..00000000000 --- a/src/operations/count_documents.ts +++ /dev/null @@ -1,42 +0,0 @@ -import type { Document } from '../bson'; -import type { Collection } from '../collection'; -import { type TODO_NODE_3286 } from '../mongo_types'; -import type { Server } from '../sdam/server'; -import type { ClientSession } from '../sessions'; -import { AggregateOperation, type AggregateOptions } from './aggregate'; - -/** @public */ -export interface CountDocumentsOptions extends AggregateOptions { - /** The number of documents to skip. */ - skip?: number; - /** The maximum amounts to count before aborting. */ - limit?: number; -} - -/** @internal */ -export class CountDocumentsOperation extends AggregateOperation { - constructor(collection: Collection, query: Document, options: CountDocumentsOptions) { - const pipeline = []; - pipeline.push({ $match: query }); - - if (typeof options.skip === 'number') { - pipeline.push({ $skip: options.skip }); - } - - if (typeof options.limit === 'number') { - pipeline.push({ $limit: options.limit }); - } - - pipeline.push({ $group: { _id: 1, n: { $sum: 1 } } }); - - super(collection.s.namespace, pipeline, options); - } - - override async execute( - server: Server, - session: ClientSession | undefined - ): Promise { - const result = await super.execute(server, session); - return result.shift()?.n ?? 0; - } -} diff --git a/test/integration/crud/abstract_operation.test.ts b/test/integration/crud/abstract_operation.test.ts index dd972cd316f..016fb39b173 100644 --- a/test/integration/crud/abstract_operation.test.ts +++ b/test/integration/crud/abstract_operation.test.ts @@ -53,11 +53,6 @@ describe('abstract operation', function () { subclassType: mongodb.CountOperation, correctCommandName: 'count' }, - { - subclassCreator: () => new mongodb.CountDocumentsOperation(collection, { a: 1 }, {}), - subclassType: mongodb.CountDocumentsOperation, - correctCommandName: 'aggregate' - }, { subclassCreator: () => new mongodb.CreateCollectionOperation(db, 'name'), subclassType: mongodb.CreateCollectionOperation, diff --git a/test/integration/crud/crud_api.test.ts b/test/integration/crud/crud_api.test.ts index 55c39f9bd16..99301abaecc 100644 --- a/test/integration/crud/crud_api.test.ts +++ b/test/integration/crud/crud_api.test.ts @@ -105,9 +105,9 @@ describe('CRUD API', function () { const spy = sinon.spy(Collection.prototype, 'find'); const result = await collection.findOne({}); expect(result).to.deep.equal({ _id: 1 }); - expect(events.at(0)).to.be.instanceOf(CommandSucceededEvent); - expect(spy.returnValues.at(0)).to.have.property('closed', true); - expect(spy.returnValues.at(0)).to.have.nested.property('session.hasEnded', true); + expect(events[0]).to.be.instanceOf(CommandSucceededEvent); + expect(spy.returnValues[0]).to.have.property('closed', true); + expect(spy.returnValues[0]).to.have.nested.property('session.hasEnded', true); }); }); @@ -117,7 +117,7 @@ describe('CRUD API', function () { if (this.currentTest) { this.currentTest.skipReason = `Cannot run fail points on server version: ${this.configuration.version}`; } - return this.skip(); + this.skip(); } const failPoint: FailPoint = { @@ -149,9 +149,90 @@ describe('CRUD API', function () { const spy = sinon.spy(Collection.prototype, 'find'); const error = await collection.findOne({}).catch(error => error); expect(error).to.be.instanceOf(MongoServerError); - expect(events.at(0)).to.be.instanceOf(CommandFailedEvent); - expect(spy.returnValues.at(0)).to.have.property('closed', true); - expect(spy.returnValues.at(0)).to.have.nested.property('session.hasEnded', true); + expect(events[0]).to.be.instanceOf(CommandFailedEvent); + expect(spy.returnValues[0]).to.have.property('closed', true); + expect(spy.returnValues[0]).to.have.nested.property('session.hasEnded', true); + }); + }); + }); + + describe('countDocuments()', () => { + let client: MongoClient; + let events; + let collection: Collection<{ _id: number }>; + + beforeEach(async function () { + client = this.configuration.newClient({ monitorCommands: true }); + events = []; + client.on('commandSucceeded', commandSucceeded => + commandSucceeded.commandName === 'aggregate' ? events.push(commandSucceeded) : null + ); + client.on('commandFailed', commandFailed => + commandFailed.commandName === 'aggregate' ? events.push(commandFailed) : null + ); + + collection = client.db('countDocuments').collection('countDocuments'); + await collection.drop().catch(() => null); + await collection.insertMany([{ _id: 1 }, { _id: 2 }]); + }); + + afterEach(async () => { + await collection.drop().catch(() => null); + await client.close(); + }); + + describe('when the aggregation operation succeeds', () => { + it('the cursor for countDocuments is closed', async function () { + const spy = sinon.spy(Collection.prototype, 'aggregate'); + const result = await collection.countDocuments({}); + expect(result).to.deep.equal(2); + expect(events[0]).to.be.instanceOf(CommandSucceededEvent); + expect(spy.returnValues[0]).to.have.property('closed', true); + expect(spy.returnValues[0]).to.have.nested.property('session.hasEnded', true); + }); + }); + + describe('when the aggregation operation fails', () => { + beforeEach(async function () { + if (semver.lt(this.configuration.version, '4.2.0')) { + if (this.currentTest) { + this.currentTest.skipReason = `Cannot run fail points on server version: ${this.configuration.version}`; + } + this.skip(); + } + + const failPoint: FailPoint = { + configureFailPoint: 'failCommand', + mode: 'alwaysOn', + data: { + failCommands: ['aggregate'], + // 1 == InternalError, but this value not important to the test + errorCode: 1 + } + }; + await client.db().admin().command(failPoint); + }); + + afterEach(async function () { + if (semver.lt(this.configuration.version, '4.2.0')) { + return; + } + + const failPoint: FailPoint = { + configureFailPoint: 'failCommand', + mode: 'off', + data: { failCommands: ['aggregate'] } + }; + await client.db().admin().command(failPoint); + }); + + it('the cursor for countDocuments is closed', async function () { + const spy = sinon.spy(Collection.prototype, 'aggregate'); + const error = await collection.countDocuments({}).catch(error => error); + expect(error).to.be.instanceOf(MongoServerError); + expect(events[0]).to.be.instanceOf(CommandFailedEvent); + expect(spy.returnValues[0]).to.have.property('closed', true); + expect(spy.returnValues[0]).to.have.nested.property('session.hasEnded', true); }); }); }); diff --git a/test/mongodb.ts b/test/mongodb.ts index 2d44f357863..887c65d2774 100644 --- a/test/mongodb.ts +++ b/test/mongodb.ts @@ -157,7 +157,6 @@ export * from '../src/operations/bulk_write'; export * from '../src/operations/collections'; export * from '../src/operations/command'; export * from '../src/operations/count'; -export * from '../src/operations/count_documents'; export * from '../src/operations/create_collection'; export * from '../src/operations/delete'; export * from '../src/operations/distinct'; From 9d753037dac32e309699b27482764a30ca0af7ab Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 10 Jun 2024 13:12:19 -0400 Subject: [PATCH 27/29] Revert "refactor: move CountDocument logic into collection API" (#4139) --- src/collection.ts | 30 +----- src/index.ts | 8 +- src/operations/count_documents.ts | 42 ++++++++ .../crud/abstract_operation.test.ts | 5 + test/integration/crud/crud_api.test.ts | 95 ++----------------- test/mongodb.ts | 1 + 6 files changed, 62 insertions(+), 119 deletions(-) create mode 100644 src/operations/count_documents.ts diff --git a/src/collection.ts b/src/collection.ts index 9b87429d6a7..459ecd34226 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -25,6 +25,7 @@ import type { import type { AggregateOptions } from './operations/aggregate'; import { BulkWriteOperation } from './operations/bulk_write'; import { CountOperation, type CountOptions } from './operations/count'; +import { CountDocumentsOperation, type CountDocumentsOptions } from './operations/count_documents'; import { DeleteManyOperation, DeleteOneOperation, @@ -93,14 +94,6 @@ import { } from './utils'; import { WriteConcern, type WriteConcernOptions } from './write_concern'; -/** @public */ -export interface CountDocumentsOptions extends AggregateOptions { - /** The number of documents to skip. */ - skip?: number; - /** The maximum amounts to count before aborting. */ - limit?: number; -} - /** @public */ export interface ModifyResult { value: WithId | null; @@ -771,23 +764,10 @@ export class Collection { filter: Filter = {}, options: CountDocumentsOptions = {} ): Promise { - const pipeline = []; - pipeline.push({ $match: filter }); - - if (typeof options.skip === 'number') { - pipeline.push({ $skip: options.skip }); - } - - if (typeof options.limit === 'number') { - pipeline.push({ $limit: options.limit }); - } - - pipeline.push({ $group: { _id: 1, n: { $sum: 1 } } }); - - const cursor = this.aggregate<{ n: number }>(pipeline, options); - const doc = await cursor.next(); - await cursor.close(); - return doc?.n ?? 0; + return await executeOperation( + this.client, + new CountDocumentsOperation(this as TODO_NODE_3286, filter, resolveOptions(this, options)) + ); } /** diff --git a/src/index.ts b/src/index.ts index ae5288a4856..9c4e3d4697c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -300,12 +300,7 @@ export type { MongoDBResponse, MongoDBResponseConstructor } from './cmap/wire_protocol/responses'; -export type { - CollectionOptions, - CollectionPrivate, - CountDocumentsOptions, - ModifyResult -} from './collection'; +export type { CollectionOptions, CollectionPrivate, ModifyResult } from './collection'; export type { COMMAND_FAILED, COMMAND_STARTED, @@ -463,6 +458,7 @@ export type { OperationParent } from './operations/command'; export type { CountOptions } from './operations/count'; +export type { CountDocumentsOptions } from './operations/count_documents'; export type { ClusteredCollectionOptions, CreateCollectionOptions, diff --git a/src/operations/count_documents.ts b/src/operations/count_documents.ts new file mode 100644 index 00000000000..62273ad0228 --- /dev/null +++ b/src/operations/count_documents.ts @@ -0,0 +1,42 @@ +import type { Document } from '../bson'; +import type { Collection } from '../collection'; +import { type TODO_NODE_3286 } from '../mongo_types'; +import type { Server } from '../sdam/server'; +import type { ClientSession } from '../sessions'; +import { AggregateOperation, type AggregateOptions } from './aggregate'; + +/** @public */ +export interface CountDocumentsOptions extends AggregateOptions { + /** The number of documents to skip. */ + skip?: number; + /** The maximum amounts to count before aborting. */ + limit?: number; +} + +/** @internal */ +export class CountDocumentsOperation extends AggregateOperation { + constructor(collection: Collection, query: Document, options: CountDocumentsOptions) { + const pipeline = []; + pipeline.push({ $match: query }); + + if (typeof options.skip === 'number') { + pipeline.push({ $skip: options.skip }); + } + + if (typeof options.limit === 'number') { + pipeline.push({ $limit: options.limit }); + } + + pipeline.push({ $group: { _id: 1, n: { $sum: 1 } } }); + + super(collection.s.namespace, pipeline, options); + } + + override async execute( + server: Server, + session: ClientSession | undefined + ): Promise { + const result = await super.execute(server, session); + return result.shift()?.n ?? 0; + } +} diff --git a/test/integration/crud/abstract_operation.test.ts b/test/integration/crud/abstract_operation.test.ts index 016fb39b173..dd972cd316f 100644 --- a/test/integration/crud/abstract_operation.test.ts +++ b/test/integration/crud/abstract_operation.test.ts @@ -53,6 +53,11 @@ describe('abstract operation', function () { subclassType: mongodb.CountOperation, correctCommandName: 'count' }, + { + subclassCreator: () => new mongodb.CountDocumentsOperation(collection, { a: 1 }, {}), + subclassType: mongodb.CountDocumentsOperation, + correctCommandName: 'aggregate' + }, { subclassCreator: () => new mongodb.CreateCollectionOperation(db, 'name'), subclassType: mongodb.CreateCollectionOperation, diff --git a/test/integration/crud/crud_api.test.ts b/test/integration/crud/crud_api.test.ts index 99301abaecc..55c39f9bd16 100644 --- a/test/integration/crud/crud_api.test.ts +++ b/test/integration/crud/crud_api.test.ts @@ -105,9 +105,9 @@ describe('CRUD API', function () { const spy = sinon.spy(Collection.prototype, 'find'); const result = await collection.findOne({}); expect(result).to.deep.equal({ _id: 1 }); - expect(events[0]).to.be.instanceOf(CommandSucceededEvent); - expect(spy.returnValues[0]).to.have.property('closed', true); - expect(spy.returnValues[0]).to.have.nested.property('session.hasEnded', true); + expect(events.at(0)).to.be.instanceOf(CommandSucceededEvent); + expect(spy.returnValues.at(0)).to.have.property('closed', true); + expect(spy.returnValues.at(0)).to.have.nested.property('session.hasEnded', true); }); }); @@ -117,7 +117,7 @@ describe('CRUD API', function () { if (this.currentTest) { this.currentTest.skipReason = `Cannot run fail points on server version: ${this.configuration.version}`; } - this.skip(); + return this.skip(); } const failPoint: FailPoint = { @@ -149,90 +149,9 @@ describe('CRUD API', function () { const spy = sinon.spy(Collection.prototype, 'find'); const error = await collection.findOne({}).catch(error => error); expect(error).to.be.instanceOf(MongoServerError); - expect(events[0]).to.be.instanceOf(CommandFailedEvent); - expect(spy.returnValues[0]).to.have.property('closed', true); - expect(spy.returnValues[0]).to.have.nested.property('session.hasEnded', true); - }); - }); - }); - - describe('countDocuments()', () => { - let client: MongoClient; - let events; - let collection: Collection<{ _id: number }>; - - beforeEach(async function () { - client = this.configuration.newClient({ monitorCommands: true }); - events = []; - client.on('commandSucceeded', commandSucceeded => - commandSucceeded.commandName === 'aggregate' ? events.push(commandSucceeded) : null - ); - client.on('commandFailed', commandFailed => - commandFailed.commandName === 'aggregate' ? events.push(commandFailed) : null - ); - - collection = client.db('countDocuments').collection('countDocuments'); - await collection.drop().catch(() => null); - await collection.insertMany([{ _id: 1 }, { _id: 2 }]); - }); - - afterEach(async () => { - await collection.drop().catch(() => null); - await client.close(); - }); - - describe('when the aggregation operation succeeds', () => { - it('the cursor for countDocuments is closed', async function () { - const spy = sinon.spy(Collection.prototype, 'aggregate'); - const result = await collection.countDocuments({}); - expect(result).to.deep.equal(2); - expect(events[0]).to.be.instanceOf(CommandSucceededEvent); - expect(spy.returnValues[0]).to.have.property('closed', true); - expect(spy.returnValues[0]).to.have.nested.property('session.hasEnded', true); - }); - }); - - describe('when the aggregation operation fails', () => { - beforeEach(async function () { - if (semver.lt(this.configuration.version, '4.2.0')) { - if (this.currentTest) { - this.currentTest.skipReason = `Cannot run fail points on server version: ${this.configuration.version}`; - } - this.skip(); - } - - const failPoint: FailPoint = { - configureFailPoint: 'failCommand', - mode: 'alwaysOn', - data: { - failCommands: ['aggregate'], - // 1 == InternalError, but this value not important to the test - errorCode: 1 - } - }; - await client.db().admin().command(failPoint); - }); - - afterEach(async function () { - if (semver.lt(this.configuration.version, '4.2.0')) { - return; - } - - const failPoint: FailPoint = { - configureFailPoint: 'failCommand', - mode: 'off', - data: { failCommands: ['aggregate'] } - }; - await client.db().admin().command(failPoint); - }); - - it('the cursor for countDocuments is closed', async function () { - const spy = sinon.spy(Collection.prototype, 'aggregate'); - const error = await collection.countDocuments({}).catch(error => error); - expect(error).to.be.instanceOf(MongoServerError); - expect(events[0]).to.be.instanceOf(CommandFailedEvent); - expect(spy.returnValues[0]).to.have.property('closed', true); - expect(spy.returnValues[0]).to.have.nested.property('session.hasEnded', true); + expect(events.at(0)).to.be.instanceOf(CommandFailedEvent); + expect(spy.returnValues.at(0)).to.have.property('closed', true); + expect(spy.returnValues.at(0)).to.have.nested.property('session.hasEnded', true); }); }); }); diff --git a/test/mongodb.ts b/test/mongodb.ts index 887c65d2774..2d44f357863 100644 --- a/test/mongodb.ts +++ b/test/mongodb.ts @@ -157,6 +157,7 @@ export * from '../src/operations/bulk_write'; export * from '../src/operations/collections'; export * from '../src/operations/command'; export * from '../src/operations/count'; +export * from '../src/operations/count_documents'; export * from '../src/operations/create_collection'; export * from '../src/operations/delete'; export * from '../src/operations/distinct'; From c1c8ba4cb6216f04a874f79f965a23fccda986fc Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 10 Jun 2024 16:10:12 -0400 Subject: [PATCH 28/29] chore: only attach encryptedResponse to cursor response --- src/cmap/connection.ts | 8 ++++++-- src/cmap/wire_protocol/responses.ts | 15 +++++++-------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index b5964af11a2..1e4afc7f387 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -63,7 +63,11 @@ import type { ClientMetadata } from './handshake/client_metadata'; import { StreamDescription, type StreamDescriptionOptions } from './stream_description'; import { type CompressorName, decompressResponse } from './wire_protocol/compression'; import { onData } from './wire_protocol/on_data'; -import { MongoDBResponse, type MongoDBResponseConstructor } from './wire_protocol/responses'; +import { + CursorResponse, + MongoDBResponse, + type MongoDBResponseConstructor +} from './wire_protocol/responses'; import { getReadPreference, isSharded } from './wire_protocol/shared'; /** @internal */ @@ -786,7 +790,7 @@ export class CryptoConnection extends Connection { if (autoEncrypter[kDecorateResult]) { if (responseType == null) { decorateDecryptionResult(decryptedResponse, encryptedResponse.toObject(), true); - } else { + } else if (decryptedResponse instanceof CursorResponse) { decryptedResponse.encryptedResponse = encryptedResponse; } } diff --git a/src/cmap/wire_protocol/responses.ts b/src/cmap/wire_protocol/responses.ts index 0bfc87c011e..0ef048e8da5 100644 --- a/src/cmap/wire_protocol/responses.ts +++ b/src/cmap/wire_protocol/responses.ts @@ -66,14 +66,6 @@ export type MongoDBResponseConstructor = { /** @internal */ export class MongoDBResponse extends OnDemandDocument { - /** - * Devtools need to know which keys were encrypted before the driver automatically decrypted them. - * If decorating is enabled (`Symbol.for('@@mdb.decorateDecryptionResult')`), this field will be set, - * storing the original encrypted response from the server, so that we can build an object that has - * the list of BSON keys that were encrypted stored at a well known symbol: `Symbol.for('@@mdb.decryptedKeys')`. - */ - encryptedResponse?: MongoDBResponse; - // Wrap error thrown from BSON public override get( name: string | number, @@ -196,6 +188,13 @@ export class MongoDBResponse extends OnDemandDocument { /** @internal */ export class CursorResponse extends MongoDBResponse { + /** + * Devtools need to know which keys were encrypted before the driver automatically decrypted them. + * If decorating is enabled (`Symbol.for('@@mdb.decorateDecryptionResult')`), this field will be set, + * storing the original encrypted response from the server, so that we can build an object that has + * the list of BSON keys that were encrypted stored at a well known symbol: `Symbol.for('@@mdb.decryptedKeys')`. + */ + encryptedResponse?: MongoDBResponse; /** * This supports a feature of the FindCursor. * It is an optimization to avoid an extra getMore when the limit has been reached From d5214c81cfd9c743aaa4fb32f7cc7921e16d318d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 11 Jun 2024 15:00:43 -0400 Subject: [PATCH 29/29] chore: clean up TS for "required" --- src/cmap/wire_protocol/on_demand/document.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmap/wire_protocol/on_demand/document.ts b/src/cmap/wire_protocol/on_demand/document.ts index 6bdb8d7bcf9..944916f10bb 100644 --- a/src/cmap/wire_protocol/on_demand/document.ts +++ b/src/cmap/wire_protocol/on_demand/document.ts @@ -254,7 +254,7 @@ export class OnDemandDocument { public get( name: string | number, as: T, - required?: false | undefined | boolean + required?: boolean | undefined ): JSTypeOf[T] | null; /** `required` will make `get` throw if name does not exist or is null/undefined */