Skip to content
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

feat: add explain support for non-cursor commands #2599

Merged

Conversation

HanaPearlman
Copy link
Contributor

@HanaPearlman HanaPearlman commented Oct 27, 2020

Description

This PR implements explain functionality for the relevant non-cursor operations: update, remove, distinct, findAndModify, and mapReduce. These operations now accept an additional explain option, either a boolean or a string specifying the requested verbosity, and will be run as explain commands with the specified verbosity.

Current syntax: collection.updateOne(..., {explain: verbosity}).
Other syntax, e.g.: collection.explain(verbosity).updateOne(...) is easy to add (or switch to) if we want.

What changed?

A new Explainable aspect was added so that, in CommandOperation, operations with that aspect can use the explain property already on the class. Logic for validating the explain verbosity passed through the options, adding the explain field to the command, and canRetryWrite have been added to CommandOperation.

Not all explainable commands actually call executeCommand, including updateOne, deleteOne, etc. These are handled in the corresponding way: the specified explain verbosity is passed through the options until the command document is created, and then the explain is added to it.

Important note: adding an explain to a command alters its shape significantly. For this reason, it should happen at the end of the command building process. This has led to some weirdness where write operations that are being explained still seem like they are doing writes (still call executeWriteOperation in server.ts). I couldn't see a workaround for this that avoided repeating lots of command building logic. Open to any other suggestions!

NODE-2852

@HanaPearlman HanaPearlman changed the title Node 2852/master/explain non cursor feat: add explain support for non-cursor commands Oct 28, 2020
@HanaPearlman HanaPearlman force-pushed the NODE-2852/master/explain-non-cursor branch 5 times, most recently from 58ef4a2 to 35d1814 Compare November 2, 2020 18:59
@HanaPearlman HanaPearlman force-pushed the NODE-2852/master/explain-non-cursor branch from d4a3fb2 to 7090c39 Compare November 2, 2020 20:31
@HanaPearlman HanaPearlman force-pushed the NODE-2852/master/explain-non-cursor branch from 7f19744 to 82f0e60 Compare November 2, 2020 22:38
@HanaPearlman HanaPearlman marked this pull request as ready for review November 3, 2020 15:01
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM with some nits 👍

src/explain.ts Outdated
}

/** Checks that the server supports explain on the given command.*/
static explainSupportedOnCmd(server: Server, cmd: Document): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we overload explainSupported to handle either an op: string or a cmd: Document, instead of having two separate methods that are pretty similar in implementation?

if (cmd.explain) {
cmd = decorateWithExplain(cmd, cmd.explain);
}

server.command(
this.ns.toString(),
cmd,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a stylistic nit, but I think it'd be better to do cmd.explain ? decorateWithExplain(cmd, cmd.explain) : cmd here rather than reassigning the cmd parameter.

src/utils.ts Outdated
delete command.explain;
}

command = { explain: command, verbosity: explain.verbosity };
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just return this object literal directly rather than reassigning command.

src/explain.ts Outdated Show resolved Hide resolved
src/explain.ts Outdated Show resolved Hide resolved
src/operations/explainable_command.ts Outdated Show resolved Hide resolved
src/cursor/cursor.ts Outdated Show resolved Hide resolved
src/explain.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

lgtm!

src/operations/explainable_command.ts Outdated Show resolved Hide resolved
src/operations/explainable_command.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

A few extra nitpicks.

src/explain.ts Outdated
import type { Server } from './sdam/server';
import { maxWireVersion } from './utils';

export const Verbosity = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we expand Verbosity to ExplainVerbosity for clarity?

return setupDatabase(this.configuration);
});

it('shouldHonorBooleanExplainWithDeleteOne', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: test names for new tests should be written in normal text with spaces rather than camel case.

src/index.ts Outdated
@@ -163,6 +163,11 @@ export type {
export type { DbPrivate, DbOptions } from './db';
export type { AutoEncryptionOptions, AutoEncryptionLoggerLevels, AutoEncrypter } from './deps';
export type { AnyError, ErrorDescription } from './error';
export type {
ExplainOptions,
ExplainVerbosity as Verbosity,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ExplainVerbosity as Verbosity,
ExplainVerbosity,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, not sure how that happened!

src/index.ts Outdated
export type {
ExplainOptions,
ExplainVerbosity as Verbosity,
ExplainVerbosityLike as VerbosityLike
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ExplainVerbosityLike as VerbosityLike
ExplainVerbosityLike

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

lgtm

@HanaPearlman HanaPearlman force-pushed the NODE-2852/master/explain-non-cursor branch from 8c5ace7 to 0b37761 Compare November 13, 2020 21:03
Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

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

LGTM!

@HanaPearlman HanaPearlman merged commit 4472308 into mongodb:master Nov 16, 2020
@HanaPearlman HanaPearlman deleted the NODE-2852/master/explain-non-cursor branch November 16, 2020 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants