From 7f197442dc33e8e689e3092bd03324941f520313 Mon Sep 17 00:00:00 2001 From: Hana Pearlman Date: Mon, 2 Nov 2020 17:21:33 -0500 Subject: [PATCH] move explain cmd/options to separate file --- src/cmap/wire_protocol/command.ts | 6 ---- src/cmap/wire_protocol/query.ts | 11 +++--- src/cmap/wire_protocol/write_command.ts | 12 +++++-- src/explain.ts | 46 ++----------------------- src/operations/command.ts | 7 ++++ src/operations/delete.ts | 5 +-- src/operations/distinct.ts | 5 +-- src/operations/explainable_command.ts | 42 ++++++++++++++++++++++ src/operations/find_and_modify.ts | 5 +-- src/operations/map_reduce.ts | 5 +-- src/operations/update.ts | 5 +-- test/functional/explain.test.js | 4 +-- 12 files changed, 84 insertions(+), 69 deletions(-) create mode 100644 src/operations/explainable_command.ts diff --git a/src/cmap/wire_protocol/command.ts b/src/cmap/wire_protocol/command.ts index b0cc54d1e54..a165cba9f3b 100644 --- a/src/cmap/wire_protocol/command.ts +++ b/src/cmap/wire_protocol/command.ts @@ -10,7 +10,6 @@ import type { Topology } from '../../sdam/topology'; import type { ReadPreferenceLike } from '../../read_preference'; import type { WriteConcernOptions, WriteConcern, W } from '../../write_concern'; import type { WriteCommandOptions } from './write_command'; -import { decorateWithExplain } from '../../explain'; /** @internal */ export interface CommandOptions extends BSONSerializeOptions { @@ -69,11 +68,6 @@ export function command( return callback(new MongoError(`command ${JSON.stringify(cmd)} does not return a cursor`)); } - // TODO: should not modify the command here - if (cmd.explain !== undefined) { - cmd = decorateWithExplain(cmd, cmd.explain); - } - if (!isClientEncryptionEnabled(server)) { _command(server, ns, cmd, options, callback); return; diff --git a/src/cmap/wire_protocol/query.ts b/src/cmap/wire_protocol/query.ts index 9291be0f2d2..70d35cdd3d4 100644 --- a/src/cmap/wire_protocol/query.ts +++ b/src/cmap/wire_protocol/query.ts @@ -7,7 +7,6 @@ import { Document, pluckBSONSerializeOptions } from '../../bson'; import type { Server } from '../../sdam/server'; import type { ReadPreferenceLike } from '../../read_preference'; import type { FindOptions } from '../../operations/find'; -import { Explain } from '../../explain'; /** @internal */ export interface QueryOptions extends CommandOptions { @@ -63,7 +62,7 @@ export function query( } function prepareFindCommand(server: Server, ns: string, cmd: Document) { - const findCmd: Document = { + let findCmd: Document = { find: collectionNamespace(ns) }; @@ -147,9 +146,11 @@ function prepareFindCommand(server: Server, ns: string, cmd: Document) { if (cmd.collation) findCmd.collation = cmd.collation; if (cmd.readConcern) findCmd.readConcern = cmd.readConcern; - // TODO: Quick fix to make tests pass; will be updated during NODE-2853 - if (cmd.explain !== undefined) { - findCmd.explain = Explain.fromOptions({ explain: cmd.explain }); + // If we have explain, we need to rewrite the find command + if (cmd.explain) { + findCmd = { + explain: findCmd + }; } return findCmd; diff --git a/src/cmap/wire_protocol/write_command.ts b/src/cmap/wire_protocol/write_command.ts index eb7585f9aa3..58b26102459 100644 --- a/src/cmap/wire_protocol/write_command.ts +++ b/src/cmap/wire_protocol/write_command.ts @@ -4,7 +4,8 @@ import { command, CommandOptions } from './command'; import type { Server } from '../../sdam/server'; import type { Document, BSONSerializeOptions } from '../../bson'; import type { WriteConcern } from '../../write_concern'; -import { Explain, ExplainOptions } from '../../explain'; +import { decorateWithExplain, Explain } from '../../explain'; +import type { ExplainOptions } from '../../operations/explainable_command'; /** @public */ export interface CollationOptions { @@ -44,7 +45,7 @@ export function writeCommand( options = options || {}; const ordered = typeof options.ordered === 'boolean' ? options.ordered : true; const writeConcern = options.writeConcern; - const writeCommand: Document = {}; + let writeCommand: Document = {}; writeCommand[type] = collectionNamespace(ns); writeCommand[opsField] = ops; writeCommand.ordered = ordered; @@ -65,8 +66,13 @@ export function writeCommand( writeCommand.bypassDocumentValidation = options.bypassDocumentValidation; } + // If a command is to be explained, we need to reformat the command after + // the other command properties are specified. if (options.explain !== undefined) { - writeCommand.explain = Explain.fromOptions(options); + const explain = Explain.fromOptions(options); + if (explain) { + writeCommand = decorateWithExplain(writeCommand, explain); + } } const commandOptions = Object.assign( diff --git a/src/explain.ts b/src/explain.ts index 2b3954fd1b3..0c406193e38 100644 --- a/src/explain.ts +++ b/src/explain.ts @@ -1,6 +1,5 @@ -import type { Callback, Document } from '.'; -import { MongoError } from './error'; -import { CommandOperation, CommandOperationOptions, OperationParent } from './operations/command'; +import type { Document } from '.'; +import type { ExplainOptions } from './operations/explainable_command'; import type { Server } from './sdam/server'; import { maxWireVersion } from './utils'; @@ -10,45 +9,6 @@ const SUPPORTS_EXPLAIN_WITH_DISTINCT = 3.2; const SUPPORTS_EXPLAIN_WITH_FIND_AND_MODIFY = 3.2; const SUPPORTS_EXPLAIN_WITH_MAP_REDUCE = 4.4; -/** @internal */ -export abstract class ExplainableCommand< - T extends ExplainOptions = ExplainOptions, - TResult = Document -> extends CommandOperation { - explain?: Explain; - - constructor(parent?: OperationParent, options?: T) { - super(parent, options); - - if (!Explain.explainOptionsValid(options)) { - throw new MongoError(`explain must be one of ${Object.keys(Verbosity)} or a boolean`); - } - - this.explain = Explain.fromOptions(options); - } - - get canRetryWrite(): boolean { - return this.explain === undefined; - } - - executeCommand(server: Server, cmd: Document, callback: Callback): void { - if (this.explain) { - if (!Explain.explainSupportedOnCmd(server, cmd)) { - callback(new MongoError(`server ${server.name} does not support explain on this command`)); - return; - } - - cmd.explain = this.explain; - } - super.executeCommand(server, cmd, callback); - } -} - -/** @public */ -export interface ExplainOptions extends CommandOperationOptions { - explain?: VerbosityLike; -} - /** @public */ export enum Verbosity { queryPlanner = 'queryPlanner', @@ -81,7 +41,7 @@ export class Explain { return new Explain(options.explain); } - static explainOptionsValid(options?: ExplainOptions): boolean { + static valid(options?: ExplainOptions): boolean { if (options == null || options.explain === undefined) { return true; } diff --git a/src/operations/command.ts b/src/operations/command.ts index 050eae00a6e..5f91ac69435 100644 --- a/src/operations/command.ts +++ b/src/operations/command.ts @@ -10,6 +10,7 @@ import type { Server } from '../sdam/server'; import { BSONSerializeOptions, Document, resolveBSONOptions } from '../bson'; import type { CollationOptions } from '../cmap/wire_protocol/write_command'; import type { ReadConcernLike } from './../read_concern'; +import { decorateWithExplain } from '../explain'; const SUPPORTS_WRITE_CONCERN_AND_COLLATION = 5; @@ -139,6 +140,12 @@ export abstract class CommandOperation< this.logger.debug(`executing command ${JSON.stringify(cmd)} against ${this.ns}`); } + // If a command is to be explained, we need to reformat the command after + // the other command properties are specified. + if (cmd.explain) { + cmd = decorateWithExplain(cmd, cmd.explain); + } + server.command( this.ns.toString(), cmd, diff --git a/src/operations/delete.ts b/src/operations/delete.ts index 1994f4f2313..a991ca73c33 100644 --- a/src/operations/delete.ts +++ b/src/operations/delete.ts @@ -7,10 +7,11 @@ import type { Server } from '../sdam/server'; import type { Collection } from '../collection'; import type { WriteCommandOptions } from '../cmap/wire_protocol/write_command'; import type { Connection } from '../cmap/connection'; -import { ExplainableCommand, ExplainOptions } from '../explain'; +import { ExplainableCommand, ExplainOptions } from '../operations/explainable_command'; +import type { CommandOperationOptions } from './command'; /** @public */ -export interface DeleteOptions extends ExplainOptions { +export interface DeleteOptions extends CommandOperationOptions, ExplainOptions { single?: boolean; hint?: Hint; } diff --git a/src/operations/distinct.ts b/src/operations/distinct.ts index 0d3c2e7e1ef..4b20fa93a54 100644 --- a/src/operations/distinct.ts +++ b/src/operations/distinct.ts @@ -3,10 +3,11 @@ import { decorateWithCollation, decorateWithReadConcern, Callback } from '../uti import type { Document } from '../bson'; import type { Server } from '../sdam/server'; import type { Collection } from '../collection'; -import { ExplainableCommand, ExplainOptions } from '../explain'; +import { ExplainableCommand, ExplainOptions } from '../operations/explainable_command'; +import type { CommandOperationOptions } from './command'; /** @public */ -export type DistinctOptions = ExplainOptions; +export interface DistinctOptions extends CommandOperationOptions, ExplainOptions {} /** * Return a list of distinct values for the given key across a collection. diff --git a/src/operations/explainable_command.ts b/src/operations/explainable_command.ts new file mode 100644 index 00000000000..afa7d4ce3ff --- /dev/null +++ b/src/operations/explainable_command.ts @@ -0,0 +1,42 @@ +import { CommandOperation, OperationParent, CommandOperationOptions } from './command'; +import { Explain, Verbosity, VerbosityLike } from '../explain'; +import { Callback, Document, MongoError, Server } from '..'; + +/** @public */ +export interface ExplainOptions { + explain?: VerbosityLike; +} + +/** @internal */ +export abstract class ExplainableCommand< + T extends ExplainOptions & CommandOperationOptions, + TResult = Document +> extends CommandOperation { + explain?: Explain; + + constructor(parent?: OperationParent, options?: T) { + super(parent, options); + + if (!Explain.valid(options)) { + throw new MongoError(`explain must be one of ${Object.keys(Verbosity)} or a boolean`); + } + + this.explain = Explain.fromOptions(options); + } + + get canRetryWrite(): boolean { + return this.explain === undefined; + } + + executeCommand(server: Server, cmd: Document, callback: Callback): void { + if (this.explain) { + if (!Explain.explainSupportedOnCmd(server, cmd)) { + callback(new MongoError(`server ${server.name} does not support explain on this command`)); + return; + } + + cmd.explain = this.explain; + } + super.executeCommand(server, cmd, callback); + } +} diff --git a/src/operations/find_and_modify.ts b/src/operations/find_and_modify.ts index 404b99728a9..7c9e1a5a517 100644 --- a/src/operations/find_and_modify.ts +++ b/src/operations/find_and_modify.ts @@ -13,10 +13,11 @@ import type { Document } from '../bson'; import type { Server } from '../sdam/server'; import type { Collection } from '../collection'; import { Sort, formatSort } from '../sort'; -import { ExplainableCommand, ExplainOptions } from '../explain'; +import { ExplainableCommand, ExplainOptions } from '../operations/explainable_command'; +import type { CommandOperationOptions } from './command'; /** @public */ -export interface FindAndModifyOptions extends ExplainOptions { +export interface FindAndModifyOptions extends CommandOperationOptions, ExplainOptions { /** When false, returns the updated document rather than the original. The default is true. */ returnOriginal?: boolean; /** Upsert the document if it does not exist. */ diff --git a/src/operations/map_reduce.ts b/src/operations/map_reduce.ts index 1d1bd7fac1d..54dd636b9ad 100644 --- a/src/operations/map_reduce.ts +++ b/src/operations/map_reduce.ts @@ -13,7 +13,8 @@ import type { Collection } from '../collection'; import type { Sort } from '../sort'; import { MongoError } from '../error'; import type { ObjectId } from '../bson'; -import { ExplainableCommand, ExplainOptions } from '../explain'; +import { ExplainableCommand, ExplainOptions } from '../operations/explainable_command'; +import type { CommandOperationOptions } from './command'; const exclusionList = [ 'explain', @@ -35,7 +36,7 @@ export type ReduceFunction = (key: string, values: Document[]) => Document; export type FinalizeFunction = (key: string, reducedValue: Document) => Document; /** @public */ -export interface MapReduceOptions extends ExplainOptions { +export interface MapReduceOptions extends CommandOperationOptions, ExplainOptions { /** Sets the output target for the map reduce job. */ out?: 'inline' | { inline: 1 } | { replace: string } | { merge: string } | { reduce: string }; /** Query filter object. */ diff --git a/src/operations/update.ts b/src/operations/update.ts index e59374fb9cc..a89ee4f33cc 100644 --- a/src/operations/update.ts +++ b/src/operations/update.ts @@ -5,10 +5,11 @@ import type { Server } from '../sdam/server'; import type { Collection } from '../collection'; import type { CollationOptions, WriteCommandOptions } from '../cmap/wire_protocol/write_command'; import type { ObjectId, Document } from '../bson'; -import { ExplainableCommand, ExplainOptions } from '../explain'; +import { ExplainableCommand, ExplainOptions } from '../operations/explainable_command'; +import type { CommandOperationOptions } from './command'; /** @public */ -export interface UpdateOptions extends ExplainOptions { +export interface UpdateOptions extends CommandOperationOptions, ExplainOptions { /** A set of filters specifying to which array elements an update should apply */ arrayFilters?: Document[]; /** If true, allows the write to opt-out of document level validation */ diff --git a/test/functional/explain.test.js b/test/functional/explain.test.js index f7e127449bb..48942ced1f2 100644 --- a/test/functional/explain.test.js +++ b/test/functional/explain.test.js @@ -256,7 +256,7 @@ describe('Explain', function () { expect(res).to.exist; // Verify explanation result contains properties of executionStats output - collection.deleteOne({ a: 1 }, { explain: 'executionStats' }, (err, explanation) => { + collection.findOneAndDelete({ a: 1 }, { explain: 'executionStats' }, (err, explanation) => { expect(err).to.not.exist; expect(explanation).to.exist; expect(explanation).property('queryPlanner').to.exist; @@ -283,7 +283,7 @@ describe('Explain', function () { expect(res).to.exist; // Verify explanation result contains properties of allPlansExecution output - collection.deleteOne({ a: 1 }, { explain: 'allPlansExecution' }, (err, explanation) => { + collection.distinct('a', {}, { explain: 'allPlansExecution' }, (err, explanation) => { expect(err).to.not.exist; expect(explanation).to.exist; expect(explanation).property('queryPlanner').to.exist;