From eaa525eb84c90a0bd391fadd9a25f1db101234cf Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 19 Jul 2023 15:05:43 -0400 Subject: [PATCH 1/4] fix(NODE-4936)!: remove unsupported options from db.command and admin.command --- src/admin.ts | 10 ++- src/db.ts | 9 +- src/mongo_client.ts | 9 +- src/operations/operation.ts | 1 - src/operations/rename.ts | 44 +++++----- src/operations/run_command.ts | 83 ++++++++----------- src/sessions.ts | 6 +- src/utils.ts | 11 +-- .../run-command/run_command.test.ts | 20 ++--- 9 files changed, 89 insertions(+), 104 deletions(-) diff --git a/src/admin.ts b/src/admin.ts index 61d67dbc509..0ec2e477637 100644 --- a/src/admin.ts +++ b/src/admin.ts @@ -1,4 +1,4 @@ -import type { Document } from './bson'; +import { type Document, resolveBSONOptions } from './bson'; import type { Db } from './db'; import { AddUserOperation, type AddUserOptions } from './operations/add_user'; import type { CommandOperationOptions } from './operations/command'; @@ -9,7 +9,7 @@ import { type ListDatabasesResult } from './operations/list_databases'; import { RemoveUserOperation, type RemoveUserOptions } from './operations/remove_user'; -import { RunCommandOperation, type RunCommandOptions } from './operations/run_command'; +import { RunAdminCommandOperation, type RunCommandOptions } from './operations/run_command'; import { ValidateCollectionOperation, type ValidateCollectionOptions @@ -76,7 +76,11 @@ export class Admin { async command(command: Document, options?: RunCommandOptions): Promise { return executeOperation( this.s.db.client, - new RunCommandOperation(this.s.db, command, { dbName: 'admin', ...options }) + new RunAdminCommandOperation(command, { + ...resolveBSONOptions(options), + session: options?.session, + readPreference: options?.readPreference + }) ); } diff --git a/src/db.ts b/src/db.ts index 4b3c2d22f50..08b9195444c 100644 --- a/src/db.ts +++ b/src/db.ts @@ -259,7 +259,14 @@ export class Db { */ async command(command: Document, options?: RunCommandOptions): Promise { // Intentionally, we do not inherit options from parent for this operation. - return executeOperation(this.client, new RunCommandOperation(this, command, options)); + return executeOperation( + this.client, + new RunCommandOperation(this, command, { + ...resolveBSONOptions(options), + session: options?.session, + readPreference: options?.readPreference + }) + ); } /** diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 442384868de..1c472a62416 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -22,6 +22,8 @@ import type { Encrypter } from './encrypter'; import { MongoInvalidArgumentError } from './error'; import { MongoLogger, type MongoLoggerOptions } from './mongo_logger'; import { TypedEventEmitter } from './mongo_types'; +import { executeOperation } from './operations/execute_operation'; +import { RunAdminCommandOperation } from './operations/run_command'; import type { ReadConcern, ReadConcernLevel, ReadConcernLike } from './read_concern'; import { ReadPreference, type ReadPreferenceMode } from './read_preference'; import type { TagSet } from './sdam/server_description'; @@ -519,12 +521,13 @@ export class MongoClient extends TypedEventEmitter { if (servers.length !== 0) { const endSessions = Array.from(this.s.sessionPool.sessions, ({ id }) => id); if (endSessions.length !== 0) { - await this.db('admin') - .command( + await executeOperation( + this, + new RunAdminCommandOperation( { endSessions }, { readPreference: ReadPreference.primaryPreferred, noResponse: true } ) - .catch(() => null); // outcome does not matter + ).catch(() => null); // outcome does not matter; } } diff --git a/src/operations/operation.ts b/src/operations/operation.ts index 43d84c2cdc4..7a86f392a1e 100644 --- a/src/operations/operation.ts +++ b/src/operations/operation.ts @@ -49,7 +49,6 @@ const kSession = Symbol('session'); */ export abstract class AbstractOperation { ns!: MongoDBNamespace; - cmd!: Document; readPreference: ReadPreference; server!: Server; bypassPinningCheck: boolean; diff --git a/src/operations/rename.ts b/src/operations/rename.ts index bb51398282a..0581445b2d2 100644 --- a/src/operations/rename.ts +++ b/src/operations/rename.ts @@ -1,12 +1,10 @@ import type { Document } from '../bson'; import { Collection } from '../collection'; -import { MongoServerError } from '../error'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { checkCollectionName } from '../utils'; -import type { CommandOperationOptions } from './command'; +import { checkCollectionName, MongoDBNamespace } from '../utils'; +import { CommandOperation, type CommandOperationOptions } from './command'; import { Aspect, defineAspects } from './operation'; -import { RunAdminCommandOperation } from './run_command'; /** @public */ export interface RenameOptions extends CommandOperationOptions { @@ -17,7 +15,7 @@ export interface RenameOptions extends CommandOperationOptions { } /** @internal */ -export class RenameOperation extends RunAdminCommandOperation { +export class RenameOperation extends CommandOperation { override options: RenameOptions; collection: Collection; newName: string; @@ -25,31 +23,33 @@ export class RenameOperation extends RunAdminCommandOperation { constructor(collection: Collection, newName: string, options: RenameOptions) { // Check the collection name checkCollectionName(newName); + super(collection, options); - // Build the command - const renameCollection = collection.namespace; - const toCollection = collection.s.namespace.withCollection(newName).toString(); - const dropTarget = typeof options.dropTarget === 'boolean' ? options.dropTarget : false; - const cmd = { renameCollection: renameCollection, to: toCollection, dropTarget: dropTarget }; - - super(collection, cmd, options); + this.ns = new MongoDBNamespace('admin', '$cmd'); this.options = options; this.collection = collection; this.newName = newName; } override async execute(server: Server, session: ClientSession | undefined): Promise { - const coll = this.collection; - - const doc = await super.execute(server, session); - // We have an error - if (doc?.errmsg) { - throw new MongoServerError(doc); - } - - const newColl: Collection = new Collection(coll.s.db, this.newName, coll.s.options); + // Build the command + const renameCollection = this.collection.namespace; + const toCollection = this.collection.s.namespace.withCollection(this.newName).toString(); + const dropTarget = + typeof this.options.dropTarget === 'boolean' ? this.options.dropTarget : false; + + const command = { + renameCollection: renameCollection, + to: toCollection, + dropTarget: dropTarget + }; + + await super.executeCommand(server, session, command); + return new Collection(this.collection.s.db, this.newName, this.collection.s.options); + } - return newColl; + protected executeCallback(_server: any, _session: any, _callback: any): void { + throw new Error('Method not implemented.'); } } diff --git a/src/operations/run_command.ts b/src/operations/run_command.ts index 352965e836b..5848e5cb88f 100644 --- a/src/operations/run_command.ts +++ b/src/operations/run_command.ts @@ -1,9 +1,11 @@ import type { BSONSerializeOptions, Document } from '../bson'; +import { type Db } from '../db'; +import { type TODO_NODE_3286 } from '../mongo_types'; import type { ReadPreferenceLike } from '../read_preference'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { type Callback, MongoDBNamespace } from '../utils'; -import { CommandCallbackOperation, type OperationParent } from './command'; +import { MongoDBNamespace } from '../utils'; +import { AbstractOperation } from './operation'; /** @public */ export type RunCommandOptions = { @@ -11,63 +13,48 @@ export type RunCommandOptions = { session?: ClientSession; /** The read preference */ readPreference?: ReadPreferenceLike; - - // The following options were "accidentally" supported - // Since the options are generally supported through inheritance - - /** @deprecated This is an internal option that has undefined behavior for this API */ - willRetryWrite?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - omitReadPreference?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - writeConcern?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - explain?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - readConcern?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - collation?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - maxTimeMS?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - comment?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - retryWrites?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - dbName?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - authdb?: any; - /** @deprecated This is an internal option that has undefined behavior for this API */ - noResponse?: any; - - /** @internal Used for transaction commands */ - bypassPinningCheck?: boolean; } & BSONSerializeOptions; /** @internal */ -export class RunCommandOperation extends CommandCallbackOperation { +export class RunCommandOperation extends AbstractOperation { override options: RunCommandOptions; command: Document; - constructor(parent: OperationParent | undefined, command: Document, options?: RunCommandOptions) { - super(parent, options); - this.options = options ?? {}; + constructor(parent: Db, command: Document, options: RunCommandOptions) { + super(options); + this.options = options; this.command = command; + this.ns = parent.s.namespace.withCollection('$cmd'); } - override executeCallback( - server: Server, - session: ClientSession | undefined, - callback: Callback - ): void { - const command = this.command; - this.executeCommandCallback(server, session, command, callback); + override async execute(server: Server, session: ClientSession | undefined): Promise { + this.server = server; + return server.commandAsync(this.ns, this.command, { + ...this.options, + readPreference: this.readPreference, + session + }) as TODO_NODE_3286; } } -export class RunAdminCommandOperation extends RunCommandOperation { - constructor(parent: OperationParent | undefined, command: Document, options?: RunCommandOptions) { - super(parent, command, options); - this.ns = new MongoDBNamespace('admin'); +export class RunAdminCommandOperation extends AbstractOperation { + command: Document; + + constructor( + command: Document, + options: RunCommandOptions & { noResponse?: boolean; bypassPinningCheck?: boolean } + ) { + super(options); + this.command = command; + this.ns = new MongoDBNamespace('admin', '$cmd'); + } + + override async execute(server: Server, session: ClientSession | undefined): Promise { + this.server = server; + return server.commandAsync(this.ns, this.command, { + ...this.options, + readPreference: this.readPreference, + session + }) as TODO_NODE_3286; } } diff --git a/src/sessions.ts b/src/sessions.ts index 544ab539368..9b4d1063526 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -754,7 +754,7 @@ function endTransaction( // send the command executeOperation( session.client, - new RunAdminCommandOperation(undefined, command, { + new RunAdminCommandOperation(command, { session, readPreference: ReadPreference.primary, bypassPinningCheck: true @@ -778,7 +778,7 @@ function endTransaction( return executeOperation( session.client, - new RunAdminCommandOperation(undefined, command, { + new RunAdminCommandOperation(command, { session, readPreference: ReadPreference.primary, bypassPinningCheck: true @@ -989,7 +989,7 @@ export function applySession( if ( session.supports.causalConsistency && session.operationTime && - commandSupportsReadConcern(command, options) + commandSupportsReadConcern(command) ) { command.readConcern = command.readConcern || {}; Object.assign(command.readConcern, { afterClusterTime: session.operationTime }); diff --git a/src/utils.ts b/src/utils.ts index 505f3bfd1d5..a77c1c8f2c4 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1155,20 +1155,11 @@ export function shuffle(sequence: Iterable, limit = 0): Array { // TODO(NODE-4936): read concern eligibility for commands should be codified in command construction // @see https://github.com/mongodb/specifications/blob/master/source/read-write-concern/read-write-concern.rst#read-concern -export function commandSupportsReadConcern(command: Document, options?: Document): boolean { +export function commandSupportsReadConcern(command: Document): boolean { if (command.aggregate || command.count || command.distinct || command.find || command.geoNear) { return true; } - if ( - command.mapReduce && - options && - options.out && - (options.out.inline === 1 || options.out === 'inline') - ) { - return true; - } - return false; } diff --git a/test/integration/run-command/run_command.test.ts b/test/integration/run-command/run_command.test.ts index acca3b5fe68..9e69f8728b2 100644 --- a/test/integration/run-command/run_command.test.ts +++ b/test/integration/run-command/run_command.test.ts @@ -42,17 +42,11 @@ describe('RunCommand API', () => { expect(command).to.not.have.property('writeConcern'); }); - // TODO(NODE-4936): We do support readConcern in options, the spec forbids this - it.skip( - 'does not support readConcern in options', - { requires: { mongodb: '>=5.0' } }, - async () => { - const command = Object.freeze({ find: 'test', filter: {} }); - const res = await db.command(command, { readConcern: ReadConcern.MAJORITY }); - expect(res).to.have.property('ok', 1); - expect(commandsStarted).to.not.have.nested.property('[0].command.readConcern'); - expect(command).to.not.have.property('readConcern'); - } - ).skipReason = - 'TODO(NODE-4936): Enable this test when readConcern support has been removed from runCommand'; + it('does not support readConcern in options', { requires: { mongodb: '>=5.0' } }, async () => { + const command = Object.freeze({ find: 'test', filter: {} }); + const res = await db.command(command, { readConcern: ReadConcern.MAJORITY }); + expect(res).to.have.property('ok', 1); + expect(commandsStarted).to.not.have.nested.property('[0].command.readConcern'); + expect(command).to.not.have.property('readConcern'); + }); }); From fac17c93f66121505ed90aac867cb13a0264d832 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 20 Jul 2023 13:13:57 -0400 Subject: [PATCH 2/4] refactor: use public in constructor args --- src/operations/rename.ts | 15 +++++---------- src/operations/run_command.ts | 17 ++++++----------- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/operations/rename.ts b/src/operations/rename.ts index 0581445b2d2..d0de350e61a 100644 --- a/src/operations/rename.ts +++ b/src/operations/rename.ts @@ -16,19 +16,14 @@ export interface RenameOptions extends CommandOperationOptions { /** @internal */ export class RenameOperation extends CommandOperation { - override options: RenameOptions; - collection: Collection; - newName: string; - - constructor(collection: Collection, newName: string, options: RenameOptions) { - // Check the collection name + constructor( + public collection: Collection, + public newName: string, + public override options: RenameOptions + ) { checkCollectionName(newName); super(collection, options); - this.ns = new MongoDBNamespace('admin', '$cmd'); - this.options = options; - this.collection = collection; - this.newName = newName; } override async execute(server: Server, session: ClientSession | undefined): Promise { diff --git a/src/operations/run_command.ts b/src/operations/run_command.ts index 5848e5cb88f..a0e306e6c62 100644 --- a/src/operations/run_command.ts +++ b/src/operations/run_command.ts @@ -17,13 +17,8 @@ export type RunCommandOptions = { /** @internal */ export class RunCommandOperation extends AbstractOperation { - override options: RunCommandOptions; - command: Document; - - constructor(parent: Db, command: Document, options: RunCommandOptions) { + constructor(parent: Db, public command: Document, public override options: RunCommandOptions) { super(options); - this.options = options; - this.command = command; this.ns = parent.s.namespace.withCollection('$cmd'); } @@ -38,14 +33,14 @@ export class RunCommandOperation extends AbstractOperation { } export class RunAdminCommandOperation extends AbstractOperation { - command: Document; - constructor( - command: Document, - options: RunCommandOptions & { noResponse?: boolean; bypassPinningCheck?: boolean } + public command: Document, + public override options: RunCommandOptions & { + noResponse?: boolean; + bypassPinningCheck?: boolean; + } ) { super(options); - this.command = command; this.ns = new MongoDBNamespace('admin', '$cmd'); } From 7746428161d8f0d0ae4a300a16ec52be9c96a634 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 20 Jul 2023 13:44:17 -0400 Subject: [PATCH 3/4] fix ts in tests --- test/integration/run-command/run_command.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/integration/run-command/run_command.test.ts b/test/integration/run-command/run_command.test.ts index 9e69f8728b2..a38014cce44 100644 --- a/test/integration/run-command/run_command.test.ts +++ b/test/integration/run-command/run_command.test.ts @@ -37,6 +37,7 @@ describe('RunCommand API', () => { it('does not support writeConcern in options', { requires: { mongodb: '>=5.0' } }, async () => { const command = Object.freeze({ insert: 'test', documents: [{ x: 1 }] }); + //@ts-expect-error: Testing WC is not supported await db.command(command, { writeConcern: new WriteConcern('majority') }); expect(commandsStarted).to.not.have.nested.property('[0].command.writeConcern'); expect(command).to.not.have.property('writeConcern'); @@ -44,6 +45,7 @@ describe('RunCommand API', () => { it('does not support readConcern in options', { requires: { mongodb: '>=5.0' } }, async () => { const command = Object.freeze({ find: 'test', filter: {} }); + //@ts-expect-error: Testing WC is not supported const res = await db.command(command, { readConcern: ReadConcern.MAJORITY }); expect(res).to.have.property('ok', 1); expect(commandsStarted).to.not.have.nested.property('[0].command.readConcern'); From a3370de14eab8fbc9d12314900f5a8c1836dc6c5 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 20 Jul 2023 13:44:33 -0400 Subject: [PATCH 4/4] fix: comment --- test/integration/run-command/run_command.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/run-command/run_command.test.ts b/test/integration/run-command/run_command.test.ts index a38014cce44..0b657e48f14 100644 --- a/test/integration/run-command/run_command.test.ts +++ b/test/integration/run-command/run_command.test.ts @@ -45,7 +45,7 @@ describe('RunCommand API', () => { it('does not support readConcern in options', { requires: { mongodb: '>=5.0' } }, async () => { const command = Object.freeze({ find: 'test', filter: {} }); - //@ts-expect-error: Testing WC is not supported + //@ts-expect-error: Testing RC is not supported const res = await db.command(command, { readConcern: ReadConcern.MAJORITY }); expect(res).to.have.property('ok', 1); expect(commandsStarted).to.not.have.nested.property('[0].command.readConcern');