feat: add explain support for non-cursor commands #2599
Conversation
HanaPearlman
changed the title
HanaPearlman
force-pushed
the
NODE-2852/master/explain-non-cursor
branch
5 times, most recently
from
58ef4a2 to
35d1814
Compare
HanaPearlman
force-pushed
the
NODE-2852/master/explain-non-cursor
branch
from
d4a3fb2 to
7090c39
Compare
HanaPearlman
force-pushed
the
NODE-2852/master/explain-non-cursor
branch
from
7f19744 to
82f0e60
Compare
src/explain.ts
Outdated
| } | ||
|
|
||
| /** Checks that the server supports explain on the given command.*/ | ||
| static explainSupportedOnCmd(server: Server, cmd: Document): boolean { |
There was a problem hiding this comment.
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?
|
|
||
| server.command( | ||
| this.ns.toString(), | ||
| cmd, |
There was a problem hiding this comment.
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 }; |
There was a problem hiding this comment.
I'd just return this object literal directly rather than reassigning command.
src/explain.ts
Outdated
| import type { Server } from './sdam/server'; | ||
| import { maxWireVersion } from './utils'; | ||
|
|
||
| export const Verbosity = { |
There was a problem hiding this comment.
Nit: can we expand Verbosity to ExplainVerbosity for clarity?
test/functional/explain.test.js
Outdated
| return setupDatabase(this.configuration); | ||
| }); | ||
|
|
||
| it('shouldHonorBooleanExplainWithDeleteOne', { |
There was a problem hiding this comment.
Nit: test names for new tests should be written in normal text with spaces rather than camel case.
src/index.ts
Outdated
| export type { AnyError, ErrorDescription } from './error'; | ||
| export type { | ||
| ExplainOptions, | ||
| ExplainVerbosity as Verbosity, |
There was a problem hiding this comment.
| ExplainVerbosity as Verbosity, | |
| ExplainVerbosity, |
There was a problem hiding this comment.
Oops, not sure how that happened!
src/index.ts
Outdated
| export type { | ||
| ExplainOptions, | ||
| ExplainVerbosity as Verbosity, | ||
| ExplainVerbosityLike as VerbosityLike |
There was a problem hiding this comment.
| ExplainVerbosityLike as VerbosityLike | |
| ExplainVerbosityLike |
HanaPearlman
force-pushed
the
NODE-2852/master/explain-non-cursor
branch
from
8c5ace7 to
0b37761
Compare
4 participants
Description
This PR implements
explainfunctionality for the relevant non-cursor operations:update,remove,distinct,findAndModify, andmapReduce. These operations now accept an additionalexplainoption, 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
Explainableaspect was added so that, inCommandOperation, operations with that aspect can use theexplainproperty already on the class. Logic for validating the explain verbosity passed through the options, adding the explain field to the command, andcanRetryWritehave been added toCommandOperation.Not all explainable commands actually call
executeCommand, includingupdateOne,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
executeWriteOperationinserver.ts). I couldn't see a workaround for this that avoided repeating lots of command building logic. Open to any other suggestions!NODE-2852