Skip to content

fix(NODE-4936)!: remove unsupported options from db.command and admin.command #3775

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/admin.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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
Expand Down Expand Up @@ -76,7 +76,11 @@ export class Admin {
async command(command: Document, options?: RunCommandOptions): Promise<Document> {
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
})
);
}

Expand Down
9 changes: 8 additions & 1 deletion src/db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,14 @@ export class Db {
*/
async command(command: Document, options?: RunCommandOptions): Promise<Document> {
// 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
})
);
}

/**
Expand Down
9 changes: 6 additions & 3 deletions src/mongo_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,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';
Expand Down Expand Up @@ -521,12 +523,13 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
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;
}
}

Expand Down
1 change: 0 additions & 1 deletion src/operations/operation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ const kSession = Symbol('session');
*/
export abstract class AbstractOperation<TResult = any> {
ns!: MongoDBNamespace;
cmd!: Document;
Copy link
Contributor

Choose a reason for hiding this comment

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

removed because this no longer exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kinda, the xx! properties on this class are tech debt from the rapid TS conversion, we couldn't sort out which subclasses actually defined these and which didn't. Specifically cmd what only being attached by the RenameOperation so we don't need it defined on this super class. But also Rename is somewhat special in that most of our commands are build in the exec function, not in the constructor, so even Rename doesn't define this property anymore.

readPreference: ReadPreference;
server!: Server;
bypassPinningCheck: boolean;
Expand Down
59 changes: 27 additions & 32 deletions src/operations/rename.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -17,39 +15,36 @@ export interface RenameOptions extends CommandOperationOptions {
}

/** @internal */
export class RenameOperation extends RunAdminCommandOperation {
override options: RenameOptions;
collection: Collection;
newName: string;

constructor(collection: Collection, newName: string, options: RenameOptions) {
// Check the collection name
export class RenameOperation extends CommandOperation<Document> {
constructor(
public collection: Collection,
public newName: string,
public override options: RenameOptions
) {
checkCollectionName(newName);

// 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.options = options;
this.collection = collection;
this.newName = newName;
super(collection, options);
this.ns = new MongoDBNamespace('admin', '$cmd');
}

override async execute(server: Server, session: ClientSession | undefined): Promise<Collection> {
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<Document> = 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.');
}
}

Expand Down
86 changes: 34 additions & 52 deletions src/operations/run_command.ts
Original file line number Diff line number Diff line change
@@ -1,73 +1,55 @@
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 = {
/** Specify ClientSession for this command */
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<T = Document> extends CommandCallbackOperation<T> {
override options: RunCommandOptions;
command: Document;

constructor(parent: OperationParent | undefined, command: Document, options?: RunCommandOptions) {
super(parent, options);
this.options = options ?? {};
this.command = command;
export class RunCommandOperation<T = Document> extends AbstractOperation<T> {
constructor(parent: Db, public command: Document, public override options: RunCommandOptions) {
super(options);
this.ns = parent.s.namespace.withCollection('$cmd');
}

override executeCallback(
server: Server,
session: ClientSession | undefined,
callback: Callback<T>
): void {
const command = this.command;
this.executeCommandCallback(server, session, command, callback);
override async execute(server: Server, session: ClientSession | undefined): Promise<T> {
this.server = server;
return server.commandAsync(this.ns, this.command, {
...this.options,
readPreference: this.readPreference,
session
}) as TODO_NODE_3286;
}
}

export class RunAdminCommandOperation<T = Document> extends RunCommandOperation<T> {
constructor(parent: OperationParent | undefined, command: Document, options?: RunCommandOptions) {
super(parent, command, options);
this.ns = new MongoDBNamespace('admin');
export class RunAdminCommandOperation<T = Document> extends AbstractOperation<T> {
constructor(
public command: Document,
public override options: RunCommandOptions & {
noResponse?: boolean;
bypassPinningCheck?: boolean;
}
) {
super(options);
this.ns = new MongoDBNamespace('admin', '$cmd');
}

override async execute(server: Server, session: ClientSession | undefined): Promise<T> {
this.server = server;
return server.commandAsync(this.ns, this.command, {
...this.options,
readPreference: this.readPreference,
session
}) as TODO_NODE_3286;
}
}
6 changes: 3 additions & 3 deletions src/sessions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -778,7 +778,7 @@ function endTransaction(

return executeOperation(
session.client,
new RunAdminCommandOperation(undefined, command, {
new RunAdminCommandOperation(command, {
session,
readPreference: ReadPreference.primary,
bypassPinningCheck: true
Expand Down Expand Up @@ -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 });
Expand Down
11 changes: 1 addition & 10 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1155,20 +1155,11 @@ export function shuffle<T>(sequence: Iterable<T>, limit = 0): Array<T> {

// 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;
}

Expand Down
22 changes: 9 additions & 13 deletions test/integration/run-command/run_command.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,18 @@ 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');
});

// 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: {} });
//@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');
expect(command).to.not.have.property('readConcern');
});
});